fixes #5834 fix(nimbus): Fix review url (#5835)

2021-06-24 15:26:14 -04:00 · 2021-06-24 15:26:14 -04:00 · ea09a4170f
--- a/app/experimenter/experiments/models/nimbus.py
+++ b/app/experimenter/experiments/models/nimbus.py
@ -331,7 +331,7 @@ class NimbusExperiment(NimbusConstants, FilterMixin, models.Model):

    @property
    def review_url(self):
-        return "{base_url}/{collection_path}/{collection}/{review_path}".format(
+        return "{base_url}{collection_path}/{collection}/{review_path}".format(
            base_url=settings.KINTO_ADMIN_URL,
            collection_path="#/buckets/main-workspace/collections",
            collection=self.application_config.rs_experiments_collection,
--- a/app/experimenter/experiments/tests/test_models/test_models_nimbus.py
+++ b/app/experimenter/experiments/tests/test_models/test_models_nimbus.py
@ -503,12 +503,21 @@ class TestNimbusExperiment(TestCase):
    def test_review_url_stage_should_return_simple_review_url(self):
        with override_settings(
            IS_STAGING=True,
-            KINTO_ADMIN_URL="https://settings-writer.stage.mozaws.net/v1/admin",
+            KINTO_ADMIN_URL="https://settings-writer.stage.mozaws.net/v1/admin/",
        ):
-            expected = (
-                "https://settings-writer.stage.mozaws.net/v1/admin/#/buckets"
-                "/main-workspace/collections/nimbus-desktop-experiments/simple-review"
+            expected = "https://settings-writer.stage.mozaws.net/v1/admin/#/buckets/main-workspace/collections/nimbus-desktop-experiments/simple-review"  # noqa E501
+            experiment = NimbusExperimentFactory.create_with_lifecycle(
+                NimbusExperimentFactory.Lifecycles.LAUNCH_APPROVE,
+                application=NimbusExperiment.Application.DESKTOP,
            )
+            self.assertEqual(experiment.review_url, expected)
+
+    def test_review_url_stage_should_return_simple_review_url_without_slash(self):
+        with override_settings(
+            IS_STAGING=True,
+            KINTO_ADMIN_URL="http://localhost:8888/v1/admin",
+        ):
+            expected = "http://localhost:8888/v1/admin#/buckets/main-workspace/collections/nimbus-desktop-experiments/simple-review"  # noqa E501
            experiment = NimbusExperimentFactory.create_with_lifecycle(
                NimbusExperimentFactory.Lifecycles.LAUNCH_APPROVE,
                application=NimbusExperiment.Application.DESKTOP,
@ -520,10 +529,7 @@ class TestNimbusExperiment(TestCase):
            IS_STAGING=False,
            KINTO_ADMIN_URL="https://settings-writer.prod.mozaws.net/v1/admin",
        ):
-            expected = (
-                "https://settings-writer.prod.mozaws.net/v1/admin/#/buckets"
-                "/main-workspace/collections/nimbus-mobile-experiments/records"
-            )
+            expected = "https://settings-writer.prod.mozaws.net/v1/admin#/buckets/main-workspace/collections/nimbus-mobile-experiments/records"  # noqa E501
            experiment = NimbusExperimentFactory.create_with_lifecycle(
                NimbusExperimentFactory.Lifecycles.LAUNCH_APPROVE,
                application=NimbusExperiment.Application.FENIX,
--- a/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/FormRemoteSettingsPending.tsx
+++ b/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/FormRemoteSettingsPending.tsx
@ -2,18 +2,19 @@
 * 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 classNames from "classnames";
 import React from "react";
 import Alert from "react-bootstrap/Alert";
-import Button from "react-bootstrap/Button";
 import Form from "react-bootstrap/Form";
+import LinkExternal from "../LinkExternal";

-const FormApproveConfirm = ({
+const FormRemoteSettingsPending = ({
  isLoading,
-  onConfirm,
+  reviewUrl,
  actionDescription,
 }: {
  isLoading: boolean;
-  onConfirm: () => void;
+  reviewUrl: string;
  actionDescription: string;
 }) => {
  return (
@ -26,14 +27,16 @@ const FormApproveConfirm = ({

        <div className="d-flex bd-highlight">
          <div>
-            <Button
+            <LinkExternal
              data-testid="open-remote-settings"
-              className="mr-2 btn btn-primary"
-              disabled={isLoading}
-              onClick={onConfirm}
+              className={classNames(
+                "mr-2 btn btn-primary",
+                isLoading && "disabled",
+              )}
+              href={reviewUrl}
            >
              Open Remote Settings
-            </Button>
+            </LinkExternal>
          </div>
        </div>
      </Form>
@ -41,4 +44,4 @@ const FormApproveConfirm = ({
  );
 };

-export default FormApproveConfirm;
+export default FormRemoteSettingsPending;
--- a/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.stories.tsx
+++ b/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.stories.tsx
@ -15,19 +15,18 @@ import {
  reviewRejectedBaseProps,
  reviewRequestedBaseProps,
  reviewTimedOutBaseProps,
+  REVIEW_URL,
 } from "./mocks";

 const Subject = ({
  rejectChange = action("rejectChange"),
  approveChange = action("approveChange"),
-  startRemoteSettingsApproval = action("startRemoteSettingsApproval"),
  ...props
 }: React.ComponentProps<typeof BaseSubject>) => (
  <BaseSubject
    {...{
      rejectChange,
      approveChange,
-      startRemoteSettingsApproval,
      ...props,
    }}
  />
@ -136,7 +135,7 @@ export const FormRemoteSettingsPendingStory = () => (
  <FormRemoteSettingsPending
    {...{
      isLoading: false,
-      onConfirm: action("confirm"),
+      reviewUrl: REVIEW_URL,
      actionDescription: "frobulate",
    }}
  />
--- a/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.test.tsx
+++ b/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.test.tsx
@ -12,19 +12,18 @@ import {
  reviewRequestedAfterRejectionBaseProps,
  reviewRequestedBaseProps,
  reviewTimedOutBaseProps,
+  REVIEW_URL,
 } from "./mocks";

 const Subject = ({
  rejectChange = () => {},
  approveChange = () => {},
-  startRemoteSettingsApproval = () => {},
  ...props
 }: React.ComponentProps<typeof BaseSubject>) => (
  <BaseSubject
    {...{
      rejectChange,
      approveChange,
-      startRemoteSettingsApproval,
      ...props,
    }}
  />
@ -38,14 +37,12 @@ describe("ChangeApprovalOperations", () => {

  it("when user can review, supports approval and opening remote settings", async () => {
    const approveChange = jest.fn();
-    const startRemoteSettingsApproval = jest.fn();
    render(
      <Subject
        {...{
          ...reviewRequestedBaseProps,
          canReview: true,
          approveChange,
-          startRemoteSettingsApproval,
        }}
      />,
    );
@ -57,8 +54,7 @@ describe("ChangeApprovalOperations", () => {
    const openRemoteSettingsButton = await screen.findByTestId(
      "open-remote-settings",
    );
-    fireEvent.click(openRemoteSettingsButton);
-    expect(startRemoteSettingsApproval).toHaveBeenCalled();
+    expect(openRemoteSettingsButton).toHaveProperty("href", REVIEW_URL);
  });

  it("when user cannot review, an approval pending notice is displayed", async () => {
@ -76,31 +72,26 @@ describe("ChangeApprovalOperations", () => {
  });

  it("when user can review and review has been approved, button to open remote settings is offered", async () => {
-    const startRemoteSettingsApproval = jest.fn();
    render(
      <Subject
        {...{
          ...reviewPendingInRemoteSettingsBaseProps,
          canReview: true,
-          startRemoteSettingsApproval,
        }}
      />,
    );
    const openRemoteSettingsButton = await screen.findByTestId(
      "open-remote-settings",
    );
-    fireEvent.click(openRemoteSettingsButton);
-    expect(startRemoteSettingsApproval).toHaveBeenCalled();
+    expect(openRemoteSettingsButton).toHaveProperty("href", REVIEW_URL);
  });

  it("when user cannot review and review has been approved, an approval pending notice is displayed", async () => {
-    const startRemoteSettingsApproval = jest.fn();
    render(
      <Subject
        {...{
          ...reviewPendingInRemoteSettingsBaseProps,
          canReview: false,
-          startRemoteSettingsApproval,
        }}
      />,
    );
--- a/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.tsx
+++ b/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/index.tsx
@ -30,7 +30,7 @@ export type ChangeApprovalOperationsProps = {
  timeoutEvent?: getExperiment_experimentBySlug["timeout"];
  rejectChange: (fields: { changelogMessage: string }) => void;
  approveChange: () => void;
-  startRemoteSettingsApproval: () => void;
+  reviewUrl: string;
 };

 export const ChangeApprovalOperations: React.FC<
@ -45,7 +45,7 @@ export const ChangeApprovalOperations: React.FC<
  timeoutEvent,
  rejectChange,
  approveChange,
-  startRemoteSettingsApproval,
+  reviewUrl,
  children,
 }) => {
  const defaultUIState = useMemo(() => {
@ -114,7 +114,7 @@ export const ChangeApprovalOperations: React.FC<
        <FormRemoteSettingsPending
          {...{
            isLoading,
-            onConfirm: startRemoteSettingsApproval,
+            reviewUrl,
            actionDescription,
          }}
        />
--- a/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/mocks.tsx
+++ b/app/experimenter/nimbus-ui/src/components/ChangeApprovalOperations/mocks.tsx
@ -12,6 +12,9 @@ type BaseSubjectProps = Partial<
  React.ComponentProps<typeof ChangeApprovalOperations>
 >;

+export const REVIEW_URL =
+  "http://localhost:8888/v1/admin/#/buckets/main-workspace/collections/nimbus-mobile-experiments/records";
+
 export const BaseSubject = ({
  actionDescription = "frobulate",
  isLoading = false,
@ -22,7 +25,6 @@ export const BaseSubject = ({
  timeoutEvent,
  rejectChange = () => {},
  approveChange = () => {},
-  startRemoteSettingsApproval = () => {},
  ...props
 }: BaseSubjectProps) => (
  <ChangeApprovalOperations
@ -36,7 +38,7 @@ export const BaseSubject = ({
      timeoutEvent,
      rejectChange,
      approveChange,
-      startRemoteSettingsApproval,
+      reviewUrl: REVIEW_URL,
      ...props,
    }}
  >
--- a/app/experimenter/nimbus-ui/src/components/PageRequestReview/index.test.tsx
+++ b/app/experimenter/nimbus-ui/src/components/PageRequestReview/index.test.tsx
@ -238,13 +238,10 @@ describe("PageRequestReview", () => {
    const openRemoteSettingsButton = await screen.findByTestId(
      "open-remote-settings",
    );
-    fireEvent.click(openRemoteSettingsButton);
-    await waitFor(() => {
-      expect(mockWindowOpen).toHaveBeenCalledWith(
-        experiment.reviewUrl,
-        "_blank",
-      );
-    });
+    expect(openRemoteSettingsButton).toHaveProperty(
+      "href",
+      experiment.reviewUrl,
+    );
  });

  it("handles rejection of launch as expected", async () => {
--- a/app/experimenter/nimbus-ui/src/components/PageRequestReview/index.tsx
+++ b/app/experimenter/nimbus-ui/src/components/PageRequestReview/index.tsx
@ -49,14 +49,10 @@ const PageContent: React.FC<{
  experiment: getExperiment_experimentBySlug;
  refetch: () => void;
 }> = ({ experiment, refetch }) => {
-  const { reviewUrl } = experiment;
  const [showLaunchToReview, setShowLaunchToReview] = useState(false);
  const { invalidPages, InvalidPagesList } = useReviewCheck(experiment);

  const status = getStatus(experiment);
-  const startRemoteSettingsApproval = async () => {
-    window.open(reviewUrl!, "_blank");
-  };

  const {
    isLoading,
@ -132,7 +128,7 @@ const PageContent: React.FC<{
            timeoutEvent,
            rejectChange: onReviewRejectedClicked,
            approveChange: onReviewApprovedClicked,
-            startRemoteSettingsApproval,
+            reviewUrl: experiment.reviewUrl!,
          }}
        >
          {status.draft &&
--- a/app/experimenter/nimbus-ui/src/components/Summary/index.test.tsx
+++ b/app/experimenter/nimbus-ui/src/components/Summary/index.test.tsx
@ -158,13 +158,10 @@ describe("Summary", () => {
      const openRemoteSettingsButton = await screen.findByTestId(
        "open-remote-settings",
      );
-      fireEvent.click(openRemoteSettingsButton);
-      await waitFor(() => {
-        expect(mockWindowOpen).toHaveBeenCalledWith(
-          experiment.reviewUrl,
-          "_blank",
-        );
-      });
+      expect(openRemoteSettingsButton).toHaveProperty(
+        "href",
+        experiment.reviewUrl,
+      );
    });

    it("handles rejection of end as expected", async () => {
--- a/app/experimenter/nimbus-ui/src/components/Summary/index.tsx
+++ b/app/experimenter/nimbus-ui/src/components/Summary/index.tsx
@ -30,7 +30,6 @@ type SummaryProps = {
 } & Partial<React.ComponentProps<typeof ChangeApprovalOperations>>; // TODO EXP-1143: temporary page-level props, should be replaced by API data for experiment & current user

 const Summary = ({ experiment, refetch }: SummaryProps) => {
-  const { reviewUrl } = experiment;
  const status = getStatus(experiment);

  const {
@ -41,10 +40,6 @@ const Summary = ({ experiment, refetch }: SummaryProps) => {
    timeout: timeoutEvent,
  } = experiment;

-  const startRemoteSettingsApproval = async () => {
-    window.open(reviewUrl!, "_blank");
-  };
-
  const {
    isLoading,
    submitError,
@ -98,7 +93,7 @@ const Summary = ({ experiment, refetch }: SummaryProps) => {
            timeoutEvent,
            rejectChange: onReviewRejectedClicked,
            approveChange: onReviewApprovedClicked,
-            startRemoteSettingsApproval,
+            reviewUrl: experiment.reviewUrl!,
          }}
        >
          {experiment.statusNext !== NimbusExperimentStatus.COMPLETE && (