Customizable URLs for studio #1316

mbertrand · 2022-05-11T12:35:33Z

Pre-Flight checklist

Screenshots and design review for any changes that affect layout or styling
Migrations
- Migration is backwards-compatible with current production code
Testing
- Code is tested
- Changes have been manually tested
Settings
- New settings are documented and present in app.json
- New settings have reasonable development defaults, if applicable

What are the relevant tickets?

Closes #1336

What's this PR do?

Adds a url_path field to Website. The migration should prepopulate it with the same value as Website.name for all sites that have already been published to production.
When publishing to draft/live, url_path can be sent in the request body to set the field's value. The serializer should allow the field value to change as long as the site hasn't been published to production yet and the value is valid. If the value is changed, all WebsiteContent objects with populated file values for the site will have their synced_checksums reset so they will be resynced to github with the updated path.
Updates the HugoMarkdownSerializer to save the correct file values (based on url_path) to the backend (git) and vice versa.
Uses a new site-url-format field in site config to suggest a pattern for this field to the user on the front end.
Changes the publish drawer form, to include a url field if the site has not been published to production yet.
Updates both individual and mass pipeline definitions so that S3 files are copied to the right places. Creates/updates the site pipeline before each publish event to ensure that it has the correct url path baked in.

How should this be manually tested?

Update the ocw-course starter config with the new line here.
Start studio with concourse docker-compose --profile concourse up
Run manage.py upsert_mass_build_pipeline and backpopulate_pipelines, they should both succeed.
Try to publish an existing site with a publish_date, the drawer should look about the same and work the same.
Try to publish a new or existing site without a publish_date, you should see a URL field with a suggested path, any existing metadata values matching the pattern should be present. Enter a valid value and click 'Publish' to draft, check to see that the url_path is updated and the pipeline is triggered (it will fail, that's ok).
Go to the github repo and check that any resources for images, pdfs, etc have a file value that contains the site's url path.
Reload the page and try to publish again, the URL field should be set to whatever you entered last time instead of the suggested path. Modify it and click the "Publish" button, the pipeline should trigger again. Also check on the git repo and make sure the resource file paths have been updated again to the latest value.
The changes here affect most parts of the site, so try just about everything and make sure it still works.
The changes to the pipelines are best tested when this hits RC.

Where should the reviewer start?

https://github.com/mitodl/hq/discussions/1

Screenshots (if appropriate)

An unpublished site without any user-supplied url path so far:

An unpublished site with a previously chosen url (still editable):

A published site (no longer editable):

codecov-commenter · 2022-05-16T19:02:30Z

Codecov Report

Merging #1316 (415089c) into master (dfbfdfa) will increase coverage by 0.04%.
The diff coverage is 95.36%.

@@            Coverage Diff             @@
##           master    #1316      +/-   ##
==========================================
+ Coverage   89.06%   89.10%   +0.04%     
==========================================
  Files         231      232       +1     
  Lines        9557     9687     +130     
  Branches     1778     1827      +49     
==========================================
+ Hits         8512     8632     +120     
- Misses        928      930       +2     
- Partials      117      125       +8

Impacted Files	Coverage Δ
main/settings.py	`94.49% <ø> (ø)`
static/js/types/websites.ts	`100.00% <ø> (ø)`
static/js/util/factories/websites.ts	`98.70% <ø> (ø)`
websites/api.py	`97.40% <ø> (ø)`
content_sync/serializers.py	`89.37% <75.00%> (-0.37%)`	⬇️
websites/views.py	`96.76% <86.95%> (-1.03%)`	⬇️
websites/models.py	`93.56% <92.30%> (-1.61%)`	⬇️
content_sync/api.py	`100.00% <100.00%> (ø)`
content_sync/pipelines/concourse.py	`89.68% <100.00%> (-0.14%)`	⬇️
static/js/components/PublishDrawer.tsx	`95.18% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfbfdfa...415089c. Read the comment docs.

ChristopherChudzicki

This will be great :) I'm still doing the manual testing but have looked through most of the code except the pipeline stuff. I left a few specific comments largey around ts stuff on the frontend. Small, but probably worth addressing since they're low effort.

Frontend bug I noticed...:

Edit and save metadata form
click publish ... publishing form is using old metadata values.

ChristopherChudzicki · 2022-05-18T14:15:02Z

websites/serializers.py

@@ -519,7 +567,7 @@ class Meta:
 class WebsiteContentCreateSerializer(
    serializers.ModelSerializer, RequestUserSerializerMixin
 ):
-    """Serializer which creates a new WebsiteContent"""
+    """Serializer mixin which validates that urls are unique"""


Is this docstring correct?

Nope, pasted in the wrong place

ChristopherChudzicki · 2022-05-18T14:30:30Z

static/js/components/forms/PublishForm.tsx

+  onSubmit: (
+    values: any,
+    formikHelpers: FormikHelpers<any>
+  ) => void | Promise<any>


Suggested change

onSubmit: (

values: any,

formikHelpers: FormikHelpers<any>

) => void | Promise<any>

onSubmit: (

values: SiteFormValues,

formikHelpers: FormikHelpers< SiteFormValues >

) => void

void is good enough for the return value; nothing should be using that value.

ChristopherChudzicki · 2022-05-18T14:36:42Z

static/js/components/forms/PublishForm.tsx

+    .trim()
+    .required()
+    .matches(
+      /^[a-zA-Z0-9\-._]*$/,


This is probably a question for Ferdi, but we may want to pick either dash or underscore and not allow both.

I personally don't like underscores in the url but I dont think we should disallow them .

ChristopherChudzicki · 2022-05-18T14:38:24Z

static/js/components/forms/PublishForm.tsx

+  option
+}: Props): JSX.Element | null => {
+  const initialValues: SiteFormValues = {
+    // @ts-ignore


Here and elsewhere, we should use @ts-expect-error for suppressing errors. That directive will throw an error if used unnecessarily. (I've encountered a fair number of unnecessary @ts-ignore in our code; @ts-expect-error is somewhat newer, so may not have existed when studio started.).

ChristopherChudzicki · 2022-05-18T14:44:46Z

static/js/components/forms/PublishForm.tsx

+  website,
+  disabled,
+  option
+}: Props): JSX.Element | null => {


This component never returns null.

My personal preference is to use the React.FC helper to declare functional component types...export const PublishForm: React.FC<Props> = ({...whateverProps}) => {}, then React takes care of the return type.

ChristopherChudzicki · 2022-05-18T14:54:14Z

static/js/components/forms/PublishForm.test.tsx

+        expect(error).toBeInstanceOf(ValidationError)
+        // @ts-ignore


There's an assertIsInstanceOf helper in test_util if you want to do this without the @ts-ignore. assertIsInstanceOf will narrow the type (expect...toBeInstanceOf... does not narrow the type).

ChristopherChudzicki · 2022-05-18T15:03:00Z

static/js/components/forms/PublishForm.test.tsx

+  ;[null, "courses/5.2-my-course"].forEach(urlPath => {
+    it(`initialValue for url_path should be based on website ${
+      urlPath ? "url_path" : "url_suggestion"
+    }`, () => {


You can use it.each([...])("Description...", option => {}) here to reduce nesting / use jest conventions. https://jestjs.io/docs/api#describeeachtablename-fn-timeout

If the array is an array of objects, you can access them by $name in the description string... My personal preference is to hardcode the title formatting like

it.each([ { urlPath: null, basedOn: "url_suggestion" }, { urlPath: "courses/5.2-my-course", basedOn: "url_path" } ]).only("initialValue ... based on website $basedOn", ({ urlPath }) => {...})

ChristopherChudzicki · 2022-05-18T15:05:54Z

static/js/components/forms/PublishForm.test.tsx

@@ -0,0 +1,130 @@
+import React from "react"
+import sinon, { SinonStub } from "sinon"
+import { shallow } from "enzyme"


This is a general comment, not worth addressing right now.

Opinion: For new tests, we should avoid enzyme and shallow rendering. Enzyme does only supports React 16 and below, which is two major versions behind current. See #1275 and or https://github.com/mitodl/hq/discussions/17

ChristopherChudzicki · 2022-05-18T15:27:03Z

websites/models.py

+            for section in re.findall(r"(\[.+?\])+", site_config.site_url_format) or []:
+                section_type, section_field = re.sub(r"[\[\]]+", "", section).split(":")


If we changed the config format to

{sitemetadata.primary_course_number}-{sitemetadata.course_title}-{sitemetadata.term}-{sitemetadata.year}

We might be able to do site_config.site_url_format.format(sitemetadata=metadata).

Do you think this would be worth doing? We'd need a version of the metadata dict that supports attribute access, but we would get formatting and some validation builtin (e.g., .format raises KeyError if keys are missing).

Nice idea! But it doesn't quite work as intended if any of the metadata fields haven't been filled out yet. For example, when I try it on a site with fields for primary_course_number and title but no term and year, I get:

slugify(site_config.site_url_format.format(**metadata).replace(".", "-")) >2.18-time-travel-101-abc-

But what I want is placeholders for the missing values like 2.18-time-travel-101-abc-{term}-{year}

ChristopherChudzicki · 2022-05-18T16:25:21Z

@mbertrand I tried the pipelines / renaming etc and everything worked as expected: URLs got updated in github when I changed the course url 👍

Do you think the frontend form using outdated metadata is a blocking issue? Probably not?

mbertrand · 2022-05-18T17:34:40Z

Do you think the frontend form using outdated metadata is a blocking issue? Probably not?

I think I know how to fix that, will give it a shot.

gumaerc · 2022-05-18T17:38:59Z

I'm not sure I could point out anything from a technical standpoint that @ChristopherChudzicki hasn't already mentioned, but for what it's worth I experienced the same behavior where updating my metadata wasn't reflected on the URL in the publish drawer until I refreshed the page. One UI thing that I noticed that I didn't see pointed out is that with the new text field in the publish drawer, it seems to open super wide:

Do we maybe want to restrict the width on this? I tried publishing my test course, which already existed, and I noticed that the file attributes have a seemingly random amount of /media prefixed at the beginning. Is this maybe something to do with my local env, or the test course itself? You can take a look at the resources in my published test course here: https://github.com/gumaerc-testorg/cpg-test-course-3/tree/main/content/resources

mbertrand · 2022-05-18T17:46:29Z

I made the publish drawer deliberately wide if the site hasn't been published to production yet, because the site-url-format string that is the default value in the URL field is so long. I mentioned to Ferdi that maybe we should come up with some shorter value to display, maybe without the sitemetadata: in each section, so [primary_course_number]-[title]-[term]-[year] (though that's still a bit long).

If you're seeing /media for files. you probably don't have OCW_STUDIO_USE_S3 set to True in your .env file.

mbertrand · 2022-05-18T17:56:41Z

The suggested url format in the publish drawer should now be updated after a metadata change.

gumaerc

👍

mbertrand added the Work in Progress label May 11, 2022

mbertrand force-pushed the mb/siteurls branch 2 times, most recently from 9769089 to ab1332c Compare May 11, 2022 20:30

This was referenced May 12, 2022

Replace s3 paths with url paths for embeds and download links mitodl/ocw-hugo-themes#684

Closed

URL pattern for ocw-studio.yaml mitodl/ocw-hugo-projects#180

Merged

mbertrand force-pushed the mb/siteurls branch from 7a08f9d to ab3daa2 Compare May 16, 2022 18:58

mbertrand force-pushed the mb/siteurls branch 2 times, most recently from f21f4e5 to 058f706 Compare May 17, 2022 17:49

mbertrand changed the title ~~Site URL based on starter config and metadata fields~~ Customizable URLs for studio May 17, 2022

mbertrand force-pushed the mb/siteurls branch from 4929b7e to bd4c3c2 Compare May 17, 2022 21:28

mbertrand added 19 commits May 17, 2022 17:40

.

a558fe1

sitemetadata new field instead of env setting?

a02aae2

.

6869174

back to env settings

4d9dcb6

back to config settings for s3_path, site_url

2f47144

Trying metadata vars again

9eaa766

more tweaks

13c527b

app.json

73fa063

lint, formatting

3201231

.

e58238b

uniqueness check

f455439

more tweaks, url_path model field

d98f866

.

7301a48

Refactor based on latest acceptance criteria

d7b379b

more tweaks

af84e65

new tests for websites app

35299b8

Update pipeline tests

222009b

Fix more tests

1a990fa

Prevent Website.url_path from being changed for a published site

21131c5

mbertrand force-pushed the mb/siteurls branch from 6865aba to b490eee Compare May 17, 2022 21:46

Preliminary front-end, some more backend tweaks

8b4b1b1

mbertrand force-pushed the mb/siteurls branch from a7028a8 to 8b4b1b1 Compare May 17, 2022 22:00

mbertrand added 2 commits May 18, 2022 07:23

more tweaks and tests

371d129

Tests for PublishForm

e373995

mbertrand force-pushed the mb/siteurls branch from 8990d2d to e373995 Compare May 18, 2022 12:24

Upsert pipeline on publish only if never published to production

57ca8fd

mbertrand added Needs Review and removed Work in Progress labels May 18, 2022

ChristopherChudzicki self-assigned this May 18, 2022

gumaerc self-assigned this May 18, 2022

ChristopherChudzicki reviewed May 18, 2022

View reviewed changes

mbertrand added 2 commits May 18, 2022 13:08

feedback

d00a9bf

lint

a8235f1

Make sure the suggested url format is updated after a metadata change

afbd061

ChristopherChudzicki approved these changes May 18, 2022

View reviewed changes

A couple minor tweaks

415089c

mbertrand force-pushed the mb/siteurls branch from 5545de5 to 415089c Compare May 18, 2022 18:52

gumaerc approved these changes May 18, 2022

View reviewed changes

mbertrand merged commit 3c0e45a into master May 18, 2022

odlbot mentioned this pull request May 18, 2022

Release 0.61.2 #1365

Closed

2 tasks

This was referenced May 19, 2022

Show error when site url is not unique #1366

Closed

Show publishing api errors in publishing drawer #1367

Merged

gumaerc mentioned this pull request May 19, 2022

update theme to use url_path mitodl/ocw-hugo-themes#700

Merged

2 tasks

odlbot mentioned this pull request May 24, 2022

Release 0.62.0 #1373

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Customizable URLs for studio #1316

Customizable URLs for studio #1316

mbertrand commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 16, 2022 •

edited

Loading

ChristopherChudzicki left a comment

ChristopherChudzicki May 18, 2022

mbertrand May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

Ferdi May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

ChristopherChudzicki May 18, 2022

mbertrand May 18, 2022

ChristopherChudzicki commented May 18, 2022 •

edited

Loading

mbertrand commented May 18, 2022

gumaerc commented May 18, 2022

mbertrand commented May 18, 2022

mbertrand commented May 18, 2022

gumaerc left a comment

		for section in re.findall(r"(\[.+?\])+", site_config.site_url_format) or []:
		section_type, section_field = re.sub(r"[\[\]]+", "", section).split(":")

Customizable URLs for studio #1316

Customizable URLs for studio #1316

Conversation

mbertrand commented May 11, 2022 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Where should the reviewer start?

Screenshots (if appropriate)

codecov-commenter commented May 16, 2022 • edited Loading

Codecov Report

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherChudzicki commented May 18, 2022 • edited Loading

mbertrand commented May 18, 2022

gumaerc commented May 18, 2022

mbertrand commented May 18, 2022

mbertrand commented May 18, 2022

gumaerc left a comment

Choose a reason for hiding this comment

mbertrand commented May 11, 2022 •

edited

Loading

codecov-commenter commented May 16, 2022 •

edited

Loading

ChristopherChudzicki commented May 18, 2022 •

edited

Loading