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

experimenter/experimenter/experiments/api/v5/serializers.py

@@ -1206,6 +1206,10 @@ class NimbusReviewSerializer(serializers.ModelSerializer):
         allow_null=False,
         error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION},
     )
+    segments = serializers.ListField(
+        child=serializers.CharField(),
+        required=False,
+    )
     is_localized = serializers.BooleanField(required=False)
     localizations = serializers.CharField(
         required=False, allow_blank=True, allow_null=True

experimenter/experimenter/experiments/tests/api/v5/test_serializers/test_nimbus_ready_for_review_serializer.py

@@ -3158,6 +3158,23 @@ def test_desktop_prefflips_warns_setpref_conflicts(
                 },
             )
 
+    def test_empty_segments(self):
+        experiment = NimbusExperimentFactory.create_with_lifecycle(
+            NimbusExperimentFactory.Lifecycles.CREATED,
+            targeting_config_slug=NimbusExperiment.TargetingConfig.NO_TARGETING,
+            application=NimbusExperiment.Application.DESKTOP,
+            firefox_min_version=NimbusConstants.MIN_REQUIRED_VERSION,
+            segments=[],
+        )
+
+        serializer = NimbusReviewSerializer(
+            experiment,
+            data=NimbusReviewSerializer(experiment, context={"user": self.user}).data,
+            context={"user": self.user},
+        )
+
+        self.assertTrue(serializer.is_valid(), serializer.errors)
+
 
 class VersionedFeatureValidationTests(MockFmlErrorMixin, TestCase):
     maxDiff = None

experimenter/experimenter/experiments/tests/factories.py

@@ -419,6 +419,11 @@ class Lifecycles(Enum):
     )
 
 
+def random_slice(lst, *, min_items=0, max_items=2):
+    count = random.randint(min_items, min(max_items, len(lst)))
+    return lst[:count]
+
+
 class NimbusExperimentFactory(factory.django.DjangoModelFactory):
     publish_status = NimbusExperiment.PublishStatus.IDLE
     owner = factory.SubFactory(UserFactory)
@@ -454,13 +459,13 @@ class NimbusExperimentFactory(factory.django.DjangoModelFactory):
         lambda o: random.choice(list(NimbusExperiment.TargetingConfig)).value
     )
     primary_outcomes = factory.LazyAttribute(
-        lambda o: [oc.slug for oc in Outcomes.all()[:2]]
+        lambda o: random_slice([oc.slug for oc in Outcomes.all()[:2]])
     )
     secondary_outcomes = factory.LazyAttribute(
-        lambda o: [oc.slug for oc in Outcomes.all()[2:]]
+        lambda o: random_slice([oc.slug for oc in Outcomes.all()[2:]])
     )
     segments = factory.LazyAttribute(
-        lambda o: [s.slug for s in Segments.all(mock_get_segments())]
+        lambda o: random_slice([s.slug for s in Segments.all(mock_get_segments())])
     )
     risk_partner_related = factory.LazyAttribute(lambda o: random.choice([True, False]))
     risk_revenue = factory.LazyAttribute(lambda o: random.choice([True, False]))

experimenter/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.test.tsx

@@ -55,14 +55,29 @@ describe("ChangeApprovalOperations", () => {
           canReview: true,
           invalidPages: ["overview", "thingy", "frobnitz"],
           InvalidPagesList: () => <span>{expectedInvalidPages}</span>,
+          ready: false,
         }}
       />,
     );
-    const invalidPagesAlert = screen.queryByTestId("invalid-pages");
-    expect(invalidPagesAlert).toBeInTheDocument();
+    const invalidPagesAlert = screen.getByTestId("invalid-pages");
     expect(invalidPagesAlert!.textContent).toContain(expectedInvalidPages);
   });
 
+  it("displays an error when ready is false but no pages contain errors", async () => {
+    render(
+      <Subject
+        {...{
+          ...reviewRequestedBaseProps,
+          canReview: true,
+          invalidPages: [],
+          InvalidPagesList: undefined,
+          ready: false,
+        }}
+      />,
+    );
+    screen.getByTestId("invalid-internal-error");
+  });
+
   it("does not display an invalid pages warning when details are missing from an archived experiment", async () => {
     const expectedInvalidPages = "overview, thingy, and frobnitz";
     render(

experimenter/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.tsx

@@ -39,6 +39,7 @@ export type ChangeApprovalOperationsProps = {
   reviewUrl: string;
   invalidPages: string[];
   InvalidPagesList: React.FC<unknown>;
+  ready: boolean;
 };
 
 export const ChangeApprovalOperations: React.FC<
@@ -59,14 +60,15 @@ export const ChangeApprovalOperations: React.FC<
   invalidPages,
   InvalidPagesList,
   children,
+  ready,
 }) => {
   const defaultUIState = useMemo(() => {
     if (status.archived) {
       // No changes to be approved with an archived experiment
       return ChangeApprovalOperationsState.None;
     }
 
-    if (invalidPages.length > 0 && status.draft) {
+    if (!ready && status.draft) {
       return ChangeApprovalOperationsState.InvalidPages;
     }
 
@@ -83,7 +85,7 @@ export const ChangeApprovalOperations: React.FC<
       default:
         return ChangeApprovalOperationsState.None;
     }
-  }, [status, publishStatus, canReview, invalidPages.length]);
+  }, [status, publishStatus, canReview, ready]);
 
   const [uiState, setUIState] =
     useState<ChangeApprovalOperationsState>(defaultUIState);
@@ -99,13 +101,22 @@ export const ChangeApprovalOperations: React.FC<
 
   switch (uiState) {
     case ChangeApprovalOperationsState.InvalidPages:
-      return (
-        <Alert variant="danger" data-testid="invalid-pages">
-          Before this experiment can be reviewed or launched, all required
-          fields must be completed. Fields on the <InvalidPagesList />{" "}
-          {invalidPages.length === 1 ? "page" : "pages"} are missing details.
-        </Alert>
-      );
+      if (invalidPages.length) {
+        return (
+          <Alert variant="danger" data-testid="invalid-pages">
+            Before this experiment can be reviewed or launched, all required
+            fields must be completed. Fields on the <InvalidPagesList />{" "}
+            {invalidPages.length === 1 ? "page" : "pages"} are missing details.
+          </Alert>
+        );
+      } else {
+        return (
+          <Alert variant="danger" data-testid="invalid-internal-error">
+            This experiment cannot be launched due to an internal Experimenter
+            error. Please ask in #ask-experimenter.
+          </Alert>
+        );
+      }
     case ChangeApprovalOperationsState.ApprovalPending:
       return (
         <Alert

experimenter/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/mocks.tsx

@@ -40,6 +40,7 @@ export const BaseSubject = ({
   rejectChange = () => {},
   approveChange = () => {},
   invalidPages = [],
+  ready = true,
   InvalidPagesList = () => <span />,
   children = (
     <Button data-testid="action-button" className="mr-2 btn btn-success">
@@ -64,6 +65,7 @@ export const BaseSubject = ({
       reviewUrl: REVIEW_URL,
       invalidPages,
       InvalidPagesList,
+      ready,
       ...props,
     }}
   >

experimenter/experimenter/nimbus-ui/src/components/PageSummary/index.tsx

@@ -41,8 +41,8 @@ const PageSummary = (props: RouteComponentProps) => {
   useExperimentPolling();
 
   const [showLaunchToReview, setShowLaunchToReview] = useState(false);
-  const { invalidPages, InvalidPagesList } = useReviewCheck(experiment);
-  const { fieldWarnings } = useReviewCheck(experiment);
+  const { invalidPages, InvalidPagesList, fieldWarnings, ready } =
+    useReviewCheck(experiment);
 
   const status = getStatus(experiment);
 
@@ -257,6 +257,7 @@ const PageSummary = (props: RouteComponentProps) => {
           reviewUrl: experiment.reviewUrl!,
           invalidPages,
           InvalidPagesList,
+          ready,
         }}
       >
         {status.draft &&