Skip to content

Commit

Permalink
fix #5742 fix #5927 feat(nimbus): add end enrollment UI
Browse files Browse the repository at this point in the history
Because:

* we want to add UI to enable manual enrollment end

This commit:

* add isEnrollmentPaused to GQL experiment input for consistency with
  serialized experiment type, mapped to is_paused in Django model

* update "rejecting this review" copy to "rejecting the request to
  {actionDescription}"

* refactors handling of strings describing lifecycle review flows, moves
  all the strings into shared constants

* refactors the Settings page and ChangeApprovalOperations components

* adds more exhaustive stories for launch, end enrollment, and end cases
  on PageSummary

* reworks rejection reason display to more properly use old_status and
  old_status_next to determine what kind of review was rejected

* adds more status-based logic to make the correct GQL queries on
  approval / rejection of review
  • Loading branch information
lmorchard committed Jul 13, 2021
1 parent 42ed93d commit 77081c0
Show file tree
Hide file tree
Showing 30 changed files with 910 additions and 250 deletions.
1 change: 1 addition & 0 deletions app/experimenter/experiments/api/v5/inputs.py
Expand Up @@ -45,6 +45,7 @@ class ExperimentInput(graphene.InputObjectType):
hypothesis = graphene.String()
application = NimbusExperimentApplication()
public_description = graphene.String()
is_enrollment_paused = graphene.Boolean()
risk_mitigation_link = graphene.String()
feature_config_id = graphene.Int()
documentation_links = graphene.List(DocumentationLinkType)
Expand Down
2 changes: 2 additions & 0 deletions app/experimenter/experiments/api/v5/serializers.py
Expand Up @@ -292,6 +292,7 @@ class NimbusExperimentSerializer(
public_description = serializers.CharField(
min_length=0, max_length=1024, required=False, allow_blank=True
)
is_enrollment_paused = serializers.BooleanField(source="is_paused", required=False)
risk_mitigation_link = serializers.URLField(
min_length=0, max_length=255, required=False, allow_blank=True
)
Expand Down Expand Up @@ -362,6 +363,7 @@ class Meta:
"hypothesis",
"application",
"public_description",
"is_enrollment_paused",
"feature_config",
"reference_branch",
"treatment_branches",
Expand Down
60 changes: 60 additions & 0 deletions app/experimenter/experiments/tests/api/v5/test_mutations.py
Expand Up @@ -668,3 +668,63 @@ def test_launch_experiment_valid_with_preview_status(self):
content = json.loads(response.content)
result = content["data"]["updateExperiment"]
self.assertEqual(result["message"], "success")

def test_request_end_enrollment(self):
user_email = "user@example.com"
experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.LIVE_ENROLLING
)
self.assertEqual(experiment.is_paused, False)

response = self.query(
UPDATE_EXPERIMENT_MUTATION,
variables={
"input": {
"id": experiment.id,
"status": NimbusExperiment.Status.LIVE.name,
"statusNext": NimbusExperiment.Status.LIVE.name,
"publishStatus": NimbusExperiment.PublishStatus.REVIEW.name,
"isEnrollmentPaused": True,
"changelogMessage": "test changelog message",
}
},
headers={settings.OPENIDC_EMAIL_HEADER: user_email},
)
self.assertEqual(response.status_code, 200, response.content)

content = json.loads(response.content)
result = content["data"]["updateExperiment"]
self.assertEqual(result["message"], "success")

experiment = NimbusExperiment.objects.get(id=experiment.id)
self.assertEqual(experiment.is_paused, True)

def test_reject_end_enrollment(self):
user_email = "user@example.com"
experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.PAUSING_REVIEW_REQUESTED
)
self.assertEqual(experiment.is_paused, True)

response = self.query(
UPDATE_EXPERIMENT_MUTATION,
variables={
"input": {
"id": experiment.id,
"status": NimbusExperiment.Status.LIVE.name,
"statusNext": None,
"publishStatus": NimbusExperiment.PublishStatus.IDLE.name,
"isEnrollmentPaused": False,
"changelogMessage": "test changelog message",
}
},
headers={settings.OPENIDC_EMAIL_HEADER: user_email},
)
self.assertEqual(response.status_code, 200, response.content)

content = json.loads(response.content)
result = content["data"]["updateExperiment"]
self.assertEqual(result["message"], "success")

experiment = NimbusExperiment.objects.get(id=experiment.id)
self.assertEqual(experiment.is_paused, False)
1 change: 1 addition & 0 deletions app/experimenter/nimbus-ui/schema.graphql
Expand Up @@ -24,6 +24,7 @@ input ExperimentInput {
hypothesis: String
application: NimbusExperimentApplication
publicDescription: String
isEnrollmentPaused: Boolean
riskMitigationLink: String
featureConfigId: Int
documentationLinks: [DocumentationLinkType]
Expand Down
Expand Up @@ -9,22 +9,22 @@ import Form from "react-bootstrap/Form";
import { getExperiment_experimentBySlug } from "../../types/getExperiment";

const FormApproveOrReject = ({
actionButtonTitle,
actionDescription,
timeoutEvent,
reviewRequestEvent,
isLoading,
onApprove,
onReject,
}: {
actionButtonTitle: string;
actionDescription: string;
timeoutEvent?: getExperiment_experimentBySlug["timeout"];
reviewRequestEvent?: getExperiment_experimentBySlug["reviewRequest"];
isLoading: boolean;
onApprove: () => void;
onReject: () => void;
}) => {
const ucActionDescription =
actionDescription.charAt(0).toUpperCase() + actionDescription.slice(1);
return (
<>
{timeoutEvent && (
Expand All @@ -33,16 +33,16 @@ const FormApproveOrReject = ({
<span role="img" aria-label="red X emoji">
</span>{" "}
Remote Settings request has timed out, please go through the{" "}
{actionDescription} request and approval flow again.
Remote Settings request has timed out, please go through the request{" "}
and approval flow to {actionDescription} again.
</p>
</Alert>
)}
<Alert variant="secondary">
<Form className="text-body">
<p>
<strong>{reviewRequestEvent!.changedBy!.email}</strong> requested to{" "}
{actionDescription} this experiment.
{actionDescription}.
</p>

<div className="d-flex bd-highlight">
Expand All @@ -54,7 +54,7 @@ const FormApproveOrReject = ({
disabled={isLoading}
onClick={onApprove}
>
Approve and {ucActionDescription}
Approve and {actionButtonTitle}
</Button>
<Button
data-testid="reject-request"
Expand Down
Expand Up @@ -15,10 +15,12 @@ type RejectReasonFieldNames = typeof rejectReasonFieldNames[number];

const FormRejectReason = ({
isLoading,
actionDescription,
onSubmit,
onCancel,
}: {
isLoading: boolean;
actionDescription: string;
onSubmit: (event: any, fields: { changelogMessage: string }) => void;
onCancel: () => void;
}) => {
Expand Down Expand Up @@ -49,8 +51,10 @@ const FormRejectReason = ({
<FormProvider {...formMethods}>
<Form className="text-body">
<p>
<strong>You are rejecting this review.</strong> Please add some
comments:
<strong>
You are rejecting the request to {actionDescription}.
</strong>{" "}
Please add some comments:
</p>
<Form.Group controlId="changelogMessage">
<Form.Control
Expand Down
Expand Up @@ -22,7 +22,7 @@ const FormRemoteSettingsPending = ({
<Form className="text-body">
<p>
<strong>Action required —</strong> Please review this change in Remote
Settings to {actionDescription} this experiment
Settings to {actionDescription}
</p>

<div className="d-flex bd-highlight">
Expand Down
@@ -0,0 +1,51 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

import React, { useMemo } from "react";
import Alert from "react-bootstrap/Alert";
import { LIFECYCLE_REVIEW_FLOWS } from "../../lib/constants";
import { humanDate } from "../../lib/dateUtils";
import { getExperiment_experimentBySlug } from "../../types/getExperiment";
import {
NimbusChangeLogOldStatus,
NimbusChangeLogOldStatusNext,
} from "../../types/globalTypes";

export const RejectionReason = ({
rejectionEvent,
}: {
rejectionEvent: getExperiment_experimentBySlug["rejection"];
}) => {
const { message, changedOn, changedBy, oldStatus, oldStatusNext } =
rejectionEvent!;

const rejectionActionDescription = useMemo(() => {
if (oldStatus === NimbusChangeLogOldStatus.LIVE) {
if (oldStatusNext === NimbusChangeLogOldStatusNext.LIVE) {
return LIFECYCLE_REVIEW_FLOWS.PAUSE.description;
}
return LIFECYCLE_REVIEW_FLOWS.END.description;
}
if (oldStatus === NimbusChangeLogOldStatus.DRAFT) {
return LIFECYCLE_REVIEW_FLOWS.LAUNCH.description;
}
}, [oldStatus, oldStatusNext]);

return (
<Alert variant="warning" data-testid="rejection-notice">
<div className="text-body">
<p className="mb-2">
The request to {rejectionActionDescription} was{" "}
<strong>Rejected</strong> due to:
</p>
<p className="mb-2">
{changedBy!.email} on {humanDate(changedOn!)}:
</p>
<p className="bg-white rounded border p-2 mb-0">{message}</p>
</div>
</Alert>
);
};

export default RejectionReason;
Expand Up @@ -111,7 +111,8 @@ export const ReviewTimedOutInRemoteSettingsUserCannotReview = storyWithProps(
export const FormApproveOrRejectStory = () => (
<FormApproveOrReject
{...{
actionDescription: "froblulate",
actionButtonTitle: "Frobulate",
actionDescription: "frobulate the thingy",
isLoading: false,
reviewRequestEvent: mockChangelog(),
timeoutEvent: mockChangelog("ghi@mozilla.com"),
Expand All @@ -125,6 +126,7 @@ export const FormRejectReasonStory = () => (
<FormRejectReason
{...{
isLoading: false,
actionDescription: "frobulate the thingy",
onSubmit: action("submit"),
onCancel: action("cancel"),
}}
Expand All @@ -136,7 +138,7 @@ export const FormRemoteSettingsPendingStory = () => (
{...{
isLoading: false,
reviewUrl: REVIEW_URL,
actionDescription: "frobulate",
actionDescription: "frobulate the thingy",
}}
/>
);
Expand Up @@ -10,6 +10,11 @@ import {
within,
} from "@testing-library/react";
import React from "react";
import { LIFECYCLE_REVIEW_FLOWS } from "../../lib/constants";
import {
NimbusChangeLogOldStatus,
NimbusChangeLogOldStatusNext,
} from "../../types/globalTypes";
import {
BaseSubject,
reviewApprovedAfterTimeoutBaseProps,
Expand Down Expand Up @@ -166,26 +171,63 @@ describe("ChangeApprovalOperations", () => {
expect(rejectChange).not.toHaveBeenCalled();
});

it("reports a rejection reason when review is rejected", async () => {
const actionDescription = "gizmofy";
const { changedBy: rejectionUser, message: rejectionMessage } =
reviewRejectedBaseProps.rejectionEvent!;
render(
<Subject
{...{
...reviewRejectedBaseProps,
actionDescription,
}}
/>,
);
await screen.findByTestId("action-button");
await screen.findByText(
`The request to ${actionDescription} this experiment was`,
{ exact: false },
);
await screen.findByText(rejectionMessage!, { exact: false });
await screen.findByText(rejectionUser!.email, { exact: false });
});
const commonRejectionCase =
(
actionDescription: string,
oldStatus: NimbusChangeLogOldStatus,
oldStatusNext: NimbusChangeLogOldStatusNext,
) =>
async () => {
const rejectionEvent = {
...reviewRejectedBaseProps.rejectionEvent!,
oldStatus,
oldStatusNext,
};
const { changedBy: rejectionUser, message: rejectionMessage } =
rejectionEvent;
render(
<Subject
{...{
...reviewRejectedBaseProps,
rejectionEvent,
actionDescription,
}}
/>,
);
await screen.findByTestId("action-button");
await screen.findByText(`The request to ${actionDescription} was`, {
exact: false,
});
await screen.findByText(rejectionMessage!, { exact: false });
await screen.findByText(rejectionUser!.email, { exact: false });
};

it(
"reports a rejection reason when launch review is rejected",
commonRejectionCase(
LIFECYCLE_REVIEW_FLOWS.LAUNCH.description,
NimbusChangeLogOldStatus.DRAFT,
NimbusChangeLogOldStatusNext.LIVE,
),
);

it(
"reports a rejection reason when end enrollment review is rejected",
commonRejectionCase(
LIFECYCLE_REVIEW_FLOWS.PAUSE.description,
NimbusChangeLogOldStatus.LIVE,
NimbusChangeLogOldStatusNext.LIVE,
),
);

it(
"reports a rejection reason when end experiment review is rejected",
commonRejectionCase(
LIFECYCLE_REVIEW_FLOWS.END.description,
NimbusChangeLogOldStatus.LIVE,
NimbusChangeLogOldStatusNext.COMPLETE,
),
);

it("when user can review and review has timed out, a timeout notice is displayed", async () => {
render(
Expand Down

0 comments on commit 77081c0

Please sign in to comment.