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

Made adjustments to support having analysis_summary inside the analys… #326

Merged
merged 9 commits into from
Sep 14, 2023

Conversation

MGS-sails
Copy link
Contributor

…is_job model

Copy link
Member

@SandyRogers SandyRogers left a comment

Choose a reason for hiding this comment

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

Thank you @MGS-sails ! Couple of suggested updates inline.

It looks like the QC upload test is also failing, so that one will need updating.

Copy link
Member

Choose a reason for hiding this comment

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

Could we squash these multiple migrations? (Or since they look like temporary testing things, delete all of the migration files, reset the sqlite EMG fixture to the one from the client repo, and run makemigrations again to just get a single migration with the needed schema changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SandyRogers I've squashed the migrations

Copy link
Member

@mberacochea mberacochea left a comment

Choose a reason for hiding this comment

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

Great stuff, I agree with Sandy's suggestions. We should also remove the tables we migrated to the json field after this is done

@SandyRogers
Copy link
Member

@MGS-sails One more thing I forgot, sorry: can we please update the analysis_summary @Property on the AnalysisJob model? As an interim we could have it return the JSONField contents unless empty, in which case fetch from the related table as it does now.

Once we release this, run the migration, we can then do another PR with the table cleanup and potentially rename the _json field to just analysis_summary, then removing the @Property completely.

@MGS-sails
Copy link
Contributor Author

@SandyRogers the tests are now passing

@mberacochea mberacochea merged commit 93b9183 into develop Sep 14, 2023
4 checks passed
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.

3 participants