fixes #5246 feat(nimbus): Alter ready for review error messages in BE, consolidate in FE (#5302)

Because:
* The server-generated messages weren't user friendly for all fields
* We have a bunch of server-side messaging scattered in Storybook that could be stuck together for consistent/easy reference

This commit:
* Adjusts ready for review error messaging in the serializer
* Creates a new SERVER_ERRORS const in the FE for Storybook and tests

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

@@ -536,6 +536,7 @@ class NimbusReadyForReviewSerializer(serializers.ModelSerializer):
     feature_config = serializers.PrimaryKeyRelatedField(
         queryset=NimbusFeatureConfig.objects.all(),
         allow_null=False,
+        error_messages={"null": NimbusConstants.ERROR_REQUIRED_FEATURE_CONFIG},
     )
     primary_outcomes = serializers.ListField(
         child=serializers.CharField(), required=False
@@ -543,9 +544,21 @@ class NimbusReadyForReviewSerializer(serializers.ModelSerializer):
     secondary_outcomes = serializers.ListField(
         child=serializers.CharField(), required=False
     )
-    risk_partner_related = serializers.BooleanField(required=True, allow_null=False)
-    risk_revenue = serializers.BooleanField(required=True, allow_null=False)
-    risk_brand = serializers.BooleanField(required=True, allow_null=False)
+    risk_partner_related = serializers.BooleanField(
+        required=True,
+        allow_null=False,
+        error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION},
+    )
+    risk_revenue = serializers.BooleanField(
+        required=True,
+        allow_null=False,
+        error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION},
+    )
+    risk_brand = serializers.BooleanField(
+        required=True,
+        allow_null=False,
+        error_messages={"null": NimbusConstants.ERROR_REQUIRED_QUESTION},
+    )
 
     class Meta:
         model = NimbusExperiment

app/experimenter/experiments/constants/nimbus.py

@@ -499,6 +499,10 @@ Optional - We believe this outcome will <describe impact> on <core metric>
 
     # Serializer validation errors
     ERROR_DUPLICATE_BRANCH_NAME = "Branch names must be unique."
+    ERROR_REQUIRED_QUESTION = "This question may not be blank."
+    ERROR_REQUIRED_FEATURE_CONFIG = (
+        "You must select a feature configuration from the drop down."
+    )
 
     # Analysis can be computed starting the week after enrollment
     # completion for "week 1" of the experiment. However, an extra

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

@@ -1584,7 +1584,37 @@ class TestNimbusReadyForReviewSerializer(TestCase):
         )
         self.assertFalse(serializer.is_valid())
         self.assertEqual(
-            serializer.errors["feature_config"], ["This field may not be null."]
+            serializer.errors["feature_config"],
+            [NimbusConstants.ERROR_REQUIRED_FEATURE_CONFIG],
+        )
+
+    def test_invalid_experiment_risk_questions(self):
+        experiment = NimbusExperimentFactory.create_with_lifecycle(
+            NimbusExperiment.Lifecycles.CREATED,
+            risk_partner_related=None,
+            risk_revenue=None,
+            risk_brand=None,
+        )
+        serializer = NimbusReadyForReviewSerializer(
+            experiment,
+            data=NimbusReadyForReviewSerializer(
+                experiment,
+                context={"user": self.user},
+            ).data,
+            context={"user": self.user},
+        )
+        self.assertFalse(serializer.is_valid())
+        self.assertEqual(
+            str(serializer.errors["risk_partner_related"][0]),
+            NimbusConstants.ERROR_REQUIRED_QUESTION,
+        )
+        self.assertEqual(
+            str(serializer.errors["risk_revenue"][0]),
+            NimbusConstants.ERROR_REQUIRED_QUESTION,
+        )
+        self.assertEqual(
+            str(serializer.errors["risk_brand"][0]),
+            NimbusConstants.ERROR_REQUIRED_QUESTION,
         )
 
 

app/experimenter/nimbus-ui/src/components/AppLayoutWithSidebar/index.stories.tsx

@@ -5,6 +5,7 @@
 import { withLinks } from "@storybook/addon-links";
 import React from "react";
 import AppLayoutWithSidebar from ".";
+import { SERVER_ERRORS } from "../../lib/constants";
 import { NimbusExperimentStatus } from "../../types/globalTypes";
 import { Subject } from "./mocks";
 
@@ -36,8 +37,8 @@ export const MissingDetails = () => (
       readyForReview: {
         ready: false,
         message: {
-          reference_branch: ["This field may not be null."],
-          channel: ["This list may not be empty."],
+          reference_branch: [SERVER_ERRORS.NULL_FIELD],
+          channel: [SERVER_ERRORS.EMPTY_LIST],
         },
       },
     }}

app/experimenter/nimbus-ui/src/components/AppLayoutWithSidebar/index.test.tsx

@@ -4,7 +4,7 @@
 
 import { act, render, screen, waitFor } from "@testing-library/react";
 import React from "react";
-import { BASE_PATH } from "../../lib/constants";
+import { BASE_PATH, SERVER_ERRORS } from "../../lib/constants";
 import { MockedCache, mockExperimentQuery } from "../../lib/mocks";
 import { renderWithRouter } from "../../lib/test-utils";
 import {
@@ -105,7 +105,7 @@ describe("AppLayoutWithSidebar", () => {
             readyForReview: {
               ready: false,
               message: {
-                channel: ["This list may not be empty."],
+                channel: [SERVER_ERRORS.EMPTY_LIST],
               },
             },
           }}

app/experimenter/nimbus-ui/src/components/PageEditBranches/FormBranches/index.stories.tsx

@@ -6,6 +6,7 @@ import { action } from "@storybook/addon-actions";
 import { storiesOf } from "@storybook/react";
 import React, { useState } from "react";
 import { FormBranches } from ".";
+import { SERVER_ERRORS } from "../../../lib/constants";
 import {
   MOCK_ANNOTATED_BRANCH,
   MOCK_EXPERIMENT,
@@ -156,8 +157,8 @@ storiesOf("pages/EditBranches/FormBranches", module)
         readyForReview: {
           ready: false,
           message: {
-            reference_branch: ["Description may not be blank"],
-            treatment_branches: [null, ["Description may not be blank"]],
+            reference_branch: [SERVER_ERRORS.BLANK_DESCRIPTION],
+            treatment_branches: [null, [SERVER_ERRORS.BLANK_DESCRIPTION]],
           },
         },
       }}

app/experimenter/nimbus-ui/src/components/PageEditBranches/FormBranches/index.test.tsx

@@ -10,6 +10,7 @@ import {
   waitFor,
 } from "@testing-library/react";
 import React from "react";
+import { SERVER_ERRORS } from "../../../lib/constants";
 import { MOCK_CONFIG } from "../../../lib/mocks";
 import {
   MOCK_BRANCH,
@@ -409,8 +410,8 @@ describe("FormBranches", () => {
     });
 
     const expectedReviewErrors = {
-      reference_branch: ["This field may not be null."],
-      treatment_branches: [null, ["Description may not be blank"]],
+      reference_branch: [SERVER_ERRORS.NULL_FIELD],
+      treatment_branches: [null, [SERVER_ERRORS.BLANK_DESCRIPTION]],
     } as const;
 
     render(

app/experimenter/nimbus-ui/src/components/PageEditBranches/index.stories.tsx

@@ -7,6 +7,7 @@ import { withQuery } from "@storybook/addon-queryparams";
 import { storiesOf } from "@storybook/react";
 import React from "react";
 import PageEditBranches from ".";
+import { SERVER_ERRORS } from "../../lib/constants";
 import { mockExperimentQuery } from "../../lib/mocks";
 import { RouterSlugProvider } from "../../lib/test-utils";
 import { NimbusExperimentApplication } from "../../types/globalTypes";
@@ -45,7 +46,7 @@ const { mock: mockMissingFields } = mockExperimentQuery("demo-slug", {
   readyForReview: {
     ready: false,
     message: {
-      reference_branch: ["This field may not be null."],
+      reference_branch: [SERVER_ERRORS.NULL_FIELD],
     },
   },
 });

app/experimenter/nimbus-ui/src/components/PageRequestReview/index.stories.tsx

@@ -5,6 +5,7 @@
 import { withLinks } from "@storybook/addon-links";
 import React from "react";
 import PageRequestReview from ".";
+import { SERVER_ERRORS } from "../../lib/constants";
 import { mockExperimentQuery } from "../../lib/mocks";
 import { RouterSlugProvider } from "../../lib/test-utils";
 import { getExperiment_experimentBySlug } from "../../types/getExperiment";
@@ -44,8 +45,8 @@ export const missingRequiredFields = storyWithExperimentProps(
     readyForReview: {
       ready: false,
       message: {
-        reference_branch: ["This field may not be null."],
-        channel: ["This list may not be empty."],
+        reference_branch: [SERVER_ERRORS.NULL_FIELD],
+        channel: [SERVER_ERRORS.EMPTY_LIST],
       },
     },
   },

app/experimenter/nimbus-ui/src/components/PageRequestReview/index.test.tsx

@@ -12,7 +12,11 @@ import {
 } from "@testing-library/react";
 import fetchMock from "jest-fetch-mock";
 import React from "react";
-import { BASE_PATH, CHANGELOG_MESSAGES } from "../../lib/constants";
+import {
+  BASE_PATH,
+  CHANGELOG_MESSAGES,
+  SERVER_ERRORS,
+} from "../../lib/constants";
 import { mockExperimentQuery, MOCK_CONFIG } from "../../lib/mocks";
 import {
   NimbusExperimentPublishStatus,
@@ -79,7 +83,7 @@ describe("PageRequestReview", () => {
       readyForReview: {
         ready: false,
         message: {
-          channel: ["This list may not be empty."],
+          channel: [SERVER_ERRORS.EMPTY_LIST],
         },
       },
     });

app/experimenter/nimbus-ui/src/hooks/useReviewCheck.test.tsx

@@ -7,24 +7,25 @@ import { render, waitFor } from "@testing-library/react";
 import { renderHook } from "@testing-library/react-hooks";
 import React from "react";
 import { editPages } from "../components/AppLayoutWithSidebar";
+import { SERVER_ERRORS } from "../lib/constants";
 import { MockedCache, mockExperimentQuery } from "../lib/mocks";
 import { useReviewCheck } from "./useReviewCheck";
 
 describe("hooks/useReviewCheck", () => {
   const readyMessages = {
-    public_description: ["This field may not be null."],
-    proposed_duration: ["This field may not be null."],
-    proposed_enrollment: ["This field may not be null."],
-    firefox_min_version: ["This field may not be null."],
-    targeting_config_slug: ["This field may not be null."],
-    reference_branch: ["This field may not be null."],
-    channel: ["This field may not be null."],
+    public_description: [SERVER_ERRORS.NULL_FIELD],
+    proposed_duration: [SERVER_ERRORS.NULL_FIELD],
+    proposed_enrollment: [SERVER_ERRORS.NULL_FIELD],
+    firefox_min_version: [SERVER_ERRORS.NULL_FIELD],
+    targeting_config_slug: [SERVER_ERRORS.NULL_FIELD],
+    reference_branch: [SERVER_ERRORS.NULL_FIELD],
+    channel: [SERVER_ERRORS.NULL_FIELD],
     population_percent: [
       "Ensure this value is greater than or equal to 0.0001.",
     ],
-    risk_brand: ["This field may not be null."],
-    risk_revenue: ["This field may not be null."],
-    risk_partner_related: ["This field may not be null."],
+    risk_brand: [SERVER_ERRORS.NULL_FIELD],
+    risk_revenue: [SERVER_ERRORS.NULL_FIELD],
+    risk_partner_related: [SERVER_ERRORS.NULL_FIELD],
   };
 
   const pageNames = {
@@ -106,9 +107,9 @@ describe("hooks/useReviewCheck", () => {
       readyForReview: {
         ready: false,
         message: {
-          public_description: ["This field may not be null."],
-          reference_branch: ["This field may not be null."],
-          channel: ["This list may not be empty."],
+          public_description: [SERVER_ERRORS.NULL_FIELD],
+          reference_branch: [SERVER_ERRORS.NULL_FIELD],
+          channel: [SERVER_ERRORS.EMPTY_LIST],
         },
       },
     });

app/experimenter/nimbus-ui/src/lib/constants.ts

@@ -11,6 +11,13 @@ export const UNKNOWN_ERROR =
 export const SUBMIT_ERROR =
   "Sorry, an error occurred while submitting. Please try again.";
 
+export const SERVER_ERRORS = {
+  REQUIRED_QUESTION: "This question may not be blank.",
+  NULL_FIELD: "This field may not be null.",
+  EMPTY_LIST: "This list may not be empty.",
+  BLANK_DESCRIPTION: "Description may not be blank.",
+};
+
 export const EXTERNAL_URLS = {
   TRAINING_AND_PLANNING_DOC:
     "https://mana.mozilla.org/wiki/display/FJT/Nimbus+Onboarding",