From 46e6a3fcd1fc3c948602bf5f702893cd7fc1ea5a Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 18 May 2022 16:08:40 -0400 Subject: [PATCH 1/7] add PublishingOption component replace two more constants with enums --- static/js/components/PublishDrawer.test.tsx | 57 +++-- static/js/components/PublishDrawer.tsx | 242 ++++++++++-------- .../js/components/forms/PublishForm.test.tsx | 11 +- static/js/components/forms/PublishForm.tsx | 6 +- static/js/constants.ts | 8 +- static/js/query-configs/websites.ts | 29 ++- 6 files changed, 189 insertions(+), 164 deletions(-) diff --git a/static/js/components/PublishDrawer.test.tsx b/static/js/components/PublishDrawer.test.tsx index a0f3e54e7..68cbd4dae 100644 --- a/static/js/components/PublishDrawer.test.tsx +++ b/static/js/components/PublishDrawer.test.tsx @@ -109,13 +109,13 @@ describe("PublishDrawer", () => { it("renders the date and url", async () => { const { wrapper } = await render() await act(async () => { - // @ts-ignore + // @ts-expect-error wrapper .find("input[type='radio']") - // @ts-ignore + // @ts-expect-error .at(idx) - // @ts-ignore - .prop("onChange")() + // @ts-expect-error + .prop("onChange")({ target: { checked: true } }) }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( @@ -137,13 +137,13 @@ describe("PublishDrawer", () => { it("renders the publish status", async () => { const { wrapper } = await render() await act(async () => { - // @ts-ignore + // @ts-expect-error wrapper .find("input[type='radio']") - // @ts-ignore + // @ts-expect-error .at(idx) - // @ts-ignore - .prop("onChange")() + // @ts-expect-error + .prop("onChange")({ target: { checked: true } }) }) wrapper.update() expect(wrapper.find("PublishStatusIndicator").prop("status")).toBe( @@ -161,13 +161,13 @@ describe("PublishDrawer", () => { website[publishDateField] = null const { wrapper } = await render() await act(async () => { - // @ts-ignore + // @ts-expect-error wrapper .find("input[type='radio']") - // @ts-ignore + // @ts-expect-error .at(idx) - // @ts-ignore - .prop("onChange")() + // @ts-expect-error + .prop("onChange")({ target: { checked: true } }) }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( @@ -178,20 +178,19 @@ describe("PublishDrawer", () => { it("has an option with the right label", async () => { const { wrapper } = await render() await act(async () => { - // @ts-ignore + // @ts-expect-error wrapper .find("input[type='radio']") - // @ts-ignore + // @ts-expect-error .at(idx) - // @ts-ignore - .prop("onChange")() + // @ts-expect-error + .prop("onChange")({ target: { checked: true } }) }) wrapper.update() - // @ts-ignore expect( wrapper .find(".publish-option label") - // @ts-ignore + // @ts-expect-error .at(idx) .text() ).toBe(label) @@ -201,8 +200,9 @@ describe("PublishDrawer", () => { website[unpublishedField] = false const { wrapper } = await render() await act(async () => { - // @ts-ignore - wrapper.find(`#publish-${action}`).prop("onChange")() + const onChange = wrapper.find(`#publish-${action}`).prop("onChange") + // @ts-expect-error + onChange(({target: { checked: true }})) }) wrapper.update() expect(wrapper.find(".btn-publish").prop("disabled")).toBe(true) @@ -220,8 +220,9 @@ describe("PublishDrawer", () => { website[unpublishedField] = true const { wrapper } = await render() await act(async () => { - // @ts-ignore - wrapper.find(`#publish-${action}`).prop("onChange")() + const onChange = wrapper.find(`#publish-${action}`).prop("onChange") + // @ts-expect-error + onChange(({target: { checked: true }})) }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( @@ -255,12 +256,12 @@ describe("PublishDrawer", () => { ) const { wrapper } = await render() await act(async () => { - // @ts-ignore - wrapper.find(`#publish-${action}`).prop("onChange")() + const onChange = wrapper.find(`#publish-${action}`).prop("onChange") + // @ts-expect-error + onChange(({target: { checked: true }})) }) wrapper.update() await act(async () => { - // @ts-ignore wrapper .find("PublishForm") .find(".btn-publish") @@ -300,8 +301,9 @@ describe("PublishDrawer", () => { ) const { wrapper } = await render() await act(async () => { - // @ts-ignore - wrapper.find(`#publish-${action}`).prop("onChange")() + const onChange = wrapper.find(`#publish-${action}`).prop("onChange") + // @ts-expect-error + onChange(({target: { checked: true }})) }) wrapper.update() expect( @@ -311,7 +313,6 @@ describe("PublishDrawer", () => { .prop("disabled") ).toBeFalsy() await act(async () => { - // @ts-ignore wrapper .find("PublishForm") .find(".btn-publish") diff --git a/static/js/components/PublishDrawer.tsx b/static/js/components/PublishDrawer.tsx index bf7dc8fbf..ab6b210cf 100644 --- a/static/js/components/PublishDrawer.tsx +++ b/static/js/components/PublishDrawer.tsx @@ -1,13 +1,12 @@ import React, { useState } from "react" import { Modal, ModalBody, ModalHeader } from "reactstrap" -import { useStore } from "react-redux" import { useMutation } from "redux-query-react" import { requestAsync } from "redux-query" import moment from "moment" import { isEmpty } from "ramda" import { - websiteAction, + websitePublishAction, websiteDetailRequest, WebsitePublishPayload } from "../query-configs/websites" @@ -16,136 +15,147 @@ import PublishStatusIndicator from "./PublishStatusIndicator" import { Website } from "../types/websites" import { PublishForm } from "./forms/PublishForm" -import { PUBLISH_OPTION_PRODUCTION, PUBLISH_OPTION_STAGING } from "../constants" +import { PublishingEnv, PublishStatus } from "../constants" +import { useAppDispatch } from "../hooks/redux" -type Props = { - visibility: boolean - toggleVisibility: () => void +interface PublishingOptionProps { website: Website + publishingEnv: PublishingEnv + selected: boolean + onSelect: (publishingEnv: PublishingEnv) => void + onPublishSuccess: () => void +} +interface PublishingInfo { + date: string | null + status: PublishStatus | null + hasUnpublishedChanges: boolean + envLabel: string } -export default function PublishDrawer(props: Props): JSX.Element { - const { visibility, toggleVisibility, website } = props - const [ - { isPending: previewIsPending }, - previewWebsite - ] = useMutation((payload: WebsitePublishPayload) => - websiteAction(website.name, "preview", payload) - ) +const getPublishingInfo = ( + website: Website, + publishingEnv: PublishingEnv +): PublishingInfo => { + if (publishingEnv === PublishingEnv.Staging) { + return { + envLabel: "Staging", + date: website.draft_publish_date, + status: website.draft_publish_status, + hasUnpublishedChanges: website.has_unpublished_draft + } + } + if (publishingEnv === PublishingEnv.Production) { + return { + envLabel: "Production", + date: website.publish_date, + status: website.live_publish_status, + hasUnpublishedChanges: website.has_unpublished_live + } + } + throw new Error("Invalid PublishingEnv") +} + +const PublishingOption: React.FC = props => { + const { publishingEnv, selected, onSelect, website, onPublishSuccess } = props + const publishingInfo = getPublishingInfo(website, publishingEnv) + const [error, setError] = useState>(null) const [ - { isPending: publishIsPending }, - publishWebsite + { isPending }, + publish ] = useMutation((payload: WebsitePublishPayload) => - websiteAction(website.name, "publish", payload) - ) - const store = useStore() - const [publishOption, setPublishOption] = useState( - PUBLISH_OPTION_STAGING + websitePublishAction(website.name, publishingEnv, payload) ) - const [errorStaging, setErrorStaging] = useState(false) - const [errorProduction, setErrorProduction] = useState(false) - const onPreview = async (payload: WebsitePublishPayload) => { - setErrorStaging(false) - if (previewIsPending) { + const handlePublish = async (payload: WebsitePublishPayload) => { + setError(null) + if (isPending) { return } - const response = await previewWebsite(payload) + const response = await publish(payload) if (!response) { return } else { if (isErrorStatusCode(response.status)) { - setErrorStaging(true) + setError(response.body) } else { - // refresh - toggleVisibility() - await store.dispatch(requestAsync(websiteDetailRequest(website.name))) + onPublishSuccess() } } } - const onPublish = async (payload: WebsitePublishPayload) => { - setErrorProduction(false) - if (publishIsPending) { - return - } - const response = await publishWebsite(payload) - if (!response) { - return - } else { - if (isErrorStatusCode(response.status)) { - setErrorProduction(true) - } else { - // refresh - toggleVisibility() - store.dispatch(requestAsync(websiteDetailRequest(website.name))) - } - } - } + return ( +
+
+ { + if (e.target.checked) { + onSelect(publishingEnv) + } + }} + />{" "} + +
+ {selected && ( +
+ Last updated:{" "} + {publishingInfo.date ? + moment(publishingInfo.date).format("dddd, MMMM D h:mma ZZ") : + "never published"} +
+ {publishingInfo.hasUnpublishedChanges && ( + <> + You have unpublished changes. +
+ + )} + {error && ( + <> + + We apologize, there was an error publishing the site. Please try + again in a few minutes. + +
+ + )} + + +
+ )} +
+ ) +} - const renderOption = (option: string) => { - const isStaging = option === PUBLISH_OPTION_STAGING - const label = isStaging ? "Staging" : "Production" - const publish = isStaging ? onPreview : onPublish - const error = isStaging ? errorStaging : errorProduction - const publishDate = isStaging ? - website.draft_publish_date : - website.publish_date - const hasUnpublishedChanges = isStaging ? - website.has_unpublished_draft : - website.has_unpublished_live - const publishStatus = isStaging ? - website.draft_publish_status : - website.live_publish_status +type Props = { + visibility: boolean + toggleVisibility: () => void + website: Website +} +export default function PublishDrawer(props: Props): JSX.Element { + const { visibility, toggleVisibility, website } = props + const dispatch = useAppDispatch() + const [selectedEnv, setSelectedEnv] = useState(PublishingEnv.Staging) - return ( -
-
- setPublishOption(option)} - />{" "} - -
- {publishOption === option ? ( -
- Last updated:{" "} - {publishDate ? - moment(publishDate).format("dddd, MMMM D h:mma ZZ") : - "never published"} -
- {hasUnpublishedChanges ? ( - <> - You have unpublished changes. -
- - ) : null} - {error ? ( - <> - - We apologize, there was an error publishing the site. Please - try again in a few minutes. - -
- - ) : null} - - -
- ) : null} -
- ) + const onPublishSuccess = async () => { + toggleVisibility() + await dispatch(requestAsync(websiteDetailRequest(website.name))) } + const userEnvs = website.is_admin ? + [PublishingEnv.Staging, PublishingEnv.Production] : + [PublishingEnv.Staging] + return ( Publish your site - {renderOption(PUBLISH_OPTION_STAGING)} - {website.is_admin ? renderOption(PUBLISH_OPTION_PRODUCTION) : null} - {website.content_warnings && !isEmpty(website.content_warnings) ? ( + {userEnvs.map(publishingEnv => ( + + ))} + {website.content_warnings && !isEmpty(website.content_warnings) && (
This site has issues that could affect publishing output. @@ -167,7 +185,7 @@ export default function PublishDrawer(props: Props): JSX.Element { ))}
- ) : null} + )} ) diff --git a/static/js/components/forms/PublishForm.test.tsx b/static/js/components/forms/PublishForm.test.tsx index 8d30c4d37..fb835fff6 100644 --- a/static/js/components/forms/PublishForm.test.tsx +++ b/static/js/components/forms/PublishForm.test.tsx @@ -5,8 +5,7 @@ import { ValidationError } from "yup" import { PublishForm, websiteUrlValidation } from "./PublishForm" import { - PUBLISH_OPTION_PRODUCTION, - PUBLISH_OPTION_STAGING + PublishingEnv } from "../../constants" import { assertInstanceOf, defaultFormikChildProps } from "../../test_util" import { makeWebsiteDetail } from "../../util/factories/websites" @@ -21,7 +20,7 @@ describe("PublishForm", () => { onSubmit={onSubmitStub} disabled={false} website={website} - option={PUBLISH_OPTION_STAGING} + option={PublishingEnv.Staging} {...props} /> ) @@ -83,7 +82,7 @@ describe("PublishForm", () => { ).toBeTruthy() }) - it.each([PUBLISH_OPTION_STAGING, PUBLISH_OPTION_PRODUCTION])( + it.each([PublishingEnv.Staging, PublishingEnv.Production])( "shows a URL link instead of a field if website is published", option => { website.publish_date = "2020-01-01" @@ -99,7 +98,7 @@ describe("PublishForm", () => { .exists() ).toBeFalsy() expect(form.find("a").prop("href")).toEqual( - option === PUBLISH_OPTION_STAGING ? website.draft_url : website.live_url + option === PublishingEnv.Staging ? website.draft_url : website.live_url ) } ) @@ -109,7 +108,7 @@ describe("PublishForm", () => { website.url_path = "courses/my-url-fall-2028" const form = renderInnerForm( { isSubmitting: false, status: "whatever" }, - { option: PUBLISH_OPTION_PRODUCTION } + { option: PublishingEnv.Production } ) //expect(form.find("a").exists()).toBeFalsy() expect(form.find("span").text()).toEqual(`${website.live_url}`) diff --git a/static/js/components/forms/PublishForm.tsx b/static/js/components/forms/PublishForm.tsx index 6a59f0958..4c9f5a725 100644 --- a/static/js/components/forms/PublishForm.tsx +++ b/static/js/components/forms/PublishForm.tsx @@ -5,7 +5,7 @@ import * as yup from "yup" import { FormError } from "./FormError" import { Website } from "../../types/websites" -import { PUBLISH_OPTION_STAGING } from "../../constants" +import { PublishingEnv } from "../../constants" export interface SiteFormValues { url_path: string // eslint-disable-line camelcase @@ -47,7 +47,7 @@ export const PublishForm: React.FC = ({ } const fullUrl = - option === PUBLISH_OPTION_STAGING ? website.draft_url : website.live_url + option === PublishingEnv.Staging ? website.draft_url : website.live_url const partialUrl = website.url_path ? fullUrl.slice(0, fullUrl.lastIndexOf("/")) : fullUrl @@ -64,7 +64,7 @@ export const PublishForm: React.FC = ({
{" "} {website.url_path ? ( - website.publish_date || option === PUBLISH_OPTION_STAGING ? ( + website.publish_date || option === PublishingEnv.Staging ? ( {fullUrl}{" "} diff --git a/static/js/constants.ts b/static/js/constants.ts index b43c8010e..48a473b3b 100644 --- a/static/js/constants.ts +++ b/static/js/constants.ts @@ -199,11 +199,13 @@ export const GOOGLE_DRIVE_SYNC_PROCESSING_STATES = [ export const IS_A_REQUIRED_FIELD = "is a required field" -export const PUBLISH_OPTION_STAGING = "staging" -export const PUBLISH_OPTION_PRODUCTION = "production" - export enum WebsiteStarterStatus { Default = "default", Active = "active", Inactive = "inactive" } + +export enum PublishingEnv { + Staging = "staging", + Production = "production" +} diff --git a/static/js/query-configs/websites.ts b/static/js/query-configs/websites.ts index df12d8ce4..2ec689123 100644 --- a/static/js/query-configs/websites.ts +++ b/static/js/query-configs/websites.ts @@ -40,6 +40,7 @@ import { WebsiteStarter, WebsiteStatus } from "../types/websites" +import { PublishingEnv } from "../constants" export type WebsiteDetails = Record @@ -158,20 +159,24 @@ export const websiteMutation = (payload: NewWebsitePayload): QueryConfig => ({ } }) -export const websiteAction = ( +export const websitePublishAction = ( name: string, - action: string, + publishingEnv: PublishingEnv, payload: WebsitePublishPayload -): QueryConfig => ({ - url: siteApiActionUrl.param({ name, action }).toString(), - options: { - method: "POST", - headers: { - "X-CSRFTOKEN": getCookie("csrftoken") || "" - } - }, - body: payload -}) +): QueryConfig => { + const action = + publishingEnv === PublishingEnv.Production ? "publish" : "preview" + return { + url: siteApiActionUrl.param({ name, action }).toString(), + options: { + method: "POST", + headers: { + "X-CSRFTOKEN": getCookie("csrftoken") || "" + } + }, + body: payload + } +} export const websiteStartersRequest = (): QueryConfig => ({ url: startersApi.toString(), From c37c7e4e38cec1bf595f0d4b2ba58af5f6ca0169 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 18 May 2022 16:28:26 -0400 Subject: [PATCH 2/7] tweak unique url error message --- websites/serializers.py | 2 +- websites/serializers_test.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/websites/serializers.py b/websites/serializers.py index 2cd71545b..089c7dbd6 100644 --- a/websites/serializers.py +++ b/websites/serializers.py @@ -126,7 +126,7 @@ def validate_url_path(self, value): .exclude(pk=self.instance.pk) .exists() ): - raise serializers.ValidationError("The website URL is not unique") + raise serializers.ValidationError("The given website URL is already in use.") return value def update(self, instance, validated_data): diff --git a/websites/serializers_test.py b/websites/serializers_test.py index 3143ffce6..fdc7a526b 100644 --- a/websites/serializers_test.py +++ b/websites/serializers_test.py @@ -703,7 +703,7 @@ def test_website_url_serializer_duplicate_url_path(ocw_site, parsed_site_config) data = {"url_path": new_url_path} serializer = WebsiteUrlSerializer(ocw_site, data) assert serializer.is_valid() is False - assert serializer.errors.get("url_path") == ["The website URL is not unique"] + assert serializer.errors.get("url_path") == ["The given website URL is already in use."] def test_website_url_serializer_url_path_published(ocw_site): From 325118526308d3fe31e8e4dacb9bc72fda52bb91 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 19 May 2022 13:41:58 -0400 Subject: [PATCH 3/7] improve IntegrationTestHelper specifically: - use spyOn to mock makeRequest, rather than a global mock. The global mock was being problematic when other imports (e.g., the old integration test helper) was also mocking `makeRequest`. - add `mockGetWebsiteDetail` - make `helper.mockRequest` more flexible in its argument matching --- .../js/testing_utils/IntegrationTestHelper.tsx | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/static/js/testing_utils/IntegrationTestHelper.tsx b/static/js/testing_utils/IntegrationTestHelper.tsx index 607df3f69..610559077 100644 --- a/static/js/testing_utils/IntegrationTestHelper.tsx +++ b/static/js/testing_utils/IntegrationTestHelper.tsx @@ -20,6 +20,8 @@ import { getQueries } from "../lib/redux_query" import { DeepPartial } from "../types/util" import * as networkInterfaceFuncs from "../store/network_interface" +import { Website } from "../types/websites" +import { siteApiDetailUrl } from "../lib/urls" export type ReduxPatch = DeepPartial @@ -35,11 +37,6 @@ export const getInitialState = (patch: ReduxPatch = {}): ReduxState => { return defaultsDeep(cloneDeep(patch), empty) } -const { makeRequest: mockMakeRequest } = networkInterfaceFuncs as jest.Mocked< - typeof networkInterfaceFuncs -> -jest.mock("../store/network_interface") - type Reponse = { status: ResponseStatus body: ResponseBody @@ -75,6 +72,7 @@ export default class IntegrationTestHelper { constructor(location: InitialEntry = "/") { this.initialEntries = [location] + const mockMakeRequest = jest.spyOn(networkInterfaceFuncs, "makeRequest") mockMakeRequest.mockClear() mockMakeRequest.mockImplementation((url, method, options) => ({ execute: async callback => { @@ -98,10 +96,10 @@ export default class IntegrationTestHelper { method: "GET" | "POST" | "PATCH" | "DELETE", responseBody: unknown, code: number, - options = {} + optionsMatcher = expect.anything() ): this => { when(this.handleRequest) - .calledWith(url, method, options) + .calledWith(url, method, optionsMatcher) .mockResolvedValue({ body: responseBody, status: code @@ -118,6 +116,12 @@ export default class IntegrationTestHelper { return this.mockRequest(url, "GET", body, code) } + mockGetWebsiteDetail = (website: Website) => + this.mockGetRequest( + siteApiDetailUrl.param({ name: website.name }).toString(), + website + ) + /** * Convenience method for mocking out a POST request */ From c158de62ffe8ed2da2452717203d2f4afff71db2 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 19 May 2022 13:46:30 -0400 Subject: [PATCH 4/7] show form errors in PublishForm --- static/js/components/PublishDrawer.test.tsx | 102 ++++++++++++++++-- static/js/components/PublishDrawer.tsx | 27 +++-- .../js/components/forms/PublishForm.test.tsx | 4 +- static/js/components/forms/PublishForm.tsx | 26 +++-- 4 files changed, 123 insertions(+), 36 deletions(-) diff --git a/static/js/components/PublishDrawer.test.tsx b/static/js/components/PublishDrawer.test.tsx index 68cbd4dae..447dbc339 100644 --- a/static/js/components/PublishDrawer.test.tsx +++ b/static/js/components/PublishDrawer.test.tsx @@ -1,27 +1,36 @@ +import React from "react" import moment from "moment" import sinon, { SinonStub } from "sinon" import { act } from "react-dom/test-utils" import { isEmpty } from "ramda" import { siteApiActionUrl, siteApiDetailUrl } from "../lib/urls" -import { shouldIf } from "../test_util" +import { assertInstanceOf, shouldIf } from "../test_util" import { makeWebsiteDetail } from "../util/factories/websites" -import IntegrationTestHelper, { +import IntegrationTestHelperOld, { TestRenderer } from "../util/integration_test_helper_old" + +import App from "../pages/App" +import { IntegrationTestHelper } from "../testing_utils" + import PublishDrawer from "./PublishDrawer" import { Website } from "../types/websites" +import userEvent from "@testing-library/user-event" +import { waitFor, screen } from "@testing-library/react" +import * as dom from "@testing-library/dom" +import _ from "lodash" describe("PublishDrawer", () => { - let helper: IntegrationTestHelper, + let helper: IntegrationTestHelperOld, website: Website, render: TestRenderer, toggleVisibilityStub: SinonStub, refreshWebsiteStub: SinonStub beforeEach(() => { - helper = new IntegrationTestHelper() + helper = new IntegrationTestHelperOld() toggleVisibilityStub = helper.sandbox.stub() website = { ...makeWebsiteDetail(), @@ -202,7 +211,7 @@ describe("PublishDrawer", () => { await act(async () => { const onChange = wrapper.find(`#publish-${action}`).prop("onChange") // @ts-expect-error - onChange(({target: { checked: true }})) + onChange({ target: { checked: true } }) }) wrapper.update() expect(wrapper.find(".btn-publish").prop("disabled")).toBe(true) @@ -222,7 +231,7 @@ describe("PublishDrawer", () => { await act(async () => { const onChange = wrapper.find(`#publish-${action}`).prop("onChange") // @ts-expect-error - onChange(({target: { checked: true }})) + onChange({ target: { checked: true } }) }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( @@ -258,7 +267,7 @@ describe("PublishDrawer", () => { await act(async () => { const onChange = wrapper.find(`#publish-${action}`).prop("onChange") // @ts-expect-error - onChange(({target: { checked: true }})) + onChange({ target: { checked: true } }) }) wrapper.update() await act(async () => { @@ -269,7 +278,7 @@ describe("PublishDrawer", () => { }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( - "We apologize, there was an error publishing the site. Please try again in a few minutes." + "There was an error publishing the site." ) sinon.assert.calledOnceWithExactly( actionStub, @@ -303,7 +312,7 @@ describe("PublishDrawer", () => { await act(async () => { const onChange = wrapper.find(`#publish-${action}`).prop("onChange") // @ts-expect-error - onChange(({target: { checked: true }})) + onChange({ target: { checked: true } }) }) wrapper.update() expect( @@ -346,3 +355,78 @@ describe("PublishDrawer", () => { } ) }) + +describe.each([ + { + publishToLabel: "Production", + api: "publish" + }, + { + publishToLabel: "Staging", + api: "preview" + } +])("Publishing Drawer Errors ($publishToLabel)", ({ publishToLabel, api }) => { + const setup = (websiteDetails: Partial = {}) => { + const website = makeWebsiteDetail({ is_admin: true, ...websiteDetails }) + const user = userEvent.setup() + const helper = new IntegrationTestHelper(`/sites/${website.name}?publish=`) + helper.mockGetWebsiteDetail(website) + const publishUrl = siteApiActionUrl + .param({ name: website.name, action: api }) + .toString() + const setPublishResult = _.partial(helper.mockPostRequest, publishUrl) + const [result] = helper.render() + return { user, result, setPublishResult } + } + + it("renders a generic error message if publihing API failed", async () => { + const { user, result, setPublishResult } = setup() + const dialog = await screen.findByRole("dialog") + const envButton = await dom.findByText(dialog, publishToLabel) + await act(() => user.click(envButton)) + + const publishButton = await dom.findByText(dialog, "Publish") + const form = dialog.querySelector("form") + expect(form).toContainElement(publishButton) + expect(publishButton).toHaveAttribute("type", "submit") + assertInstanceOf(form, HTMLFormElement) + + setPublishResult(undefined, 500) + act(() => form.submit()) + await waitFor(() => { + expect(dialog).toHaveTextContent( + "We apologize, there was a problem publishing your site." + ) + }) + result.unmount() + }) + + it("renders a specific error for url issues", async () => { + const { user, result, setPublishResult } = setup({ + publish_date: null, + url_path: "some_path_in_use" + }) + const dialog = await screen.findByRole("dialog") + const envButton = await dom.findByText(dialog, publishToLabel) + await act(() => user.click(envButton)) + + const publishButton = await dom.findByText(dialog, "Publish") + const form = dialog.querySelector("form") + expect(form).toContainElement(publishButton) + expect(publishButton).toHaveAttribute("type", "submit") + assertInstanceOf(form, HTMLFormElement) + + setPublishResult({ url_path: "Some url error" }, 400) + act(() => form.submit()) + + await waitFor(() => { + expect(dialog).toHaveTextContent( + "We apologize, there was a problem publishing your site." + ) + }) + const errorMsg = await dom.findByText(dialog, "Some url error") + assertInstanceOf(errorMsg.previousSibling, HTMLInputElement) + expect(errorMsg.previousSibling.name).toBe("url_path") + result.unmount() + }) +}) diff --git a/static/js/components/PublishDrawer.tsx b/static/js/components/PublishDrawer.tsx index ab6b210cf..e653bc424 100644 --- a/static/js/components/PublishDrawer.tsx +++ b/static/js/components/PublishDrawer.tsx @@ -14,7 +14,10 @@ import { isErrorStatusCode } from "../lib/util" import PublishStatusIndicator from "./PublishStatusIndicator" import { Website } from "../types/websites" -import { PublishForm } from "./forms/PublishForm" +import PublishForm, { + OnSubmitPublish, + SiteFormValues +} from "./forms/PublishForm" import { PublishingEnv, PublishStatus } from "../constants" import { useAppDispatch } from "../hooks/redux" @@ -58,7 +61,6 @@ const getPublishingInfo = ( const PublishingOption: React.FC = props => { const { publishingEnv, selected, onSelect, website, onPublishSuccess } = props const publishingInfo = getPublishingInfo(website, publishingEnv) - const [error, setError] = useState>(null) const [ { isPending }, @@ -67,8 +69,7 @@ const PublishingOption: React.FC = props => { websitePublishAction(website.name, publishingEnv, payload) ) - const handlePublish = async (payload: WebsitePublishPayload) => { - setError(null) + const handlePublish: OnSubmitPublish = async (payload, helpers) => { if (isPending) { return } @@ -77,7 +78,14 @@ const PublishingOption: React.FC = props => { return } else { if (isErrorStatusCode(response.status)) { - setError(response.body) + const errorBody: Partial | undefined = response.body + const errors = { + url_path: errorBody?.url_path + } + helpers.setErrors(errors) + helpers.setStatus( + "We apologize, there was a problem publishing your site." + ) } else { onPublishSuccess() } @@ -115,15 +123,6 @@ const PublishingOption: React.FC = props => {
)} - {error && ( - <> - - We apologize, there was an error publishing the site. Please try - again in a few minutes. - -
- - )} +) => void + type Props = { - onSubmit: ( - values: SiteFormValues, - formikHelpers: FormikHelpers - ) => void + onSubmit: OnSubmitPublish website: Website option: string disabled: boolean @@ -33,7 +35,7 @@ export const websiteUrlValidation = yup.object().shape({ ) }) -export const PublishForm: React.FC = ({ +const PublishForm: React.FC = ({ onSubmit, website, disabled, @@ -85,7 +87,7 @@ export const PublishForm: React.FC = ({
)} -
+
+ {status && ( + // Status is being used to store non-field errors +
+ {status} +
+ )}
- {status && ( - // Status is being used to store non-field errors -
{status}
- )} )} ) } + +export default PublishForm From f212943a478f9de462621f74c74825f1edf3f9b8 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 19 May 2022 13:55:44 -0400 Subject: [PATCH 5/7] remove old error test --- static/js/components/PublishDrawer.test.tsx | 44 --------------------- 1 file changed, 44 deletions(-) diff --git a/static/js/components/PublishDrawer.test.tsx b/static/js/components/PublishDrawer.test.tsx index 447dbc339..d1ef74ae6 100644 --- a/static/js/components/PublishDrawer.test.tsx +++ b/static/js/components/PublishDrawer.test.tsx @@ -252,50 +252,6 @@ describe("PublishDrawer", () => { }) }) - it("renders an error message if the publish didn't work", async () => { - const actionStub = helper.mockPostRequest( - siteApiActionUrl - .param({ - name: website.name, - action: api - }) - .toString(), - {}, - 500 - ) - const { wrapper } = await render() - await act(async () => { - const onChange = wrapper.find(`#publish-${action}`).prop("onChange") - // @ts-expect-error - onChange({ target: { checked: true } }) - }) - wrapper.update() - await act(async () => { - wrapper - .find("PublishForm") - .find(".btn-publish") - .simulate("submit") - }) - wrapper.update() - expect(wrapper.find(".publish-option-description").text()).toContain( - "There was an error publishing the site." - ) - sinon.assert.calledOnceWithExactly( - actionStub, - `/api/websites/${website.name}/${api}/`, - "POST", - { - body: { - url_path: website.url_path - }, - headers: { "X-CSRFTOKEN": "" }, - credentials: undefined - } - ) - sinon.assert.notCalled(refreshWebsiteStub) - sinon.assert.notCalled(toggleVisibilityStub) - }) - it("publish button sends the expected request", async () => { const actionStub = helper.mockPostRequest( siteApiActionUrl From 2bbdb7c56d2e85f3f2af4373582f2a9f64f837ad Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 19 May 2022 13:55:53 -0400 Subject: [PATCH 6/7] fix PublishForm import --- static/js/components/forms/PublishForm.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/static/js/components/forms/PublishForm.test.tsx b/static/js/components/forms/PublishForm.test.tsx index e3faa840b..6919bc152 100644 --- a/static/js/components/forms/PublishForm.test.tsx +++ b/static/js/components/forms/PublishForm.test.tsx @@ -3,7 +3,7 @@ import sinon, { SinonStub } from "sinon" import { shallow } from "enzyme" import { ValidationError } from "yup" -import { PublishForm, websiteUrlValidation } from "./PublishForm" +import PublishForm, { websiteUrlValidation } from "./PublishForm" import { PublishingEnv } from "../../constants" import { assertInstanceOf, defaultFormikChildProps } from "../../test_util" import { makeWebsiteDetail } from "../../util/factories/websites" From fbf007d396e1cb9cdf34fb7e7fd68dc778d86b5a Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 19 May 2022 14:09:25 -0400 Subject: [PATCH 7/7] format python changes --- websites/serializers.py | 4 +++- websites/serializers_test.py | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/websites/serializers.py b/websites/serializers.py index 089c7dbd6..a1becc257 100644 --- a/websites/serializers.py +++ b/websites/serializers.py @@ -126,7 +126,9 @@ def validate_url_path(self, value): .exclude(pk=self.instance.pk) .exists() ): - raise serializers.ValidationError("The given website URL is already in use.") + raise serializers.ValidationError( + "The given website URL is already in use." + ) return value def update(self, instance, validated_data): diff --git a/websites/serializers_test.py b/websites/serializers_test.py index fdc7a526b..224220d3e 100644 --- a/websites/serializers_test.py +++ b/websites/serializers_test.py @@ -703,7 +703,9 @@ def test_website_url_serializer_duplicate_url_path(ocw_site, parsed_site_config) data = {"url_path": new_url_path} serializer = WebsiteUrlSerializer(ocw_site, data) assert serializer.is_valid() is False - assert serializer.errors.get("url_path") == ["The given website URL is already in use."] + assert serializer.errors.get("url_path") == [ + "The given website URL is already in use." + ] def test_website_url_serializer_url_path_published(ocw_site):