Merge pull request #12922 from edx/ahsan/ECOM-4681-Tech-Debt-on-Certificate-generate-panel

Tech Debt on Certificate generate panel

lms/djangoapps/certificates/models.py

@@ -89,6 +89,8 @@ class CertificateStatuses(object):
     audit_passing = 'audit_passing'
     audit_notpassing = 'audit_notpassing'
     unverified = 'unverified'
+    invalidated = 'invalidated'
+    requesting = 'requesting'
 
     readable_statuses = {
         downloadable: "already received",

lms/djangoapps/courseware/tests/test_views.py

@@ -1104,6 +1104,7 @@ class StartDateTests(ModuleStoreTestCase):
         self.assertContains(response, "2015-JULY-17")
 
 
+# pylint: disable=protected-access, no-member
 @attr('shard_1')
 @ddt.ddt
 class ProgressPageTests(ModuleStoreTestCase):
@@ -1310,7 +1311,8 @@ class ProgressPageTests(ModuleStoreTestCase):
         )
         self.assertNotContains(resp, u"View Your Certificate")
         self.assertNotContains(resp, u"You can now view your certificate")
-        self.assertContains(resp, u"We're creating your certificate.")
+        self.assertContains(resp, "working on it...")
+        self.assertContains(resp, "creating your certificate")
 
     @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': False})
     @patch('courseware.grades.grade', Mock(return_value={'grade': 'Pass', 'percent': 0.75, 'section_breakdown': [],
@@ -1401,8 +1403,6 @@ class ProgressPageTests(ModuleStoreTestCase):
         generated_certificate = self.generate_certificate(
             "http://www.example.com/certificate.pdf", "honor"
         )
-        CertificateGenerationConfiguration(enabled=True).save()
-        certs_api.set_cert_generation_enabled(self.course.id, True)
 
         # Course certificate configurations
         certificates = [
@@ -1438,14 +1438,14 @@ class ProgressPageTests(ModuleStoreTestCase):
             "http://www.example.com/certificate.pdf", "honor"
         )
 
-        CertificateGenerationConfiguration(enabled=True).save()
-        certs_api.set_cert_generation_enabled(self.course.id, True)
         resp = self.client.get(
             reverse('progress', args=[unicode(self.course.id)])
         )
         self.assertContains(resp, u'Download Your Certificate')
         self.assert_invalidate_certificate(generated_certificate)
 
+    @patch('courseware.grades.grade', Mock(return_value={'grade': 'Pass', 'percent': 0.75, 'section_breakdown': [],
+                                                         'grade_breakdown': []}))
     def test_message_for_audit_mode(self):
         """ Verify that message appears on progress page, if learner is enrolled
          in audit mode.
@@ -1462,6 +1462,80 @@ class ProgressPageTests(ModuleStoreTestCase):
             u'You are enrolled in the audit track for this course. The audit track does not include a certificate.'
         )
 
+    def test_invalidated_cert_data(self):
+        """
+        Verify that invalidated cert data is returned if cert is invalidated.
+        """
+        generated_certificate = self.generate_certificate(
+            "http://www.example.com/certificate.pdf", "honor"
+        )
+
+        CertificateInvalidationFactory.create(
+            generated_certificate=generated_certificate,
+            invalidated_by=self.user
+        )
+        # Invalidate user certificate
+        generated_certificate.invalidate()
+        response = views._get_cert_data(self.user, self.course, self.course.id, True, CourseMode.HONOR)
+        self.assertEqual(response.cert_status, 'invalidated')
+        self.assertEqual(response.title, 'Your certificate has been invalidated')
+
+    def test_downloadable_get_cert_data(self):
+        """
+        Verify that downloadable cert data is returned if cert is downloadable.
+        """
+        self.generate_certificate(
+            "http://www.example.com/certificate.pdf", "honor"
+        )
+        with patch('certificates.api.certificate_downloadable_status',
+                   return_value=self.mock_certificate_downloadable_status(is_downloadable=True)):
+            response = views._get_cert_data(self.user, self.course, self.course.id, True, CourseMode.HONOR)
+
+        self.assertEqual(response.cert_status, 'downloadable')
+        self.assertEqual(response.title, 'Your certificate is available')
+
+    def test_generating_get_cert_data(self):
+        """
+        Verify that generating cert data is returned if cert is generating.
+        """
+        self.generate_certificate(
+            "http://www.example.com/certificate.pdf", "honor"
+        )
+        with patch('certificates.api.certificate_downloadable_status',
+                   return_value=self.mock_certificate_downloadable_status(is_generating=True)):
+            response = views._get_cert_data(self.user, self.course, self.course.id, True, CourseMode.HONOR)
+
+        self.assertEqual(response.cert_status, 'generating')
+        self.assertEqual(response.title, "We're working on it...")
+
+    def test_unverified_get_cert_data(self):
+        """
+        Verify that unverified cert data is returned if cert is unverified.
+        """
+        self.generate_certificate(
+            "http://www.example.com/certificate.pdf", "honor"
+        )
+        with patch('certificates.api.certificate_downloadable_status',
+                   return_value=self.mock_certificate_downloadable_status(is_unverified=True)):
+            response = views._get_cert_data(self.user, self.course, self.course.id, True, CourseMode.HONOR)
+
+        self.assertEqual(response.cert_status, 'unverified')
+        self.assertEqual(response.title, "Certificate unavailable")
+
+    def test_request_get_cert_data(self):
+        """
+        Verify that requested cert data is returned if cert is to be requested.
+        """
+        self.generate_certificate(
+            "http://www.example.com/certificate.pdf", "honor"
+        )
+        with patch('certificates.api.certificate_downloadable_status',
+                   return_value=self.mock_certificate_downloadable_status()):
+            response = views._get_cert_data(self.user, self.course, self.course.id, True, CourseMode.HONOR)
+
+        self.assertEqual(response.cert_status, 'requesting')
+        self.assertEqual(response.title, "Congratulations, you qualified for a certificate!")
+
     def assert_invalidate_certificate(self, certificate):
         """ Dry method to mark certificate as invalid. And assert the response. """
         CertificateInvalidationFactory.create(
@@ -1473,21 +1547,38 @@ class ProgressPageTests(ModuleStoreTestCase):
         resp = self.client.get(
             reverse('progress', args=[unicode(self.course.id)])
         )
+
         self.assertNotContains(resp, u'Request Certificate')
-        self.assertContains(resp, u'Your certificate has been invalidated.')
+        self.assertContains(resp, u'Your certificate has been invalidated')
         self.assertContains(resp, u'Please contact your course team if you have any questions.')
         self.assertNotContains(resp, u'View Your Certificate')
         self.assertNotContains(resp, u'Download Your Certificate')
 
     def generate_certificate(self, url, mode):
         """ Dry method to generate certificate. """
-        return GeneratedCertificateFactory.create(
+
+        generated_certificate = GeneratedCertificateFactory.create(
             user=self.user,
             course_id=self.course.id,
             status=CertificateStatuses.downloadable,
             download_url=url,
             mode=mode
         )
+        CertificateGenerationConfiguration(enabled=True).save()
+        certs_api.set_cert_generation_enabled(self.course.id, True)
+        return generated_certificate
+
+    def mock_certificate_downloadable_status(
+            self, is_downloadable=False, is_generating=False, is_unverified=False, uuid=None, download_url=None
+    ):
+        """Dry method to mock certificate downloadable status response."""
+        return {
+            'is_downloadable': is_downloadable,
+            'is_generating': is_generating,
+            'is_unverified': is_unverified,
+            'download_url': uuid,
+            'uuid': download_url,
+        }
 
 
 @attr('shard_1')

lms/djangoapps/courseware/views/views.py

@@ -5,7 +5,7 @@ Courseware views functions
 import json
 import logging
 import urllib
-from collections import OrderedDict
+from collections import OrderedDict, namedtuple
 from datetime import datetime
 
 import analytics
@@ -40,6 +40,7 @@ import survey.utils
 import survey.views
 from lms.djangoapps.ccx.utils import prep_course_for_grading
 from certificates import api as certs_api
+from certificates.models import CertificateStatuses
 from course_blocks.api import get_course_blocks
 from openedx.core.djangoapps.models.course_details import CourseDetails
 from commerce.utils import EcommerceService
@@ -103,6 +104,8 @@ log = logging.getLogger("edx.courseware")
 # credit and verified modes.
 REQUIREMENTS_DISPLAY_MODES = CourseMode.CREDIT_MODES + [CourseMode.VERIFIED]
 
+CertData = namedtuple("CertData", ["cert_status", "title", "msg", "download_url", "cert_web_view_url"])
+
 
 def user_groups(user):
     """
@@ -691,17 +694,17 @@ def _progress(request, course_key, student_id):
         return redirect(reverse('course_survey', args=[unicode(course.id)]))
 
     staff_access = bool(has_access(request.user, 'staff', course))
-    try:
-        coach_access = has_ccx_coach_role(request.user, course_key)
-    except CCXLocatorValidationException:
-        coach_access = False
-
-    has_access_on_students_profiles = staff_access or coach_access
 
     if student_id is None or student_id == request.user.id:
         # always allowed to see your own profile
         student = request.user
     else:
+        try:
+            coach_access = has_ccx_coach_role(request.user, course_key)
+        except CCXLocatorValidationException:
+            coach_access = False
+
+        has_access_on_students_profiles = staff_access or coach_access
         # Requesting access to a different student's profile
         if not has_access_on_students_profiles:
             raise Http404
@@ -721,26 +724,16 @@ def _progress(request, course_key, student_id):
     course_structure = get_course_blocks(student, course.location)
 
     courseware_summary = grades.progress_summary(student, course, course_structure)
-    grade_summary = grades.grade(student, course, course_structure=course_structure)
-    studio_url = get_studio_url(course, 'settings/grading')
-
     if courseware_summary is None:
-        #This means the student didn't have access to the course (which the instructor requested)
+        # This means the student didn't have access to the course (which the instructor requested)
         raise Http404
 
+    grade_summary = grades.grade(student, course, course_structure=course_structure)
+    studio_url = get_studio_url(course, 'settings/grading')
+
     # checking certificate generation configuration
     enrollment_mode, is_active = CourseEnrollment.enrollment_mode_for_user(student, course_key)
 
-    # If the learner is in verified modes and the student did not have
-    # their ID verified, we need to show message to ask learner to verify their ID first
-    missing_required_verification = enrollment_mode in CourseMode.VERIFIED_MODES and \
-        not SoftwareSecurePhotoVerification.user_is_verified(student)
-
-    show_generate_cert_btn = (
-        is_active and CourseMode.is_eligible_for_certificate(enrollment_mode)
-        and certs_api.cert_generation_enabled(course_key)
-    )
-
     context = {
         'course': course,
         'courseware_summary': courseware_summary,
@@ -749,40 +742,115 @@ def _progress(request, course_key, student_id):
         'staff_access': staff_access,
         'student': student,
         'passed': is_course_passed(course, grade_summary),
-        'show_generate_cert_btn': show_generate_cert_btn,
         'credit_course_requirements': _credit_course_requirements(course_key, student),
-        'missing_required_verification': missing_required_verification,
-        'certificate_invalidated': False,
-        'enrollment_mode': enrollment_mode,
+        'certificate_data': _get_cert_data(student, course, course_key, is_active, enrollment_mode)
     }
 
-    if show_generate_cert_btn:
-        # If current certificate is invalidated by instructor
-        # then show the certificate invalidated message.
-        context.update({
-            'certificate_invalidated': certs_api.is_certificate_invalid(student, course_key)
-        })
+    with outer_atomic():
+        response = render_to_response('courseware/progress.html', context)
+
+    return response
+
+
+def _get_cert_data(student, course, course_key, is_active, enrollment_mode):
+    """Returns students course certificate related data.
+
+    Arguments:
+        student (User): Student for whom certificate to retrieve.
+        course (Course): Course object for which certificate data to retrieve.
+        course_key (CourseKey): Course identifier for course.
+        is_active (Bool): Boolean value to check if course is active.
+        enrollment_mode (String): Course mode in which student is enrolled.
+
+    Returns:
+        returns dict if course certificate is available else None.
+    """
+
+    if enrollment_mode == CourseMode.AUDIT:
+        return CertData(
+            CertificateStatuses.audit_passing,
+            'Your enrollment: Audit track',
+            'You are enrolled in the audit track for this course. The audit track does not include a certificate.',
+            download_url=None,
+            cert_web_view_url=None
+        )
 
-        cert_status = certs_api.certificate_downloadable_status(student, course_key)
-        context.update(cert_status)
-        # showing the certificate web view button if feature flags are enabled.
+    show_generate_cert_btn = (
+        is_active and CourseMode.is_eligible_for_certificate(enrollment_mode)
+        and certs_api.cert_generation_enabled(course_key)
+    )
+
+    if not show_generate_cert_btn:
+        return None
+
+    if certs_api.is_certificate_invalid(student, course_key):
+        return CertData(
+            CertificateStatuses.invalidated,
+            'Your certificate has been invalidated',
+            'Please contact your course team if you have any questions.',
+            download_url=None,
+            cert_web_view_url=None
+        )
+
+    cert_downloadable_status = certs_api.certificate_downloadable_status(student, course_key)
+
+    if cert_downloadable_status['is_downloadable']:
+        cert_status = CertificateStatuses.downloadable
+        title = 'Your certificate is available'
+        msg = 'You can keep working for a higher grade, or request your certificate now.'
         if certs_api.has_html_certificates_enabled(course_key, course):
             if certs_api.get_active_web_certificate(course) is not None:
-                context.update({
-                    'show_cert_web_view': True,
-                    'cert_web_view_url': certs_api.get_certificate_url(course_id=course_key, uuid=cert_status['uuid']),
-                })
+                cert_web_view_url = certs_api.get_certificate_url(
+                    course_id=course_key, uuid=cert_downloadable_status['uuid']
+                )
+                return CertData(cert_status, title, msg, download_url=None, cert_web_view_url=cert_web_view_url)
             else:
-                context.update({
-                    'is_downloadable': False,
-                    'is_generating': True,
-                    'download_url': None
-                })
+                return CertData(
+                    CertificateStatuses.generating,
+                    "We're working on it...",
+                    "We're creating your certificate. You can keep working in your courses and a link "
+                    "to it will appear here and on your Dashboard when it is ready.",
+                    download_url=None,
+                    cert_web_view_url=None
+                )
 
-    with outer_atomic():
-        response = render_to_response('courseware/progress.html', context)
+        return CertData(
+            cert_status, title, msg, download_url=cert_downloadable_status['download_url'], cert_web_view_url=None
+        )
 
-    return response
+    if cert_downloadable_status['is_generating']:
+        return CertData(
+            CertificateStatuses.generating,
+            "We're working on it...",
+            "We're creating your certificate. You can keep working in your courses and a link to "
+            "it will appear here and on your Dashboard when it is ready.",
+            download_url=None,
+            cert_web_view_url=None
+        )
+
+    # If the learner is in verified modes and the student did not have
+    # their ID verified, we need to show message to ask learner to verify their ID first
+    missing_required_verification = enrollment_mode in CourseMode.VERIFIED_MODES and \
+        not SoftwareSecurePhotoVerification.user_is_verified(student)
+
+    if missing_required_verification or cert_downloadable_status['is_unverified']:
+        platform_name = theming_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME)
+        return CertData(
+            CertificateStatuses.unverified,
+            'Certificate unavailable',
+            'You have not received a certificate because you do not have a current {platform_name} verified '
+            'identity.'.format(platform_name=platform_name),
+            download_url=None,
+            cert_web_view_url=None
+        )
+
+    return CertData(
+        CertificateStatuses.requesting,
+        'Congratulations, you qualified for a certificate!',
+        'You can keep working for a higher grade, or request your certificate now.',
+        download_url=None,
+        cert_web_view_url=None
+    )
 
 
 def _credit_course_requirements(course_key, student):

lms/templates/courseware/progress.html

@@ -3,6 +3,7 @@
 <%def name="online_help_token()"><% return "progress" %></%def>
 <%!
 from course_modes.models import CourseMode
+from certificates.models import CertificateStatuses
 from django.utils.translation import ugettext as _
 from django.core.urlresolvers import reverse
 from util.date_utils import get_time_display, DEFAULT_SHORT_DATE_FORMAT
@@ -49,66 +50,29 @@ from django.utils.http import urlquote_plus
             <h1 class="progress-certificates-title">${_("Course Progress for Student '{username}' ({email})").format(username=student.username, email=student.email) | h}</h1>
           </header>
         <div class="wrapper-msg wrapper-auto-cert">
-          %if enrollment_mode == CourseMode.AUDIT:
-            <div class="auto-cert-message">
-              <div class="msg-content">
-                <h2 class="title">${_("Your enrollment: Audit track")}</h2>
-                <p class="copy">${_("You are enrolled in the audit track for this course. The audit track does not include a certificate.")}</p>
-              </div>
-            </div>
-          %elif show_generate_cert_btn:
+          %if certificate_data:
             <div id="errors-info" class="errors-info"></div>
             %if passed:
             <div class="auto-cert-message" id="course-success">
               <div class="has-actions">
                   <% post_url = reverse('generate_user_cert', args=[unicode(course.id)]) %>
-                  ## If current certificate is invalidated by instructor then don't show the generate button.
-                  % if certificate_invalidated:
-                  <p class="copy">${_("Your certificate has been invalidated. Please contact your course team if you have any questions.")}</p>
-                  %elif is_downloadable:
                   <div class="msg-content">
-                    <h2 class="title">${_("Your certificate is available")}</h2>
-                    <p class="copy">
-                      ${_("You can keep working for a higher grade, or request your certificate now.")}
-                    </p>
+                    <h2 class="title">${certificate_data.title | h}</h2>
+                    <p class="copy">${certificate_data.msg | h}</p>
                   </div>
                   <div class="msg-actions">
-                    %if show_cert_web_view and cert_web_view_url:
-                    <a class="btn" href="${cert_web_view_url | h}" target="_blank" title="${_('View certificate in a new browser window or tab.')}">
+                    %if certificate_data.cert_web_view_url:
+                    <a class="btn" href="${certificate_data.cert_web_view_url | h}" target="_blank" title="${_('View certificate in a new browser window or tab.')}">
                       ${_("View Certificate")}
                     </a>
-                    %elif download_url:
-                    <a class="btn" href="${download_url | h}" target="_blank" title="${_('PDF will open in a new browser window or tab.')}">
+                    %elif certificate_data.cert_status == CertificateStatuses.downloadable and certificate_data.download_url:
+                    <a class="btn" href="${certificate_data.download_url | h}" target="_blank" title="${_('PDF will open in a new browser window or tab.')}">
                       ${_("Download Your Certificate")}
                     </a>
+                    %elif certificate_data.cert_status == CertificateStatuses.requesting:
+                      <button class="btn generate_certs" data-endpoint="${post_url | h}" id="btn_generate_cert">${_('Request Certificate')}</button>
                     %endif
                   </div>
-                  %elif is_generating:
-                  <div class="msg-content">
-                    ## Translators: This message appears to users when the system is processessing course certificates, which can take a few hours.
-                    <h2 class="title">${_("We're working on it...")}</h2>
-                    <p class="copy">${_("We're creating your certificate. You can keep working in your courses and a link to it will appear here and on your Dashboard when it is ready.")}</p>
-                  </div>
-                  <div class="msg-actions"></div>
-                  %elif missing_required_verification or is_unverified:
-                  ## The is_unverified variable comes from certificate status.
-                  ## At the moment, the assumption is we should not show generate certificate button
-                  ## if the certificate previously generated is in unverified status
-                  <div class="msg-content">
-                    ## Translators: This message appears to users when the users have not completed identity verification.
-                    <h2 class="title">${_("Certificate unavailable")}</h2>
-                    <p class="copy">${_("You have not received a certificate because you do not have a current {platform_name} verified identity. ").format(platform_name=settings.PLATFORM_NAME)} <a href="${reverse('verify_student_reverify')}"> ${_("Verify your identity now.")}</a></p>
-                  </div>
-                  <div class="msg-actions"></div>
-                  %else:
-                  <div class="msg-content">
-                    <h2 class="title">${_("Congratulations, you qualified for a certificate!")}</h2>
-                    <p class="copy">${_("You can keep working for a higher grade, or request your certificate now.")}</p>
-                  </div>
-                  <div class="msg-actions">
-                    <button class="btn generate_certs" data-endpoint="${post_url | h}" id="btn_generate_cert">${_('Request Certificate')}</button>
-                  </div>
-                  %endif
               </div>
             </div>
             %endif