feat(nimbus): Add validation for Firefox Labs fields#13313
Merged
freshstrangemusic merged 1 commit intomainfrom Aug 20, 2025
Merged
feat(nimbus): Add validation for Firefox Labs fields#13313freshstrangemusic merged 1 commit intomainfrom
freshstrangemusic merged 1 commit intomainfrom
Conversation
freshstrangemusic
requested review from
jaredlockhart and
yashikakhurana
as code owners
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
from
81294d9 to
993c549
Compare
jaredlockhart
requested changes
Aug 20, 2025
Collaborator
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Looking good! Thnx I don't know how I missed adding the validation around this 🙈
Some questions and thoughts but generally looks right 🙏
...er/experiments/migrations/0288_nimbusexperiment_firefox_labs_description_links_text_field.py
Show resolved
Hide resolved
...er/experiments/migrations/0288_nimbusexperiment_firefox_labs_description_links_text_field.py
Outdated
Show resolved
Hide resolved
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
4 times, most recently
from
daf2d57 to
6320be6
Compare
jaredlockhart
requested changes
Aug 20, 2025
Collaborator
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Yep looking good.
Seems like we're throwing a validation error that's not being displayed, maybe the review serializer isn't hooked up to all the fields in the template:
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
2 times, most recently
from
e721e34 to
18b3656
Compare
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
2 times, most recently
from
cea3d42 to
cee8b26
Compare
jaredlockhart
approved these changes
Aug 20, 2025
Collaborator
jaredlockhart
left a comment
jaredlockhart
left a comment
There was a problem hiding this comment.
Okey doke, tested locally, everything looks good, tests are good, thnx @freshstrangemusic 🙏 🎉
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
2 times, most recently
from
608cb65 to
ea1cc9d
Compare
Because: - Firefox Labs fields were originally behind the Django admin panel and not user-editable and thus we decided not to add validation to the `NimbusReviewSerializer` at implementation time; - we have completed the UI migration to htmx and we now have Firefox Labs fields that are user-editable; - these fields were added without adding validation to the `NimbusReviewSerializer`, thus making it possible to create and launch invalid Firefox Labs rollouts; - the `firefox_labs_description_links` field was defined as a JSON field in the model, but the UI was defined as a text field, leading to differences between the serialized and saved data; and - we were saving the NimbusExperiment multiple times in `NimbusBranchesForm.save()` this commit: - adds validation to the `NimbusReviewSerializer` for Firefox Labs fields; - migrates the `firefox_labs_description_links` field to a `TextField`; and - adds a `NimbusBranchesForm.clean()` to enforce that all Firefox Labs deliveries are rollouts. Fixes #13304
freshstrangemusic
force-pushed
the
push-ynktywxtupml
branch
from
ea1cc9d to
b281dd4
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Because:
NimbusReviewSerializerat implementation time;NimbusReviewSerializer, thus making it possible to create and launch invalid Firefox Labs rollouts;firefox_labs_description_linksfield was defined as a JSON field in the model, but the UI was defined as a text field, leading to differences between the serialized and saved data; andNimbusBranchesForm.save()this commit:
NimbusReviewSerializerfor Firefox Labs fields;firefox_labs_description_linksfield to aTextField; andNimbusBranchesForm.clean()to enforce that all Firefox Labs deliveries are rollouts.Fixes #13304