Course publish without metadata (unforked) (#2183)

* fixed code styling issues * Added doc strings * added the doc string * replaced forked themes repo to original * Added testcase to test has_site_metadata in websites/serializers_test.py * Added testcase for publish drawer button disability based on website's has_site_metadata in static/js/components/PublishDrawer.test.tsx * removed unwanted import * Fixed lint error * Update testcase and metadata check logic * disabled pylint too many args warning * Disabled pylint too many args * Disabled pylint too many args * Set the sitemetadata fields validation * Fixed the linting issues * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * chore(deps): lock file maintenance (#1884) * chore(deps): lock file maintenance * added check for website starter to disable publish only for courses in production * updated test for publish drawer * updated check for warning messages for missing metadata * updated check for failing unit tests --------- Co-authored-by: Muhammad Anas <[email protected]> Co-authored-by: Umar Hassan <[email protected]>
mitodl · May 23, 2024 · a01ad56 · a01ad56
1 parent 775c848
commit a01ad56
Show file tree

Hide file tree

Showing 12 changed files with 185 additions and 14 deletions.
diff --git a/localdev/configs/ocw-course-site-config.yml b/localdev/configs/ocw-course-site-config.yml
@@ -199,6 +199,11 @@ collections:
     - label: "Extra Course Numbers (comma separated list)"
       name: "extra_course_numbers"
       widget: "string"
+    - label: "Hide Course Download"
+      name: "hide_download"
+      required: true
+      widget: "boolean"
+      help: "Enable this setting to hide the course download button"
     - label: Course Image
       name: course_image
       widget: relation

diff --git a/static/js/components/PublishDrawer.test.tsx b/static/js/components/PublishDrawer.test.tsx
@@ -17,7 +17,7 @@ import { IntegrationTestHelper } from "../testing_utils"
 
 import PublishDrawer from "./PublishDrawer"
 
-import { Website } from "../types/websites"
+import { Starter, Website } from "../types/websites"
 import userEvent from "@testing-library/user-event"
 import { waitFor, screen } from "@testing-library/react"
 import * as dom from "@testing-library/dom"
@@ -95,6 +95,7 @@ describe("PublishDrawer", () => {
       urlField: "draft_url",
       publishDateField: "draft_publish_date",
       publishStatusField: "draft_publish_status",
+      hasSiteMetaData: "has_site_metadata",
       idx: 0,
     },
     {
@@ -105,6 +106,7 @@ describe("PublishDrawer", () => {
       urlField: "live_url",
       publishDateField: "publish_date",
       publishStatusField: "live_publish_status",
+      hasSiteMetaData: "has_site_metadata",
       idx: 1,
     },
   ])(
@@ -117,6 +119,7 @@ describe("PublishDrawer", () => {
       urlField,
       publishDateField,
       publishStatusField,
+      hasSiteMetaData,
       idx,
     }) => {
       ;[true, false].forEach((visible) => {
@@ -194,6 +197,16 @@ describe("PublishDrawer", () => {
         expect(wrapper.find(".btn-publish").prop("disabled")).toBe(true)
       })
 
+      it("disables publish button in production if no metadata is set", async () => {
+        website[hasSiteMetaData] = false
+        const { wrapper } = await render()
+        await simulateClickPublish(wrapper, action)
+        wrapper.update()
+        expect(wrapper.find(".btn-publish").prop("disabled")).toBe(
+          action === "production" && website.starter?.name !== Starter.ocw_www,
+        )
+      })
+
       it("render only the preview button if user is not an admin", async () => {
         website["is_admin"] = false
         const { wrapper } = await render()
@@ -240,9 +253,10 @@ describe("PublishDrawer", () => {
         const { wrapper } = await render()
         await simulateClickPublish(wrapper, action)
         wrapper.update()
-        expect(
-          wrapper.find("PublishForm").find(".btn-publish").prop("disabled"),
-        ).toBeFalsy()
+        website.has_site_metadata &&
+          expect(
+            wrapper.find("PublishForm").find(".btn-publish").prop("disabled"),
+          ).toBeFalsy()
         await act(async () => {
           wrapper.find("PublishForm").find(".btn-publish").simulate("submit")
         })

diff --git a/static/js/components/PublishDrawer.tsx b/static/js/components/PublishDrawer.tsx
@@ -13,7 +13,7 @@ import {
 import { isErrorStatusCode } from "../lib/util"
 import PublishStatusIndicator from "./PublishStatusIndicator"
 
-import { Website } from "../types/websites"
+import { Starter, Website } from "../types/websites"
 import PublishForm, {
   OnSubmitPublish,
   SiteFormValues,
@@ -61,7 +61,11 @@ const getPublishingInfo = (
 const PublishingOption: React.FC<PublishingOptionProps> = (props) => {
   const { publishingEnv, selected, onSelect, website, onPublishSuccess } = props
   const publishingInfo = getPublishingInfo(website, publishingEnv)
-
+  const isPublishDisabled =
+    !publishingInfo.hasUnpublishedChanges ||
+    (!website.has_site_metadata &&
+      website.starter?.name !== Starter.ocw_www &&
+      publishingEnv === PublishingEnv.Production)
   const [{ isPending }, publish] = useMutation(
     (payload: WebsitePublishPayload) =>
       websitePublishAction(website.name, publishingEnv, payload),
@@ -123,7 +127,7 @@ const PublishingOption: React.FC<PublishingOptionProps> = (props) => {
           )}
           <PublishForm
             onSubmit={handlePublish}
-            disabled={!publishingInfo.hasUnpublishedChanges}
+            disabled={isPublishDisabled}
             website={website}
             option={publishingEnv}
           />

diff --git a/static/js/resources/ocw-course-site-config.json b/static/js/resources/ocw-course-site-config.json
@@ -266,6 +266,13 @@
               "name": "extra_course_numbers",
               "widget": "string"
             },
+            {
+              "help": "Enable this setting to hide the course download button",
+              "label": "Hide Course Download",
+              "name": "hide_download",
+              "required": true,
+              "widget": "boolean"
+            },
             {
               "collection": "resource",
               "display_field": "title",

diff --git a/static/js/types/websites.ts b/static/js/types/websites.ts
@@ -236,6 +236,7 @@ export interface WebsiteStatus {
   synced_on: string | null
   sync_errors: Array<string> | null
   unpublished: boolean
+  has_site_metadata: boolean
 }
 
 export type Website = WebsiteStatus & {
@@ -333,3 +334,8 @@ export interface WebsiteInitials {
   title: string
   short_id: string
 }
+
+export enum Starter {
+  ocw_course = "ocw-course-v2",
+  ocw_www = "ocw-www",
+}
diff --git a/static/js/util/factories/websites.ts b/static/js/util/factories/websites.ts
@@ -227,6 +227,7 @@ export const makeWebsiteDetail = (
   url_suggestion: "[sitemetadata:primary_course_number]-[sitemetdata:title]",
   s3_path: `courses/${casual.word}`,
   unpublished: false,
+  has_site_metadata: false,
   ...overrides,
 })
 
@@ -249,6 +250,7 @@ export const makeWebsiteStatus = (
     synced_on: null,
     sync_errors: null,
     unpublished: false,
+    has_site_metadata: website.has_site_metadata,
   }
 }
 

diff --git a/websites/api.py b/websites/api.py
@@ -24,6 +24,7 @@
 )
 from websites.constants import (
     CONTENT_FILENAME_MAX_LEN,
+    OCW_WWW_STARTER_SLUG,
     PUBLISH_STATUS_ABORTED,
     PUBLISH_STATUS_ERRORED,
     PUBLISH_STATUS_SUCCEEDED,
@@ -398,6 +399,11 @@ def get_content_warnings(website):
 
     messages = []
 
+    if not website.has_site_metadata and (
+        website.starter and website.starter.name != OCW_WWW_STARTER_SLUG
+    ):
+        messages.append("The course is missing metadata.")
+
     if len(missing_youtube_ids_titles) > 0:
         messages.append(
             f"The following video resources require YouTube IDs: {', '.join(missing_youtube_ids_titles)}"  # noqa: E501

diff --git a/websites/api_test.py b/websites/api_test.py
@@ -1,9 +1,12 @@
 """Tests for websites API functionality"""
 
+from pathlib import Path
 from uuid import UUID
 
 import factory
 import pytest
+import yaml
+from django.conf import settings
 from mitol.common.utils import now_in_utc
 
 from content_sync.constants import VERSION_DRAFT, VERSION_LIVE
@@ -41,6 +44,7 @@
     PreviewOrPublishSuccessMessage,
 )
 from websites.models import Website
+from websites.serializers_test import VALID_METADATA
 
 pytestmark = pytest.mark.django_db
 
@@ -487,11 +491,31 @@ def test_update_unpublished_website_status(status, version):
 @pytest.mark.parametrize("has_missing_captions", [True, False])
 @pytest.mark.parametrize("has_truncatable_text", [True, False])
 @pytest.mark.parametrize("is_draft", [True, False])
-def test_get_content_warnings(
-    mocker, has_missing_ids, has_missing_captions, has_truncatable_text, is_draft
+@pytest.mark.parametrize("valid_metadata", [True, False])
+def test_get_content_warnings(  # pylint: disable=too-many-arguments  # noqa: PLR0913
+    mocker,
+    has_missing_ids,
+    has_missing_captions,
+    has_truncatable_text,
+    is_draft,
+    valid_metadata,
 ):
     """get_content_warnings should return expected warning messages"""
-    website = WebsiteFactory.create()
+    site_config_path = "localdev/configs/ocw-course-site-config.yml"
+    starter = WebsiteStarterFactory(
+        config=yaml.load(
+            (Path(settings.BASE_DIR) / site_config_path).read_text(),
+            Loader=yaml.SafeLoader,
+        )
+    )
+    website = WebsiteFactory.create(starter=starter)
+    metadata = {**VALID_METADATA}
+    if not valid_metadata:
+        del metadata["course_title"]
+
+    WebsiteContentFactory.create(
+        type="sitemetadata", website=website, metadata=metadata
+    )
     video_content = WebsiteContentFactory.create_batch(3, website=website)
     no_yt_ids = video_content[0:2] if has_missing_ids else []
     no_caps = video_content[1:3] if has_missing_captions else []
@@ -515,26 +539,36 @@ def test_get_content_warnings(
     )
     warnings = get_content_warnings(website)
     warnings_len = 0
+    if not valid_metadata:
+        warnings_len += 1
     if has_missing_ids:
         warnings_len += 1
         for content in no_yt_ids:
-            assert content.title in warnings[0]
+            assert content.title in warnings[1 if not valid_metadata else 0]
     if has_missing_captions:
         warnings_len += 1
         for content in no_caps:
-            assert content.title in warnings[1 if has_missing_ids else 0]
+            assert (
+                content.title
+                in warnings[int(not valid_metadata) + int(has_missing_ids)]
+            )
     if has_truncatable_text:
         warnings_len += 1
         assert (
             video_content[2].title
-            in warnings[int(has_missing_ids) + int(has_missing_captions)]
+            in warnings[
+                int(not valid_metadata)
+                + int(has_missing_ids)
+                + int(has_missing_captions)
+            ]
         )
     if is_draft:
         warnings_len += 1
         assert len(warnings) == warnings_len
         assert video_content[1].title in warnings[warnings_len - 1]
     if (
-        not has_missing_ids
+        valid_metadata
+        and not has_missing_ids
         and not has_missing_captions
         and not has_truncatable_text
         and not is_draft

diff --git a/websites/models.py b/websites/models.py
@@ -41,6 +41,7 @@
 from websites.site_config_api import ConfigItem, SiteConfig
 from websites.utils import (
     get_dict_field,
+    is_metadata_has_required_fields,
     permissions_group_name_for_role,
     set_dict_field,
 )
@@ -282,6 +283,21 @@ def s3_path(self):
         ]
         return "/".join([part.strip("/") for part in url_parts if part])
 
+    @property
+    def has_site_metadata(self):
+        """
+        Get True when required fields are set in WebsiteContent metadata
+        otherwise False
+        """
+        site_metadata = self.websitecontent_set.filter(type="sitemetadata")
+        return bool(
+            site_metadata
+            and site_metadata[0].metadata
+            and is_metadata_has_required_fields(
+                SiteConfig(self.starter.config), site_metadata[0].metadata
+            )
+        )
+
     class Meta:
         permissions = (
             ("publish_website", "Publish or unpublish a website"),

diff --git a/websites/serializers.py b/websites/serializers.py
@@ -253,6 +253,7 @@ class WebsiteDetailSerializer(
     live_url = serializers.SerializerMethodField(read_only=True)
     draft_url = serializers.SerializerMethodField(read_only=True)
     unpublished = serializers.ReadOnlyField()
+    has_site_metadata = serializers.ReadOnlyField()
 
     def get_is_admin(self, obj):
         """Determine if the request user is an admin"""
@@ -297,6 +298,7 @@ class Meta:
             "sync_errors",
             "synced_on",
             "content_warnings",
+            "has_site_metadata",
         ]
         read_only_fields = [
             "uuid",
@@ -329,6 +331,8 @@ class WebsiteStatusSerializer(
 ):
     """Serializer for website status fields"""
 
+    has_site_metadata = serializers.ReadOnlyField()
+
     class Meta:
         model = Website
         fields = [
@@ -349,6 +353,7 @@ class Meta:
             "synced_on",
             "content_warnings",
             "url_suggestion",
+            "has_site_metadata",
         ]
         read_only_fields = fields
 

diff --git a/websites/serializers_test.py b/websites/serializers_test.py
@@ -4,7 +4,9 @@
 from types import SimpleNamespace
 
 import pytest
+import yaml
 from dateutil.parser import parse as parse_date
+from django.conf import settings
 from django.core.files.uploadedfile import SimpleUploadedFile
 from django.db.models import CharField, Value
 from mitol.common.utils import now_in_utc
@@ -175,6 +177,48 @@ def test_website_status_serializer(mocker, settings, drive_folder, warnings):
         assert serialized_data.get(key) == value
 
 
+VALID_METADATA = {
+    "status": True,
+    "course_title": "example",
+    "primary_course_number": "1",
+    "department_numbers": ["3"],
+    "hide_download": True,
+}
+
+
+@pytest.mark.parametrize(
+    "missing_field",
+    [
+        None,
+        "course_title",
+        "primary_course_number",
+        "department_numbers",
+        "hide_download",
+    ],
+)
+@pytest.mark.parametrize(
+    "serializer_class", [WebsiteDetailSerializer, WebsiteStatusSerializer]
+)
+def test_website_content_has_metadata(mocker, missing_field, serializer_class):
+    """has_site_metadata should be true if we have metadata in WebsiteContent model"""
+    site_config_path = "localdev/configs/ocw-course-site-config.yml"
+    starter = WebsiteStarterFactory(
+        config=yaml.load(
+            (Path(settings.BASE_DIR) / site_config_path).read_text(),
+            Loader=yaml.SafeLoader,
+        )
+    )
+    website = WebsiteFactory.create(starter=starter)
+    metadata = {**VALID_METADATA}
+    if missing_field:
+        del metadata[missing_field]
+    WebsiteContentFactory.create(
+        type="sitemetadata", website=website, metadata=metadata
+    )
+    serialized_data = serializer_class(instance=website).data
+    assert serialized_data["has_site_metadata"] == bool(not missing_field)
+
+
 @pytest.mark.parametrize("has_starter", [True, False])
 @pytest.mark.parametrize("drive_folder", [None, "abc123"])
 @pytest.mark.parametrize("drive_credentials", [None, {"creds: True"}])