Skip to content

Commit

Permalink
fix #5742 fix #5927 feat(nimbus): add end enrollment UI (#5852)
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

* add support for isEnrollmentPausePending to indicate when the review
  is in progress

* rework useChangeOperationMutation to indicate loading while awaiting
  experiment refetch

* 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

Co-authored-by: Lauren Zugai <lauren@zugai.com>

Co-authored-by: Lauren Zugai <lauren@zugai.com>
  • Loading branch information
lmorchard and LZoog committed Jul 15, 2021
1 parent e3c1ae9 commit 26ead8a
Show file tree
Hide file tree
Showing 40 changed files with 1,087 additions and 313 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
4 changes: 4 additions & 0 deletions app/experimenter/experiments/api/v5/types.py
Expand Up @@ -185,6 +185,7 @@ class NimbusExperimentType(DjangoObjectType):
start_date = graphene.DateTime()
computed_end_date = graphene.DateTime()
is_enrollment_paused = graphene.Boolean()
is_enrollment_pause_pending = graphene.Boolean()
enrollment_end_date = graphene.DateTime()
computed_enrollment_days = graphene.Int()
computed_duration_days = graphene.Int()
Expand Down Expand Up @@ -222,6 +223,9 @@ def resolve_jexl_targeting_expression(self, info):
def resolve_is_enrollment_paused(self, info):
return self.is_paused_published

def resolve_is_enrollment_pause_pending(self, info):
return self.is_paused and not self.is_paused_published

def resolve_enrollment_end_date(self, info):
return self.proposed_enrollment_end_date

Expand Down
63 changes: 63 additions & 0 deletions app/experimenter/experiments/tests/api/v5/test_mutations.py
Expand Up @@ -668,3 +668,66 @@ 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)
self.assertEqual(experiment.is_paused_published, 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")

# is_paused set to True in local DB, but is_paused_published is not yet True
experiment = NimbusExperiment.objects.get(id=experiment.id)
self.assertEqual(experiment.is_paused, True)
self.assertEqual(experiment.is_paused_published, False)

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)
33 changes: 33 additions & 0 deletions app/experimenter/experiments/tests/api/v5/test_queries.py
Expand Up @@ -5,6 +5,7 @@
from django.urls import reverse
from graphene.utils.str_converters import to_snake_case
from graphene_django.utils.testing import GraphQLTestCase
from parameterized import parameterized

from experimenter.base.models import Country, Locale
from experimenter.experiments.api.v6.serializers import NimbusExperimentSerializer
Expand Down Expand Up @@ -745,6 +746,38 @@ def test_paused_experiment_returns_date(self):
self.assertEqual(experiment_data["isEnrollmentPaused"], True)
self.assertEqual(experiment_data["enrollmentEndDate"], "2021-01-08")

@parameterized.expand(
[
[NimbusExperimentFactory.Lifecycles.PAUSING_REVIEW_REQUESTED, False, True],
[NimbusExperimentFactory.Lifecycles.PAUSING_APPROVE, False, True],
[NimbusExperimentFactory.Lifecycles.PAUSING_APPROVE_WAITING, False, True],
[NimbusExperimentFactory.Lifecycles.PAUSING_APPROVE_TIMEOUT, False, True],
[NimbusExperimentFactory.Lifecycles.PAUSING_APPROVE_APPROVE, True, False],
[NimbusExperimentFactory.Lifecycles.PAUSING_REJECT, False, False],
[NimbusExperimentFactory.Lifecycles.PAUSING_APPROVE_REJECT, False, False],
]
)
def test_experiment_pause_pending(self, lifecycle, expected_paused, expected_pending):
user_email = "user@example.com"
experiment = NimbusExperimentFactory.create_with_lifecycle(lifecycle)
response = self.query(
"""
query experimentBySlug($slug: String!) {
experimentBySlug(slug: $slug) {
isEnrollmentPaused
isEnrollmentPausePending
}
}
""",
variables={"slug": experiment.slug},
headers={settings.OPENIDC_EMAIL_HEADER: user_email},
)
self.assertEqual(response.status_code, 200, response.content)
content = json.loads(response.content)
experiment_data = content["data"]["experimentBySlug"]
self.assertEqual(experiment_data["isEnrollmentPaused"], expected_paused)
self.assertEqual(experiment_data["isEnrollmentPausePending"], expected_pending)

def test_signoff_recommendations(self):
user_email = "user@example.com"
experiment = NimbusExperimentFactory.create_with_lifecycle(
Expand Down
10 changes: 8 additions & 2 deletions app/experimenter/experiments/tests/factories/nimbus.py
Expand Up @@ -137,9 +137,12 @@ class Lifecycles(Enum):
LAUNCH_APPROVE_TIMEOUT = LAUNCH_APPROVE_WAITING + (LifecycleStates.DRAFT_REVIEW,)
LIVE_ENROLLING = LAUNCH_APPROVE_APPROVE + (LifecycleStates.LIVE_IDLE_ENROLLING,)
PAUSING_REVIEW_REQUESTED = LIVE_ENROLLING + (LifecycleStates.LIVE_REVIEW_PAUSING,)
PAUSING_REJECT = PAUSING_REVIEW_REQUESTED + (
LifecycleStates.LIVE_IDLE_REJECT_PAUSING,
)
PAUSING_APPROVE = PAUSING_REVIEW_REQUESTED + (LifecycleStates.LIVE_APPROVED_PAUSING,)
PAUSING_APPROVE_WAITING = PAUSING_APPROVE + (LifecycleStates.LIVE_WAITING_PAUSING,)
PAUSING_APPROVE_APPROVE = PAUSING_APPROVE_WAITING + (LifecycleStates.COMPLETE_IDLE,)
PAUSING_APPROVE_APPROVE = PAUSING_APPROVE_WAITING + (LifecycleStates.LIVE_IDLE,)
PAUSING_APPROVE_REJECT = PAUSING_APPROVE_WAITING + (
LifecycleStates.LIVE_IDLE_REJECT_PAUSING,
)
Expand Down Expand Up @@ -290,7 +293,10 @@ def create_with_lifecycle(cls, lifecycle, with_random_timespan=False, **kwargs):
for state in lifecycle.value:
experiment.apply_lifecycle_state(state)

if experiment.status == experiment.Status.LIVE:
if (
experiment.status == experiment.Status.LIVE
and experiment.status_next is None
):
experiment.published_dto = NimbusExperimentSerializer(experiment).data

experiment.save()
Expand Down
2 changes: 2 additions & 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 @@ -345,6 +346,7 @@ type NimbusExperimentType {
startDate: DateTime
computedEndDate: DateTime
isEnrollmentPaused: Boolean
isEnrollmentPausePending: Boolean
enrollmentEndDate: DateTime
computedEnrollmentDays: Int
computedDurationDays: Int
Expand Down
Expand Up @@ -18,7 +18,7 @@ import PageLoading from "../PageLoading";

type AppLayoutWithExperimentChildrenProps = {
experiment: getExperiment_experimentBySlug;
refetch: () => void;
refetch: () => Promise<unknown>;
analysis?: AnalysisData;
};

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",
}}
/>
);

0 comments on commit 26ead8a

Please sign in to comment.