refactored code to make certificate PDF generation optional

improved coverage

refactoring based on feedback

fixed quality violations

changes based on feedback

lms/djangoapps/certificates/api.py

@@ -6,8 +6,11 @@ rather than importing Django models directly.
 """
 import logging
 
+from django.conf import settings
 from django.core.urlresolvers import reverse
 
+from xmodule.modulestore.django import modulestore
+
 from certificates.models import (
     CertificateStatuses as cert_status,
     certificate_status_for_student,
@@ -21,7 +24,7 @@ from certificates.queue import XQueueCertInterface
 log = logging.getLogger("edx.certificate")
 
 
-def generate_user_certificates(student, course_key, course=None):
+def generate_user_certificates(student, course_key, course=None, insecure=False):
     """
     It will add the add-cert request into the xqueue.
 
@@ -36,9 +39,41 @@ def generate_user_certificates(student, course_key, course=None):
     Keyword Arguments:
         course (Course): Optionally provide the course object; if not provided
             it will be loaded.
+        insecure - (Boolean)
     """
     xqueue = XQueueCertInterface()
-    xqueue.add_cert(student, course_key, course=course)
+    if insecure:
+        xqueue.use_https = False
+    generate_pdf = not has_html_certificates_enabled(course_key, course)
+    return xqueue.add_cert(student, course_key, course=course, generate_pdf=generate_pdf)
+
+
+def regenerate_user_certificates(student, course_key, course=None,
+                                 forced_grade=None, template_file=None, insecure=False):
+    """
+    It will add the regen-cert request into the xqueue.
+
+    A new record will be created to track the certificate
+    generation task.  If an error occurs while adding the certificate
+    to the queue, the task will have status 'error'.
+
+    Args:
+        student (User)
+        course_key (CourseKey)
+
+    Keyword Arguments:
+        course (Course): Optionally provide the course object; if not provided
+            it will be loaded.
+        grade_value - The grade string, such as "Distinction"
+        template_file - The template file used to render this certificate
+        insecure - (Boolean)
+    """
+    xqueue = XQueueCertInterface()
+    if insecure:
+        xqueue.use_https = False
+
+    generate_pdf = not has_html_certificates_enabled(course_key, course)
+    return xqueue.regen_cert(student, course_key, course, forced_grade, template_file, generate_pdf)
 
 
 def certificate_downloadable_status(student, course_key):
@@ -156,6 +191,18 @@ def generate_example_certificates(course_key):
         xqueue.add_example_cert(cert)
 
 
+def has_html_certificates_enabled(course_key, course=None):
+    """
+    It determines if course has html certificates enabled
+    """
+    html_certificates_enabled = False
+    if settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False):
+        course = course if course else modulestore().get_course(course_key, depth=0)
+        if get_active_web_certificate(course) is not None:
+            html_certificates_enabled = True
+    return html_certificates_enabled
+
+
 def example_certificates_status(course_key):
     """Check the status of example certificates for a course.
 

lms/djangoapps/certificates/management/commands/regenerate_user.py

@@ -9,9 +9,8 @@ from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from xmodule.modulestore.django import modulestore
-from certificates.queue import XQueueCertInterface
 from certificates.models import BadgeAssertion
-
+from certificates.api import regenerate_user_certificates
 
 LOGGER = logging.getLogger(__name__)
 
@@ -107,14 +106,11 @@ class Command(BaseCommand):
             )
 
             # Add the certificate request to the queue
-            xqueue_interface = XQueueCertInterface()
-            if options['insecure']:
-                xqueue_interface.use_https = False
-
-            ret = xqueue_interface.regen_cert(
+            ret = regenerate_user_certificates(
                 student, course_id, course=course,
                 forced_grade=options['grade_value'],
-                template_file=options['template_file']
+                template_file=options['template_file'],
+                insecure=options['insecure']
             )
 
             try:

lms/djangoapps/certificates/management/commands/ungenerated_certs.py

@@ -7,7 +7,7 @@ import datetime
 from pytz import UTC
 from django.core.management.base import BaseCommand, CommandError
 from certificates.models import certificate_status_for_student
-from certificates.queue import XQueueCertInterface
+from certificates.api import generate_user_certificates
 from django.contrib.auth.models import User
 from optparse import make_option
 from opaque_keys import InvalidKeyError
@@ -108,9 +108,6 @@ class Command(BaseCommand):
                 courseenrollment__course_id=course_key
             )
 
-            xq = XQueueCertInterface()
-            if options['insecure']:
-                xq.use_https = False
             total = enrolled_students.count()
             count = 0
             start = datetime.datetime.now(UTC)
@@ -144,7 +141,12 @@ class Command(BaseCommand):
 
                     if not options['noop']:
                         # Add the certificate request to the queue
-                        ret = xq.add_cert(student, course_key, course=course)
+                        ret = generate_user_certificates(
+                            student,
+                            course_key,
+                            course=course,
+                            insecure=options['insecure']
+                        )
 
                         if ret == 'generating':
                             LOGGER.info(

lms/djangoapps/certificates/queue.py

@@ -104,7 +104,7 @@ class XQueueCertInterface(object):
         self.restricted = UserProfile.objects.filter(allow_certificate=False)
         self.use_https = True
 
-    def regen_cert(self, student, course_id, course=None, forced_grade=None, template_file=None):
+    def regen_cert(self, student, course_id, course=None, forced_grade=None, template_file=None, generate_pdf=True):
         """(Re-)Make certificate for a particular student in a particular course
 
         Arguments:
@@ -154,7 +154,7 @@ class XQueueCertInterface(object):
         except GeneratedCertificate.DoesNotExist:
             pass
 
-        return self.add_cert(student, course_id, course, forced_grade, template_file)
+        return self.add_cert(student, course_id, course, forced_grade, template_file, generate_pdf)
 
     def del_cert(self, student, course_id):
 
@@ -173,7 +173,9 @@ class XQueueCertInterface(object):
 
         raise NotImplementedError
 
-    def add_cert(self, student, course_id, course=None, forced_grade=None, template_file=None, title='None'):
+    # pylint: disable=too-many-statements
+    def add_cert(self, student, course_id, course=None, forced_grade=None, template_file=None,
+                 title='None', generate_pdf=True):
         """
         Request a new certificate for a student.
 
@@ -183,6 +185,7 @@ class XQueueCertInterface(object):
           forced_grade - a string indicating a grade parameter to pass with
                          the certificate request. If this is given, grading
                          will be skipped.
+          generate_pdf - Boolean should a message be sent in queue to generate certificate PDF
 
         Will change the certificate status to 'generating'.
 
@@ -340,35 +343,36 @@ class XQueueCertInterface(object):
                     }
                     if template_file:
                         contents['template_pdf'] = template_file
-                    new_status = status.generating
+                    new_status = status.generating if generate_pdf else status.downloadable
                     cert.status = new_status
                     cert.save()
 
-                    try:
-                        self._send_to_xqueue(contents, key)
-                    except XQueueAddToQueueError as exc:
-                        new_status = ExampleCertificate.STATUS_ERROR
-                        cert.status = new_status
-                        cert.error_reason = unicode(exc)
-                        cert.save()
-                        LOGGER.critical(
-                            (
-                                u"Could not add certificate task to XQueue.  "
-                                u"The course was '%s' and the student was '%s'."
-                                u"The certificate task status has been marked as 'error' "
-                                u"and can be re-submitted with a management command."
-                            ), student.id, course_id
-                        )
-                    else:
-                        LOGGER.info(
-                            (
-                                u"The certificate status has been set to '%s'.  "
-                                u"Sent a certificate grading task to the XQueue "
-                                u"with the key '%s'. "
-                            ),
-                            key,
-                            new_status
-                        )
+                    if generate_pdf:
+                        try:
+                            self._send_to_xqueue(contents, key)
+                        except XQueueAddToQueueError as exc:
+                            new_status = ExampleCertificate.STATUS_ERROR
+                            cert.status = new_status
+                            cert.error_reason = unicode(exc)
+                            cert.save()
+                            LOGGER.critical(
+                                (
+                                    u"Could not add certificate task to XQueue.  "
+                                    u"The course was '%s' and the student was '%s'."
+                                    u"The certificate task status has been marked as 'error' "
+                                    u"and can be re-submitted with a management command."
+                                ), course_id, student.id
+                            )
+                        else:
+                            LOGGER.info(
+                                (
+                                    u"The certificate status has been set to '%s'.  "
+                                    u"Sent a certificate grading task to the XQueue "
+                                    u"with the key '%s'. "
+                                ),
+                                new_status,
+                                key
+                            )
             else:
                 new_status = status.notpassing
                 cert.status = new_status

lms/djangoapps/certificates/tests/test_api.py

@@ -4,6 +4,7 @@ import ddt
 
 from django.test import TestCase, RequestFactory
 from django.test.utils import override_settings
+from django.conf import settings
 from mock import patch
 from nose.plugins.attrib import attr
 
@@ -25,6 +26,9 @@ from certificates.models import (
 from certificates.queue import XQueueCertInterface, XQueueAddToQueueError
 from certificates.tests.factories import GeneratedCertificateFactory
 
+FEATURES_WITH_CERTS_ENABLED = settings.FEATURES.copy()
+FEATURES_WITH_CERTS_ENABLED['CERTIFICATES_HTML_VIEW'] = True
+
 
 @attr('shard_1')
 class CertificateDownloadableStatusTests(ModuleStoreTestCase):
@@ -134,7 +138,7 @@ class GenerateUserCertificatesTest(ModuleStoreTestCase):
 
         # Verify that the certificate has status 'generating'
         cert = GeneratedCertificate.objects.get(user=self.student, course_id=self.course.id)
-        self.assertEqual(cert.status, 'generating')
+        self.assertEqual(cert.status, CertificateStatuses.generating)
 
     def test_xqueue_submit_task_error(self):
         with self._mock_passing_grade():
@@ -146,6 +150,19 @@ class GenerateUserCertificatesTest(ModuleStoreTestCase):
         self.assertEqual(cert.status, 'error')
         self.assertIn(self.ERROR_REASON, cert.error_reason)
 
+    @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED)
+    def test_new_cert_requests_returns_generating_for_html_certificate(self):
+        """
+        Test no message sent to Xqueue if HTML certificate view is enabled
+        """
+        self._setup_course_certificate()
+        with self._mock_passing_grade():
+            certs_api.generate_user_certificates(self.student, self.course.id)
+
+        # Verify that the certificate has status 'downloadable'
+        cert = GeneratedCertificate.objects.get(user=self.student, course_id=self.course.id)
+        self.assertEqual(cert.status, CertificateStatuses.downloadable)
+
     @contextmanager
     def _mock_passing_grade(self):
         """Mock the grading function to always return a passing grade. """
@@ -166,6 +183,26 @@ class GenerateUserCertificatesTest(ModuleStoreTestCase):
 
             yield mock_send_to_queue
 
+    def _setup_course_certificate(self):
+        """
+        Creates certificate configuration for course
+        """
+        certificates = [
+            {
+                'id': 1,
+                'name': 'Test Certificate Name',
+                'description': 'Test Certificate Description',
+                'course_title': 'tes_course_title',
+                'org_logo_path': '/t4x/orgX/testX/asset/org-logo.png',
+                'signatories': [],
+                'version': 1,
+                'is_active': True
+            }
+        ]
+        self.course.certificates = {'certificates': certificates}
+        self.course.save()
+        self.store.update_item(self.course, self.user.id)
+
 
 @attr('shard_1')
 @ddt.ddt

lms/djangoapps/certificates/tests/test_cert_management.py

 """Tests for the resubmit_error_certificates management command. """
 import ddt
+from contextlib import contextmanager
 from django.core.management.base import CommandError
 from nose.plugins.attrib import attr
 from django.test.utils import override_settings
@@ -10,7 +11,7 @@ from certificates.tests.factories import BadgeAssertionFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
-from certificates.management.commands import resubmit_error_certificates, regenerate_user
+from certificates.management.commands import resubmit_error_certificates, regenerate_user, ungenerated_certs
 from certificates.models import GeneratedCertificate, CertificateStatuses, BadgeAssertion
 
 
@@ -157,7 +158,7 @@ class RegenerateCertificatesTest(CertificateManagementTest):
         self.course = self.courses[0]
 
     @override_settings(CERT_QUEUE='test-queue')
-    @patch('certificates.management.commands.regenerate_user.XQueueCertInterface', spec=True)
+    @patch('certificates.api.XQueueCertInterface', spec=True)
     def test_clear_badge(self, xqueue):
         """
         Given that I have a user with a badge
@@ -174,6 +175,72 @@ class RegenerateCertificatesTest(CertificateManagementTest):
             grade_value=None
         )
         xqueue.return_value.regen_cert.assert_called_with(
-            self.user, key, course=self.course, forced_grade=None, template_file=None
+            self.user, key, self.course, None, None, True
         )
         self.assertFalse(BadgeAssertion.objects.filter(user=self.user, course_id=key))
+
+    @override_settings(CERT_QUEUE='test-queue')
+    @patch('capa.xqueue_interface.XQueueInterface.send_to_queue', spec=True)
+    def test_regenerating_certificate(self, mock_send_to_queue):
+        """
+        Given that I have a user who has not passed course
+        If I run regeneration for that user
+        Then certificate generation will be not be requested
+        """
+        key = self.course.location.course_key
+        self._create_cert(key, self.user, CertificateStatuses.downloadable)
+        self._run_command(
+            username=self.user.email, course=unicode(key), noop=False, insecure=True, template_file=None,
+            grade_value=None
+        )
+        certificate = GeneratedCertificate.objects.get(
+            user=self.user,
+            course_id=key
+        )
+        self.assertEqual(certificate.status, CertificateStatuses.notpassing)
+        self.assertFalse(mock_send_to_queue.called)
+
+
+@attr('shard_1')
+class UngenerateCertificatesTest(CertificateManagementTest):
+    """
+    Tests for generating certificates.
+    """
+    command = ungenerated_certs
+
+    def setUp(self):
+        """
+        We just need one course here.
+        """
+        super(UngenerateCertificatesTest, self).setUp()
+        self.course = self.courses[0]
+
+    @override_settings(CERT_QUEUE='test-queue')
+    @patch('capa.xqueue_interface.XQueueInterface.send_to_queue', spec=True)
+    def test_ungenerated_certificate(self, mock_send_to_queue):
+        """
+        Given that I have ended course
+        If I run ungenerated certs command
+        Then certificates should be generated for all users who passed course
+        """
+        mock_send_to_queue.return_value = (0, "Successfully queued")
+        key = self.course.location.course_key
+        self._create_cert(key, self.user, CertificateStatuses.unavailable)
+        with self._mock_passing_grade():
+            self._run_command(
+                course=unicode(key), noop=False, insecure=True, force=False
+            )
+        self.assertTrue(mock_send_to_queue.called)
+        certificate = GeneratedCertificate.objects.get(
+            user=self.user,
+            course_id=key
+        )
+        self.assertEqual(certificate.status, CertificateStatuses.generating)
+
+    @contextmanager
+    def _mock_passing_grade(self):
+        """Mock the grading function to always return a passing grade. """
+        symbol = 'courseware.grades.grade'
+        with patch(symbol) as mock_grade:
+            mock_grade.return_value = {'grade': 'Pass', 'percent': 0.75}
+            yield

lms/djangoapps/certificates/tests/test_queue.py

@@ -54,6 +54,17 @@ class XQueueCertInterfaceAddCertificateTest(ModuleStoreTestCase):
         actual_header = json.loads(kwargs['header'])
         self.assertIn('https://edx.org/update_certificate?key=', actual_header['lms_callback_url'])
 
+    def test_no_create_action_in_queue_for_html_view_certs(self):
+        """
+        Tests there is no certificate create message in the queue if generate_pdf is False
+        """
+        with patch('courseware.grades.grade', Mock(return_value={'grade': 'Pass', 'percent': 0.75})):
+            with patch.object(XQueueInterface, 'send_to_queue') as mock_send:
+                self.xqueue.add_cert(self.user, self.course.id, generate_pdf=False)
+
+        # Verify that add_cert method does not add message to queue
+        self.assertFalse(mock_send.called)
+
 
 @attr('shard_1')
 @override_settings(CERT_QUEUE='certificates')

lms/djangoapps/certificates/tests/test_views.py

@@ -4,6 +4,7 @@ import json
 import ddt
 from uuid import uuid4
 from nose.plugins.attrib import attr
+from mock import patch
 
 from django.conf import settings
 from django.core.cache import cache
@@ -14,13 +15,20 @@ from django.test.utils import override_settings
 
 from opaque_keys.edx.locator import CourseLocator
 from openedx.core.lib.tests.assertions.events import assert_event_matches
-from student.tests.factories import UserFactory
+from student.tests.factories import UserFactory, CourseEnrollmentFactory
 from track.tests import EventTrackingTestCase
 from xmodule.modulestore.tests.factories import CourseFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from certificates.api import get_certificate_url
-from certificates.models import ExampleCertificateSet, ExampleCertificate, GeneratedCertificate, BadgeAssertion
+from certificates.models import (
+    ExampleCertificateSet,
+    ExampleCertificate,
+    GeneratedCertificate,
+    BadgeAssertion,
+    CertificateStatuses
+)
+
 from certificates.tests.factories import (
     CertificateHtmlViewConfigurationFactory,
     LinkedInAddToProfileConfigurationFactory,
@@ -210,6 +218,10 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase):
             mode='honor',
             name=self.user.profile.name,
         )
+        CourseEnrollmentFactory.create(
+            user=self.user,
+            course_id=self.course_id
+        )
         CertificateHtmlViewConfigurationFactory.create()
         LinkedInAddToProfileConfigurationFactory.create()
 
@@ -453,6 +465,31 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase):
             self.get_event()
         )
 
+    @override_settings(FEATURES=FEATURES_WITH_CERTS_DISABLED)
+    def test_request_certificate_without_passing(self):
+        self.cert.status = CertificateStatuses.unavailable
+        self.cert.save()
+        request_certificate_url = reverse('certificates.views.request_certificate')
+        response = self.client.post(request_certificate_url, {'course_id': unicode(self.course.id)})
+        self.assertEqual(response.status_code, 200)
+        response_json = json.loads(response.content)
+        self.assertEqual(CertificateStatuses.notpassing, response_json['add_status'])
+
+    @override_settings(FEATURES=FEATURES_WITH_CERTS_DISABLED)
+    @override_settings(CERT_QUEUE='test-queue')
+    def test_request_certificate_after_passing(self):
+        self.cert.status = CertificateStatuses.unavailable
+        self.cert.save()
+        request_certificate_url = reverse('certificates.views.request_certificate')
+        with patch('capa.xqueue_interface.XQueueInterface.send_to_queue') as mock_queue:
+            mock_queue.return_value = (0, "Successfully queued")
+            with patch('courseware.grades.grade') as mock_grade:
+                mock_grade.return_value = {'grade': 'Pass', 'percent': 0.75}
+                response = self.client.post(request_certificate_url, {'course_id': unicode(self.course.id)})
+                self.assertEqual(response.status_code, 200)
+                response_json = json.loads(response.content)
+                self.assertEqual(CertificateStatuses.generating, response_json['add_status'])
+
 
 class TrackShareRedirectTest(ModuleStoreTestCase, EventTrackingTestCase):
     """

lms/djangoapps/certificates/views.py

@@ -16,7 +16,7 @@ from django.views.decorators.csrf import csrf_exempt
 from django.views.decorators.http import require_POST
 
 from capa.xqueue_interface import XQUEUE_METRIC_NAME
-from certificates.api import get_active_web_certificate, get_certificate_url
+from certificates.api import get_active_web_certificate, get_certificate_url, generate_user_certificates
 from certificates.models import (
     certificate_status_for_student,
     CertificateStatuses,
@@ -56,7 +56,6 @@ def request_certificate(request):
     """
     if request.method == "POST":
         if request.user.is_authenticated():
-            xqci = XQueueCertInterface()
             username = request.user.username
             student = User.objects.get(username=username)
             course_key = SlashSeparatedCourseKey.from_deprecated_string(request.POST.get('course_id'))
@@ -66,7 +65,7 @@ def request_certificate(request):
             if status in [CertificateStatuses.unavailable, CertificateStatuses.notpassing, CertificateStatuses.error]:
                 log_msg = u'Grading and certification requested for user %s in course %s via /request_certificate call'
                 logger.info(log_msg, username, course_key)
-                status = xqci.add_cert(student, course_key, course=course)
+                status = generate_user_certificates(student, course_key, course=course)
             return HttpResponse(json.dumps({'add_status': status}), mimetype='application/json')
         return HttpResponse(json.dumps({'add_status': 'ERRORANONYMOUSUSER'}), mimetype='application/json')