Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #5742 feat(nimbus): implement UI for manual enrollment end #5852

Merged
merged 1 commit into from Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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

Comment on lines +226 to +228
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This combined with status=LIVE / status_next = LIVE should give us all the hints needed on the frontend

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait explain this. Shouldn't you just be able to infer all this based on publish_status? Why do you need to check the difference between the pause and published pause state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't infer specifically that this is a pending change to end enrollment, and I wanted to be specific #5852 (comment)

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):
Comment on lines +749 to +760
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured I might as well try exhausting all the cases here to make sure the pending vs published pause states worked as expected.

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;
};
Comment on lines 19 to 23
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where else we're using refetch - this change doesn't seem to have broken any other tests, but we might need to update this type elsewhere eventually. Technically the refetch from Apollo is an async function and not () => void, but we didn't have that plumbed through for use in useChangeOperationMutation


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:
lmorchard marked this conversation as resolved.
Show resolved Hide resolved
</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",
}}
/>
);