Extension point to customize certificate views

[kaizoku]

We'd like to optionally require logins for viewing and downloading certificates.
This PR adds an extension point using a new feature added in openedx/edx-django-utils#64 to allow modifying the webview for certificates.

JIRA tickets: Implements BB-1388.

Dependencies: None

Sandbox URL: https://pr20957.sandbox.opencraft.hosting/

Testing instructions:

1. Generate a certificate to test with.

   /edx/bin/{python,manage}.edxapp lms --settings=devstack_docker generate_fake_cert verified course-v1:edX+DemoX+Demo_Course
   

2. Check the certificate renders normally with no login required in an logged-out browser: http://localhost:18000/certificates/user/8/course/course-v1:edX+DemoX+Demo_Course
3. Add the following to lms/envs/private.py in the devstack

   OVERRIDE_RENDER_CERTIFICATE_VIEW = "lms.envs.private.certificate_render_html_view_override"
   from django.contrib.auth.decorators import login_required
   def certificate_render_html_view_override(prev_fn, request, user_id, course_id):
       return login_required(prev_fn)(request, user_id, course_id)
   

4. Check the certificate page again and ensure it requires login now.

Author notes and concerns: None

Reviewers

• (@giovannicimolin )
• @bradenmacdonald

[openedx-webhooks]

Thanks for the pull request, @kaizoku! I've created OSPR-3752 to keep track of it in JIRA, where we prioritize reviews. 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:

• 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.

[natabene]

@kaizoku Thank you for your contribution. Please let me know once tests are green and this is ready for our review.

[kaizoku]

@natabene , I think that test failure was a fluke, it's an HTTP Connection error from selenium, and doesn't come from any components changed in this PR. The test works successfully on this branch in my devstack. I'll see if I can get jenkins to re-run it.

[kaizoku]

jenkins run bokchoy

[giovannicimolin]

👍

• I tested this:

1. Checked out this branch.
2. Generated fake certificate for user.
3. Checked that the certificate was publicly accessible ✔️
4. Switched certificates.require_login on
5. Checked that the certificate page required login to be accessible, and the redirect pointed back to the correct page. ✔️

• I read through the code
• Includes documentation ⚠️
• I checked for accessibility issues NA

@kaizoku Nice work! Very simple and clean implementation. 😁

I have a question: isn't it better for this feature to be enabled/disabled per course instead of instance wide?

And a comment about your notes:

Should we create a waffle configuration in a new file for the certificates application?

This is not really required, but you could add a namespace variable to the top of the file where the switch is being used and use that, like this:

CERTIFICATES_NAMESPACE = 'certificates'
WaffleSwitch(CERTIFICATES_NAMESPACE, 'require_login').is_enabled()

There's not that many examples in the codebase related to that, and it's not a blocker for merging.

[kaizoku]

Thanks @giovannicimolin!

Hm, perhaps we should use a CourseWaffleFlag, that would provide more fine grained control.

My worry there is that course authors would have to create a new waffle flag for every course they create, and authentication wouldn't be enforced if they hadn't gotten the memo about creating waffle flags for their course. Whereas an organization who wants authentication for their certificates would likely want them for all courses, so missing this on one course would maybe be a kind of breach.

Also for the documentation note, I'm not sure where waffle flags are typically documented.
I did see a post today about documentation of waffle flags, which references OEP-17, but that OEP mostly describes when and why to create a waffle flag, but not much of how or where to document it.

[kaizoku]

@giovannicimolin, I've switched this around to using a CourseWaffleFlag, since it defers to WaffleFlag, so we can still set a global, but have overrides for each course as needed.

[giovannicimolin]

@kaizoku Nice work here! I tested it again and it works perfectly.

👍

• I tested this:

1. Ran the same tests as before, but switching to a flag instead of a switch.
2. Checked that viewing certificates can require login either on a per course basis or instance wide using the certificates.require_login. ✔️

• I read through the code
• Includes documentation ⚠️
• I checked for accessibility issues NA

[giovannicimolin]

@natabene This is ready for edX review.

[natabene]

@ormsbee This is ready for your review, when you have a chance.

[natabene]

@marcotuts Do you want to review this before it goes to engineering?

[kaizoku]

@marcotuts, @ormsbee any chance you've been able to check this out?

[natabene]

@kaizoku Sorry for delay, we will try to look at this this week.

[kaizoku]

No problem @natabene , thanks for the response :)

[kaizoku]

@natabene, any chance you've been able to look at this PR?

[natabene]

@kaizoku Sorry, not yet. We will try today.

[kaizoku]

@natabene has this come up for review yet?

[pomegranited]

@kaizoku CC @bradenmacdonald

We are working to change the implementation here to a plugin approach so that there is no increase in code complexity to the core platform.

So can we close this OSPR?

[kaizoku]

@pomegranited, we'll still need to add one line to enable the plugin point after https://github.com/edx/edx-platform/pull/21433 is merged.
This PR currently has most of the contents from that PR for testing.
Once it's merged though, I'll cut it down to the decorator which creates the pluggability point: @pluggable_override('RENDER_CERTIFICATE_VIEW_IMPL')

[pomegranited]

No worries, thanks for the update @kaizoku !

[Agrendalath]

@kaizoku, I see that the docstring, from which this has been copied, was not updated after the reworks. I'll fix this in #27466. Please use the prefix OVERRIDE_ for this, as described here (e.g. OVERRIDE_GET_UNIT_ICON), to keep it consistent.

[kaizoku]

Thanks for pointing that out. I've updated the variable name here to match.

[kaizoku]

This PR is ready for upstream review.

[bradenmacdonald]

@kaizoku Code looks good to me 👍🏻

Can you please: (1) Update the PR description to match the final approach taken here, and (2) rebase+squash to one commit with a conventional commit prefix?

[kaizoku]

Thanks for the review @bradenmacdonald. I've squashed the PR, updated the commit message, and updated the PR description here. This should be ready now.

[bradenmacdonald]

jenkins run js

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[openedx-webhooks]

@kaizoku 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[kaizoku]

Thanks for the review @bradenmacdonald !