aerie-ui icon indicating copy to clipboard operation
aerie-ui copied to clipboard

Plan Model Migration

Open ivydeliz opened this issue 8 months ago • 5 comments

___REQUIRES_AERIE_PR___="1665" https://github.com/NASA-AMMOS/aerie/pull/1665/

Plan Model Migration is brought to you by @ivydeliz and @joshhaug (and now @AaronPlave)! this was a dual effort UI and backend separately but here is the fully integrated feature.

Issues: aerie-ui: Closes #1631 aerie: https://github.com/NASA-AMMOS/aerie/issues/1530

Figma design reference: https://www.figma.com/design/dNrdkpcAobXVqj97JaSm1l/AERIE-Mission-Model-Migration?node-id=177-3478&t=eJDYjUp1Bqx8Cza8-0

To fully test you need to do a few steps.... i'll be specific sorry if obvious.

In the aerie repo... (NOT aerie-ui)

  1. Checkout feature/plan-model-migration branch in aerie repo
  2. docker-compose down -v
  3. ./gradlew assemble
  4. docker-compose up --build --detach

In the aerie-ui repo 5. Checkout feature/plan-model-migration branch in aerie-ui 6. npm run dev

In your browser 7. Open localhost:3000 and login to Aerie 8. Import like 3 different models 9. Create a plan with one of the models and add several activity directives to it - if there's no activities there will be no problems 10. Go to the /plans table 11. Click on a plan 12. You will see a new button next to the Model input with arrows left and right, tooltip says "Change Plan Model" 13. Click on button and it will open up a new modal "Change Mission Model", it should show the a table of imported models except the model that the plan has been created with 14. Click on one of the models, and if there's any conflicts it should preview them on the right panel 15. Click on "Change Mission Model" button and in the table it should update and Selected plan panel the new model should be updated 16. 🎉 When you merge this one, remember to merge the aerie repo branch!

ivydeliz avatar Apr 02 '25 15:04 ivydeliz

Some notes and TODOS:

Notes:

  • Should merge request behavior (or lack thereof at the moment) be a blocker for V1? Know Theresa listed additional backend work as TODO re merging https://github.com/NASA-AMMOS/aerie/issues/1530#issuecomment-2631961190. Right now a user can make a merge request from a child plan into a parent plan with a different model. This doesn't seem to break anything but not sure if we want to allow it. We could also prevent this from the UI for now if desired.
  • How should we store a bananation-v2 model in the UI repo long term? For now we could copy over the altered banananation model into the e2e data directory but in the future if banananation changes in such a way that we need to update it for UI e2e testing we’d also need to create an updated v2 model.

TODOS:

  • [x] Update plan metadata subscription to include mission model data so that a refresh is not required on model migration
  • [x] Consider allowing users to migrate models from within a plan
  • [x] Make expected migration errors non-selectable
  • [x] Chat with @duranb about extraneous argument clearing and if that is still an issue https://github.com/NASA-AMMOS/aerie/issues/1530#issuecomment-2635216445
  • [x] Investigate case of changing param type from struct <-> string/boolean/etc, see if it updates properly upon user input
  • [x] Should v1 UI block model migration on plan if it has incoming merge requests? Currently backend fails the request and provides a useful error message but you have to know to check the error tray at the bottom to get info on why it failed.
  • [x] Ensure migration can't happen in plan read only mode
  • [x] Add model id to migration UI
  • [x] Prevent users from viewing plan snapshots and simulation that have old models
  • [ ] Review permissions
  • [x] ~When viewing a simulation that was performed using a different model, should the UI use that model's parameter schema when viewing span parameters~ (future work)
  • [X] Prevent merge request submission when source and target plan models do not match
  • [x] Clear incompatible simulation upon model migration if found
  • [x] e2e tests

AaronPlave avatar Apr 14 '25 15:04 AaronPlave

I think the migration should be blocked if there's an open merge request, it's unlikely we'll prioritize supporting a merge in this situation in the near future

joswig avatar Apr 15 '25 20:04 joswig

I think the migration should be blocked if there's an open merge request, it's unlikely we'll prioritize supporting a merge in this situation in the near future

@joswig migration blocked from the UI specifically?

AaronPlave avatar Apr 15 '25 20:04 AaronPlave

I think the migration should be blocked if there's an open merge request, it's unlikely we'll prioritize supporting a merge in this situation in the near future

@joswig migration blocked from the UI specifically?

yes please, unless there's a complication to the query

joswig avatar Apr 15 '25 21:04 joswig