Support schema 300 (settings) #366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jpprins1
reviewed
Mar 18, 2024
This was referenced Mar 18, 2024
margrietpalm
requested review from
elisalle and
jpprins1
and removed request for
jpprins1 and
elisalle
caspervdw
requested changes
Apr 8, 2024
There was a problem hiding this comment.
Wat een werk! Dit is wel iets waar klanten direct baat bij hebben als het goed zit. Ik heb een paar dingen aangetekend, niets groots, kijk er even naar.
Met name het "join" probleem dat is geintroduceerd door de xpliciete foreign keys tussen de settings tabellen weg te halen.
Het lijkt me prima als je overal een "cartesian" join doet, wat betekent dat alle combinaties van records tussen tabel A en B worden gemaakt. Alleen sqlalchemy geeft daar een waarschuwing voor. Zie mijn comments om daar omheen te werken.
…e check with grep on all python files
Co-authored-by: Casper van der Wel <casper.vanderwel@nelen-schuurmans.nl>
…/threedi-modelchecker into margriet_363_schema_300_settings_v2
caspervdw
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should be used with latest schema, currently on
margriet_46_schema_300!Apologies for the large number of changes. Most changes just involve rename tables and or columns. But due to the changes in the schema, some checks have been modified, removed or added. Please see the comments I made in this PR for some extra context.
In this updated version the settings tables, except
AggregationSettings, have exactly one row. The migration takes care of enforcing this and retaining the correct settings. Previously, filters for the first row ofGlobalSettingand matching setting table rows were created and the checks were only performed on that data. This machinery has been removed because there should be only one row.The integration tests ensure that the results of the model checker are unchanged, except for expected results due to the schema changes.