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

lmorchard · 2021-06-26T00:19:28Z

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

lmorchard · 2021-06-26T00:29:29Z

Opening as a draft with work in progress because:

There's still work to be done around tests & coverage
I'll be out for another week after the wellness week and someone else might want to grab the baton from me

There's also possibly some some thinking to do about how manual enrollment end is handled on the backend, which could require some further tweaks.

For automated pause, we do not set is_paused on the Experimenter side first.

Instead, we set "isEnrollmentPaused" in the Remote Settings record. Then, we wait until it's approved in Remote Settings before setting is_paused=True in Experimenter. That means, on the Experimenter side, we never show that enrollment is paused until it's fully resolved on the Remote Settings side.

For manual pause, we might want to set is_paused=True on the Experimenter side first.

Otherwise, if we follow that same pattern as automated pause, then the only thing that represents a request to pause is status=LIVE / status_next=LIVE. (Because is_paused=False until the very end.)

Could also be a thing to punt to follow-ups, if/when we want other status=LIVE / status_next=LIVE updates other than enrollment ending.

lmorchard · 2021-06-26T00:42:27Z

Rebased against master to pick up the changes to PageSummary and approve / reject handlers

lmorchard · 2021-06-26T01:13:44Z

Looks like I got the storybook to build, so here are some interesting places to look:

Enrollment active (with new button) - https://storage.googleapis.com/mozilla-storybooks-experimenter/commits/07b3a25088c07c202b64c2ef25f48efe2587c6fa/nimbus-ui/index.html?path=/story/components-summary--enrollment-active
Enrollment ended (no button) - https://storage.googleapis.com/mozilla-storybooks-experimenter/commits/07b3a25088c07c202b64c2ef25f48efe2587c6fa/nimbus-ui/index.html?path=/story/components-summary--enrollment-ended
Lots of new launch, pause, and end stories - https://storage.googleapis.com/mozilla-storybooks-experimenter/commits/07b3a25088c07c202b64c2ef25f48efe2587c6fa/nimbus-ui/index.html?path=/story/pages-summary--live-update-pending-can-review

LZoog · 2021-07-07T21:55:17Z

Responding to Les' comment + leaving some notes for myself and the reviewer:

Per Jared's comment here, automatically triggered pausing will be removed in favor of requiring manual end enrollment.

Otherwise, if we follow that same pattern as automated pause, then the only thing that represents a request to pause is status=LIVE / status_next=LIVE. (Because is_paused=False until the very end.)

I think for now, this is OK since there's not any other statuses/flows that use this set and we won't need to differentiate between automatic or manual enrollment requests. That status set (which we're calling pauseRequested) does seem a little more fragile than say, endRequested, which checks for status=LIVE / status_next=COMPLETE and I could see that perhaps is_paused (isEnrollmentPaused) could potentially do a little more in the FE for us but we are using isEnrollmentPaused in conjunction with pauseRequested to conditionally show the EndEnrollment component. 🤔 FWIW we also aren't using it for end enrollment RS timeouts either since we just check for timeout on the experiment.

If we do have another flow later that needs status=LIVE / status_next=LIVE, we should be able to alter pauseRequested on the FE fairly easily. With all of this said, I think this PR mostly just needed some hard squinting at, test fixes and additions, warning fixes, and a couple follow up issues filed, all of which I've got underway. Just wanted to dump my thoughts here before finishing those up.

app/experimenter/experiments/models/nimbus.py

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

app/experimenter/nimbus-ui/src/components/Summary/index.tsx

LZoog · 2021-07-08T19:46:21Z

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

          changelogMessage: CHANGELOG_MESSAGES.REQUESTED_REVIEW_END,
+          status: NimbusExperimentStatus.LIVE,
          statusNext: NimbusExperimentStatus.COMPLETE,


FWIW we should end up using shared common props for flows when we do #5851. I didn't pull in the mocks from PageSummary since we probably need to pull those shared props into a separate file at a higher level which would require some more refactoring and make this PR even longer, plus that's what #5851 is for.

LZoog · 2021-07-09T14:21:30Z

To follow up here, yesterday I pushed FE test fixes and additions plus fixed some compilation warnings and made a couple of other tweaks, but left BE test fixes (5 fail) since we need to wait until there's a PR closing #5739.

I also already rebased/squashed the commits because of habit and I didn't realize until after pushing that this PR is probably going to stick around by the time @lmorchard gets back. 😅

app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/RejectionReason.tsx

lmorchard · 2021-07-13T00:36:51Z

Going to pick this back up tomorrow - PR #5935 will result in a totally different backend situation along with requiring we send is_paused={True,False} from the frontend.

So, some reworking will be required. But, hopefully not more than ripping out the backend changes in this PR and just updating the GQL queries on the frontend though.

lmorchard · 2021-07-13T23:25:07Z

Alright, I think this is ready for review now. Stripped out the defunct backend changes, added some to allow mutating isEnrollmentPaused / is_paused. Also tossed in a fix for #5927 as a quick bonus. The Co-authored-by might disqualify @LZoog from reviewing, since she helped carry this PR for the week I was out and wrapped up test coverage.

jaredlockhart · 2021-07-14T14:21:55Z

Clicking the End Experiment button sends an API request but doesn't update the UI. Refreshing the page kicked it over, but looks like we're missing something there.

Edit: Same behaviour for End Enrollment

jaredlockhart · 2021-07-14T14:23:19Z

Approve dialog missing some text for End Enrollment:

jaredlockhart · 2021-07-14T14:25:18Z

Clicking reject also seems to not update UI:

Edit: also looks like it's not firing an API request from looking at my server logs

jaredlockhart

Hit a bunch of snags up front, added some gifs. I'll stop testing there for now and look again when I can get further through the flow.

LZoog · 2021-07-14T14:59:50Z

Clicking the End Experiment button sends an API request but doesn't update the UI. Refreshing the page kicked it over, but looks like we're missing something there.

@lmorchard FWIW I think that's taken care of in #5939 if you rebase

lmorchard · 2021-07-14T17:27:50Z

I see what happened with the missing text & GQL query: I added an isEnrollmentPaused condition to status.pauseRequested and that broke things. Turns out isEnrollmentPaused is now derived from is_paused_published rather than is_paused, and that only turns true after review completion. Might need to add another property to represent is_paused vs is_paused_published

lmorchard · 2021-07-14T18:07:40Z

@lmorchard FWIW I think that's taken care of in #5939 if you rebase

Hmm, that only takes care of the end experiment UI, not this new UI. Need to copy that over / see if it can be more generalized

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>

lmorchard

This PR has become monstrous, but I think it's got some needed refactorings. Commenting on some additions that should fix the UI with respect text, loading indication, and using the correct GQL mutation for end enrollment

lmorchard · 2021-07-14T21:22:24Z

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

+    @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):


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

lmorchard · 2021-07-14T21:23:43Z

app/experimenter/nimbus-ui/src/hooks/useChangeOperationMutation.tsx

+              if (refetch) {
+                setIsLoadingRefetch(true);
+                await refetch();
+                setIsLoadingRefetch(false);
+              }


This seems to better ensure that the UI reflects that something's still in-flight for the refetch after the mutation

lmorchard · 2021-07-14T21:25:58Z

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

-    expect(isLoading).toBeFalsy();
-    await act(async () => void callback());
-    waitFor(() => expect(isLoading).toBeTruthy());
+  it("indicates when loading mutation", async () => {


Reworking these tests to use await waitForNextUpdate() seems to exercise the functionality better. But I suspect there might be an intermittent race condition in here still that I haven't been able to flush out

Circle reveals all.

lmorchard · 2021-07-14T21:28:13Z

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

-  const mocks = customMocks.length
-    ? customMocks
-    : mutationSets.reduce<MockedResponse[]>((acc, cur) => {


Seemed like customMocks was never used in these tests?

Huh, I must've been on some idea and then forgot to remove it. Thanks for catching!

lmorchard · 2021-07-14T21:30:12Z

app/experimenter/nimbus-ui/src/components/AppLayoutWithExperiment/index.tsx

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


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

lmorchard · 2021-07-14T21:49:11Z

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

+    const refetchState = {
+      called: false,
+      resolve: null as null | (() => void),
+    };
+    const refetch = () => {
+      refetchState.called = true;
+      return new Promise(
+        (resolve) => (refetchState.resolve = () => resolve(null)),
+      );
+    };


This feels a little dirty, but seems like a decent way to predictably walk through the states around refetch.

lmorchard · 2021-07-14T21:54:13Z

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

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


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

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?

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

jaredlockhart · 2021-07-14T22:27:39Z

@lmorchard If you feel like there's refactors in here that are necessary but don't actually implement the new UI directly you could pull them out into separate more isolated PRs, review and land those first, and then rebase this PR to just contain the actual logic to implement the new UI. Up to you.

lmorchard · 2021-07-14T22:33:58Z

If you feel like there's refactors in here that are necessary but don't actually implement the new UI directly you could pull them out into separate more isolated PRs, review and land those first, and then rebase this PR to just contain the actual logic to implement the new UI. Up to you.

No, it's all pretty much inseparable at this point. The refactors were necessary to get the shared logic working between the now three review cases we have.

lmorchard · 2021-07-14T22:54:20Z

Filed #5960 to capture a refactor idea for useChangeOperationMutation and keep this PR from getting bigger.

jodyheavener

Stared at this for a bit and had a few thoughts, but nothing major. I was able to go through the flow from start to end (as well as rejections) and didn't get hung up on anything. 🥳

jodyheavener · 2021-07-15T13:28:10Z

app/experimenter/nimbus-ui/src/components/Summary/EndEnrollment.tsx

+          disabled={isLoading}
+          data-testid="end-enrollment-start"
+        >
+          End Enrollment for Experiment


Nit: "for Experiment" seems redundant?

Such looong button.

I can't find an issue (which I thought existed) but I think we've been pushing to be more verbose and explicit about these messages. Might be worth a follow-up issue to revisit though

jodyheavener · 2021-07-15T13:29:22Z

app/experimenter/nimbus-ui/src/components/Summary/EndEnrollment.tsx

+  );
+
+  return (
+    <div className="mb-4" data-testid="enrollment-end">


I assume this is not easily doable due to the layout complexity of the other states once you click the button, but is it possible to place the "End Experiment" and "End Enrollment for Experiment" buttons inline?

Could be worth a follow-up issue, e.g. to combine these two buttons into a component side-by-side with state to display the confirmation form mutually exclusively for one or the other. Seems a decent enough chunk of work to defer and avoid making this PR bigger

jodyheavener · 2021-07-15T13:36:43Z

app/experimenter/nimbus-ui/src/components/Summary/EndEnrollment.tsx

+
+  return (
+    <div className="mb-4" data-testid="enrollment-end">
+      {showEndConfirmation ? (


I can enter into both of these states at the same time (though, when one is confirmed the other does disappear). Should we hide the other button when one is clicked?

Bonus rant: this is maybe more of a design/product thought, but these two flows have considerably different outcomes while having very similar-looking UI. I know we have the RS safeguard in place, but I wonder if we can do better to distinguish these two flows. "End experiment" almost feels like a red or yellow button to me now, instead of blue.

Yeah good point, interesting thought. You're right that they're pretty heavily guarded by the multiple review steps. I'd say leave it and if it starts tripping people up we can revisit it, but good thing to point out. 👍

jodyheavener · 2021-07-15T13:56:00Z

app/experimenter/nimbus-ui/src/components/PageSummary/index.tsx

+          actionDescription,
+          isLoading,
+          status,
+          // TODO: refactor to just take `experiment` rather than all these separate props?


+1 for Context

jodyheavener · 2021-07-15T13:59:10Z

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

-  const mocks = customMocks.length
-    ? customMocks
-    : mutationSets.reduce<MockedResponse[]>((acc, cur) => {


Huh, I must've been on some idea and then forgot to remove it. Thanks for catching!

jodyheavener · 2021-07-15T14:05:03Z

app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.tsx

  actionDescription: string;
  isLoading: boolean;
  canReview: boolean;
+  // TODO: refactor to just take `experiment` rather than all these separate props?


Somewhat ironically, if this took experiment (or we were using Context) we also wouldn't need to pass in invalidPages or InvalidPageList since the component could itself just use useReviewCheck.

Yeah, I keep thinking some kind of Context with experiment could be a good refactor, maybe also take the place of that functional child pattern we use for all the experiment-using pages

jodyheavener · 2021-07-15T14:13:51Z

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

+  PAUSE: {
+    buttonTitle: "End Enrollment for Experiment",
+    description: "end enrollment for this experiment",
+    requestSummary: "Requested End Enrollment",


Nit: I might phrase these as "X Requested", but def feel free to ignore :P

jaredlockhart · 2021-07-15T18:18:53Z

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

+    // TODO: EXP-1325 Need to check something else here for end enrollment in particular?
+    pauseRequested:
+      status === NimbusExperimentStatus.LIVE &&
+      statusNext === NimbusExperimentStatus.LIVE &&
+      isEnrollmentPausePending === true,


The only LIVE>LIVE update we have right now is pausing so that should be sufficient to know we're in a pausing flow?

Wanted to be specific here and only catch the pending end enrollment case. It's the only update right now, but didn't seem like a lot to make it specific to pausing

Oh I see what you mean. I think we'll end up having to do a lot more infrastructure before we do any non pause live updates, so we shouldn't really need to worry about it for now.

jaredlockhart

Tested locally, everything went off without a hitch. Looks great, works great, thanks @lmorchard ! 🎉 🎉 🎉 🎉

jaredlockhart · 2021-07-15T19:24:06Z

app/experimenter/nimbus-ui/src/components/Summary/EndEnrollment.tsx

+
+  return (
+    <div className="mb-4" data-testid="enrollment-end">
+      {showEndConfirmation ? (


Yeah good point, interesting thought. You're right that they're pretty heavily guarded by the multiple review steps. I'd say leave it and if it starts tripping people up we can revisit it, but good thing to point out. 👍

jaredlockhart · 2021-07-15T19:25:06Z

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

-    expect(isLoading).toBeFalsy();
-    await act(async () => void callback());
-    waitFor(() => expect(isLoading).toBeTruthy());
+  it("indicates when loading mutation", async () => {


Circle reveals all.

lmorchard · 2021-07-15T20:50:57Z

Alright, I'm gonna hit the merge button to get this monster landed, then file a few follow up issues that are hopefully quick maintenance sized things

lmorchard · 2021-07-15T21:03:19Z

Tried to capture some of @jodyheavener's feedback in follow ups:

Refining the end action buttons - Refine End Experiment / End Enrollment action button UX #5970
Thinking about Context to pass experiment around - Use React Context for fetched Experiment, rather than function-as-child-component? #5971

lmorchard force-pushed the 5742-end-enrollment-ui branch from d955415 to d3000aa Compare June 26, 2021 00:41

lmorchard force-pushed the 5742-end-enrollment-ui branch from d3000aa to 07b3a25 Compare June 26, 2021 00:58

LZoog force-pushed the 5742-end-enrollment-ui branch from 07b3a25 to 67ad88d Compare July 8, 2021 19:14

LZoog reviewed Jul 8, 2021

View reviewed changes

app/experimenter/experiments/models/nimbus.py Outdated Show resolved Hide resolved

LZoog reviewed Jul 8, 2021

View reviewed changes

app/experimenter/nimbus-ui/src/components/PageSummary/index.test.tsx Show resolved Hide resolved

LZoog reviewed Jul 8, 2021

View reviewed changes

app/experimenter/nimbus-ui/src/components/Summary/index.tsx Outdated Show resolved Hide resolved

LZoog reviewed Jul 8, 2021

View reviewed changes

LZoog reviewed Jul 12, 2021

View reviewed changes

app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/RejectionReason.tsx Show resolved Hide resolved

lmorchard force-pushed the 5742-end-enrollment-ui branch 2 times, most recently from 77081c0 to 4f32b39 Compare July 13, 2021 22:51

lmorchard marked this pull request as ready for review July 13, 2021 23:22

lmorchard requested review from jodyheavener and jaredlockhart July 13, 2021 23:22

jaredlockhart requested changes Jul 14, 2021

View reviewed changes

lmorchard force-pushed the 5742-end-enrollment-ui branch from 4f32b39 to ca60a06 Compare July 14, 2021 16:44

lmorchard force-pushed the 5742-end-enrollment-ui branch 2 times, most recently from cb5f741 to f408be1 Compare July 14, 2021 20:56

lmorchard force-pushed the 5742-end-enrollment-ui branch from f408be1 to d9052d8 Compare July 14, 2021 21:47

lmorchard commented Jul 14, 2021

View reviewed changes

lmorchard requested review from jaredlockhart and LZoog July 14, 2021 21:55

lmorchard mentioned this pull request Jul 14, 2021

Refactor useChangeOperationMutation hook to better organize named mutation callbacks? #5960

Closed

jodyheavener reviewed Jul 15, 2021

View reviewed changes

jaredlockhart reviewed Jul 15, 2021

View reviewed changes

jaredlockhart approved these changes Jul 15, 2021

View reviewed changes

lmorchard merged commit 26ead8a into main Jul 15, 2021

lmorchard deleted the 5742-end-enrollment-ui branch July 15, 2021 20:51

lmorchard mentioned this pull request Jul 15, 2021

Refine End Experiment / End Enrollment action button UX #5970

Closed

Mardak mentioned this pull request Sep 9, 2021

Ending enrollment with request and review seems unnecessarily complicated #6402

Closed

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

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

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

Conversation

lmorchard commented Jun 26, 2021 • edited

lmorchard commented Jun 26, 2021

lmorchard commented Jun 26, 2021

lmorchard commented Jun 26, 2021

LZoog commented Jul 7, 2021

Choose a reason for hiding this comment

LZoog commented Jul 9, 2021 • edited

lmorchard commented Jul 13, 2021

lmorchard commented Jul 13, 2021

jaredlockhart commented Jul 14, 2021 • edited

jaredlockhart commented Jul 14, 2021

jaredlockhart commented Jul 14, 2021 • edited

jaredlockhart left a comment

Choose a reason for hiding this comment

LZoog commented Jul 14, 2021

lmorchard commented Jul 14, 2021 • edited

lmorchard commented Jul 14, 2021

lmorchard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredlockhart commented Jul 14, 2021

lmorchard commented Jul 14, 2021 • edited

lmorchard commented Jul 14, 2021 • edited

jodyheavener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard Jul 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard Jul 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredlockhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard commented Jul 15, 2021

lmorchard commented Jul 15, 2021

lmorchard commented Jun 26, 2021 •

edited

LZoog commented Jul 9, 2021 •

edited

jaredlockhart commented Jul 14, 2021 •

edited

jaredlockhart commented Jul 14, 2021 •

edited

lmorchard commented Jul 14, 2021 •

edited

lmorchard commented Jul 14, 2021 •

edited

lmorchard commented Jul 14, 2021 •

edited

lmorchard Jul 15, 2021 •

edited

lmorchard Jul 15, 2021 •

edited