feat: [FC-0044] Certificates page by khudym · Pull Request #872 · openedx/frontend-app-authoring

khudym · 2024-03-07T07:09:22Z

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/openedx/edx-platform.git
EDX_PLATFORM_VERSION: master

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_certificates_page
    everyone: true

Description

This PR adds fully implemented Certificates page.

Issue: openedx/platform-roadmap#317

Developer notes

~~the page reloads after the user saves changes to the certificate or signature. This issue will be fixed within Group Configurations pull request;~~
~~temporary added ModalNotification component that should be removed after merging this PR;~~

Design

Figma design

Testing instructions

Run master devstack.
Start platform make dev.up.lms+cms + frontend-app-course-authoring and make checkout on this branch.
Enable the new Certificates page by adding a waffle flag contentstore.new_studio_mfe.use_new_certificates_page in the CMS admin panel.
Go to the Certificates page from the Settings navbar.

new-certificates-page.mp4

openedx-webhooks · 2024-03-07T07:09:27Z

Thanks for the pull request, @khudym! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

open-craft-grove · 2024-03-07T12:36:48Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

codecov · 2024-03-07T15:22:24Z

Codecov Report

Attention: Patch coverage is 93.25397% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 91.99%. Comparing base (80bf869) to head (2364f8e).
Report is 6 commits behind head on master.

Files	Patch %	Lines
src/generic/modal-dropzone/useModalDropzone.jsx	81.03%	11 Missing ⚠️
...ficates/certificate-details/CertificateDetails.jsx	83.33%	3 Missing ⚠️
...certificate-signatories/hooks/useEditSignatory.jsx	82.35%	3 Missing ⚠️
src/certificates/data/slice.js	85.71%	3 Missing ⚠️
src/certificates/data/thunks.js	95.00%	3 Missing ⚠️
...ificate-edit-form/hooks/useCertificateEditForm.jsx	91.30%	2 Missing ⚠️
...ertificate-signatories/signatory/SignatoryForm.jsx	93.10%	2 Missing ⚠️
src/generic/modal-dropzone/ModalDropzone.jsx	86.66%	1 Missing and 1 partial ⚠️
...ates/certificate-edit-form/CertificateEditForm.jsx	92.85%	1 Missing ⚠️
src/certificates/data/api.js	95.23%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #872      +/-   ##
==========================================
+ Coverage   91.91%   91.99%   +0.08%     
==========================================
  Files         572      612      +40     
  Lines       10169    10734     +565     
  Branches     2199     2299     +100     
==========================================
+ Hits         9347     9875     +528     
- Misses        795      830      +35     
- Partials       27       29       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

open-craft-grove · 2024-03-07T15:52:44Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-08T12:57:00Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-08T13:59:34Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-12T10:29:37Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

KristinAoki · 2024-03-12T15:18:55Z

+  const previewUrl = useMemo(() => {
+    if (!certificateWebViewUrl) { return ''; }
+
+    const url = new URL(certificateWebViewUrl, window.location.origin);


Suggested change

const url = new URL(certificateWebViewUrl, window.location.origin);

const url = () => new URL(certificateWebViewUrl, window.location.origin);

I wonder what the benefit is, since the creation of a url instance is already optimized by React.useMemo. Plus, this is no reusable logic, the url object is used immediately. Could you please explain this decision?

There have been previous issues where URL was not created when it was not part of a function. See #706 for details about how it might throw an error

Thanks, i see, i will fix it now

KristinAoki · 2024-03-12T15:26:18Z

+    dispatch(fetchCertificates(courseId));
+  }, [courseId]);
+
+  document.title = getPageHeadTitle(courseTitle, intl.formatMessage(messages.headingTitleTabText));


Remove this and use <Helmet /> in Certificates.jsx. See AccessibilityPages.jsx L20-L26. getPageHeadTitle can still be used to format the mesage.

KristinAoki · 2024-03-12T15:27:42Z

+        <Stack direction="horizontal" className="justify-content-center align-items-center" gap="3.5">
+          <span className="small">{intl.formatMessage(messages.noCertificatesText)}</span>
+          <Button
+            iconBefore={AddIcon}
+            onClick={handleCreateMode}
+          >
+            {intl.formatMessage(messages.setupCertificateBtn)}
+          </Button>


It makes more sense for this to be use ActionRow instead of Stack and ActionRow.Spacer to separate the text and button.

Thanks, did not know.
Fixed

Is it ok if it differs from the design?

Design

KristinAoki · 2024-03-12T15:44:48Z

+  },
+  editCertificateConfirmBtnText: {
+    id: 'course-authoring.certificates.details.confirm.edit.btn.text',
+    defaultMessage: 'Yes, continue',


Suggested change

defaultMessage: 'Yes, continue',

defaultMessage: 'Edit',

Fixed

Just for context, this is the message for the modal that appears when you click on the edit button at the top of the activated certificate.
Long text forced us to change the modal size, as the button text does not fit inside the button.

As it is now on the old site

Example with large text

arbrandes

Looks great for the most part, but I do have a couple of questions and requests for changes.

arbrandes · 2024-03-12T20:43:39Z

+                  </Card.Section>
+                  <Card.Footer className="justify-content-start">
+                    <Button type="submit">
+                      {intl.formatMessage(commonMesages.saveTooltip)}


Suggested change

{intl.formatMessage(commonMesages.saveTooltip)}

{intl.formatMessage(commonMessages.saveTooltip)}

Even if this was an inherited typo, I believe we should fix it as part of this PR.

Sure, fixed

arbrandes · 2024-03-12T20:56:03Z

+  handleBlur: PropTypes.func,
+  setFieldValue: PropTypes.func,
+  setEditModes: PropTypes.func,
+  arrayHelpers: PropTypes.shape({}),


Is the shape of arrayHelpers unpredictable?

arbrandes · 2024-03-12T21:14:20Z

+    id: 'course-authoring.certificates.sidebar.about2.title',
+    defaultMessage: 'Issuing certificates to learners',
+  },
+  about_2_Description_1: {


These message identifiers aren't very descriptive. Can we try and come up with some less generic ones? (It's bad enough that none of the messages have descriptions, but I'm not going to hold the PRs up on this... for now.)

Renamed

Is this more appropriate?)

workingWithCertificatesTitle: { id: 'course-authoring.certificates.sidebar.working-with-certificates.title', defaultMessage: 'Working with certificates', }, workingWithCertificatesFirstParagraph: { id: 'course-authoring.certificates.sidebar.working-with-certificates.first-paragraph', defaultMessage: 'Specify a course title to use on the certificate if the course\'s official title is too long to be displayed well.', }, workingWithCertificatesSecondParagraph: { id: 'course-authoring.certificates.sidebar.working-with-certificates.second-paragraph', defaultMessage: 'For verified certificates, specify between one and four signatories and upload the associated images. To edit or delete a certificate before it is activated, hover over the top right corner of the form and select {strongText} or the delete icon.', },

khudym

Thank you for the review!
Fixed most of the comments but also left a couple questions

khudym · 2024-03-14T08:02:53Z

+  },
+  editCertificateConfirmBtnText: {
+    id: 'course-authoring.certificates.details.confirm.edit.btn.text',
+    defaultMessage: 'Yes, continue',


Fixed

Just for context, this is the message for the modal that appears when you click on the edit button at the top of the activated certificate.
Long text forced us to change the modal size, as the button text does not fit inside the button.

As it is now on the old site

Example with large text

khudym · 2024-03-14T08:06:02Z

+                  </Card.Section>
+                  <Card.Footer className="justify-content-start">
+                    <Button type="submit">
+                      {intl.formatMessage(commonMesages.saveTooltip)}


Sure, fixed

khudym · 2024-03-14T12:29:38Z

+  handleBlur: PropTypes.func,
+  setFieldValue: PropTypes.func,
+  setEditModes: PropTypes.func,
+  arrayHelpers: PropTypes.shape({}),


khudym · 2024-03-14T12:37:42Z

+        <Stack direction="horizontal" className="justify-content-center align-items-center" gap="3.5">
+          <span className="small">{intl.formatMessage(messages.noCertificatesText)}</span>
+          <Button
+            iconBefore={AddIcon}
+            onClick={handleCreateMode}
+          >
+            {intl.formatMessage(messages.setupCertificateBtn)}
+          </Button>


Thanks, did not know.
Fixed

Is it ok if it differs from the design?

Design

khudym · 2024-03-14T12:59:11Z

+    dispatch(fetchCertificates(courseId));
+  }, [courseId]);
+
+  document.title = getPageHeadTitle(courseTitle, intl.formatMessage(messages.headingTitleTabText));


khudym · 2024-03-14T13:25:10Z

+    id: 'course-authoring.certificates.sidebar.about2.title',
+    defaultMessage: 'Issuing certificates to learners',
+  },
+  about_2_Description_1: {


Renamed

Is this more appropriate?)

workingWithCertificatesTitle: { id: 'course-authoring.certificates.sidebar.working-with-certificates.title', defaultMessage: 'Working with certificates', }, workingWithCertificatesFirstParagraph: { id: 'course-authoring.certificates.sidebar.working-with-certificates.first-paragraph', defaultMessage: 'Specify a course title to use on the certificate if the course\'s official title is too long to be displayed well.', }, workingWithCertificatesSecondParagraph: { id: 'course-authoring.certificates.sidebar.working-with-certificates.second-paragraph', defaultMessage: 'For verified certificates, specify between one and four signatories and upload the associated images. To edit or delete a certificate before it is activated, hover over the top right corner of the form and select {strongText} or the delete icon.', },

khudym · 2024-03-14T13:42:59Z

+  const previewUrl = useMemo(() => {
+    if (!certificateWebViewUrl) { return ''; }
+
+    const url = new URL(certificateWebViewUrl, window.location.origin);


I wonder what the benefit is, since the creation of a url instance is already optimized by React.useMemo. Plus, this is no reusable logic, the url object is used immediately. Could you please explain this decision?

open-craft-grove · 2024-03-14T18:59:37Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-15T16:12:59Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-15T16:41:42Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

khudym · 2024-03-18T09:15:41Z

This PR depends on BE

open-craft-grove · 2024-03-18T09:42:57Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-18T10:05:32Z

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-18T11:20:40Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-25T17:20:49Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-27T10:05:03Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-03-28T09:08:33Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

khudym · 2024-03-28T09:22:38Z

In sandbox there is an issue with white background for subheader that comes from css overrides.

This case doesnt reproduce locally (devstack)

open-craft-grove · 2024-04-02T11:53:56Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

KristinAoki · 2024-04-03T15:43:11Z

Please add descriptions to messages

KristinAoki · 2024-04-03T15:43:50Z

Please add descriptions to messages

KristinAoki · 2024-04-03T15:46:14Z

Please add descriptions to messages

open-craft-grove · 2024-04-04T12:25:12Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

arbrandes

Works for me!

openedx-webhooks · 2024-04-04T17:28:07Z

@khudym 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 7, 2024

khudym marked this pull request as ready for review March 7, 2024 07:27

khudym requested a review from a team March 7, 2024 07:27

GlugovGrGlib added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 7, 2024

GlugovGrGlib requested review from KristinAoki and arbrandes March 7, 2024 07:37

peterkulko assigned khudym Mar 7, 2024

khudym removed their assignment Mar 7, 2024

ruzniaievdm mentioned this pull request Mar 7, 2024

feat: [FC-0044] certificates API DRF openedx/openedx-platform#34339

Merged

2 tasks

khudym force-pushed the hudym/certificates-page branch from 42210e0 to 75e7835 Compare March 12, 2024 09:46

KristinAoki suggested changes Mar 12, 2024

View reviewed changes

peterkulko assigned khudym Mar 12, 2024

arbrandes requested changes Mar 12, 2024

View reviewed changes

khudym commented Mar 14, 2024

View reviewed changes

khudym force-pushed the hudym/certificates-page branch from 75e7835 to 2c22c7c Compare March 14, 2024 18:16

khudym requested a review from a team as a code owner March 14, 2024 18:16

khudym force-pushed the hudym/certificates-page branch from 42e3f41 to 28baed5 Compare March 15, 2024 15:40

khudym removed their assignment Mar 15, 2024

peterkulko assigned khudym Mar 15, 2024

peterkulko added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Mar 15, 2024

khudym marked this pull request as draft March 18, 2024 09:15

peterkulko added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Mar 18, 2024

arbrandes mentioned this pull request Mar 19, 2024

New Certificates Page (Studio MFE conversion) openedx/platform-roadmap#317

Closed

khudym marked this pull request as ready for review March 20, 2024 07:42

khudym force-pushed the hudym/certificates-page branch from 28baed5 to fdc514a Compare March 25, 2024 16:40

khudym requested review from KristinAoki and arbrandes March 26, 2024 08:32

khudym force-pushed the hudym/certificates-page branch from fdc514a to 77173dd Compare March 28, 2024 08:17

feat: [FC-0044] Certificates page

5d6a68a

khudym force-pushed the hudym/certificates-page branch from 77173dd to 5d6a68a Compare April 2, 2024 11:10

KristinAoki suggested changes Apr 3, 2024

View reviewed changes

feat: add descriptions for details, signatories, sidebar i18n messages

2364f8e

KristinAoki approved these changes Apr 4, 2024

View reviewed changes

arbrandes approved these changes Apr 4, 2024

View reviewed changes

KristinAoki merged commit e306b62 into openedx:master Apr 4, 2024

	const url = new URL(certificateWebViewUrl, window.location.origin);
	const url = () => new URL(certificateWebViewUrl, window.location.origin);

	{intl.formatMessage(commonMesages.saveTooltip)}
	{intl.formatMessage(commonMessages.saveTooltip)}

Uh oh!

Conversation

khudym commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Developer notes

Design

Testing instructions

Uh oh!

openedx-webhooks commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

open-craft-grove commented Mar 7, 2024

Uh oh!

codecov Bot commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

open-craft-grove commented Mar 7, 2024

Uh oh!

open-craft-grove commented Mar 8, 2024

Uh oh!

open-craft-grove commented Mar 8, 2024

Uh oh!

open-craft-grove commented Mar 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khudym Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khudym left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

khudym commented Mar 7, 2024 •

edited

Loading

openedx-webhooks commented Mar 7, 2024 •

edited

Loading

codecov Bot commented Mar 7, 2024 •

edited

Loading

khudym Mar 14, 2024 •

edited

Loading

khudym Mar 14, 2024 •

edited

Loading