fixes 2-456: Preserve all data from strategy import #2720
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
Previously the strategy was to use the IMinimalStrategyRow. This PR changes to use the full data from the export in the import. * Discussion point: Should we see if we can add some invariants to our import/export so that we can write some proptests for it. One candidate is commutativity of import/export. On a fresh database importing and then exporting should yield the same file that was imported provided all flags are turned on
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
chriswk
changed the title
| @@ -23,6 +23,16 @@ export interface IMinimalStrategy { | |||
| parameters?: any[]; | |||
| } | |||
|
|
|||
| export interface IStrategyImport { | |||
There was a problem hiding this comment.
Looking at IMinimalStrategy there's a missing property in IStrategyImport:
editable?: boolean
Just checking if this is intentional
There was a problem hiding this comment.
Yeah, we call it builtIn in the export as well as built_in in the table. where editable is built_in === 0. Unfortunately built_in is still an integer in our table for some reason :)
There was a problem hiding this comment.
Oh, I just checked the previous code using
unleash/src/lib/db/strategy-store.ts
Lines 127 to 133 in be045dc
and I see that editable although present in the type was completely ignored 👍
gastonfournier
approved these changes
Dec 21, 2022
chriswk
deleted the
chriswk/2-456-when-importing-strategies-we-are-not
branch
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.
What
Previously when importing strategies we've used the same data type we've used for creating strategies (the minimal, a name, an optional description, optional parameters and an optional editable column). This meant that exporting strategies and then importing them would reactivate deprecated strategies. This PR changes to allow the import to preserve all the data coming in the export file.
Tests
Added four new tests, two new unit tests using our fake stores and two new e2e tests. Interestingly the ones in the fake store ran green before this change as well, probably because we just insert the parsed json object in the fake store, whereas the real store actually converts the object from camelCasing to the postgresql snake_casing standard.
Discussion points:
Mismatch between fake and real stores
This is inevitable since storing things in javascript arrays vs saving in a real database will have some differences, but this again shows the value of our e2e tests.
Invariants
Should we see if we can add some invariants to our import/export so that we can write some proptests for it? One candidate is commutativity of import/export. On a fresh database importing and then exporting should yield the same file that was imported provided all flags are turned on. Candidate for Q1 improvement of import/export.