Skip to content

Commit

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

* we want to change ending enrollment from an automatic to a manual action

This commit:

* refactors the Settings page and ChangeApprovalOperations components

* tweak the DB queries and Celery tasks to handle manually-approved
  enrollment end

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

* refactors handling of strings describing lifecycle review flows, moves
  all the strings into shared constants
  • Loading branch information
lmorchard committed Jun 26, 2021
1 parent 3296058 commit 07b3a25
Show file tree
Hide file tree
Showing 24 changed files with 607 additions and 268 deletions.
7 changes: 3 additions & 4 deletions app/experimenter/experiments/models/nimbus.py
Expand Up @@ -34,9 +34,6 @@ def pause_queue(self, applications):
return self.filter(
NimbusExperiment.Filters.IS_PAUSE_QUEUED,
application__in=applications,
id__in=[
experiment.id for experiment in self.all() if experiment.should_pause
],
)

def end_queue(self, applications):
Expand Down Expand Up @@ -155,7 +152,9 @@ class Filters:
)
IS_PAUSE_QUEUED = Q(
status=NimbusConstants.Status.LIVE,
publish_status=NimbusConstants.PublishStatus.IDLE,
status_next=NimbusConstants.Status.LIVE,
publish_status=NimbusConstants.PublishStatus.APPROVED,
# TODO: EXP-1325 Need to check for something else here for end enrollment?
is_paused=False,
)
IS_PAUSING = Q(
Expand Down
11 changes: 1 addition & 10 deletions app/experimenter/kinto/tasks.py
Expand Up @@ -93,16 +93,7 @@ def handle_pending_review(applications, kinto_client):

if experiment:
if experiment.should_timeout:
if (
experiment.status == NimbusExperiment.Status.LIVE
and experiment.status_next == NimbusExperiment.Status.LIVE
):
# TODO EXP-1325 / EXP-1349: Temporarily handle automated updates to live
# experiment (i.e. pausing) as not reviewable, bounce back to idle.
experiment.status_next = None
experiment.publish_status = NimbusExperiment.PublishStatus.IDLE
else:
experiment.publish_status = NimbusExperiment.PublishStatus.REVIEW
experiment.publish_status = NimbusExperiment.PublishStatus.REVIEW
experiment.save()

generate_nimbus_changelog(
Expand Down
43 changes: 0 additions & 43 deletions app/experimenter/kinto/tests/test_tasks.py
Expand Up @@ -250,49 +250,6 @@ def test_check_with_timeout_end_review_and_queued_launch_rolls_back_and_pushes(
).exists()
)

# TODO EXP-1325 / EXP-1349: Temporarily handle automated updates to live
# experiment (i.e. pausing) as not reviewable, bounce back to idle.
@override_settings(KINTO_REVIEW_TIMEOUT=0)
def test_check_with_timeout_pause_rolls_back_to_idle_and_pushes(
self,
):
pending_experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.LIVE_ENROLLING_WAITING,
application=NimbusExperiment.Application.DESKTOP,
)
launching_experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.LAUNCH_APPROVE,
application=NimbusExperiment.Application.DESKTOP,
)

self.setup_kinto_pending_review()

tasks.nimbus_check_kinto_push_queue_by_collection(
settings.KINTO_COLLECTION_NIMBUS_DESKTOP
)

self.mock_push_task.assert_called_with(
settings.KINTO_COLLECTION_NIMBUS_DESKTOP, launching_experiment.id
)
self.mock_kinto_client.patch_collection.assert_called_with(
id=settings.KINTO_COLLECTION_NIMBUS_DESKTOP,
data={"status": "to-rollback"},
bucket="main-workspace",
)

pending_experiment = NimbusExperiment.objects.get(id=pending_experiment.id)
self.assertEqual(
pending_experiment.publish_status, NimbusExperiment.PublishStatus.IDLE
)
self.assertTrue(
pending_experiment.changes.filter(
old_status=NimbusExperiment.Status.LIVE,
old_publish_status=NimbusExperiment.PublishStatus.WAITING,
new_status=NimbusExperiment.Status.LIVE,
new_publish_status=NimbusExperiment.PublishStatus.IDLE,
).exists()
)

def test_check_with_rejected_launch_rolls_back_and_pushes(self):
rejected_experiment = NimbusExperimentFactory.create_with_lifecycle(
NimbusExperimentFactory.Lifecycles.LAUNCH_APPROVE_WAITING,
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 @@ -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;
}
}, [rejectionEvent]);

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 @@ -136,7 +137,7 @@ export const FormRemoteSettingsPendingStory = () => (
{...{
isLoading: false,
reviewUrl: REVIEW_URL,
actionDescription: "frobulate",
actionDescription: "frobulate the thingy",
}}
/>
);
Expand Up @@ -4,6 +4,11 @@

import { fireEvent, render, screen, waitFor } 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 @@ -150,26 +155,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 07b3a25

Please sign in to comment.