feat: add view certificate button #121

DawoudSheraz · 2021-05-24T18:09:10Z

PROD-2357

Description

Add certificate tool with the following features:

Get certificate details
Generate certificate
Regenerate certificate

The backend APIs are the same as old support tools, located at https://github.com/edx/edx-platform/blob/master/lms/djangoapps/certificates/views/support.py. The view cert button is added to the enrollment listing, as requested in the ticket.

codecov · 2021-05-28T07:21:10Z

Codecov Report

Merging #121 (95d4802) into master (dbb9cfb) will increase coverage by 2.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   76.23%   78.26%   +2.03%     
==========================================
  Files          43       45       +2     
  Lines         930     1017      +87     
  Branches      183      199      +16     
==========================================
+ Hits          709      796      +87     
  Misses        210      210              
  Partials       11       11

Impacted Files	Coverage Δ
src/users/data/api.js	`99.33% <100.00%> (+0.08%)`	⬆️
src/users/data/test/certificates.js	`100.00% <100.00%> (ø)`
src/users/data/urls.js	`100.00% <100.00%> (ø)`
src/users/enrollments/Certificates.jsx	`100.00% <100.00%> (ø)`
src/users/enrollments/Enrollments.jsx	`98.27% <100.00%> (+0.16%)`	⬆️

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 dbb9cfb...95d4802. Read the comment docs.

Ali-D-Akbar

A handful of queries to be addressed:

Can you please add a screenshot for regenerate button scenario? Just wanted to confirm that only one of the button is shown
What does a user see as a verification that an operation was successful? Some sort of a success message or the whole page/component refresh?

Ali-D-Akbar · 2021-06-24T18:07:40Z

src/users/data/api.test.js

+  const certificatesUrl = `${getConfig().LMS_BASE_URL}/certificates/search?user=${testUsername}&course_id=${testCourseId}`;
+  const generateCertificateUrl = `${getConfig().LMS_BASE_URL}/certificates/generate`;
+  const regenerateCertificateUrl = `${getConfig().LMS_BASE_URL}/certificates/regenerate`;


Why're you not using getCertificateUrl, regenerateCertificateUrl and regenerateCertificateUrl from urls.js?

Right, I will change it

Ali-D-Akbar · 2021-06-24T18:32:34Z

src/users/enrollments/Certificates.test.jsx

+  const props = {
+    username: testUser,
+    courseId: testCourseId,
+    closeHandler: jest.fn(() => {}),
+  };


Move this in src/users/data/test/certificates.js.

This is not the exact data. The data is already in cert.js.

Ali-D-Akbar · 2021-06-24T18:43:50Z

src/users/enrollments/Certificates.test.jsx

+      await waitForComponentToPaint(wrapper);
+      generateButton = wrapper.find('button#generate-certificate');
+      expect(wrapper.find('h3').length).toEqual(0);
+      expect(generateButton.prop('disabled')).toBeFalsy();


why is the generate-certificate button still not disabled after flow has been successful?
And are we showing some success message on success of certificate generation?

Because it is still possible to generate/regenerate after one attempt. The backend does not restrict reattempts for generation/regeneration. Ideally, the generate button would change into the regenerate button. But the mock in this current test is returning the generatable certificate data.

DawoudSheraz · 2021-06-25T09:38:29Z

A handful of queries to be addressed:

Can you please add a screenshot for regenerate button scenario? Just wanted to confirm that only one of the button is shown

What does a user see as a verification that an operation was successful? Some sort of a success message or the whole page/component refresh?

Ok, although there won't be much difference. Only status text is changed.
If the generate/regenerate is successful, only the cert component reloads. This behavior is the same as the old support tools, where the reloading is done via django.

azanbinzahid · 2021-06-25T09:57:46Z

src/users/data/api.js

+    const { data } = await getAuthenticatedHttpClient().get(
+      AppUrls.getCertificateUrl(username, courseKey),
+    );
+    return Array.isArray(data) && data.length > 0 ? data[0] : data;


In what cases this endpoint will return an array or single item?

If course id is mentioned, then the single item, otherwise a list of certificates. We might not need this check. I added it for cautious purposes(unexpected data on stage/prod). Once the changes are live, I will validate and remove the unnecessary check.

azanbinzahid · 2021-06-25T10:01:49Z

src/users/data/api.js

+      AppUrls.generateCertificateUrl(),
+      formData,
+    );
+    return data;


What does data contain in this case? from here it says its a http response. Are we using it anywhere?

Yeah, it is just a response. We are not using it anywhere though. The return value is used only in tests to verify the api behavior.

azanbinzahid · 2021-06-25T10:10:38Z

src/users/data/urls.js

+
+export const getCertificateUrl = (username, courseKey) => `${
+  LMS_BASE_URL
+}/certificates/search?user=${username}&course_id=${courseKey}`;


we may need to encode courseKey.

Hmm, the backend is running quote_plus on the course key. https://github.com/edx/edx-platform/blob/master/lms/djangoapps/certificates/views/support.py#L109. I will check if the encoding is necessary.
https://docs.python.org/3/library/urllib.parse.html#urllib.parse.quote_plus

Encoding is not needed. Encoding the courseId results in 404 on backend.

azanbinzahid · 2021-06-25T10:16:07Z

src/users/enrollments/Certificates.jsx

+      <div className="m-3">
+        <h2>Course ID: {certificate.courseKey}</h2>
+        {status && (<h3>Status: {status} </h3>)}
+        <table className="table">


Is there a specific reason for not using Table component?

Yes, the Table component is more suited for listing with headings on top and data listed vertically. In cert case, the headers are on the left and data on right i.e. one header and one piece of data are on the same row. furthermore, the Table component has many other operations not needed in this case.

DawoudSheraz force-pushed the dsheraz/PROD-2357 branch from cf35eaa to dcebca3 Compare June 4, 2021 07:35

DawoudSheraz force-pushed the dsheraz/PROD-2357 branch 2 times, most recently from 8222190 to 35f15db Compare June 23, 2021 15:58

DawoudSheraz changed the title ~~[WIP]feat: add view certificate button~~ feat: add view certificate button Jun 24, 2021

DawoudSheraz force-pushed the dsheraz/PROD-2357 branch from 0eb1f74 to 076d278 Compare June 24, 2021 13:16

feat: add view certificate button

f1e08b6

DawoudSheraz force-pushed the dsheraz/PROD-2357 branch from 076d278 to f1e08b6 Compare June 24, 2021 13:30

DawoudSheraz requested review from azanbinzahid and Ali-D-Akbar June 24, 2021 13:50

Ali-D-Akbar requested changes Jun 24, 2021

View reviewed changes

address comments

95d4802

Ali-D-Akbar approved these changes Jun 25, 2021

View reviewed changes

azanbinzahid reviewed Jun 25, 2021

View reviewed changes

azanbinzahid approved these changes Jun 28, 2021

View reviewed changes

DawoudSheraz merged commit 6630c19 into master Jun 28, 2021

DawoudSheraz deleted the dsheraz/PROD-2357 branch June 28, 2021 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add view certificate button #121

feat: add view certificate button #121

DawoudSheraz commented May 24, 2021 •

edited

Loading

codecov bot commented May 28, 2021 •

edited

Loading

Ali-D-Akbar left a comment •

edited

Loading

Ali-D-Akbar Jun 24, 2021

DawoudSheraz Jun 25, 2021

Ali-D-Akbar Jun 24, 2021

DawoudSheraz Jun 25, 2021

Ali-D-Akbar Jun 24, 2021

DawoudSheraz Jun 25, 2021

DawoudSheraz commented Jun 25, 2021

azanbinzahid Jun 25, 2021

DawoudSheraz Jun 25, 2021

azanbinzahid Jun 25, 2021

DawoudSheraz Jun 25, 2021

azanbinzahid Jun 25, 2021

DawoudSheraz Jun 25, 2021

DawoudSheraz Jun 28, 2021

azanbinzahid Jun 25, 2021

DawoudSheraz Jun 25, 2021

feat: add view certificate button #121

feat: add view certificate button #121

Conversation

DawoudSheraz commented May 24, 2021 • edited Loading

Description

codecov bot commented May 28, 2021 • edited Loading

Codecov Report

Ali-D-Akbar left a comment • edited Loading

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

DawoudSheraz commented Jun 25, 2021

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

DawoudSheraz commented May 24, 2021 •

edited

Loading

codecov bot commented May 28, 2021 •

edited

Loading

Ali-D-Akbar left a comment •

edited

Loading