diff --git a/RELEASE.rst b/RELEASE.rst index 05b4d8c07..a0f1a5281 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,12 @@ Release Notes ============= +Version 0.61.2 +-------------- + +- Customizable URLs for studio (#1316) +- improve legacy shortcode handling (#1349) + Version 0.61.1 (Released May 17, 2022) -------------- diff --git a/content_sync/api.py b/content_sync/api.py index 6f1602b3f..3eaf92664 100644 --- a/content_sync/api.py +++ b/content_sync/api.py @@ -80,12 +80,6 @@ def create_website_backend(website: Website): tasks.create_website_backend.delay(website.name) -@is_publish_pipeline_enabled -def create_website_publishing_pipeline(website: Website): - """ Create the publish pipeline for a website""" - tasks.upsert_website_publishing_pipeline.delay(website.name) - - @is_sync_enabled def update_website_backend(website: Website): """ Update the backend content for a website""" @@ -136,6 +130,10 @@ def publish_website( # pylint: disable=too-many-arguments if trigger_pipeline and settings.CONTENT_SYNC_PIPELINE_BACKEND: pipeline = get_sync_pipeline(website, api=pipeline_api) + # Always upsert pipeline in case the site url path changed, + # unless it's been published to production (url path permanent). + if not website.publish_date: + pipeline.upsert_pipeline() pipeline.unpause_pipeline(version) build_id = pipeline.trigger_pipeline_build(version) update_kwargs = { diff --git a/content_sync/api_test.py b/content_sync/api_test.py index 40b281de7..2f4041a63 100644 --- a/content_sync/api_test.py +++ b/content_sync/api_test.py @@ -194,34 +194,13 @@ def test_get_sync_pipeline(settings, mocker, pipeline_api): import_string_mock.assert_any_call(website, api=pipeline_api) -def test_create_website_publishing_pipeline(settings, mocker): - """upsert_website_publishing_pipeline task should be called if pipelines are enabled""" - settings.CONTENT_SYNC_PIPELINE_BACKEND = "concourse" - mock_task = mocker.patch( - "content_sync.api.tasks.upsert_website_publishing_pipeline.delay" - ) - website = WebsiteFactory.create() - api.create_website_publishing_pipeline(website) - mock_task.assert_called_once_with(website.name) - - -def test_create_website_publishing_pipeline_disabled(settings, mocker): - """upsert_website_publishing_pipeline task should not be called if pipelines are disabled""" - settings.CONTENT_SYNC_PIPELINE_BACKEND = None - mock_task = mocker.patch( - "content_sync.api.tasks.upsert_website_publishing_pipeline.delay" - ) - website = WebsiteFactory.create() - api.create_website_publishing_pipeline(website) - mock_task.assert_not_called() - - @pytest.mark.parametrize("prepublish", [True, False]) @pytest.mark.parametrize("prepublish_actions", [[], ["some.Action"]]) @pytest.mark.parametrize("has_api", [True, False]) @pytest.mark.parametrize("version", [VERSION_LIVE, VERSION_DRAFT]) @pytest.mark.parametrize("status", [None, PUBLISH_STATUS_NOT_STARTED]) @pytest.mark.parametrize("trigger", [True, False]) +@pytest.mark.parametrize("publish_date", [None, now_in_utc()]) def test_publish_website( # pylint:disable=redefined-outer-name,too-many-arguments settings, mocker, @@ -232,10 +211,11 @@ def test_publish_website( # pylint:disable=redefined-outer-name,too-many-argume version, status, trigger, + publish_date, ): """Verify that the appropriate backend calls are made by the publish_website function""" settings.PREPUBLISH_ACTIONS = prepublish_actions - website = WebsiteFactory.create() + website = WebsiteFactory.create(publish_date=publish_date) setattr(website, f"{version}_publish_status", status) if status: setattr(website, f"{version}_publish_status_updated_on", now_in_utc()) @@ -263,6 +243,7 @@ def test_publish_website( # pylint:disable=redefined-outer-name,too-many-argume mock_api_funcs.mock_get_pipeline.assert_called_once_with( website, api=pipeline_api ) + assert pipeline.upsert_pipeline.call_count == (0 if publish_date else 1) pipeline.trigger_pipeline_build.assert_called_once_with(version) pipeline.unpause_pipeline.assert_called_once_with(version) assert getattr(website, f"latest_build_id_{version}") == build_id diff --git a/content_sync/pipelines/concourse.py b/content_sync/pipelines/concourse.py index bd19a81d3..cfde87c07 100644 --- a/content_sync/pipelines/concourse.py +++ b/content_sync/pipelines/concourse.py @@ -30,7 +30,6 @@ from content_sync.utils import check_mandatory_settings from websites.constants import OCW_HUGO_THEMES_GIT, STARTER_SOURCE_GITHUB from websites.models import Website -from websites.site_config_api import SiteConfig log = logging.getLogger(__name__) @@ -273,14 +272,12 @@ def upsert_pipeline(self): # pylint:disable=too-many-locals # Invalid github url, so skip return - site_config = SiteConfig(self.website.starter.config) - site_url = f"{site_config.root_url_path}/{self.website.name}".strip("/") if self.website.name == settings.ROOT_WEBSITE_NAME: base_url = "" theme_created_trigger = "true" theme_deployed_trigger = "false" else: - base_url = site_url + base_url = self.website.get_url_path() theme_created_trigger = "false" theme_deployed_trigger = "true" hugo_projects_url = urljoin( @@ -342,8 +339,9 @@ def upsert_pipeline(self): # pylint:disable=too-many-locals .replace("((ocw-site-repo))", self.website.short_id) .replace("((ocw-site-repo-branch))", branch) .replace("((config-slug))", self.website.starter.slug) + .replace("((s3-path))", self.website.s3_path) .replace("((base-url))", base_url) - .replace("((site-url))", site_url) + .replace("((site-url))", self.website.get_url_path()) .replace("((site-name))", self.website.name) .replace("((purge-url))", f"purge/{self.website.name}") .replace("((purge_header))", purge_header) diff --git a/content_sync/pipelines/concourse_test.py b/content_sync/pipelines/concourse_test.py index 4630ad36f..4b6fe6b96 100644 --- a/content_sync/pipelines/concourse_test.py +++ b/content_sync/pipelines/concourse_test.py @@ -141,10 +141,13 @@ def test_upsert_website_pipelines( if home_page: name = settings.ROOT_WEBSITE_NAME starter.config["root-url-path"] = "" + site_path = None else: name = "standard-course" starter.config["root-url-path"] = "courses" - website = WebsiteFactory.create(starter=starter, name=name) + site_path = "courses/my-site-fall-2020" + + website = WebsiteFactory.create(starter=starter, name=name, url_path=site_path) instance_vars = f"%7B%22site%22%3A%20%22{website.name}%22%7D" url_path = f"/api/v1/teams/{settings.CONCOURSE_TEAM}/pipelines/{version}/config?vars={instance_vars}" @@ -188,6 +191,7 @@ def test_upsert_website_pipelines( assert settings.OCW_GTM_ACCOUNT_ID in config_str assert settings.OCW_IMPORT_STARTER_SLUG in config_str assert api_url in config_str + if home_page: assert ( f"s3 sync s3://{settings.AWS_STORAGE_BUCKET_NAME}/{website.name} s3://{bucket}/{website.name}" @@ -196,11 +200,11 @@ def test_upsert_website_pipelines( assert f"aws s3 sync course-markdown/public s3://{bucket}/" in config_str else: assert ( - f"s3 sync s3://{settings.AWS_STORAGE_BUCKET_NAME}/courses/{website.name} s3://{bucket}/courses/{website.name}" + f"s3 sync s3://{settings.AWS_STORAGE_BUCKET_NAME}/courses/{website.name} s3://{bucket}/{website.url_path}" in config_str ) assert ( - f"aws s3 sync course-markdown/public s3://{bucket}/courses/{website.name}" + f"aws s3 sync course-markdown/public s3://{bucket}/{website.url_path}" in config_str ) assert f"purge/{website.name}" in config_str @@ -392,10 +396,10 @@ def test_upsert_pipeline(settings, mocker, mock_auth, pipeline_exists): @pytest.mark.parametrize("pipeline_exists", [True, False]) @pytest.mark.parametrize("version", [VERSION_DRAFT, VERSION_LIVE]) -def test_upsert_mass_publish_pipeline( +def test_upsert_mass_build_pipeline( settings, pipeline_settings, mocker, mock_auth, pipeline_exists, version ): # pylint:disable=too-many-locals,too-many-arguments - """The mass publish pipeline should have expected configuration""" + """The mass build pipeline should have expected configuration""" hugo_projects_path = "https://github.com/org/repo" WebsiteFactory.create( starter=WebsiteStarterFactory.create( @@ -437,6 +441,10 @@ def test_upsert_mass_publish_pipeline( api_url = settings.OCW_STUDIO_LIVE_URL config_str = json.dumps(kwargs) assert settings.OCW_GTM_ACCOUNT_ID in config_str + assert ( + f"s3://{settings.AWS_STORAGE_BUCKET_NAME}/$S3_PATH s3://{bucket}/$SITE_URL" + in config_str + ) assert bucket in config_str assert version in config_str assert f"{hugo_projects_path}.git" in config_str diff --git a/content_sync/pipelines/definitions/concourse/mass-build-sites.yml b/content_sync/pipelines/definitions/concourse/mass-build-sites.yml index 05f84615e..d61fb7100 100644 --- a/content_sync/pipelines/definitions/concourse/mass-build-sites.yml +++ b/content_sync/pipelines/definitions/concourse/mass-build-sites.yml @@ -105,14 +105,19 @@ jobs: NAME=$(echo $1 | jq -c '.name' | tr -d '"') SHORT_ID=$(echo $1 | jq -c '.short_id' | tr -d '"') STARTER_SLUG=$(echo $1| jq -c '.starter_slug' | tr -d '"') + S3_PATH=$(echo $1 | jq -c '.s3_path' | tr -d '"') SITE_URL=$(echo $1 | jq -c '.site_url' | tr -d '"') BASE_URL=$(echo $1 | jq -c '.base_url' | tr -d '"') + if [[ "$SITE_URL" == null || "$S3_PATH" == null ]] + then + return 1 + fi git -c core.sshCommand="ssh $GITKEYSSH -o StrictHostKeyChecking=no" clone -b ((ocw-site-repo-branch)) ((markdown-uri))/$SHORT_ID.git || return 1 cd $CURDIR/$SHORT_ID cp ../webpack-json/webpack.json ../ocw-hugo-themes/base-theme/data hugo --config ../ocw-hugo-projects/$STARTER_SLUG/config.yaml --baseUrl /$BASE_URL --themesDir ../ocw-hugo-themes/ ((build-drafts)) || return 1 cd $CURDIR - aws s3 sync s3://((ocw-studio-bucket))/$SITE_URL s3://((ocw-bucket))/$SITE_URL --metadata site-id=$NAME || return 1 + aws s3 sync s3://((ocw-studio-bucket))/$S3_PATH s3://((ocw-bucket))/$SITE_URL --metadata site-id=$NAME || return 1 aws s3 sync $SHORT_ID/public s3://((ocw-bucket))/$BASE_URL --metadata site-id=$NAME || return 1 curl -X POST -H 'Content-Type: application/json' --data '{"webhook_key":"((open-webhook-key))","prefix":"'"$SITE_URL"/'","version":"((version))"}' ((open-discussions-url))/api/v0/ocw_next_webhook/ curl -X POST -H 'Content-Type: application/json' -H 'Authorization: Bearer ((api-token))' --data '{"version":"((version))","status":"succeeded"}' ((ocw-studio-url))/api/websites/$NAME/pipeline_status/ diff --git a/content_sync/pipelines/definitions/concourse/site-pipeline.yml b/content_sync/pipelines/definitions/concourse/site-pipeline.yml index 5b5567dee..99231f099 100644 --- a/content_sync/pipelines/definitions/concourse/site-pipeline.yml +++ b/content_sync/pipelines/definitions/concourse/site-pipeline.yml @@ -178,7 +178,7 @@ jobs: args: - -exc - | - aws s3 sync s3://((ocw-studio-bucket))/((site-url)) s3://((ocw-bucket))/((site-url)) --metadata site-id=((site-name)) + aws s3 sync s3://((ocw-studio-bucket))/((s3-path)) s3://((ocw-bucket))/((site-url)) --metadata site-id=((site-name)) aws s3 sync course-markdown/public s3://((ocw-bucket))/((base-url)) --metadata site-id=((site-name)) on_failure: try: diff --git a/content_sync/serializers.py b/content_sync/serializers.py index 4be6b7ade..3229a3392 100644 --- a/content_sync/serializers.py +++ b/content_sync/serializers.py @@ -113,7 +113,11 @@ def deserialize( # pylint:disable=too-many-locals omitted_keys.append(file_field["name"]) file_url = front_matter_data.get(file_field["name"], None) if file_url is not None: + s3_path = website.s3_path + url_path = website.url_path file_url = urlparse(file_url).path.lstrip("/") + if url_path and s3_path != url_path: + file_url = file_url.replace(url_path, s3_path, 1) base_defaults = { "metadata": dict_without_keys(front_matter_data, *omitted_keys), diff --git a/content_sync/serializers_test.py b/content_sync/serializers_test.py index b7993e4b3..c81f28376 100644 --- a/content_sync/serializers_test.py +++ b/content_sync/serializers_test.py @@ -4,6 +4,7 @@ import pytest import yaml +from django.core.files.uploadedfile import SimpleUploadedFile from moto import mock_s3 from content_sync.serializers import ( @@ -16,6 +17,7 @@ deserialize_file_to_website_content, serialize_content_to_file, ) +from websites.constants import WEBSITE_CONFIG_ROOT_URL_PATH_KEY from websites.factories import ( WebsiteContentFactory, WebsiteFactory, @@ -43,7 +45,7 @@ title: Example File content_type: resource uid: abcdefg -image: https://test.edu/courses/website_name/image.png +image: https://test.edu/courses/website_name-fall-2025/image.png --- # My markdown - abc @@ -91,21 +93,26 @@ def get_example_menu_data(): ] +@mock_s3 @pytest.mark.django_db @pytest.mark.parametrize( "markdown, exp_sections", [["# Some markdown...\n- and\n- a\n- list", 2], [None, 1]] ) -def test_hugo_file_serialize(markdown, exp_sections): +def test_hugo_file_serialize(settings, markdown, exp_sections): """HugoMarkdownFileSerializer.serialize should create the expected file contents""" + settings.OCW_STUDIO_USE_S3 = True metadata = {"metadata1": "dummy value 1", "metadata2": "dummy value 2"} content = WebsiteContentFactory.create( text_id="abcdefg", title="Content Title", - type="sometype", + type="resource", markdown=markdown, metadata=metadata, + file=SimpleUploadedFile("mysite/test.pdf", b"content"), + website=WebsiteFactory.create(name="mysite", url_path="sites/mysite-fall-2025"), ) site_config = SiteConfig(content.website.starter.config) + file_content = HugoMarkdownFileSerializer(site_config).serialize( website_content=content ) @@ -127,6 +134,10 @@ def test_hugo_file_serialize(markdown, exp_sections): ] + [f"{k}: {v}" for k, v in metadata.items()] ) + assert ( + f"image: /media/{content.website.url_path}/{content.file.name.split('/')[-1]}" + in front_matter_lines + ) if exp_sections > 1: assert md_file_sections[1] == markdown @@ -222,19 +233,20 @@ def test_hugo_menu_yaml_deserialize(omnibus_config): def test_hugo_file_deserialize_with_file(settings): """HugoMarkdownFileSerializer.deserialize should create the expected content object from some file contents""" settings.DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage" - website = WebsiteFactory.create() + website = WebsiteFactory.create(url_path="courses/website_name-fall-2025") + website.starter.config[WEBSITE_CONFIG_ROOT_URL_PATH_KEY] = "courses" site_config = SiteConfig(website.starter.config) website_content = HugoMarkdownFileSerializer(site_config).deserialize( website=website, filepath="/test/file.md", file_contents=EXAMPLE_HUGO_MARKDOWN_WITH_FILE, ) - path = "courses/website_name/image.png" + expected_path = f"courses/{website.name}/image.png" assert "image" not in website_content.metadata.keys() - assert website_content.file == path + assert website_content.file == expected_path assert ( website_content.file.url - == f"https://s3.amazonaws.com/{settings.AWS_STORAGE_BUCKET_NAME}/{path}" + == f"https://s3.amazonaws.com/{settings.AWS_STORAGE_BUCKET_NAME}/{expected_path}" ) diff --git a/localdev/configs/ocw-course-site-config.yml b/localdev/configs/ocw-course-site-config.yml index cd0c472c1..0ee3bdee6 100644 --- a/localdev/configs/ocw-course-site-config.yml +++ b/localdev/configs/ocw-course-site-config.yml @@ -1,5 +1,6 @@ --- root-url-path: courses +site-url-format: "[sitemetadata:primary_course_number]-[sitemetadata:course_title]-[sitemetadata:term]-[sitemetadata:year]" collections: - category: Content folder: content/pages diff --git a/main/settings.py b/main/settings.py index fcd185710..e5d7d9055 100644 --- a/main/settings.py +++ b/main/settings.py @@ -24,7 +24,7 @@ # pylint: disable=too-many-lines -VERSION = "0.61.1" +VERSION = "0.61.2" SITE_ID = get_int( name="OCW_STUDIO_SITE_ID", @@ -558,7 +558,6 @@ default="course_title", description="The site metadata field for title", ) - # YouTube OCW metadata fields YT_FIELD_CAPTIONS = get_string( name="YT_FIELD_CAPTIONS", diff --git a/static/js/components/PublishDrawer.test.tsx b/static/js/components/PublishDrawer.test.tsx index 301542e19..a0f3e54e7 100644 --- a/static/js/components/PublishDrawer.test.tsx +++ b/static/js/components/PublishDrawer.test.tsx @@ -27,7 +27,8 @@ describe("PublishDrawer", () => { ...makeWebsiteDetail(), has_unpublished_draft: true, has_unpublished_live: true, - is_admin: true + is_admin: true, + url_path: "mysite-fall-2025" } refreshWebsiteStub = helper.mockGetRequest( siteApiDetailUrl.param({ name: website.name }).toString(), @@ -260,7 +261,10 @@ describe("PublishDrawer", () => { wrapper.update() await act(async () => { // @ts-ignore - wrapper.find(".btn-publish").prop("onClick")() + wrapper + .find("PublishForm") + .find(".btn-publish") + .simulate("submit") }) wrapper.update() expect(wrapper.find(".publish-option-description").text()).toContain( @@ -271,7 +275,9 @@ describe("PublishDrawer", () => { `/api/websites/${website.name}/${api}/`, "POST", { - body: {}, + body: { + url_path: website.url_path + }, headers: { "X-CSRFTOKEN": "" }, credentials: undefined } @@ -288,7 +294,9 @@ describe("PublishDrawer", () => { action: api }) .toString(), - {} + { + url_path: website.url_path + } ) const { wrapper } = await render() await act(async () => { @@ -296,17 +304,27 @@ describe("PublishDrawer", () => { wrapper.find(`#publish-${action}`).prop("onChange")() }) wrapper.update() - expect(wrapper.find(".btn-publish").prop("disabled")).toBeFalsy() + expect( + wrapper + .find("PublishForm") + .find(".btn-publish") + .prop("disabled") + ).toBeFalsy() await act(async () => { // @ts-ignore - wrapper.find(".btn-publish").prop("onClick")() + wrapper + .find("PublishForm") + .find(".btn-publish") + .simulate("submit") }) sinon.assert.calledOnceWithExactly( actionStub, `/api/websites/${website.name}/${api}/`, "POST", { - body: {}, + body: { + url_path: website.url_path + }, headers: { "X-CSRFTOKEN": "" }, credentials: undefined } diff --git a/static/js/components/PublishDrawer.tsx b/static/js/components/PublishDrawer.tsx index 2987ab5c0..bf7dc8fbf 100644 --- a/static/js/components/PublishDrawer.tsx +++ b/static/js/components/PublishDrawer.tsx @@ -6,14 +6,17 @@ import { requestAsync } from "redux-query" import moment from "moment" import { isEmpty } from "ramda" -import { websiteAction, websiteDetailRequest } from "../query-configs/websites" +import { + websiteAction, + websiteDetailRequest, + WebsitePublishPayload +} from "../query-configs/websites" import { isErrorStatusCode } from "../lib/util" import PublishStatusIndicator from "./PublishStatusIndicator" import { Website } from "../types/websites" - -const STAGING = "staging" -const PRODUCTION = "production" +import { PublishForm } from "./forms/PublishForm" +import { PUBLISH_OPTION_PRODUCTION, PUBLISH_OPTION_STAGING } from "../constants" type Props = { visibility: boolean @@ -23,23 +26,32 @@ type Props = { export default function PublishDrawer(props: Props): JSX.Element { const { visibility, toggleVisibility, website } = props - const [{ isPending: previewIsPending }, previewWebsite] = useMutation(() => - websiteAction(website.name, "preview") + const [ + { isPending: previewIsPending }, + previewWebsite + ] = useMutation((payload: WebsitePublishPayload) => + websiteAction(website.name, "preview", payload) ) - const [{ isPending: publishIsPending }, publishWebsite] = useMutation(() => - websiteAction(website.name, "publish") + + const [ + { isPending: publishIsPending }, + publishWebsite + ] = useMutation((payload: WebsitePublishPayload) => + websiteAction(website.name, "publish", payload) ) const store = useStore() - const [publishOption, setPublishOption] = useState(STAGING) + const [publishOption, setPublishOption] = useState( + PUBLISH_OPTION_STAGING + ) const [errorStaging, setErrorStaging] = useState(false) const [errorProduction, setErrorProduction] = useState(false) - const onPreview = async () => { + const onPreview = async (payload: WebsitePublishPayload) => { setErrorStaging(false) if (previewIsPending) { return } - const response = await previewWebsite() + const response = await previewWebsite(payload) if (!response) { return } else { @@ -53,12 +65,12 @@ export default function PublishDrawer(props: Props): JSX.Element { } } - const onPublish = async () => { + const onPublish = async (payload: WebsitePublishPayload) => { setErrorProduction(false) if (publishIsPending) { return } - const response = await publishWebsite() + const response = await publishWebsite(payload) if (!response) { return } else { @@ -73,11 +85,10 @@ export default function PublishDrawer(props: Props): JSX.Element { } const renderOption = (option: string) => { - const isStaging = option === STAGING + const isStaging = option === PUBLISH_OPTION_STAGING const label = isStaging ? "Staging" : "Production" const publish = isStaging ? onPreview : onPublish const error = isStaging ? errorStaging : errorProduction - const siteUrl = isStaging ? website.draft_url : website.live_url const publishDate = isStaging ? website.draft_publish_date : website.publish_date @@ -102,10 +113,6 @@ export default function PublishDrawer(props: Props): JSX.Element { {publishOption === option ? (
- - {siteUrl} - -
Last updated:{" "} {publishDate ? moment(publishDate).format("dddd, MMMM D h:mma ZZ") : @@ -126,13 +133,12 @@ export default function PublishDrawer(props: Props): JSX.Element {
) : null} - + website={website} + option={option} + >
) : null} @@ -141,11 +147,15 @@ export default function PublishDrawer(props: Props): JSX.Element { } return ( - + Publish your site - {renderOption(STAGING)} - {website.is_admin ? renderOption(PRODUCTION) : null} + {renderOption(PUBLISH_OPTION_STAGING)} + {website.is_admin ? renderOption(PUBLISH_OPTION_PRODUCTION) : null} {website.content_warnings && !isEmpty(website.content_warnings) ? (
diff --git a/static/js/components/forms/PublishForm.test.tsx b/static/js/components/forms/PublishForm.test.tsx new file mode 100644 index 000000000..9911cbf4d --- /dev/null +++ b/static/js/components/forms/PublishForm.test.tsx @@ -0,0 +1,133 @@ +import React from "react" +import sinon, { SinonStub } from "sinon" +import { shallow } from "enzyme" +import { ValidationError } from "yup" + +import { PublishForm, websiteUrlValidation } from "./PublishForm" +import { + PUBLISH_OPTION_PRODUCTION, + PUBLISH_OPTION_STAGING +} from "../../constants" +import { assertInstanceOf, defaultFormikChildProps } from "../../test_util" +import { makeWebsiteDetail } from "../../util/factories/websites" + +describe("PublishForm", () => { + let sandbox, onSubmitStub: SinonStub + const website = makeWebsiteDetail() + + const renderForm = (props = {}) => + shallow( + + ) + + const renderInnerForm = ( + formikChildProps: { [key: string]: any }, + wrapperProps: { [key: string]: any } + ) => { + const wrapper = renderForm(wrapperProps) + return ( + wrapper + .find("Formik") + // @ts-ignore + .renderProp("children")({ + ...defaultFormikChildProps, + ...formikChildProps + }) + ) + } + + beforeEach(() => { + sandbox = sinon.createSandbox() + onSubmitStub = sandbox.stub() + }) + + it("passes onSubmit to Formik", () => { + const wrapper = renderForm() + + const props = wrapper.find("Formik").props() + expect(props.onSubmit).toBe(onSubmitStub) + }) + + it.each([ + { urlPath: null, basedOn: "url_suggestion" }, + { urlPath: "courses/5.2-my-course", basedOn: "url_path" } + ])( + "initialValue for url_path should be based on website $basedOn", + ({ urlPath }) => { + website.publish_date = null + website.url_path = urlPath + const wrapper = renderForm() + expect(wrapper.prop("initialValues")).toEqual({ + url_path: website.url_path ? "5.2-my-course" : website.url_suggestion + }) + } + ) + + it("shows a field for URL Path if website is not published", () => { + website.publish_date = null + const form = renderInnerForm( + { isSubmitting: false, status: "whatever" }, + {} + ) + expect( + form + .find("Field") + .filterWhere(node => node.prop("name") === "url_path") + .exists() + ).toBeTruthy() + }) + + it.each([PUBLISH_OPTION_STAGING, PUBLISH_OPTION_PRODUCTION])( + "shows a URL link instead of a field if website is published", + option => { + website.publish_date = "2020-01-01" + website.url_path = "courses/my-url-fall-2028" + const form = renderInnerForm( + { isSubmitting: false, status: "whatever" }, + { option: option } + ) + expect( + form + .find("Field") + .filterWhere(node => node.prop("name") === "url_path") + .exists() + ).toBeFalsy() + expect(form.find("a").prop("href")).toEqual( + option === PUBLISH_OPTION_STAGING ? website.draft_url : website.live_url + ) + } + ) + + describe("validation", () => { + it("rejects an empty url", async () => { + try { + await expect( + await websiteUrlValidation.validateAt("url_path", { url_path: "" }) + ).rejects.toThrow() + } catch (error) { + assertInstanceOf(error, ValidationError) + expect(error.errors).toStrictEqual(["URL Path is a required field"]) + } + }) + it("rejects a URL Path with invalid characters", async () => { + try { + await expect( + await websiteUrlValidation.validateAt("url_path", { + url_path: "courses/bad-url-fall-2024" + }) + ).rejects.toThrow() + } catch (error) { + assertInstanceOf(error, ValidationError) + expect(error.errors).toStrictEqual([ + "Only alphanumeric characters, periods, dashes, or underscores allowed" + ]) + } + }) + }) +}) diff --git a/static/js/components/forms/PublishForm.tsx b/static/js/components/forms/PublishForm.tsx new file mode 100644 index 000000000..cc03003e8 --- /dev/null +++ b/static/js/components/forms/PublishForm.tsx @@ -0,0 +1,94 @@ +import React from "react" +import { Formik, Form, ErrorMessage, Field, FormikHelpers } from "formik" +import * as yup from "yup" + +import { FormError } from "./FormError" + +import { Website } from "../../types/websites" +import { PUBLISH_OPTION_STAGING } from "../../constants" + +export interface SiteFormValues { + url_path: string // eslint-disable-line camelcase +} + +type Props = { + onSubmit: ( + values: SiteFormValues, + formikHelpers: FormikHelpers + ) => void + website: Website + option: string + disabled: boolean +} + +export const websiteUrlValidation = yup.object().shape({ + url_path: yup + .string() + .label("URL Path") + .trim() + .required() + .matches( + /^[a-zA-Z0-9\-._]*$/, + "Only alphanumeric characters, periods, dashes, or underscores allowed" + ) +}) + +export const PublishForm: React.FC = ({ + onSubmit, + website, + disabled, + option +}) => { + const initialValues: SiteFormValues = { + // @ts-expect-error + url_path: website.url_path ? + website.url_path.split("/").pop() : + website.url_suggestion + } + + const fullUrl = + option === PUBLISH_OPTION_STAGING ? website.draft_url : website.live_url + const partialUrl = website.url_path ? + fullUrl.slice(0, fullUrl.lastIndexOf("/")) : + fullUrl + + return ( + + {({ isSubmitting, status }) => ( +
+ {!website.publish_date ? ( +
+ {`${partialUrl}/`} + + +
+ ) : ( + + )} +
+ +
+ {status && ( + // Status is being used to store non-field errors +
{status}
+ )} +
+ )} +
+ ) +} diff --git a/static/js/constants.ts b/static/js/constants.ts index 95e6f159d..fbcee29bd 100644 --- a/static/js/constants.ts +++ b/static/js/constants.ts @@ -198,3 +198,6 @@ 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" diff --git a/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts b/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts index d6e57b1a7..7783c8ec7 100644 --- a/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts +++ b/static/js/lib/ckeditor/plugins/LegacyShortcodes.test.ts @@ -5,7 +5,7 @@ import LegacyShortcodes from "./LegacyShortcodes" import { LEGACY_SHORTCODES } from "./constants" import Paragraph from "@ckeditor/ckeditor5-paragraph/src/paragraph" -const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution / >}}{{< /quiz_multiple_choice >}}` +const quizTestMD = `{{< quiz_multiple_choice questionId="Q1_div" >}}{{< quiz_choices >}}{{< quiz_choice isCorrect="false" >}}sound, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, satisfiable{{< /quiz_choice >}}{{< quiz_choice isCorrect="true" >}}sound, valid{{< /quiz_choice >}}{{< quiz_choice isCorrect="false" >}}valid, true{{< /quiz_choice >}}{{< /quiz_choices >}}{{< quiz_solution >}}{{< /quiz_multiple_choice >}}` const getEditor = createTestEditor([Paragraph, LegacyShortcodes, Markdown]) @@ -31,20 +31,42 @@ describe("ResourceEmbed plugin", () => { test.each(LEGACY_SHORTCODES)( "should take in and return %p closing shortcode", async shortcode => { - const md = `{{< /${shortcode} >}}` + const md1 = `{{< /${shortcode} >}}` + const editor1 = await getEditor(md1) + // @ts-ignore + expect(editor1.getData()).toBe(md1) + + /** + * Hugo documentation uses the first version so it should be preferred. + * But Hugo accepts this version and image-gallery uses it. + * The editor will normalize it to the first version. + */ + const md2 = `{{}}` + const editor2 = await getEditor(md2) + // @ts-ignore + expect(editor2.getData()).toBe(md1) + } + ) + + test.each(LEGACY_SHORTCODES)( + "should take in and return %p shortcode with positional parameters", + async shortcode => { + const md = `{{< ${shortcode} arguments "and chemistry H{{< sub 2 >}}0" >}}` + const expected = `{{< ${shortcode} "arguments" "and chemistry H{{< sub 2 >}}0" >}}` const editor = await getEditor(md) // @ts-ignore - expect(editor.getData()).toBe(md) + expect(editor.getData()).toBe(expected) } ) test.each(LEGACY_SHORTCODES)( - "should take in and return %p shortcode with arguments", + "should take in and return %p shortcode with named parameters", async shortcode => { - const md = `{{< ${shortcode} arguments foo=123 html= >}}` + const md = `{{< ${shortcode} foo=123 html="" >}}` + const expected = `{{< ${shortcode} foo="123" html="" >}}` const editor = await getEditor(md) // @ts-ignore - expect(editor.getData()).toBe(md) + expect(editor.getData()).toBe(expected) } ) diff --git a/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts b/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts index 6164629e6..67bd0f233 100644 --- a/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts +++ b/static/js/lib/ckeditor/plugins/LegacyShortcodes.ts @@ -5,6 +5,7 @@ import { editor } from "@ckeditor/ckeditor5-core" import MarkdownSyntaxPlugin from "./MarkdownSyntaxPlugin" import { TurndownRule } from "../../../types/ckeditor_markdown" import { LEGACY_SHORTCODES } from "./constants" +import { Shortcode } from "./util" const shortcodeClass = (shortcode: string) => `legacy-shortcode-${shortcode}` @@ -18,33 +19,35 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin { get showdownExtension() { return function legacyShortcodeExtension(): ShowdownExtension[] { - return LEGACY_SHORTCODES.map( - (shortcode: string): ShowdownExtension => { - const shortcodeRegex = new RegExp(`{{< /?${shortcode} (.*?)>}}`, "g") - const closingShortcodeRegex = new RegExp( - `{{< /${shortcode} (.*?)>}}`, - "g" - ) - - return { - type: "lang", - regex: shortcodeRegex, - replace: (stringMatch: string, shortcodeArgs: string | null) => { - const isClosing = JSON.stringify( - closingShortcodeRegex.test(stringMatch) - ) - - const tag = `` - - return tag - } + const nameRegex = new RegExp(LEGACY_SHORTCODES.join("|")) + return [ + { + type: "lang", + /** + * It's important that there's a single regex for all the legacy + * shortcodes, rather than one per shortcode. Otherwise the order of + * the replacements is important. + * + * For example, image-gallery-item and sub are both legacy shortcodes + * and sometimes they are used together: + * {{< image-gallery-item ... "H{{< sub 2 >}}O" >}} + * If separate regexes are used, then image-gallery-item would need to + * come before sub so that the sub-replacement is not used on the above + * example. + */ + regex: Shortcode.regex(nameRegex, false), + replace: (stringMatch: string) => { + const shortcode = Shortcode.fromString(stringMatch) + const { isClosing } = shortcode + const params = shortcode.params.map(p => p.toHugo()).join(" ") + const tag = `` + + return tag } } - ) + ] } } @@ -64,7 +67,7 @@ class LegacyShortcodeSyntax extends MarkdownSyntaxPlugin { return `{{< ${isClosingTag ? "/" : ""}${shortcode} ${ rawShortcodeArgs !== undefined && rawShortcodeArgs !== null ? - decodeURIComponent(rawShortcodeArgs) : + `${decodeURIComponent(rawShortcodeArgs)} ` : "" }>}}` } diff --git a/static/js/lib/ckeditor/plugins/util.test.ts b/static/js/lib/ckeditor/plugins/util.test.ts index 57cee29d3..cafd14b11 100644 --- a/static/js/lib/ckeditor/plugins/util.test.ts +++ b/static/js/lib/ckeditor/plugins/util.test.ts @@ -46,15 +46,15 @@ describe("unescapeStringQuotedWith", () => { }) describe("Shortcode", () => { - describe("Shortcode.parse", () => { + describe("Shortcode.fromString", () => { it("parses shortcodes with named params", () => { const text = - '{{< some_shortcode cool_arg="cats and dogs" href_uuid=uuid456 >}}' + '{{< some_shortcode param-with-dash="cats and dogs" with_underscore=uuid456 >}}' const result = Shortcode.fromString(text) const params = [ - { name: "cool_arg", value: "cats and dogs" }, - { name: "href_uuid", value: "uuid456" } + { name: "param-with-dash", value: "cats and dogs" }, + { name: "with_underscore", value: "uuid456" } ].map(makeParams) expect(result).toStrictEqual( new Shortcode("some_shortcode", params, false) @@ -165,6 +165,28 @@ describe("Shortcode", () => { ) }) + it.each([ + { + text: "{{< /some_shortcode >}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{< / some_shortcode >}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{}}", + expected: new Shortcode("some_shortcode", [], false, true) + }, + { + text: "{{% /some_shortcode %}}", + expected: new Shortcode("some_shortcode", [], true, true) + } + ])("parses closing shortcodes", ({ text, expected }) => { + const result = Shortcode.fromString(text) + expect(result).toStrictEqual(expected) + }) + it("does not allow mixing named and positional params", () => { const text = '{{< resource uuid123 href_uuid="uuid456" >}}' expect(() => Shortcode.fromString(text)).toThrow( @@ -367,21 +389,37 @@ describe("Shortcode", () => { ]) }) + it("captures closing and opening shortcodes", () => { + const regex = Shortcode.regex("some_shortcode", false) + const text = ` + a {{< some_shortcode xyz >}} b + c {{< /some_shortcode >}} d + e {{}} f + ` + const match = text.match(regex) + expect(match).toStrictEqual([ + "{{< some_shortcode xyz >}}", + "{{< /some_shortcode >}}", + "{{}}" + ]) + }) + it.each([ { - name: "some_shortcode", - text: "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}", - expected: "{{< some_shortcode xyz >}}" + name: "some_shortcode", + text: + "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}} {{< /some_shortcode xyz >}}", + expected: ["{{< some_shortcode xyz >}}", "{{< /some_shortcode xyz >}}"] }, { name: "some_shortcode_two", text: "a {{< some_shortcode xyz >}} b {{< some_shortcode_two 123 >}}", - expected: "{{< some_shortcode_two 123 >}}" + expected: ["{{< some_shortcode_two 123 >}}"] } ])("captures shortcodes of specified name", ({ text, expected, name }) => { const regex = Shortcode.regex(name, false) const match = text.match(regex) - expect(match).toStrictEqual([expected]) + expect(match).toStrictEqual(expected) }) it("tolerates an odd number of escaped quotes", () => { @@ -390,5 +428,12 @@ describe("Shortcode", () => { const match = text.match(regex) expect(match).toStrictEqual(['{{< shortcode "cat \\" >}}" >}}']) }) + + it("can take a regex as name parameter", () => { + const regex = Shortcode.regex(/cat|dog/, false) + const text = "a {{< cat meow >}} {{< dog woof >}} {{< dragon roar >}} b" + const match = text.match(regex) + expect(match).toStrictEqual(["{{< cat meow >}}", "{{< dog woof >}}"]) + }) }) }) diff --git a/static/js/lib/ckeditor/plugins/util.ts b/static/js/lib/ckeditor/plugins/util.ts index c70d36176..2bd54f5ab 100644 --- a/static/js/lib/ckeditor/plugins/util.ts +++ b/static/js/lib/ckeditor/plugins/util.ts @@ -102,14 +102,18 @@ export class Shortcode { isPercentDelimited: boolean + isClosing: boolean + constructor( name: string, params: ShortcodeParam[], - isPercentDelimited = false + isPercentDelimited = false, + isClosing = false ) { this.name = name this.params = params this.isPercentDelimited = isPercentDelimited + this.isClosing = isClosing const hasPositionalParams = params.some(p => p.name === undefined) const hasNamedParams = params.some(p => p.name !== undefined) @@ -127,19 +131,22 @@ export class Shortcode { */ toHugo() { const stringifiedArgs = this.params.map(p => p.toHugo()) - const interior = [this.name, ...stringifiedArgs].join(" ") + const name = this.isClosing ? `/${this.name}` : this.name + const interior = [name, ...stringifiedArgs].join(" ") if (this.isPercentDelimited) { return `{{% ${interior} %}}` } return `{{< ${interior} >}}` } + private static IS_CLOSING_REGEXP = /\s*(?\/)?\s*/ + /** * Regexp used for matching individual shortcode arguments. */ private static ARG_REGEXP = new RegExp( [ - /((?[a-zA-Z_]+)=)?/.source, + /((?[a-zA-Z\-_]+)=)?/.source, "(", /("(?.*?)(? { return new ShortcodeParam( @@ -167,7 +180,7 @@ export class Shortcode { ) }) - return new Shortcode(name, params, isPercentDelmited) + return new Shortcode(name, params, isPercentDelmited, isClosing) } /** @@ -231,13 +244,15 @@ export class Shortcode { * Returns a global regex that matches shortcodes of given name. Useful for * use in Showdown rules. */ - static regex(name: string, isPercentDelimited: boolean) { + static regex(name: string | RegExp, isPercentDelimited: boolean) { const opener = isPercentDelimited ? "{{%" : "{{<" const closer = isPercentDelimited ? "%}}" : ">}}" const regex = new RegExp( [ opener, - String.raw`\s${name}\s`, + Shortcode.IS_CLOSING_REGEXP.source, + name instanceof RegExp ? `(${name.source})` : name, + String.raw`\s`, /** * Non-greedily capture anything up until the closer. Except if there is * a non-escaped quotation mark, then there must be another. diff --git a/static/js/query-configs/websites.ts b/static/js/query-configs/websites.ts index 7ea89d87f..df12d8ce4 100644 --- a/static/js/query-configs/websites.ts +++ b/static/js/query-configs/websites.ts @@ -158,7 +158,11 @@ export const websiteMutation = (payload: NewWebsitePayload): QueryConfig => ({ } }) -export const websiteAction = (name: string, action: string): QueryConfig => ({ +export const websiteAction = ( + name: string, + action: string, + payload: WebsitePublishPayload +): QueryConfig => ({ url: siteApiActionUrl.param({ name, action }).toString(), options: { method: "POST", @@ -166,7 +170,7 @@ export const websiteAction = (name: string, action: string): QueryConfig => ({ "X-CSRFTOKEN": getCookie("csrftoken") || "" } }, - body: {} + body: payload }) export const websiteStartersRequest = (): QueryConfig => ({ @@ -464,6 +468,11 @@ export type NewWebsiteContentPayload = { text_id?: string } +export type WebsitePublishPayload = { + // eslint-disable-next-line camelcase + url_path?: string +} + export const createWebsiteContentMutation = ( siteName: string, payload: NewWebsiteContentPayload diff --git a/static/js/resources/ocw-course-site-config.json b/static/js/resources/ocw-course-site-config.json index 0fddc336c..8427a57a9 100644 --- a/static/js/resources/ocw-course-site-config.json +++ b/static/js/resources/ocw-course-site-config.json @@ -866,5 +866,6 @@ "name": "menu" } ], - "root-url-path": "courses" + "root-url-path": "courses", + "site-url-format": "[sitemetadata:primary_course_number]-[sitemetadata:course_title]-[sitemetadata:term]-[sitemetadata:year]" } \ No newline at end of file diff --git a/static/js/types/websites.ts b/static/js/types/websites.ts index 7afac74b9..60d881436 100644 --- a/static/js/types/websites.ts +++ b/static/js/types/websites.ts @@ -251,6 +251,9 @@ export type Website = WebsiteStatus & { has_unpublished_draft: boolean has_unpublished_live: boolean content_warnings?: Array + url_path: string | null + url_suggestion: string + s3_path: string | null } type WebsiteRoleEditable = typeof ROLE_ADMIN | typeof ROLE_EDITOR diff --git a/static/js/util/factories/websites.ts b/static/js/util/factories/websites.ts index 81b0cdee9..b7709972c 100644 --- a/static/js/util/factories/websites.ts +++ b/static/js/util/factories/websites.ts @@ -214,6 +214,9 @@ export const makeWebsiteDetail = ( sync_errors: null, is_admin: casual.boolean, content_warnings: [], + url_path: null, + url_suggestion: "[sitemetadata:primary_course_number]-[sitemetdata:title]", + s3_path: `courses/${casual.word}`, ...overrides }) diff --git a/videos/api.py b/videos/api.py index a0814ceec..e9bc72440 100644 --- a/videos/api.py +++ b/videos/api.py @@ -2,7 +2,6 @@ import json import logging import os -from urllib.parse import urljoin, urlparse import boto3 from django.conf import settings @@ -34,9 +33,11 @@ def prepare_video_download_file(video: Video): ).first() if not video_file: return - new_s3_key = urljoin( - f"{urlparse(video.website.get_url()).path}/", - f"{video_file.s3_key.split('/')[-1]}", + new_s3_key = "/".join( + [ + f"{video.website.s3_path}", + f"{video_file.s3_key.split('/')[-1]}", + ] ).strip("/") if new_s3_key != video_file.s3_key: move_s3_object(video_file.s3_key, new_s3_key) diff --git a/videos/api_test.py b/videos/api_test.py index fe3ab7b7d..b73cad707 100644 --- a/videos/api_test.py +++ b/videos/api_test.py @@ -106,9 +106,9 @@ def test_prepare_video_download_file(settings, mocker, files_exist): if files_exist: mock_move_s3.assert_called_once_with( f"{settings.VIDEO_S3_TRANSCODE_PREFIX}/fakejobid/{video.website.name}/{dl_video_name}", - f"sites/{video.website.name}/{dl_video_name}", + f"{video.website.s3_path}/{dl_video_name}", ) - assert content.file.name == f"sites/{video.website.name}/{dl_video_name}" + assert content.file.name == f"{video.website.s3_path}/{dl_video_name}" else: mock_move_s3.assert_not_called() assert content.file.name == "" diff --git a/websites/api.py b/websites/api.py index b8c22f8fe..fdc2a518a 100644 --- a/websites/api.py +++ b/websites/api.py @@ -279,7 +279,7 @@ def mail_on_publish(website_name: str, version: str, success: bool, user_id: int { "site": { "title": website.title, - "url": website.get_url(version), + "url": website.get_full_url(version), }, "version": version, }, diff --git a/websites/api_test.py b/websites/api_test.py index b5b1f23e3..73cca9de3 100644 --- a/websites/api_test.py +++ b/websites/api_test.py @@ -372,7 +372,7 @@ def test_mail_on_publish(settings, mocker, success, version, permission_groups): { "site": { "title": website.title, - "url": f"http://test.{version}.edu/{website.starter.config['root-url-path']}/{website.name}", + "url": f"http://test.{version}.edu/{website.url_path}", }, "version": version, }, diff --git a/websites/config_schema/site-config-schema.yml b/websites/config_schema/site-config-schema.yml index b84f5d0f4..22dc4528a 100644 --- a/websites/config_schema/site-config-schema.yml +++ b/websites/config_schema/site-config-schema.yml @@ -1,4 +1,5 @@ root-url-path: str() +site-url-format: str(required=False) content-dir: str(required=False) collections: list(include('content_item')) --- diff --git a/websites/conftest.py b/websites/conftest.py index 74eecc168..5e5919563 100644 --- a/websites/conftest.py +++ b/websites/conftest.py @@ -9,7 +9,12 @@ from users.factories import UserFactory from websites import constants -from websites.factories import WebsiteContentFactory, WebsiteFactory +from websites.constants import CONTENT_TYPE_METADATA +from websites.factories import ( + WebsiteContentFactory, + WebsiteFactory, + WebsiteStarterFactory, +) from websites.permissions import create_global_groups @@ -30,7 +35,7 @@ def permission_groups(): site_admin, site_editor, ) = UserFactory.create_batch(5) - websites = WebsiteFactory.create_batch(2, owner=site_owner) + websites = WebsiteFactory.create_batch(2, owner=site_owner, with_url_path=True) global_admin.groups.add(Group.objects.get(name=constants.GLOBAL_ADMIN)) global_author.groups.add(Group.objects.get(name=constants.GLOBAL_AUTHOR)) site_admin.groups.add(websites[0].admin_group) @@ -105,3 +110,15 @@ def site_config_singleton_only(basic_site_config): "file" in file_config_item ), "Expected collections.2.files.0 to be a singleton config item" return {**site_config, "collections": [files_config_item]} + + +@pytest.fixture() +def ocw_site(parsed_site_config): + """ OCW Course site with metadata""" + website = WebsiteFactory.create( + starter=WebsiteStarterFactory.create(config=parsed_site_config), + not_published=True, + url_path=None, + ) + WebsiteContentFactory(type=CONTENT_TYPE_METADATA, website=website) + return website diff --git a/websites/constants.py b/websites/constants.py index 0d37f814d..3b94ad905 100644 --- a/websites/constants.py +++ b/websites/constants.py @@ -27,6 +27,7 @@ WEBSITE_CONFIG_CONTENT_DIR_KEY = "content-dir" WEBSITE_CONFIG_DEFAULT_CONTENT_DIR = "content" WEBSITE_CONFIG_ROOT_URL_PATH_KEY = "root-url-path" +WEBSITE_CONFIG_SITE_URL_FORMAT_KEY = "site-url-format" WEBSITE_CONTENT_FILETYPE = "md" CONTENT_MENU_FIELD = "menu" diff --git a/websites/factories.py b/websites/factories.py index 4d6d1b447..9b98822c2 100644 --- a/websites/factories.py +++ b/websites/factories.py @@ -40,6 +40,7 @@ class WebsiteFactory(DjangoModelFactory): title = factory.Sequence(lambda n: "Site %s" % n) name = factory.Sequence(lambda n: "site-name-%s" % n) + url_path = factory.Sequence(lambda n: "courses/site-path-%s" % n) short_id = factory.Sequence(lambda n: "site-shortid-%s" % n) metadata = factory.Faker("json") publish_date = factory.Faker("date_time", tzinfo=pytz.utc) @@ -54,6 +55,9 @@ class Meta: model = Website class Params: + with_url_path = factory.Trait( + url_path=factory.Sequence(lambda n: "courses/site-path-%s" % n) + ) published = factory.Trait( publish_date=factory.Faker("past_datetime", tzinfo=pytz.utc), first_published_to_production=factory.Faker( diff --git a/websites/migrations/0049_website_url_path.py b/websites/migrations/0049_website_url_path.py new file mode 100644 index 000000000..a134b83ea --- /dev/null +++ b/websites/migrations/0049_website_url_path.py @@ -0,0 +1,39 @@ +# Generated by Django 3.1.14 on 2022-05-12 18:40 +from urllib.parse import urljoin + +from django.conf import settings +from django.db import migrations, models, transaction + +from websites.constants import CONTENT_TYPE_METADATA +from websites.site_config_api import SiteConfig +from websites.utils import set_dict_field + + +def populate_url_path(apps, schema_editor): + """ + Populate url_path for all sites that have already been published to production + """ + Website = apps.get_model("websites", "Website") + for site in Website.objects.exclude(publish_date__isnull=True): + if site.starter is None: + continue + site_config = SiteConfig(site.starter.config) + root = site_config.root_url_path + site.url_path = "/".join([root, site.name]).strip("/") + site.save() + + +class Migration(migrations.Migration): + + dependencies = [ + ("websites", "0048_www_pages_to_page"), + ] + + operations = [ + migrations.AddField( + model_name="website", + name="url_path", + field=models.CharField(max_length=2048, null=True, blank=True, unique=True), + ), + migrations.RunPython(populate_url_path, migrations.RunPython.noop), + ] diff --git a/websites/models.py b/websites/models.py index 0370a322e..1a069a187 100644 --- a/websites/models.py +++ b/websites/models.py @@ -1,8 +1,10 @@ """ websites models """ import json +import logging +import re from hashlib import sha256 from typing import Dict -from urllib.parse import urljoin +from urllib.parse import urljoin, urlparse from uuid import uuid4 import yaml @@ -29,9 +31,13 @@ CONTENT_DIRPATH_MAX_LEN, CONTENT_FILENAME_MAX_LEN, CONTENT_FILEPATH_UNIQUE_CONSTRAINT, + CONTENT_TYPE_METADATA, ) from websites.site_config_api import SiteConfig -from websites.utils import permissions_group_name_for_role +from websites.utils import get_dict_field, permissions_group_name_for_role + + +log = logging.getLogger(__name__) def validate_yaml(value): @@ -127,6 +133,13 @@ class Website(TimestampedModel): related_name="unpublisher", ) + """ + URL path values should include the starter config prefix (ie "courses/") so that sites + with a url_path of "courses/my-site-fall-2020" can coexist with "sites/my-site-fall-2020" + without unique key violations being raised. + """ + url_path = models.CharField(max_length=2048, unique=True, blank=True, null=True) + @property def unpublished(self): """ Indicate whether or not site has been unpublished""" @@ -161,7 +174,16 @@ def collaborators(self): + list(self.editor_group.user_set.all()) ) - def get_url(self, version=VERSION_LIVE): + def get_site_root_path(self): + """Get the site root url path""" + if self.starter is None: + return None + site_config = SiteConfig(self.starter.config) + if site_config: + return site_config.root_url_path + return "" + + def get_full_url(self, version=VERSION_LIVE): """Get the home page (live or draft) of the website""" if self.starter is None: # if there is no starter, there is no ability to publish @@ -172,13 +194,72 @@ def get_url(self, version=VERSION_LIVE): if version == VERSION_LIVE else settings.OCW_STUDIO_DRAFT_URL ) - site_config = SiteConfig(self.starter.config) - site_url = ( - "" - if self.name == settings.ROOT_WEBSITE_NAME - else f"{site_config.root_url_path}/{self.name}".strip("/") + url_path = self.url_path + if url_path: + return urljoin(base_url, url_path) + else: + return urljoin(base_url, self.get_site_root_path()) + + def get_url_path(self, with_prefix=True): + """Get the current/potential url path, with or without site prefix""" + url_path = self.url_path + if not url_path: + sitemeta = self.websitecontent_set.filter( + type=CONTENT_TYPE_METADATA + ).first() + url_path = self.url_path_from_metadata( + metadata=sitemeta.metadata if sitemeta else None + ) + root_path = self.get_site_root_path() + if with_prefix: + if root_path and not url_path.startswith(root_path): + url_path = self.assemble_full_url_path(url_path) + elif url_path is not None: + url_path = re.sub(f"^{root_path}/", "", url_path, 1) + return url_path + + def assemble_full_url_path(self, path): + """Combine site prefix and url path""" + return "/".join( + part.strip("/") for part in [self.get_site_root_path(), path] if part ) - return urljoin(base_url, site_url) + + def url_path_from_metadata(self, metadata: Dict = None): + """ Get the url path based on site config and metadata""" + if self.starter is None: + return None + site_config = SiteConfig(self.starter.config) + url_format = site_config.site_url_format + if not url_format or self.publish_date: + # use name for published sites or for any sites without a `url_path` in config. + url_format = self.name + elif url_format: + for section in re.findall(r"(\[.+?\])+", site_config.site_url_format) or []: + section_type, section_field = re.sub(r"[\[\]]+", "", section).split(":") + value = None + if metadata: + value = get_dict_field(metadata, section_field) + if not metadata or not value: + content = self.websitecontent_set.filter(type=section_type).first() + if content: + value = get_dict_field(content.metadata, section_field) + if not value: + # Incomplete metadata required for url + value = section + else: + value = slugify(value.replace(".", "-")) + url_format = url_format.replace(section, value) + return url_format + + @property + def s3_path(self): + """ Get the S3 object path for uploaded files""" + site_config = SiteConfig(self.starter.config) + url_parts = [ + site_config.root_url_path, + self.name, + ] + return "/".join([part.strip("/") for part in url_parts if part]) class Meta: permissions = ( @@ -208,10 +289,8 @@ class WebsiteContent(TimestampedModel, SafeDeleteModel): def upload_file_to(self, filename): """Return the appropriate filepath for an upload""" - site_config = SiteConfig(self.website.starter.config) url_parts = [ - site_config.root_url_path, - self.website.name, + self.website.s3_path, f"{self.text_id.replace('-', '')}_{filename}", ] return "/".join([part for part in url_parts if part != ""]) @@ -287,7 +366,12 @@ def full_metadata(self) -> Dict: else {} ) if self.file and self.file.url: - full_metadata[file_field["name"]] = self.file.url + s3_path = self.website.s3_path + url_path = self.website.url_path + file_url = self.file.url + if url_path and s3_path != url_path: + file_url = file_url.replace(s3_path, url_path, 1) + full_metadata[file_field["name"]] = urlparse(file_url).path else: full_metadata[file_field["name"]] = None return full_metadata diff --git a/websites/models_test.py b/websites/models_test.py index 6ccc26ebe..48c015b3d 100644 --- a/websites/models_test.py +++ b/websites/models_test.py @@ -3,12 +3,18 @@ import pytest from django.core.files.uploadedfile import SimpleUploadedFile +from mitol.common.utils import now_in_utc +from websites.constants import WEBSITE_CONFIG_ROOT_URL_PATH_KEY from websites.factories import ( WebsiteContentFactory, WebsiteFactory, WebsiteStarterFactory, ) +from websites.site_config_api import SiteConfig + + +pytestmark = pytest.mark.django_db @pytest.mark.parametrize( @@ -48,7 +54,6 @@ def test_websitecontent_calculate_checksum(metadata, markdown, dirpath, exp_chec assert content.calculate_checksum() == exp_checksum -@pytest.mark.django_db @pytest.mark.parametrize("has_file_widget", [True, False]) @pytest.mark.parametrize("has_file", [True, False]) def test_websitecontent_full_metadata(has_file_widget, has_file): @@ -90,7 +95,6 @@ def test_websitecontent_full_metadata(has_file_widget, has_file): assert content.full_metadata == {"title": title, "description": description} -@pytest.mark.django_db def test_website_starter_unpublished(): """Website should set has_unpublished_live and has_unpublished_draft if the starter is updated""" website = WebsiteFactory.create( @@ -108,7 +112,6 @@ def test_website_starter_unpublished(): assert second_website.has_unpublished_live is True -@pytest.mark.django_db def test_website_content_unpublished(): """Website should set has_unpublished_live and has_unpublished_draft if any related content is updated""" website = WebsiteFactory.create() @@ -128,7 +131,6 @@ def test_website_content_unpublished(): assert website.has_unpublished_draft is True -@pytest.mark.django_db @pytest.mark.parametrize( "name,root_url,is_home,version,expected_path", [ @@ -136,10 +138,10 @@ def test_website_content_unpublished(): ["ocw-home-page", "", True, "draft", ""], ], ) -def test_website_get_url( +def test_website_get_full_url( settings, name, root_url, is_home, version, expected_path ): # pylint:disable=too-many-arguments - """Verify that Website.get_url returns the expected value""" + """Verify that Website.get_full_url returns the expected value""" settings.OCW_STUDIO_LIVE_URL = "http://test-live.edu" settings.OCW_STUDIO_DRAFT_URL = "http://test-draft.edu" expected_domain = ( @@ -151,13 +153,113 @@ def test_website_get_url( starter.config["root-url-path"] = root_url starter.save() settings.ROOT_WEBSITE_NAME = name if is_home else "test-home" - website = WebsiteFactory.create(name=name, starter=starter) - assert website.get_url(version) == urljoin(expected_domain, expected_path) + website = WebsiteFactory.create(name=name, starter=starter, url_path=expected_path) + assert website.get_full_url(version) == urljoin(expected_domain, expected_path) -@pytest.mark.django_db -def test_website_get_url_no_starter(): - """Verify that Website.get_url returns None if there is no starter""" +def test_website_get_full_url_no_starter(): + """Verify that Website.get_full_url returns None if there is no starter""" website = WebsiteFactory.create(starter=None) - assert website.get_url("draft") is None - assert website.get_url("live") is None + assert website.get_full_url("draft") is None + assert website.get_full_url("live") is None + + +@pytest.mark.parametrize( + "name,root_url,expected_path", + [ + ["test-course-1", "courses", "courses/test-course-1"], + ["test-course-2", "sites", "sites/test-course-2"], + ["test-course-3", "", "test-course-3"], + ], +) +def test_website_s3_path(name, root_url, expected_path): + """The correct s3 path should be returned for a site""" + starter = WebsiteStarterFactory.create(config={"root-url-path": root_url}) + website = WebsiteFactory.create(name=name, starter=starter) + assert website.s3_path == expected_path + + +def test_website_url_path_from_metadata_no_starter(): + """ None should be returned for a site without a starter""" + assert WebsiteFactory.build(starter=None).url_path_from_metadata() is None + + +def test_website_url_path_from_metadata_no_format(): + """ Website.name should be returned for a site without site-url-format in starter config""" + starter = WebsiteStarterFactory.create( + config={WEBSITE_CONFIG_ROOT_URL_PATH_KEY: "sites"} + ) + website = WebsiteFactory.build(starter=starter) + assert website.url_path_from_metadata() == website.name + + +def test_website_url_path_from_metadata_published(ocw_site): + """ Website.name should be returned for a that's already been published""" + ocw_site.publish_date = now_in_utc() + assert ocw_site.url_path_from_metadata() == ocw_site.name + + +@pytest.mark.parametrize( + "missing_keys", [[], ["course_nr", "year"], ["term"], ["title", "year"]] +) +def test_website_url_path_from_metadata(missing_keys): + """ The expected url should be returned based on starter config format and supplied metadata""" + metadata = { + "course_nr": "1.1", + "title": "My Course", + "term": "Fall", + "year": "2025", + } + for key in missing_keys: + metadata.pop(key) + starter = WebsiteStarterFactory.create( + config={ + "site-url-format": "[meta:course_nr]-[meta:title]-[meta:term]-[meta:year]" + } + ) + website = WebsiteFactory.create(starter=starter, not_published=True) + expected_url_path = "{course_nr}-{title}-{term}-{year}".format( + course_nr=( + "1-1" if metadata.get("course_nr") is not None else "[meta:course_nr]" + ), + title=("my-course" if metadata.get("title") is not None else "[meta:title]"), + term=("fall" if metadata.get("term") is not None else "[meta:term]"), + year=metadata.get("year", "[meta:year]"), + ) + assert website.url_path_from_metadata(metadata) == expected_url_path + + +@pytest.mark.parametrize("root_path", ["", "courses", "sites"]) +@pytest.mark.parametrize("url_path", ["", "my-site", "other-site-fall-2024"]) +def test_assemble_full_url_path(root_path, url_path): + """assemble_full_url_path should combine the root url path and site url path""" + website = WebsiteFactory.create( + starter=WebsiteStarterFactory( + config={WEBSITE_CONFIG_ROOT_URL_PATH_KEY: root_path} + ) + ) + assert website.assemble_full_url_path(url_path) == f"{root_path}/{url_path}".strip( + "/" + ) + + +@pytest.mark.parametrize( + "url_path", [None, "courses/1-1-my-site-fall-2019", "courses/2-2-my-site-fall-2049"] +) +@pytest.mark.parametrize("with_prefix", [True, False]) +@pytest.mark.parametrize("published", [True, False]) +def test_get_url_path(ocw_site, url_path, with_prefix, published): + """get_url_path should return the expected url path with or without a prefix""" + ocw_site.publish_date = now_in_utc() if published else None + ocw_site.url_path = url_path + config = SiteConfig(ocw_site.starter.config) + root_path = config.root_url_path + if url_path is None and published: + site_path = ocw_site.name + else: + site_path = ( + (url_path or config.site_url_format).replace(root_path, "").strip("/") + ) + assert ocw_site.get_url_path(with_prefix=with_prefix) == ( + f"{config.root_url_path}/{site_path}" if with_prefix else site_path + ) diff --git a/websites/serializers.py b/websites/serializers.py index 2094b14b7..35fb55f53 100644 --- a/websites/serializers.py +++ b/websites/serializers.py @@ -1,5 +1,6 @@ """ Serializers for websites """ import logging +import re from collections import defaultdict from urllib.parse import urljoin @@ -10,12 +11,9 @@ from rest_framework import serializers from rest_framework.exceptions import ValidationError -from content_sync.api import ( - create_website_backend, - create_website_publishing_pipeline, - update_website_backend, -) +from content_sync.api import create_website_backend, update_website_backend from content_sync.constants import VERSION_DRAFT, VERSION_LIVE +from content_sync.models import ContentSyncState from gdrive_sync.api import gdrive_root_url, is_gdrive_enabled from gdrive_sync.tasks import create_gdrive_folders from main.serializers import RequestUserSerializerMixin @@ -27,7 +25,7 @@ sync_website_title, update_youtube_thumbnail, ) -from websites.constants import CONTENT_TYPE_METADATA +from websites.constants import CONTENT_TYPE_METADATA, CONTENT_TYPE_RESOURCE from websites.models import Website, WebsiteContent, WebsiteStarter from websites.permissions import is_global_admin, is_site_admin from websites.site_config_api import SiteConfig @@ -99,16 +97,63 @@ class Meta: "metadata", "starter", "owner", + "url_path", ] extra_kwargs = {"owner": {"write_only": True}} -class WebsitePublishSerializer(serializers.ModelSerializer): +class WebsiteUrlSerializer(serializers.ModelSerializer): + """ Serializer for assigning website urls """ + + def validate_url_path(self, value): + """ + Check that the website url will be unique and template sections have been replaced. + """ + if not value and self.instance.url_path is None: + raise serializers.ValidationError("The URL path cannot be blank") + url = self.instance.assemble_full_url_path(value) + if self.instance.publish_date and url != self.instance.url_path: + raise serializers.ValidationError( + "The URL cannot be changed after publishing." + ) + if re.findall(r"[\[\]]+", url): + raise serializers.ValidationError( + "You must replace the url sections in brackets" + ) + if ( + url + and Website.objects.filter(url_path=url) + .exclude(pk=self.instance.pk) + .exists() + ): + raise serializers.ValidationError("The website URL is not unique") + return value + + def update(self, instance, validated_data): + """ Update the website url_path""" + url_path = validated_data.get("url_path") + with transaction.atomic(): + instance.url_path = instance.assemble_full_url_path(url_path) + instance.save() + # Force a backend resync of all associated content with file paths + ContentSyncState.objects.filter( + content__in=instance.websitecontent_set.filter(file__isnull=False) + ).update(synced_checksum=None) + + class Meta: + model = Website + fields = [ + "url_path", + ] + + +class WebsiteMassBuildSerializer(serializers.ModelSerializer): """ Serializer for mass building websites """ starter_slug = serializers.SerializerMethodField(read_only=True) base_url = serializers.SerializerMethodField(read_only=True) site_url = serializers.SerializerMethodField(read_only=True) + s3_path = serializers.SerializerMethodField(read_only=True) def get_starter_slug(self, instance): """Get the website starter slug""" @@ -116,8 +161,11 @@ def get_starter_slug(self, instance): def get_site_url(self, instance): """Get the website relative url""" - site_config = SiteConfig(instance.starter.config) - return f"{site_config.root_url_path}/{instance.name}".strip("/") + return instance.url_path + + def get_s3_path(self, instance): + """Get the website s3 path""" + return instance.s3_path def get_base_url(self, instance): """Get the base url (should be same as site_url except for the root site)""" @@ -127,13 +175,7 @@ def get_base_url(self, instance): class Meta: model = Website - fields = [ - "name", - "short_id", - "starter_slug", - "site_url", - "base_url", - ] + fields = ["name", "short_id", "starter_slug", "site_url", "base_url", "s3_path"] read_only_fields = fields @@ -145,8 +187,7 @@ class WebsiteUnpublishSerializer(serializers.ModelSerializer): def get_site_url(self, instance): """Get the website relative url""" - site_config = SiteConfig(instance.starter.config) - return f"{site_config.root_url_path}/{instance.name}".strip("/") + return instance.url_path def get_site_uid(self, instance): """Get the website uid""" @@ -170,11 +211,22 @@ class Meta: read_only_fields = fields +class WebsiteUrlSuggestionMixin(serializers.Serializer): + """Add the url_suggestion custom field""" + + url_suggestion = serializers.SerializerMethodField(read_only=True) + + def get_url_suggestion(self, instance): + """Get the current or potential url path for the site""" + return instance.get_url_path(with_prefix=False) + + class WebsiteDetailSerializer( serializers.ModelSerializer, WebsiteGoogleDriveMixin, WebsiteValidationMixin, RequestUserSerializerMixin, + WebsiteUrlSuggestionMixin, ): """ Serializer for websites with serialized config """ @@ -192,11 +244,11 @@ def get_is_admin(self, obj): def get_live_url(self, instance): """Get the live url for the site""" - return instance.get_url(version=VERSION_LIVE) + return instance.get_full_url(version=VERSION_LIVE) def get_draft_url(self, instance): """Get the draft url for the site""" - return instance.get_url(version=VERSION_DRAFT) + return instance.get_full_url(version=VERSION_DRAFT) def update(self, instance, validated_data): """ Remove owner attribute if present, it should not be changed""" @@ -212,9 +264,10 @@ class Meta: "is_admin", "draft_url", "live_url", + "url_path", + "url_suggestion", "has_unpublished_live", "has_unpublished_draft", - "gdrive_url", "live_publish_status", "live_publish_status_updated_on", "draft_publish_status", @@ -244,11 +297,15 @@ class Meta: "sync_errors", "synced_on", "content_warnings", + "url_path", ] class WebsiteStatusSerializer( - serializers.ModelSerializer, WebsiteGoogleDriveMixin, WebsiteValidationMixin + serializers.ModelSerializer, + WebsiteGoogleDriveMixin, + WebsiteValidationMixin, + WebsiteUrlSuggestionMixin, ): """Serializer for website status fields""" @@ -271,6 +328,7 @@ class Meta: "sync_errors", "synced_on", "content_warnings", + "url_suggestion", ] read_only_fields = fields @@ -293,7 +351,6 @@ def create(self, validated_data): with transaction.atomic(): website = super().create(validated_data) create_website_backend(website) - create_website_publishing_pipeline(website) create_gdrive_folders.delay(website.short_id) return website @@ -403,7 +460,7 @@ class WebsiteContentDetailSerializer( def update(self, instance, validated_data): """Add updated_by to the data""" - if instance.type == "resource": + if instance.type == CONTENT_TYPE_RESOURCE: update_youtube_thumbnail( instance.website.uuid, validated_data.get("metadata"), overwrite=True ) @@ -528,7 +585,7 @@ def create(self, validated_data): for field in {"is_page_content", "filename", "dirpath", "text_id"} if field in self.context } - if validated_data.get("type") == "resource": + if validated_data.get("type") == CONTENT_TYPE_RESOURCE: update_youtube_thumbnail( self.context["website_id"], validated_data.get("metadata") ) diff --git a/websites/serializers_test.py b/websites/serializers_test.py index b76d97f61..96789b00d 100644 --- a/websites/serializers_test.py +++ b/websites/serializers_test.py @@ -5,6 +5,7 @@ from dateutil.parser import parse as parse_date from django.core.files.uploadedfile import SimpleUploadedFile from django.db.models import CharField, Value +from mitol.common.utils import now_in_utc from main.constants import ISO_8601_FORMAT from users.factories import UserFactory @@ -14,6 +15,7 @@ CONTENT_TYPE_METADATA, CONTENT_TYPE_RESOURCE, ROLE_EDITOR, + WEBSITE_CONFIG_ROOT_URL_PATH_KEY, WEBSITE_SOURCE_OCW_IMPORT, ) from websites.factories import ( @@ -28,12 +30,13 @@ WebsiteContentDetailSerializer, WebsiteContentSerializer, WebsiteDetailSerializer, - WebsitePublishSerializer, + WebsiteMassBuildSerializer, WebsiteSerializer, WebsiteStarterDetailSerializer, WebsiteStarterSerializer, WebsiteStatusSerializer, WebsiteUnpublishSerializer, + WebsiteUrlSerializer, ) from websites.site_config_api import SiteConfig @@ -49,9 +52,6 @@ def mocked_website_funcs(mocker): update_website_backend=mocker.patch( "websites.serializers.update_website_backend" ), - create_website_pipeline=mocker.patch( - "websites.serializers.create_website_publishing_pipeline" - ), ) @@ -189,8 +189,8 @@ def test_website_detail_serializer( assert ( parse_date(serialized_data["draft_publish_date"]) == website.draft_publish_date ) - assert serialized_data["live_url"] == website.get_url("live") - assert serialized_data["draft_url"] == website.get_url("draft") + assert serialized_data["live_url"] == website.get_full_url("live") + assert serialized_data["draft_url"] == website.get_full_url("draft") assert serialized_data["has_unpublished_live"] == website.has_unpublished_live assert serialized_data["has_unpublished_draft"] == website.has_unpublished_draft assert serialized_data["gdrive_url"] == ( @@ -198,6 +198,8 @@ def test_website_detail_serializer( if drive_credentials is not None and drive_folder is not None else None ) + assert serialized_data["url_path"] == website.url_path + assert serialized_data["url_suggestion"] == website.url_path @pytest.mark.parametrize("user_is_admin", [True, False]) @@ -218,6 +220,42 @@ def test_website_detail_serializer_is_admin(mocker, user_is_admin, has_user): is_site_admin_mock.assert_called_once_with(user, website) +def test_website_detail_serializer_with_url_format(mocker, ocw_site): + """ The url suggestion should be equal to the starter config site-url-format""" + user = UserFactory.create(is_superuser=True) + serialized = WebsiteDetailSerializer( + instance=ocw_site, + context={"request": mocker.Mock(user=user)}, + ).data + assert serialized["url_path"] is None + assert ( + serialized["url_suggestion"] + == SiteConfig(ocw_site.starter.config).site_url_format + ) + + +def test_website_detail_serializer_with_url_format_partial(mocker, ocw_site): + """ The url suggestion should have relevant metadata fields filled in""" + user = UserFactory.create(is_superuser=True) + term = "Fall" + year = "2028" + content = ocw_site.websitecontent_set.get(type=CONTENT_TYPE_METADATA) + content.metadata["term"] = term + content.metadata["year"] = year + content.save() + expected_suggestion = ( + SiteConfig(ocw_site.starter.config) + .site_url_format.replace("[sitemetadata:term]", term.lower()) + .replace("[sitemetadata:year]", year) + ) + serialized = WebsiteDetailSerializer( + instance=ocw_site, + context={"request": mocker.Mock(user=user)}, + ).data + assert serialized["url_path"] is None + assert serialized["url_suggestion"] == expected_suggestion + + def test_website_collaborator_serializer(): """ WebsiteCollaboratorSerializer should serialize a User object with correct fields """ collaborator = ( @@ -256,6 +294,18 @@ def test_website_content_detail_serializer(): assert serialized_data["metadata"] == content.metadata +def test_website_content_detail_serializer_with_url_format(): + """WebsiteContentDetailSerializer should serialize all relevant fields to the frontend""" + content = WebsiteContentFactory.create() + serialized_data = WebsiteContentDetailSerializer(instance=content).data + assert serialized_data["text_id"] == str(content.text_id) + assert serialized_data["title"] == content.title + assert serialized_data["type"] == content.type + assert serialized_data["updated_on"] == content.updated_on.isoformat()[:-6] + "Z" + assert serialized_data["markdown"] == content.markdown + assert serialized_data["metadata"] == content.metadata + + @pytest.mark.parametrize("is_resource", [True, False]) def test_website_content_detail_serializer_youtube_ocw(settings, is_resource): """WebsiteContent serializers should conditionally fill in youtube thumbnail metadata""" @@ -446,7 +496,6 @@ def test_website_content_detail_serializer_save(mocker, mocked_website_funcs): } assert content.updated_by == user mocked_website_funcs.update_website_backend.assert_called_once_with(content.website) - mocked_website_funcs.create_website_pipeline.assert_not_called() @pytest.mark.parametrize("is_new", [True]) @@ -523,7 +572,6 @@ def test_website_content_detail_serializer_save_null_metadata( assert content.metadata == {"meta": "data"} assert content.updated_by == user mocked_website_funcs.update_website_backend.assert_called_once_with(content.website) - mocked_website_funcs.create_website_pipeline.assert_not_called() @pytest.mark.parametrize("add_context_data", [True, False]) @@ -590,13 +638,10 @@ def test_website_content_create_serializer( @pytest.mark.parametrize("is_root_site", [True, False]) def test_website_publish_serializer_base_url(settings, is_root_site): """ The WebsitePublishSerializer should return the correct base_url value """ - site = WebsiteFactory.create() - site_config = SiteConfig(site.starter.config) + site = WebsiteFactory.create(url_path="courses/my-site") settings.ROOT_WEBSITE_NAME = site.name if is_root_site else "some_other_root_name" - serializer = WebsitePublishSerializer(site) - assert serializer.data["base_url"] == ( - "" if is_root_site else f"{site_config.root_url_path}/{site.name}".strip("/") - ) + serializer = WebsiteMassBuildSerializer(site) + assert serializer.data["base_url"] == ("" if is_root_site else site.url_path) @pytest.mark.parametrize("has_metadata", [True, False]) @@ -617,3 +662,53 @@ def test_website_unpublish_serializer(has_legacy_uid, has_metadata): if has_legacy_uid and has_metadata else site.uuid.hex ) + + +def test_website_url_serializer_update(ocw_site, parsed_site_config): + """WebsiteUrlSerializer should update the website url_path""" + new_url_path = "1.45-test-course-fall-2012" + data = {"url_path": new_url_path} + serializer = WebsiteUrlSerializer(ocw_site, data) + assert serializer.is_valid() + assert serializer.validated_data["url_path"] == new_url_path + serializer.update(ocw_site, serializer.validated_data) + ocw_site.refresh_from_db() + assert ( + ocw_site.url_path + == f"{parsed_site_config[WEBSITE_CONFIG_ROOT_URL_PATH_KEY]}/{new_url_path}" + ) + + +def test_website_url_serializer_incomplete_url_path(ocw_site): + """WebsiteUrlSerializer should invalidate a url_path that still has brackets""" + new_url_path = "1.45-test-course-[metadata.semester]-2012" + data = {"url_path": new_url_path} + serializer = WebsiteUrlSerializer(ocw_site, data) + assert serializer.is_valid() is False + assert serializer.errors.get("url_path") == [ + "You must replace the url sections in brackets" + ] + + +def test_website_url_serializer_duplicate_url_path(ocw_site, parsed_site_config): + """WebsiteUrlSerializer should invalidate a duplicate url_path""" + new_url_path = "1.45-test-course-spring-2022" + WebsiteFactory.create( + url_path=f"{parsed_site_config[WEBSITE_CONFIG_ROOT_URL_PATH_KEY]}/{new_url_path}" + ) + 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"] + + +def test_website_url_serializer_url_path_published(ocw_site): + """WebsiteUrlSerializer should invalidate a url_path for a published site""" + ocw_site.publish_date = now_in_utc() + new_url_path = "1.45-test-course-spring-2022" + data = {"url_path": new_url_path} + serializer = WebsiteUrlSerializer(ocw_site, data) + assert serializer.is_valid() is False + assert serializer.errors.get("url_path") == [ + "The URL cannot be changed after publishing." + ] diff --git a/websites/site_config_api.py b/websites/site_config_api.py index 6b2010b86..a2b6ae4d7 100644 --- a/websites/site_config_api.py +++ b/websites/site_config_api.py @@ -9,6 +9,7 @@ WEBSITE_CONFIG_CONTENT_DIR_KEY, WEBSITE_CONFIG_DEFAULT_CONTENT_DIR, WEBSITE_CONFIG_ROOT_URL_PATH_KEY, + WEBSITE_CONFIG_SITE_URL_FORMAT_KEY, ) @@ -82,6 +83,13 @@ def root_url_path(self) -> str: """ return self.raw_data.get(WEBSITE_CONFIG_ROOT_URL_PATH_KEY, "").strip("/") + @cached_property + def site_url_format(self) -> str: + """ + Returns the site url format described in the site config + """ + return self.raw_data.get(WEBSITE_CONFIG_SITE_URL_FORMAT_KEY, "").strip("/") + def iter_items(self) -> Iterator[ConfigItem]: """Yields all config items for which users can enter data""" collections = self.raw_data.get("collections") diff --git a/websites/views.py b/websites/views.py index 7e33367be..b21f32269 100644 --- a/websites/views.py +++ b/websites/views.py @@ -59,12 +59,13 @@ WebsiteContentDetailSerializer, WebsiteContentSerializer, WebsiteDetailSerializer, - WebsitePublishSerializer, + WebsiteMassBuildSerializer, WebsiteSerializer, WebsiteStarterDetailSerializer, WebsiteStarterSerializer, WebsiteStatusSerializer, WebsiteUnpublishSerializer, + WebsiteUrlSerializer, WebsiteWriteSerializer, ) from websites.site_config_api import SiteConfig @@ -161,6 +162,8 @@ def get_serializer_class(self): self.request.query_params.get("only_status") ): return WebsiteStatusSerializer + elif self.action in ("preview", "publish"): + return WebsiteUrlSerializer else: return WebsiteDetailSerializer @@ -181,49 +184,55 @@ def get_serializer(self, *args, **kwargs): return serializer_class(*args, **kwargs) - @action( - detail=True, methods=["post"], permission_classes=[HasWebsitePreviewPermission] - ) - def preview(self, request, name=None): - """Trigger a preview task for the website""" + def publish_version(self, name, version, request): + """Process a publish request for the specified version""" try: website = self.get_object() - - Website.objects.filter(pk=website.pk).update( - has_unpublished_draft=False, - draft_publish_status=constants.PUBLISH_STATUS_NOT_STARTED, - draft_publish_status_updated_on=now_in_utc(), - latest_build_id_draft=None, - draft_last_published_by=request.user, - ) - trigger_publish(website.name, VERSION_DRAFT) + if website.publish_date is None: + if not request.data.get("url_path"): + request.data["url_path"] = None + serializer = WebsiteUrlSerializer(data=request.data, instance=website) + if serializer.is_valid(raise_exception=True): + serializer.update(website, serializer.validated_data) + if version == VERSION_DRAFT: + Website.objects.filter(pk=website.pk).update( + has_unpublished_draft=False, + draft_publish_status=constants.PUBLISH_STATUS_NOT_STARTED, + draft_publish_status_updated_on=now_in_utc(), + latest_build_id_draft=None, + draft_last_published_by=request.user, + ) + else: + Website.objects.filter(pk=website.pk).update( + has_unpublished_live=False, + live_publish_status=constants.PUBLISH_STATUS_NOT_STARTED, + live_publish_status_updated_on=now_in_utc(), + latest_build_id_live=None, + live_last_published_by=request.user, + unpublish_status=None, + last_unpublished_by=None, + ) + trigger_publish(website.name, version) return Response(status=200) + except ValidationError as ve: + return Response(data=ve.detail, status=status.HTTP_400_BAD_REQUEST) except Exception as exc: # pylint: disable=broad-except - log.exception("Error previewing %s", name) + log.exception("Error publishing %s version for %s", version, name) return Response(status=500, data={"details": str(exc)}) + @action( + detail=True, methods=["post"], permission_classes=[HasWebsitePreviewPermission] + ) + def preview(self, request, name=None): + """Trigger a preview task for the website""" + return self.publish_version(name, VERSION_DRAFT, request) + @action( detail=True, methods=["post"], permission_classes=[HasWebsitePublishPermission] ) def publish(self, request, name=None): """Trigger a publish task for the website""" - try: - website = self.get_object() - - Website.objects.filter(pk=website.pk).update( - has_unpublished_live=False, - live_publish_status=constants.PUBLISH_STATUS_NOT_STARTED, - live_publish_status_updated_on=now_in_utc(), - latest_build_id_live=None, - live_last_published_by=request.user, - unpublish_status=None, - last_unpublished_by=None, - ) - trigger_publish(website.name, VERSION_LIVE) - return Response(status=200) - except Exception as exc: # pylint: disable=broad-except - log.exception("Error publishing %s", name) - return Response(status=500, data={"details": str(exc)}) + return self.publish_version(name, VERSION_LIVE, request) @action( detail=True, methods=["post"], permission_classes=[HasWebsitePublishPermission] @@ -260,7 +269,7 @@ def pipeline_status(self, request, name=None): class WebsiteMassBuildViewSet(viewsets.ViewSet): """Return a list of previously published sites, with the info required by the mass-build-sites pipeline""" - serializer_class = WebsitePublishSerializer + serializer_class = WebsiteMassBuildSerializer permission_classes = (BearerTokenPermission,) def list(self, request): @@ -273,12 +282,14 @@ def list(self, request): ) # Get all sites, minus any sites that have never been successfully published - sites = Website.objects.exclude(Q(**{f"{publish_date_field}__isnull": True})) + sites = Website.objects.exclude( + Q(**{f"{publish_date_field}__isnull": True}) | Q(url_path__isnull=True) + ) # For live builds, exclude previously published sites that have been unpublished if version == VERSION_LIVE: sites = sites.exclude(unpublish_status__isnull=False) sites = sites.prefetch_related("starter").order_by("name") - serializer = WebsitePublishSerializer(instance=sites, many=True) + serializer = WebsiteMassBuildSerializer(instance=sites, many=True) return Response({"sites": serializer.data}) diff --git a/websites/views_test.py b/websites/views_test.py index ad007187b..0a337cdee 100644 --- a/websites/views_test.py +++ b/websites/views_test.py @@ -142,9 +142,6 @@ def test_websites_endpoint_list_create(mocker, drf_client, permission_groups): mock_create_website_backend = mocker.patch( "websites.serializers.create_website_backend" ) - mock_create_website_pipeline = mocker.patch( - "websites.serializers.create_website_publishing_pipeline" - ) starter = WebsiteStarterFactory.create(source=constants.STARTER_SOURCE_GITHUB) for [user, has_perm] in [ [permission_groups.global_admin, True], @@ -167,7 +164,6 @@ def test_websites_endpoint_list_create(mocker, drf_client, permission_groups): website = Website.objects.get(name=f"{user.username}_site") assert website.owner == user mock_create_website_backend.assert_any_call(website) - mock_create_website_pipeline.assert_any_call(website) @pytest.mark.parametrize("method", ["put", "patch", "delete"]) @@ -224,9 +220,6 @@ def test_websites_endpoint_detail_update(mocker, drf_client): mock_update_website_backend = mocker.patch( "websites.serializers.update_website_backend" ) - mock_create_website_pipeline = mocker.patch( - "websites.serializers.create_website_publishing_pipeline" - ) website = WebsiteFactory.create() admin_user = UserFactory.create() admin_user.groups.add(website.admin_group) @@ -241,7 +234,6 @@ def test_websites_endpoint_detail_update(mocker, drf_client): assert updated_site.title == new_title assert updated_site.owner == website.owner mock_update_website_backend.assert_called_once_with(website) - mock_create_website_pipeline.assert_not_called() def test_websites_endpoint_preview(mocker, drf_client): @@ -249,7 +241,7 @@ def test_websites_endpoint_preview(mocker, drf_client): mock_trigger_publish = mocker.patch("websites.views.trigger_publish") now = datetime.datetime(2020, 1, 1, tzinfo=pytz.utc) mocker.patch("websites.views.now_in_utc", return_value=now) - website = WebsiteFactory.create() + website = WebsiteFactory.create(url_path="courses/mysite") editor = UserFactory.create() editor.groups.add(website.editor_group) drf_client.force_login(editor) @@ -345,6 +337,62 @@ def test_websites_endpoint_publish_error(mocker, drf_client): assert resp.data == {"details": "422 {}"} +@pytest.mark.parametrize("is_published", [True, False]) +@pytest.mark.parametrize("action", ["preview", "publish"]) +def test_websites_endpoint_publish_with_url( + mocker, drf_client, ocw_site, is_published, action +): + """url path should be updated if provided and site is not published""" + mock_publish = mocker.patch("websites.views.trigger_publish") + new_url_path = "5-new-site-fall-2020" + old_url_path = "courses/old-path" + ocw_site.url_path = old_url_path + ocw_site.publish_date = now_in_utc() if is_published else None + ocw_site.save() + + data = {"url_path": new_url_path} + drf_client.force_login(UserFactory.create(is_superuser=True)) + drf_client.post( + reverse(f"websites_api-{action}", kwargs={"name": ocw_site.name}), data=data + ) + ocw_site.refresh_from_db() + assert ocw_site.url_path == ( + ocw_site.assemble_full_url_path(new_url_path) + if not is_published + else old_url_path + ) + mock_publish.assert_called_once() + + +@pytest.mark.parametrize("action", ["preview", "publish"]) +def test_websites_endpoint_publish_with_url_error(drf_client, ocw_site, action): + """An error should be returned if url_path validation fails""" + drf_client.force_login(UserFactory.create(is_superuser=True)) + ocw_site.publish_date = None + ocw_site.save() + response = drf_client.post( + reverse(f"websites_api-{action}", kwargs={"name": ocw_site.name}), + data={"url_path": "new-path=[metadata:year]"}, + ) + assert response.status_code == 400 + assert response.json() == { + "url_path": ["You must replace the url sections in brackets"] + } + + +@pytest.mark.parametrize("action", ["preview", "publish"]) +def test_websites_endpoint_publish_with_url_blank_error(drf_client, action): + """An error should be returned if a site has no url_path yet""" + drf_client.force_login(UserFactory.create(is_superuser=True)) + website = WebsiteFactory.create(not_published=True, url_path=None) + response = drf_client.post( + reverse(f"websites_api-{action}", kwargs={"name": website.name}), + data={}, + ) + assert response.status_code == 400 + assert response.json() == {"url_path": ["The URL path cannot be blank"]} + + def test_websites_endpoint_unpublish(mocker, drf_client): """A user with admin permissions should be able to request a website unpublish""" mock_unpublished_removal = mocker.patch( @@ -510,11 +558,8 @@ def test_website_endpoint_empty_search(drf_client): assert expected_uuids == sorted([site["uuid"] for site in resp.data["results"]]) -def test_websites_autogenerate_name(mocker, drf_client): +def test_websites_autogenerate_name(drf_client): """ Website POST endpoint should auto-generate a name if one is not supplied """ - mock_create_website_pipeline = mocker.patch( - "websites.serializers.create_website_publishing_pipeline" - ) superuser = UserFactory.create(is_superuser=True) drf_client.force_login(superuser) starter = WebsiteStarterFactory.create(source=constants.STARTER_SOURCE_GITHUB) @@ -528,7 +573,6 @@ def test_websites_autogenerate_name(mocker, drf_client): assert resp.status_code == status.HTTP_201_CREATED assert resp.data["name"] == slugified_title assert resp.data["short_id"] == website_short_id - mock_create_website_pipeline.assert_called_once() def test_website_starters_list(settings, drf_client, course_starter): @@ -1200,6 +1244,7 @@ def test_mass_build_endpoint_list(settings, drf_client, version, unpublished): publish_date=None, first_published_to_production=now, unpublish_status=constants.PUBLISH_STATUS_NOT_STARTED if unpublished else None, + with_url_path=True, ) live_published = WebsiteFactory.create_batch( 2, @@ -1207,6 +1252,7 @@ def test_mass_build_endpoint_list(settings, drf_client, version, unpublished): publish_date=now_in_utc(), first_published_to_production=now, unpublish_status=constants.PUBLISH_STATUS_NOT_STARTED if unpublished else None, + with_url_path=True, ) expected_sites = draft_published if version == VERSION_DRAFT else live_published settings.API_BEARER_TOKEN = "abc123"