HTML Certs changes for Tahoe #212

melvinsoft · 2018-02-25T00:04:57Z

The three main part of the PR are:

Creates a signal to set honor as default course enrollment mode in all new courses. This is because the edX default mode is audit and this enrollment mode doesn't allow certification. In the future we can support to let AMC admins to set the one or more courses modes in a course, mostly used when e-commerce is enabled.
Fix a view that return certificates available, that fallbacks automatically to PDF certs if the enrollment mode is honor. Since we aren't going to use PDF certs, we can return None.
Makes the feature flag CERTIFICATES_HTML_VIEW microsite aware, so we can enabled it per microsite.

another courseware fix

melvinsoft · 2018-02-25T00:06:14Z

(14) Connect AMC page to manage HTML Certs Configuration

bryanlandia

All of this looks good to me, Maxi!

abeals

Read my comments with the appropriate context that you all are more familiar with the details of this codebase than I am.

abeals · 2018-02-25T14:07:56Z

cms/djangoapps/contentstore/views/certificates.py

@@ -394,7 +396,12 @@ def certificates_list_handler(request, course_key_string):
                certificate_web_view_url = None
            certificates = None
            is_active = False
-            if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False):
+            current_organization = request.user.organizations.first()


In my experience, first() is often risky, and/or reveals encoded assumptions that should be addressed elsewhere. It assumes an order to the list, and it also assumes that the first org is the one you always want.

If we're using this pattern elsewhere, I'd rather see us wrap this in a method we can modify to change the logic more universally.

@bezidejni is this how we're handling user orgs elsewhere?

@abeals I think it is, and I agree on extracting that to a separate method.

@abeals Thanks for bringing this up. Yes, this is how we're retrieving the user org in Studio and in AMC as well (at least I can see one case with a quick search).

This is something that we need to figure out how to fix, but I think it's beyond the scope of this card. I've created this card labeled as tech debt.

abeals · 2018-02-25T14:10:27Z

cms/envs/amc.py

@@ -1,6 +1,8 @@
 from .aws import *
 import dj_database_url

+from django.utils.translation import ugettext_lazy as _


Glad you pulled this in, though I'm surprised we didn't need this earlier in AMC settings. We need some I18N / L10N tests in our test suite.

abeals · 2018-02-25T14:26:19Z

cms/envs/amc.py

@@ -97,3 +100,6 @@
 CLONE_COURSE_FOR_NEW_SIGNUPS = False
 HIJACK_ALLOW_GET_REQUESTS = True
 HIJACK_LOGOUT_REDIRECT_URL = '/admin/auth/user'
+
+DEFAULT_COURSE_MODE_SLUG = ENV_TOKENS.get('EDXAPP_DEFAULT_COURSE_MODE_SLUG', 'audit')


What functional impact does this have for people coming into the system from a stock Open edX installation where 'honor' is the default? Is the setting (and its side effects) going to be obvious to course authors, or do we need to make it clear?

There is not difference for course authors between enrollment modes. The enrollment modes basically affects if the user can receive or not a certificate, and which type of certificate.

audit: No certificate

honor: Certificate

verified: Certificate with identity verification

The visible difference is when you've e commerce enabled, because you let the users choose between the enrollment mode (even after the course is approved). The enrollment mode model has a FK to the course, and also a fee associated.

Since we don't have e commerce enabled on Tahoe yet, should be transparent for the user, all enrollments are 'honor' but if you don't have certs enabled, all is the same for the user and course instructor.

abeals · 2018-02-25T14:31:29Z

lms/djangoapps/certificates/api.py

@@ -363,7 +364,7 @@ def has_html_certificates_enabled(course_key, course=None):
        course (CourseDescriptor|CourseOverview): A course.
    """
    # If the feature is disabled, then immediately return a False
-    if not settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False):
+    if not configuration_helpers.get_value("CERTIFICATES_HTML_VIEW", False):


Why doesn't this one call get_value_for_org?

@abeals This is inside LMS/microsite and not Studio, so the org/microsite is already automatically set. Studio is not microsite aware so we need to get the microsite differently.

abeals · 2018-02-25T14:33:18Z

cms/templates/widgets/header.html

@@ -32,7 +34,7 @@ <h1 class="branding">
            advanced_settings_url = reverse('contentstore.views.advanced_settings_handler', kwargs={'course_key_string': unicode(course_key)})
            tabs_url = reverse('contentstore.views.tabs_handler', kwargs={'course_key_string': unicode(course_key)})
            certificates_url = ''
-            if settings.FEATURES.get("CERTIFICATES_HTML_VIEW") and context_course.cert_html_view_enabled:
+            if configuration_helpers.get_value_for_org(current_organization.name, "CERTIFICATES_HTML_VIEW", False) and context_course.cert_html_view_enabled:


Why are we getting value for org based on current_organization.name instead of the org primary key (which I assume is something fixed like an ID)? We don't have so big of a table that the speed differences in indexing integers vs strings in the DB would be obvious, but using the ID is a standard pattern.

@abeals That's how edX's configuration helpers work, they expect a name/course org filter in this case instead of the ID.

abeals · 2018-02-25T16:18:44Z

openedx/core/djangoapps/appsembler/html_certificates/signals.py

+
+from course_modes.models import CourseMode
+
+@receiver(SignalHandler.course_published)


Is this signal fired once per course or every time the publish button is pressed in studio?

@abeals Yes, it is. Looking at the course signals is our best option: https://github.com/appsembler/edx-platform/blob/appsembler%2Famc%2Fdevelop/common/lib/xmodule/xmodule/modulestore/django.py#L91

I agree isn't the optimal.

abeals · 2018-02-25T16:21:11Z

openedx/core/djangoapps/appsembler/html_certificates/signals.py

+        default_mode = CourseMode.objects.get(
+            course_id=course_key,
+            mode_slug=settings.DEFAULT_COURSE_MODE_SLUG
+        )


Is there a risk here of ending up with multiple CourseMode objects for a course with this? That is, what if there's a CourseMode with the same course_id but a different mode_slug? This code looks like it would create a second CourseMode object without removing the first. Does that matter?

This could probably be replaced with get_or_create

@bezidejni Good idea!

@abeals replying your question: edX's idea is to have more than 1 course mode per course, so you can have different enrollment types. The only to create a course mode in edX is through the Django's admin, by default there is no course mode for a course (zero objects) so the platform fall backs to audit, if there is a at least one course mode object for a course, it will became the default.

abeals · 2018-02-25T16:23:30Z

openedx/core/djangoapps/appsembler/html_certificates/signals.py

+from django.conf import settings
+from django.core.exceptions import ObjectDoesNotExist
+from django.dispatch.dispatcher import receiver
+from xmodule.modulestore.django import modulestore, SignalHandler


Doesn't look like modulestore is used in this file.

bezidejni

Except for minor nitpicks, it generally looks good to me, but bear in mind that I'm not too familiar with certificates in edX.

bezidejni · 2018-02-27T12:42:10Z

cms/djangoapps/contentstore/views/certificates.py

@@ -394,7 +396,12 @@ def certificates_list_handler(request, course_key_string):
                certificate_web_view_url = None
            certificates = None
            is_active = False
-            if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False):
+            current_organization = request.user.organizations.first()


@abeals I think it is, and I agree on extracting that to a separate method.

bezidejni · 2018-02-27T14:03:11Z

cms/templates/widgets/header.html

@@ -32,7 +34,7 @@ <h1 class="branding">
            advanced_settings_url = reverse('contentstore.views.advanced_settings_handler', kwargs={'course_key_string': unicode(course_key)})
            tabs_url = reverse('contentstore.views.tabs_handler', kwargs={'course_key_string': unicode(course_key)})
            certificates_url = ''
-            if settings.FEATURES.get("CERTIFICATES_HTML_VIEW") and context_course.cert_html_view_enabled:
+            if configuration_helpers.get_value_for_org(current_organization.name, "CERTIFICATES_HTML_VIEW", False) and context_course.cert_html_view_enabled:


@abeals That's how edX's configuration helpers work, they expect a name/course org filter in this case instead of the ID.

bezidejni · 2018-02-27T14:04:21Z

lms/djangoapps/certificates/api.py

@@ -363,7 +364,7 @@ def has_html_certificates_enabled(course_key, course=None):
        course (CourseDescriptor|CourseOverview): A course.
    """
    # If the feature is disabled, then immediately return a False
-    if not settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False):
+    if not configuration_helpers.get_value("CERTIFICATES_HTML_VIEW", False):


@abeals This is inside LMS/microsite and not Studio, so the org/microsite is already automatically set. Studio is not microsite aware so we need to get the microsite differently.

bezidejni · 2018-02-27T14:06:35Z

openedx/core/djangoapps/appsembler/html_certificates/signals.py

+        default_mode = CourseMode.objects.get(
+            course_id=course_key,
+            mode_slug=settings.DEFAULT_COURSE_MODE_SLUG
+        )


This could probably be replaced with get_or_create

abeals · 2018-02-27T16:52:37Z

Changes look good to me.

melvinsoft · 2018-02-27T20:29:47Z

@abeals @bezidejni Thank you both for the reviews, merging now to deploy to staging.

this makes it easier to find and debug, also allows room for testing in the future the original work was on #212

melvinsoft added 3 commits February 22, 2018 10:54

Add new app that wires signal to create courses with default mode honor

5f5d7c8

modify courseware view since we don’t use pdf certs

a767dbd

another courseware fix

make CERTIFICATES_HTML_VIEW microsite aware

3c34002

melvinsoft requested review from bezidejni and bryanlandia February 25, 2018 00:06

bryanlandia approved these changes Feb 25, 2018

View reviewed changes

abeals reviewed Feb 25, 2018

View reviewed changes

bezidejni reviewed Feb 27, 2018

View reviewed changes

melvinsoft added 2 commits February 27, 2018 12:54

remove unused import

e111e9c

change course creation for get_or_create

4452bc5

melvinsoft merged commit 806ce36 into appsembler/amc/develop Feb 27, 2018

OmarIthawi deleted the appsembler/amc/feature/html-certs branch November 11, 2020 07:59

OmarIthawi mentioned this pull request Nov 27, 2020

Enable tests in the lms/courseware app #767

Merged

OmarIthawi added a commit that referenced this pull request Nov 27, 2020

refactor pdf cert fixes into own feature flag

97e0394

this makes it easier to find and debug, also allows room for testing in the future the original work was on #212

OmarIthawi mentioned this pull request Jul 28, 2022

Studio support for SiteConfiguration.api_adapter #1196

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML Certs changes for Tahoe #212

HTML Certs changes for Tahoe #212

melvinsoft commented Feb 25, 2018 •

edited

melvinsoft commented Feb 25, 2018

bryanlandia left a comment

abeals left a comment

abeals Feb 25, 2018

bezidejni Feb 27, 2018

melvinsoft Feb 27, 2018

abeals Feb 25, 2018

abeals Feb 25, 2018

melvinsoft Feb 27, 2018

abeals Feb 25, 2018

bezidejni Feb 27, 2018

abeals Feb 25, 2018

bezidejni Feb 27, 2018

abeals Feb 25, 2018

melvinsoft Feb 27, 2018 •

edited

abeals Feb 25, 2018

bezidejni Feb 27, 2018

melvinsoft Feb 27, 2018

abeals Feb 25, 2018

bezidejni left a comment

bezidejni Feb 27, 2018

bezidejni Feb 27, 2018

bezidejni Feb 27, 2018

bezidejni Feb 27, 2018

abeals commented Feb 27, 2018

melvinsoft commented Feb 27, 2018


		from course_modes.models import CourseMode

		@receiver(SignalHandler.course_published)

HTML Certs changes for Tahoe #212

HTML Certs changes for Tahoe #212

Conversation

melvinsoft commented Feb 25, 2018 • edited

melvinsoft commented Feb 25, 2018

bryanlandia left a comment

Choose a reason for hiding this comment

abeals 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

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

melvinsoft Feb 27, 2018 • edited

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

bezidejni 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

abeals commented Feb 27, 2018

melvinsoft commented Feb 27, 2018

melvinsoft commented Feb 25, 2018 •

edited

melvinsoft Feb 27, 2018 •

edited