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.

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

View Definition Auto Migration #1504

Merged
merged 15 commits into from
Oct 22, 2024
Merged

View Definition Auto Migration #1504

merged 15 commits into from
Oct 22, 2024

Conversation

AaronPlave
Copy link
Contributor

@AaronPlave AaronPlave commented Oct 9, 2024

Auto migrate old UI view definitions. Closes #1490

Merge only after #1396 is merged.

Changes:

  • Adds "version" as a property of the view definition
  • Auto migrates old views from upload or when loading from db
  • Notifies users if a view

Testing:

  1. Load a plan with no view specified in the url. The plan should load the default view.
  2. Verify that you can load a view made on the current branch. Try making a complex view with activity layer customizations since these are affected by the v0 -> v1 migration.
  3. Upload a view made prior to this branch and verify that it loads correctly (no success toast will appear since no additional db changes are needed besides nominal insert of the view during upload)
  4. Insert a view made prior to this branch using hasura playground. Load this view from a plan using the browse modal and verify that the view loads and a toast message alerts you that the view has been updated. Refresh the page and verify that no toast appears, indicating that the view was in fact updated in the db.
  5. Force a failure in the migration code (look at the 0 -> 1 migration function) and repeat step 4 and verify that an error toast alerts you that the migration of the view failed and that the default view is loaded. In this case there will only be a single row for the default view.
  6. Insert a view made prior to this branch using hasura playground. Load this view from a plan by specifying the view id in the URL. No toast will appear since migration is being performed on the server side during initial SSR.
  7. Force a failure in the migration code and repeat step 6 and verify that an error toast alerts you that the migration of the view failed and that the default view is loaded. In this case there will be a number of rows since our initial page load default view fetches various activity and resource types and attempts to display them.
  8. Test migration with a default mission model view

TODO:

  • Figure out what to do when view can't be migrated when loading from url, view browser, and imported views. May want to handle them differently? Right now they fall back to default view generation.
  • Figure out if we need to migrate the model default views?
  • Add and fix view tests

@AaronPlave AaronPlave added the DON'T MERGE Do Not Merge This Branch label Oct 9, 2024
@AaronPlave AaronPlave requested a review from a team as a code owner October 9, 2024 23:44
@AaronPlave AaronPlave marked this pull request as draft October 9, 2024 23:44
@AaronPlave AaronPlave mentioned this pull request Oct 10, 2024
20 tasks
@AaronPlave AaronPlave marked this pull request as ready for review October 10, 2024 20:39
@AaronPlave AaronPlave force-pushed the feature/view-migration branch from 7debcdc to a1f1927 Compare October 10, 2024 20:40
@dandelany
Copy link
Collaborator

dandelany commented Oct 11, 2024

Tested thoroughly with @AaronPlave today and everything is looking good. Two small TODOs:

  • If the migration code throws an error while uploading a view, it's not correctly reported in the UI & looks like a schema validation error instead. Can simulate this case by throwing an error in migrateViewDefinitionV0toV1
  • @AaronPlave will go through changes to the view schema since 2.11 release (May 2024) or earlier, and handle any additional changes in the migration to support views as far back as possible

We'd like to get a code review from @duranb next week but 👍 otherwise

@AaronPlave
Copy link
Contributor Author

Tested thoroughly with @AaronPlave today and everything is looking good. Two small TODOs:

  • If the migration code throws an error while uploading a view, it's not correctly reported in the UI & looks like a schema validation error instead. Can simulate this case by throwing an error in migrateViewDefinitionV0toV1
  • @AaronPlave will go through changes to the view schema since 2.11 release (May 2024) or earlier, and handle any additional changes in the migration to support views as far back as possible

We'd like to get a code review from @duranb next week but 👍 otherwise

Fixed the first issue and went through a year of view schema updates and didn't see anything else that needed to be handled.

@AaronPlave AaronPlave self-assigned this Oct 11, 2024
src/utilities/effects.ts Outdated Show resolved Hide resolved
@pranav-super pranav-super force-pushed the feature/external-event branch from a670831 to 1bb5728 Compare October 15, 2024 15:20
@AaronPlave AaronPlave force-pushed the feature/view-migration branch from c017864 to 31f2d6b Compare October 15, 2024 17:39
@JosephVolosin JosephVolosin force-pushed the feature/external-event branch 2 times, most recently from 7cee587 to 4f383e0 Compare October 21, 2024 13:41
Base automatically changed from feature/external-event to develop October 22, 2024 17:21
@AaronPlave AaronPlave force-pushed the feature/view-migration branch from 737f393 to 1561393 Compare October 22, 2024 17:33
@AaronPlave AaronPlave removed the DON'T MERGE Do Not Merge This Branch label Oct 22, 2024
Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

Some small things, but otherwise good with me!

src/utilities/effects.ts Outdated Show resolved Hide resolved
src/utilities/view.ts Outdated Show resolved Hide resolved
@AaronPlave
Copy link
Contributor Author

@duranb made the requested changes

Copy link
Collaborator

@duranb duranb left a comment

Choose a reason for hiding this comment

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

🎉

@AaronPlave AaronPlave force-pushed the feature/view-migration branch from 758e6c0 to 1942fb6 Compare October 22, 2024 21:44
@AaronPlave AaronPlave merged commit 7ec44aa into develop Oct 22, 2024
5 checks passed
@AaronPlave AaronPlave deleted the feature/view-migration branch October 22, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frontend "migration" to upgrade old versions of Views when schema changes
4 participants