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

bug(nimbus): Prevent mismatches between NimbusReviewSerializer and the front-end allowing invalid experiments #11773

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

brennie
Copy link
Member

@brennie brennie commented Nov 14, 2024

Because:

  • #112832 added the segments field to the NimbusExperiment model.
  • It did not add it to the NimbusReviewSerializer, which caused the serializer to infer a default serializer for the field which marks it as a required field.
  • Therefore, the NimbusReviewSerializer was always throwing before we hit its validate() method (due to the invalid segments field), which prevented the majority of the validation from running.
  • And the front-end was not checking the ready property from the useReviewCheck() hook, which allowed launching invalid experiments.
  • Our tests were always creating NimbusExperiments with non-empty segments fields.

This commit:

  • Adds a segments field to the NimbusReviewSerializer which makes the field optional.
  • Adds a test that explicitly creates a NimbusExperiment with an empty segments field and verifies it does not raise a ValidationError. (This test fails without the changes to the serializer.)
  • Changes our NimbusExperimentFactory to randomly use between 0 and 2 values for segments, primary_outcomes, and secondary_outcomes so that we have broader test coverage over all possible inputs.
  • Updates our front-end to use the ready key instead of invalidPages to prevent launches.
  • Updates the front-end to instruct users to come to #ask-experimenter if ready is false but there are no invalid pages (i.e., they've hit a programming error).

Fixes #11767

…e front-end allowing invalid experiments

Because:

- #112832 added the `segments` field to the `NimbusExperiment` model.
- It did not add it to the `NimbusReviewSerializer`, which caused the
  serializer to infer a default serializer for the field which marks it
  as a required field.
- Therefore, the `NimbusReviewSerializer` was always throwing **before**
  we hit its `validate()` method (due to the invalid `segments` field),
  which prevented the majority of the validation from running.
- And the front-end was not checking the `ready` property from the
  `useReviewCheck()` hook, which allowed launching invalid experiments.
- Our tests were always creating `NimbusExperiment`s with non-empty
  `segments` fields.

This commit:

- Adds a `segments` field to the `NimbusReviewSerializer` which makes
  the field optional.
- Adds a test that explicitly creates a `NimbusExperiment` with an empty
  `segments` field and verifies it does not raise a `ValidationError`.
  (This test fails without the changes to the serializer.)
- Changes our `NimbusExperimentFactory` to randomly use between 0 and 2
  values for `segments`, `primary_outcomes`, and `secondary_outcomes` so
  that we have broader test coverage over all possible inputs.
- Updates our front-end to use the `ready` key instead of `invalidPages`
  to prevent launches.
- Updates the front-end to instruct users to come to #ask-experimenter
  if `ready` is false but there are no invalid pages (i.e., they've hit
  a programming error).

Fixes #11767
Copy link
Collaborator

@jaredlockhart jaredlockhart left a comment

Choose a reason for hiding this comment

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

Okay I tested everything locally, all looks and works good!

Thanks for catching and picking this up so quickly @brennie 🙏 🙏 🙏

@brennie brennie added this pull request to the merge queue Nov 14, 2024
Merged via the queue into main with commit 62db605 Nov 14, 2024
17 checks passed
@brennie brennie deleted the beth/fix-prod branch November 14, 2024 23:36
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.

NimbusReviewSerializer is busted, letting experiments be published without full validation
2 participants