sites without metadata fail to publish

gumaerc commented 1 year ago

Expected Behavior

If required metadata is not set on a course, ocw-studio should not allow you to click the publish button, at least for publishing live.

Current Behavior

You can add content to a course and attempt to publish it without ever visiting the Metadata section. The publishing pipeline will subsequently fail with the following error:

ERROR 2023/10/10 19:44:54 render of "page" failed: "/tmp/build/ed66b15e/ocw-hugo-themes-git/course-v2/layouts/_default/baseof.html:17:32": execute of template failed: template: resources/single.html:17:32: executing "subheader" at <partial "course_banner.html" .>: error calling partial: "/tmp/build/ed66b15e/ocw-hugo-themes-git/course-v2/layouts/partials/course_banner.html:6:137": execute of template failed: template: partials/course_banner.html:6:137: executing "partials/course_banner.html" at <delimit $courseData.level ", ">: error calling delimit: can't iterate over <nil>
Total in 51 ms

Steps to Reproduce

Create a new site
Add some content, Pages, Resources, etc.
Attempt to publish the site to either draft or live

Possible Solution

There are a couple possible solutions:

These fields are not marked as required in ocw-hugo-projects/ocw-course-v2/ocw-studio.yaml, so they likely should be. When opening the publish drawer, disable the publish button if required metadata fields are not filled out.
Move this issue to ocw-hugo-themes, and add more robust null checking to places in the theme that use metadata values. Some refactoring could likely be done to accomplish this, although it may not achieve the desired behavior. For instance, it's probably fine to publish draft with missing information, but we would likely want to prevent publishing a site live with course level metadata missing, especially in production.

The full solution may involve a combination of both, allowing course authors to publish draft before filling out the metadata hassle-free while simultaneously protecting them from accidentally publishing a site live with missing metadata.

pdpinch commented 1 year ago

Which fields specifically are causing the error?

If it's the pipeline that's failing, why doesn't the usual error handling for notifying on publish failures work?

gumaerc commented 1 year ago

@pdpinch In this case, the field causing the error seems to be level. Courses can have multiple levels, and Hugo is attempting to iterate an array. The error handling in the pipeline did work, as we were notified in Slack and the build was aborted.

HussainTaj-arbisoft commented 1 year ago

I'm reviewing https://github.com/mitodl/ocw-studio/pull/2015 and I'd like to clarify a few requirements.

The approach in https://github.com/mitodl/ocw-studio/pull/2015 produces the expected behavior. That is, production course publish requires the metadata content to be set from the frontend. It simply checks if the metadata object exists, but not its content.

Should we check every required metadata field individually? I wonder if this will slow down the API response time.
Is the level field required? I see two courses in production that are published without this information. Our select widget also doesn't support the required attribute, so we'll need to add it.

The following are the courses that don't have the level metadata but are published to production:

@gumaerc what are your thoughts on this? I'll also tag Peter once he returns from his vacation.

gumaerc commented 1 year ago

@HussainTaj-arbisoft I believe level is just the first field that Hugo tries to process from the metadata during the build process. The problem isn't the metadata being empty, the problem is the course.json file not existing at all in the data folder. Another approach to this could even be potentially including a "skeleton" blank metadata file in the theme itself, because if one exists in the site content then it should take priority. As long as the site will build without a hard error from Hugo, the user should be allowed to publish draft.

This being said, I think more stringent validation should be performed before allowing a publish to production. I don't know if this means we want to check every single field or use required, but it seems that if we do not do any kind of validation it would be possible for an author to publish a site missing data that would cause blank spots. What we should validate there is more of a product question, but at a bare minimum I think everything in Course Info should be filled out so nothing is blank when the site is published to production.

HussainTaj-arbisoft commented 1 year ago

Thank you for the explanation, @gumaerc.

Anas added a check in https://github.com/mitodl/ocw-hugo-themes/pull/1274 which should allow the publish to succeed without the metadata.

I think more stringent validation should be performed before allowing a publish to production.

This makes sense. More validation is always better.

HussainTaj-arbisoft commented 11 months ago

@pdpinch, I'd like to hear what your opinion on the questions in https://github.com/mitodl/ocw-studio/issues/1996#issuecomment-1801706569 is.

pdpinch commented 11 months ago

Do we have a mechanism to validate only on publish to production?

We need to find a balance between allowing authors to publish incomplete work to staging, and requiring valid data in production.

pt2302 commented 11 months ago

There is currently a PREPUBLISH_ACTIONS setting: https://github.com/mitodl/ocw-studio/blob/26830bccdf487ed14504f2cd19ef7a67edd57983/main/settings.py#L1105-L1108

One possible approach would be to add an analogous setting for PREPUBLISH_LIVE_ACTIONS, and only run those tasks on publish to production.

HussainTaj-arbisoft commented 11 months ago

Do we have a mechanism to validate only on publish to production?

https://github.com/mitodl/ocw-studio/pull/2015 checks if the site has metadata before the user clicks "publish". So the validation logic always runs. However, it does allow staging courses without any restrictions regarding metadata. Running the validation early also allows us to show a warning message telling the user why they can't publish.

The problem with having this validation after the user clicks publish is that we have no UI to show an error message. We don't show any error message when publishes fail. We only show a status.

@pt2302 thank you for the suggestion, I wasn't aware we had PREPUBLISH_ACTIONS. Unfortunately, it appears they don't have a way to return a reason message either (for when they raise an exception).

Adding post-publish validation messages doesn't seem required right now, and that might have an overhead too. So early validation should be a relatively more convenient option.

I'll advise Anas to go with an optimized way to perform validation for the required fields. We can test out the prototype and then decide if post-publish error messages need to be added.

@pdpinch, one thing I'd like to confirm right now is if you'd like to make the level field required. The current solution treats it as an optional field. (Making it required will take more work.)

HussainTaj-arbisoft commented 11 months ago

Anas has pointed out that we have a hide_download required field in the configuration. However, not all courses have this field set. We assume hide_download to be false when it's missing.

We're moving ahead with validating individual metadata fields. In consequence, old courses will not be published to production by the author unless they fill all the required fields. The authors can simply click "save" on the metadata page to achieve this. But, it isn't obvious what's missing at first glance (since the boolean has a default false value).

Since hide_download is not really required, I think we can mark it optional in the configuration.

pdpinch commented 10 months ago

@HussainTaj-arbisoft @Anas12091101 what's the status of this? I've reread the comments and it's not clear me to what the next step is.

HussainTaj-arbisoft commented 10 months ago

Anas won't be working on this and I'll probably be taking over from here. I've been prioritizing things on the board in ready over this issue.

@pdpinch, I don't know the priority of this issue. Would it be okay if I move this to ready and prioritize working on it?

pdpinch commented 10 months ago

Actually, it's not high priority. I was only commenting because it had been sitting for so long.

If you think it's less than a days work to finish, then let's do it and get the value of what Ana's started.

If it's more work than that, it will have to back into "needs triage"

HussainTaj-arbisoft commented 10 months ago

It might take longer than a day.

The task right now is to decide what to do with the hide_download field. It's marked required but not present in all courses. We discussed two options:

Make the field optional.
Backpopulate the field for all courses.

umar8hassan commented 5 months ago

This shall be deployed after fixing https://github.com/mitodl/hq/issues/4499

pdpinch commented 3 months ago

@umar8hassan can this be closed?

umar8hassan commented 3 months ago

@umar8hassan can this be closed?

@pdpinch No, this should be still kept open. The relevant code for this issue was reverted due to fix required in https://github.com/mitodl/hq/issues/4499. No code change is needed. But we need to deploy the connected branches again and test on RC.

pdpinch commented 1 month ago

@umar8hassan what's the status of this now?

mitodl / ocw-studio