Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[NU-1772] Scenario activities BE implementation #6823

Merged
merged 50 commits into from
Sep 23, 2024
Merged

[NU-1772] Scenario activities BE implementation #6823

merged 50 commits into from
Sep 23, 2024

Conversation

mgoworko
Copy link
Contributor

@mgoworko mgoworko commented Sep 9, 2024 •
edited
Loading

Describe your changes

What is done:

  • new API contract for FE (in PR [NU-1772] Scenario activities API contract (without implementation) #6822)
  • BE implemenetation of that API in this PR
  • new table that replaces actions and comments
  • possibility to save and return to FE all scenario-related activities
  • full backwards compatibility with old FE API, and old Action mechanism, only changed the db underneath
  • migration of data from 2 old tables - comments and actions - to new "scenario_activities" table
  • some integration tests of the new API

What is left to do in further PRs

  1. Veryfing what is already done and saving information of scenario activities that has not been saved until now (like incoming and outgoing migrations, automatic updates of scenarios etc.)
  2. Integrating periodics into this new scenario activity mechanism.
  3. Clean up endpoint with scenario details, to not include versions and comments, they will be handled by this new API

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

@mgoworko
API specification
b421bc7
@mgoworko
qs
20a1c6d
@mgoworko
Test fixes, activities handling improvements
ae8b5b2
@mgoworko
Restored legacy PDF exporter, legacy API endpoints
3e928f9
@mgoworko
Added proper comment handling
be9cb20
@mgoworko
API draft, DB table and migration
e12fc29
@mgoworko
Fix Scala 2.12 build
0fa2984
@mgoworko
Fix Scala 2.12 build
9629d4e
@mgoworko
Fix Scala 2.12 build
3c84065
@mgoworko
Merge remote-tracking branch 'origin/NU-1772-scenario-activity-api-co…
5355ed0 
…ntract' into NU-1772-scenario-activities

# Conflicts:
#	designer/server/src/main/scala/pl/touk/nussknacker/ui/api/ScenarioActivityApiHttpService.scala
#	designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/scenarioActivity/Dtos.scala
#	designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/scenarioActivity/Endpoints.scala
#	designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/scenarioActivity/InputOutput.scala
#	docs-internal/api/nu-designer-openapi.yaml
@mgoworko
qs
5391527
@mgoworko mgoworko marked this pull request as ready for review September 9, 2024 14:36
@mgoworko mgoworko force-pushed the NU-1772-scenario-activity-api-contract branch from 6000152 to 874206e Compare September 10, 2024 15:08
lukasz-bigorajski
lukasz-bigorajski requested changes Sep 11, 2024
View reviewed changes
designer/server/src/main/scala/db/migration/V1_055__CreateScenarioActivitiesDefinition.scala Outdated Show resolved Hide resolved
...ain/scala/db/migration/V1_056__MigrateActionsAndCommentsToScenarioActivitiesDefinition.scala Outdated Show resolved Hide resolved
...ain/scala/db/migration/V1_056__MigrateActionsAndCommentsToScenarioActivitiesDefinition.scala Outdated Show resolved Hide resolved
...ain/scala/db/migration/V1_056__MigrateActionsAndCommentsToScenarioActivitiesDefinition.scala Outdated Show resolved Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/ScenarioActivityApiHttpService.scala Outdated Show resolved Hide resolved
utils/test-utils/src/main/resources/logback-test.xml Outdated Show resolved Hide resolved
utils/test-utils/src/main/resources/logback-test.xml Outdated Show resolved Hide resolved
...cala/pl/touk/nussknacker/ui/process/repository/activities/DbScenarioActivityRepository.scala Show resolved Hide resolved
...cala/pl/touk/nussknacker/ui/process/repository/activities/DbScenarioActivityRepository.scala Outdated Show resolved Hide resolved
...cala/pl/touk/nussknacker/ui/process/repository/activities/DbScenarioActivityRepository.scala Outdated Show resolved Hide resolved
lukasz-bigorajski
lukasz-bigorajski reviewed Sep 11, 2024
View reviewed changes
Copy link
Contributor

@lukasz-bigorajski lukasz-bigorajski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it's a great job

@github-actions github-actions bot removed ui client client main fe labels Sep 17, 2024
@github-actions github-actions bot mentioned this pull request Sep 17, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 17, 2024 •
edited
Loading

created: #6887
⚠️ Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

@github-actions github-actions bot mentioned this pull request Sep 17, 2024
Closed
arkadius
arkadius approved these changes Sep 18, 2024
View reviewed changes
Copy link
Member

@arkadius arkadius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parts of the code that I read are fine for me, let's w8 for @lukasz-bigorajski approve as he checked it more extensively..

lukasz-bigorajski
lukasz-bigorajski approved these changes Sep 19, 2024
View reviewed changes
Copy link
Contributor

@lukasz-bigorajski lukasz-bigorajski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I responded to the questions.

@mgoworko
Copy link
Contributor Author

Yesterday I tested this change using real data from prod environments. It turned out, that migration was not fully correct, and existing test did not catch that. Some data, including standalone comments, was missing.

Done after review and approve:

  • added 9 tests for migrating each old process action type and standalone comments
  • fixed migration, including proper handling of comment prefixes and standalone comments
  • change in API contract - finishedAt field made optional in two activities, comment added for "run now" activity

lukasz-bigorajski
lukasz-bigorajski approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@lukasz-bigorajski lukasz-bigorajski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One question.

...cala/pl/touk/nussknacker/ui/process/repository/activities/DbScenarioActivityRepository.scala Show resolved Hide resolved
...rver/src/test/scala/db/migration/V1_057__MigrateActionsAndCommentsToScenarioActivities.scala Outdated Show resolved Hide resolved
@mgoworko mgoworko merged commit aa1747b into staging Sep 23, 2024
17 checks passed
@mgoworko mgoworko deleted the NU-1772-scenario-activities branch September 23, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasz-bigorajski lukasz-bigorajski lukasz-bigorajski approved these changes

@arkadius arkadius arkadius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mgoworko @arkadius @lukasz-bigorajski