website publish without metadata #1

Anas12091101 · 2023-10-20T09:56:41Z

What are the relevant tickets?

Fixes # 1996

Description (What does it do?)

You can add content to a course and publish it in staging without ever visiting the Metadata section. Production, however will require metadata to be set.

Screenshots (if appropriate):

How can this be tested?

Go to the studio website and login as admin
Create a new site with ocw-course starter
(Optional) Add pages to the website
Open the publish drawer, you can see that the publish button is enabled in the staging and disabled in production (as we have not set metadata yet)
Press the publish button in staging
After the pipeline finishes, you get the published website on the url

Additional Context

The course-v2/layouts/partials/course_banner.html file was changed in the ocw-hugo-themes repository. You can view the changes here

HussainTaj-arbisoft

The general approach seems good to me.

I have a few questions and suggestions. I didn't get the time to run the code myself. I'll do that tomorrow and get back to you if I find something new.

HussainTaj-arbisoft · 2023-10-23T11:59:26Z

static/js/components/PublishDrawer.tsx

@@ -123,7 +123,7 @@ const PublishingOption: React.FC<PublishingOptionProps> = (props) => {
          )}
          <PublishForm
            onSubmit={handlePublish}
-            disabled={!publishingInfo.hasUnpublishedChanges}
+            disabled={!publishingInfo.hasUnpublishedChanges || (!website.has_site_metadata && publishingEnv==="production")}


Would it be better to use the PublishingEnv enum from static/js/constants.ts?

Suggested change

disabled={!publishingInfo.hasUnpublishedChanges || (!website.has_site_metadata && publishingEnv==="production")}

disabled={!publishingInfo.hasUnpublishedChanges || (!website.has_site_metadata && publishingEnv===PublishingEnv.Production)}

yes, seems more clean

HussainTaj-arbisoft · 2023-10-23T12:03:23Z

static/js/components/PublishDrawer.tsx

@@ -123,7 +123,7 @@ const PublishingOption: React.FC<PublishingOptionProps> = (props) => {
          )}
          <PublishForm
            onSubmit={handlePublish}
-            disabled={!publishingInfo.hasUnpublishedChanges}
+            disabled={!publishingInfo.hasUnpublishedChanges || (!website.has_site_metadata && publishingEnv==="production")}


This condition is rather long, maybe we could refactor it into a separate variable?

This is just a suggestion. It is up to your discretion.

const isPublishDisabled = ( !publishingInfo.hasUnpublishedChanges || !website.has_site_metadata && publishingEnv==="production" )

HussainTaj-arbisoft · 2023-10-23T12:11:04Z

websites/serializers.py

@@ -263,6 +257,10 @@ def get_draft_url(self, instance):
        """Get the draft url for the site"""
        return instance.get_full_url(version=VERSION_DRAFT)

+    def get_has_site_metadata(self, instance):
+        site_metadata = instance.websitecontent_set.filter(type="sitemetadata")
+        return True if (site_metadata and site_metadata.metadata) else False


Your code is correct. In my opinion, we can make it easier to read too.

Suggested change

return True if (site_metadata and site_metadata.metadata) else False

return bool(site_metadata and site_metadata.metadata)

HussainTaj-arbisoft · 2023-10-23T12:42:56Z

websites/serializers.py

+    def get_has_site_metadata(self, instance):
+        site_metadata = instance.websitecontent_set.filter(type="sitemetadata")
+        return True if (site_metadata and site_metadata.metadata) else False


I think we need to add this in WebsiteStatusSerializer.

I'm not sure if it is required in WebsiteDetailSerializer. If it is required in both serializers then I think it would be best to create a mixin, like WebsiteHasMetadataSerializer.

HussainTaj-arbisoft · 2023-10-23T12:46:09Z

static/js/components/SiteContentEditor.tsx

+    // Updating website's has_site_metadata to true
+    store.dispatch(updateEntities({websiteDetails : (prevValue)=>{ 
+      const updatedSite = {...prevValue[site.name], has_site_metadata:true}
+      let newVal = prevValue;
+      newVal[site.name] = updatedSite;
+      return  newVal}}))
+  }


Is this required?

I thought the following code was enough to fetch new data from the backend.

store.dispatch(requestAsync(websiteStatusRequest(site.name)))

Also, since this piece of code sets has_site_metadata: true for any form submission, I think saving a page will enable the button even when the metadata is not present.

Yes, I didn't go through the working of WebsiteStatus requests in the code. Will update it soon.

HussainTaj-arbisoft

Functionally this is good. I do have some suggestions on refactoring some code. And it would be awesome to format this code using our configured formatters.

HussainTaj-arbisoft · 2023-10-25T11:11:15Z

static/js/components/SiteContentEditor.tsx

@@ -6,6 +6,7 @@ import { requestAsync } from "redux-query"

 import SiteContentForm from "./forms/SiteContentForm"
 import { useWebsite } from "../context/Website"
+import { updateEntities } from 'redux-query';


Is this import being used?

No, removing it

HussainTaj-arbisoft · 2023-10-25T11:13:44Z

websites/serializers.py

+    has_site_metadata = serializers.SerializerMethodField(read_only=True)
+
    """Serializer for website status fields"""

+    def get_has_site_metadata(self, instance):
+        site_metadata = instance.websitecontent_set.filter(type="sitemetadata")
+        return bool(site_metadata and site_metadata[0].metadata)
+


We're repeating this logic twice.

Maybe we should create a Mixin called WebsiteStatusMixin. Let me know if you need help with this.

HussainTaj-arbisoft

Looks good. Just a minor suggestion.

HussainTaj-arbisoft · 2023-10-25T13:49:24Z

websites/serializers.py

+class WebsiteHasMetadataMixin(serializers.Serializer):
+    has_site_metadata = serializers.SerializerMethodField(read_only=True)


It's always a good idea to add doc strings.

Suggested change

class WebsiteHasMetadataMixin(serializers.Serializer):

has_site_metadata = serializers.SerializerMethodField(read_only=True)

class WebsiteHasMetadataMixin(serializers.Serializer):

"""Adds has_site_metadata custom serializer field."""

has_site_metadata = serializers.SerializerMethodField(read_only=True)

HussainTaj-arbisoft · 2023-10-25T13:51:04Z

websites/serializers.py

+    def get_has_site_metadata(self, instance):
+        site_metadata = instance.websitecontent_set.filter(type="sitemetadata")


Suggested change

def get_has_site_metadata(self, instance):

site_metadata = instance.websitecontent_set.filter(type="sitemetadata")

def get_has_site_metadata(self, instance):

"""Get whether or not the site has metadata."""

site_metadata = instance.websitecontent_set.filter(type="sitemetadata")

HussainTaj-arbisoft · 2023-10-30T08:37:29Z

websites/serializers.py

-    """Serializer for website status fields"""
-


Was there a reason for removing this docstring?

No, removed by mistake

HussainTaj-arbisoft suggested changes Oct 23, 2023

View reviewed changes

Anas12091101 force-pushed the Issue_metadata branch from 57c1c1f to 880a7ac Compare October 24, 2023 01:43

HussainTaj-arbisoft suggested changes Oct 25, 2023

View reviewed changes

Anas12091101 force-pushed the Issue_metadata branch 3 times, most recently from 841d148 to d177b90 Compare October 25, 2023 12:28

fixed code styling issues

b5e9fa4

Anas12091101 force-pushed the Issue_metadata branch from d177b90 to b5e9fa4 Compare October 25, 2023 12:34

HussainTaj-arbisoft suggested changes Oct 25, 2023

View reviewed changes

Added doc strings

f67ebed

Anas12091101 requested a review from HussainTaj-arbisoft October 25, 2023 13:57

HussainTaj-arbisoft reviewed Oct 30, 2023

View reviewed changes

added the doc string

a15ee03

Anas12091101 requested a review from HussainTaj-arbisoft October 30, 2023 09:15

HussainTaj-arbisoft approved these changes Oct 31, 2023

View reviewed changes

Anas12091101 merged commit 5054711 into master Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

website publish without metadata #1

website publish without metadata #1

Anas12091101 commented Oct 20, 2023 •

edited

Loading

HussainTaj-arbisoft left a comment

HussainTaj-arbisoft Oct 23, 2023

Anas12091101 Oct 23, 2023

HussainTaj-arbisoft Oct 23, 2023

HussainTaj-arbisoft Oct 23, 2023

HussainTaj-arbisoft Oct 23, 2023

HussainTaj-arbisoft Oct 23, 2023

Anas12091101 Oct 23, 2023

HussainTaj-arbisoft left a comment

HussainTaj-arbisoft Oct 25, 2023

Anas12091101 Oct 25, 2023

HussainTaj-arbisoft Oct 25, 2023

HussainTaj-arbisoft left a comment

HussainTaj-arbisoft Oct 25, 2023

HussainTaj-arbisoft Oct 25, 2023

HussainTaj-arbisoft Oct 30, 2023

Anas12091101 Oct 30, 2023

	disabled={!publishingInfo.hasUnpublishedChanges \|\| (!website.has_site_metadata && publishingEnv==="production")}
	disabled={!publishingInfo.hasUnpublishedChanges \|\| (!website.has_site_metadata && publishingEnv===PublishingEnv.Production)}

	return True if (site_metadata and site_metadata.metadata) else False
	return bool(site_metadata and site_metadata.metadata)

		class WebsiteHasMetadataMixin(serializers.Serializer):
		has_site_metadata = serializers.SerializerMethodField(read_only=True)

		def get_has_site_metadata(self, instance):
		site_metadata = instance.websitecontent_set.filter(type="sitemetadata")

website publish without metadata #1

website publish without metadata #1

Conversation

Anas12091101 commented Oct 20, 2023 • edited Loading

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Additional Context

HussainTaj-arbisoft 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

HussainTaj-arbisoft 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

HussainTaj-arbisoft 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

Anas12091101 commented Oct 20, 2023 •

edited

Loading