B 20034 #12631
Merged
B 20034 #12631
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
CoryKleinjanCACI
requested review from
MInthavongsay,
antgmann,
r-mettler and
ajlusk
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
danieljordan-caci
approved these changes
May 9, 2024
deandreJones
reviewed
May 9, 2024
| @@ -0,0 +1,7 @@ | |||
| ALTER TABLE application_parameters | |||
| ADD COLUMN IF NOT EXISTS parameter_name TEXT NOT NULL, | |||
| ADD COLUMN IF NOT EXISTS parameter_value TEXT NOT NULL, | |||
There was a problem hiding this comment.
these new columns can't be set to null, until they are populated with something
There was a problem hiding this comment.
Oh, good catch because we have the existing row being used already. Didn't think about that.
Would adding in a default value be a valid approach?
ADD COLUMN IF NOT EXISTS parameter_name TEXT NOT NULL DEFAULT 'NULL',
ADD COLUMN IF NOT EXISTS parameter_value TEXT NOT NULL DEFAULT 'NULL',
There was a problem hiding this comment.
I think we either need to populate it with the current data with how we want it to work, then a separate migration to set it to not null for new rows
There was a problem hiding this comment.
or don't have the not null restriction
I haven't looked through the code yet to see if validation code is pulling off the old columns or the new param_value
There was a problem hiding this comment.
He could remove the NOT NULL and then a simple update to the model and make them pointers. That might be easier than making them NOT NULL and the subsequent updates.
What do you think?
There was a problem hiding this comment.
I don't see an issue with the param_name and param_value being allowed to be null, unless someone else sees one. The validation codes are using param_value now.
r-mettler
approved these changes
May 14, 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.
20034
Integration Review
Summary
This PR updates B-19507 so that the application_parameters table can hold a wider array of parameters.
The tests here are to validate that 19507 still functions as expected.
How to test
Couple of setup things you need to do first:
make db_dev_migrate(this will create theapplication_parameterstableparameter_name, 'parameter_value' andid,updated_atandcreated_atwill auto populate.envrc, setexport FEATURE_FLAG_VALIDATION_CODE_REQUIRED=trueclientandserverFor the happiest path
application_parameterstableFor the saddest path
Screenshots
Mobile
Mobile
