From caaf351e63e50a7582194e44b9365b43867ae4ef Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Tue, 31 Jul 2018 16:29:59 -0400 Subject: [PATCH 1/2] Fix visible dates sent to Credentials If we have a future available_date, send that to Credentials rather than sending the modified date. LEARNER-5995 --- openedx/core/djangoapps/certificates/api.py | 20 +++++---- .../djangoapps/certificates/tests/test_api.py | 43 +++++++++++++++++++ .../djangoapps/programs/tasks/v1/tasks.py | 4 +- .../programs/tasks/v1/tests/test_tasks.py | 5 ++- 4 files changed, 61 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/certificates/api.py b/openedx/core/djangoapps/certificates/api.py index ea3e335213..861962141f 100644 --- a/openedx/core/djangoapps/certificates/api.py +++ b/openedx/core/djangoapps/certificates/api.py @@ -95,13 +95,17 @@ def can_show_certificate_available_date_field(course): return _enabled_and_instructor_paced(course) -def display_date_for_certificate(course, certificate): - if ( - auto_certificate_generation_enabled() and - not course.self_paced and - course.certificate_available_date and - course.certificate_available_date < datetime.now(UTC) - ): - return course.certificate_available_date +def _course_uses_available_date(course): + return can_show_certificate_available_date_field(course) and course.certificate_available_date + +def available_date_for_certificate(course, certificate): + if _course_uses_available_date(course): + return course.certificate_available_date + return certificate.modified_date + + +def display_date_for_certificate(course, certificate): + if _course_uses_available_date(course) and course.certificate_available_date < datetime.now(UTC): + return course.certificate_available_date return certificate.modified_date diff --git a/openedx/core/djangoapps/certificates/tests/test_api.py b/openedx/core/djangoapps/certificates/tests/test_api.py index 03db4e1df2..f75b85e71b 100644 --- a/openedx/core/djangoapps/certificates/tests/test_api.py +++ b/openedx/core/djangoapps/certificates/tests/test_api.py @@ -121,3 +121,46 @@ class CertificatesApiTestCase(TestCase): self.certificate.status = certificate_status self.assertEqual(expected_value, api.is_certificate_valid(self.certificate)) + + @ddt.data( + (CourseMode.VERIFIED, CertificateStatuses.downloadable, True), + (CourseMode.VERIFIED, CertificateStatuses.notpassing, False), + (CourseMode.AUDIT, CertificateStatuses.downloadable, False) + ) + @ddt.unpack + def test_available_date(self, enrollment_mode, certificate_status, expected_value): + self.enrollment.mode = enrollment_mode + self.enrollment.save() + + self.certificate.mode = CourseMode.VERIFIED + self.certificate.status = certificate_status + + self.assertEqual(expected_value, api.is_certificate_valid(self.certificate)) + + @ddt.data( + (True, True, False), # feature enabled and self-paced should return False + (True, False, True), # feature enabled and instructor-paced should return True + (False, True, False), # feature not enabled and self-paced should return False + (False, False, False), # feature not enabled and instructor-paced should return False + ) + @ddt.unpack + def test_available_vs_display_date( + self, feature_enabled, is_self_paced, uses_avail_date + ): + self.course.self_paced = is_self_paced + with configure_waffle_namespace(feature_enabled): + maybe_avail = self.course.certificate_available_date if uses_avail_date else self.certificate.modified_date + + # With no available_date set, both return modified_date + self.assertEqual(self.certificate.modified_date, api.available_date_for_certificate(self.course)) + self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course)) + + # With an available date set in the past, both return the available date (if configured) + self.course.certificate_available_date = datetime(2017, 2, 1, tzinfo=pytz.UTC) + self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course)) + self.assertEqual(maybe_avail, api.display_date_for_certificate(self.course)) + + # With a future available date, they each return a different date + self.course.certificate_available_date = datetime.max + self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course)) + self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course)) diff --git a/openedx/core/djangoapps/programs/tasks/v1/tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tasks.py index 64e02902c1..f93c42558b 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tasks.py +++ b/openedx/core/djangoapps/programs/tasks/v1/tasks.py @@ -12,7 +12,7 @@ from opaque_keys.edx.keys import CourseKey from course_modes.models import CourseMode from lms.djangoapps.certificates.models import GeneratedCertificate -from openedx.core.djangoapps.certificates.api import display_date_for_certificate +from openedx.core.djangoapps.certificates.api import available_date_for_certificate from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credentials.models import CredentialsApiConfig from openedx.core.djangoapps.credentials.utils import get_credentials, get_credentials_api_client @@ -301,7 +301,7 @@ def award_course_certificate(self, username, course_run_key): # FIXME This may result in visible dates that do not update alongside the Course Overview if that changes # This is a known limitation of this implementation and was chosen to reduce the amount of replication, # endpoints, celery tasks, and jenkins jobs that needed to be written for this functionality - visible_date = display_date_for_certificate(course_overview, certificate) + visible_date = available_date_for_certificate(course_overview, certificate) post_course_certificate(credentials_client, username, certificate, visible_date) LOGGER.info('Awarded certificate for course %s to user %s', course_key, username) diff --git a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py index 60d6bb7429..3366f002d9 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py @@ -414,8 +414,10 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): def setUp(self): super(AwardCourseCertificatesTestCase, self).setUp() + self.available_date = datetime.datetime.max self.course = CourseOverviewFactory.create( - self_paced=True # Any option to allow the certificate to be viewable for the course + self_paced=True, # Any option to allow the certificate to be viewable for the course + certificate_available_date=self.available_date, ) self.student = UserFactory.create(username='test-student') # Instantiate the Certificate first so that the config doesn't execute issuance @@ -440,6 +442,7 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): call_args, _ = mock_post_course_certificate.call_args self.assertEqual(call_args[1], self.student.username) self.assertEqual(call_args[2], self.certificate) + self.assertEqual(call_args[3], self.available_date) def test_award_course_cert_not_called_if_disabled(self, mock_post_course_certificate): """ From 718c6a9340d13f0d8e9e5fd14aa17538157e3a26 Mon Sep 17 00:00:00 2001 From: Diana Huang Date: Fri, 3 Aug 2018 08:30:10 -0400 Subject: [PATCH 2/2] Test fixes for branch. --- .../djangoapps/certificates/tests/test_api.py | 19 ++++++++++-------- .../programs/tasks/v1/tests/test_tasks.py | 20 +++++++++++++++++-- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/openedx/core/djangoapps/certificates/tests/test_api.py b/openedx/core/djangoapps/certificates/tests/test_api.py index f75b85e71b..6b728ed5e5 100644 --- a/openedx/core/djangoapps/certificates/tests/test_api.py +++ b/openedx/core/djangoapps/certificates/tests/test_api.py @@ -51,6 +51,8 @@ class MockGeneratedCertificate(object): self.course_id = course_id self.mode = mode self.status = status + self.created_date = datetime.now(pytz.UTC) + self.modified_date = datetime.now(pytz.UTC) def is_valid(self): """ @@ -149,18 +151,19 @@ class CertificatesApiTestCase(TestCase): ): self.course.self_paced = is_self_paced with configure_waffle_namespace(feature_enabled): - maybe_avail = self.course.certificate_available_date if uses_avail_date else self.certificate.modified_date # With no available_date set, both return modified_date - self.assertEqual(self.certificate.modified_date, api.available_date_for_certificate(self.course)) - self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course)) + self.assertEqual(self.certificate.modified_date, api.available_date_for_certificate(self.course, self.certificate)) + self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course, self.certificate)) # With an available date set in the past, both return the available date (if configured) self.course.certificate_available_date = datetime(2017, 2, 1, tzinfo=pytz.UTC) - self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course)) - self.assertEqual(maybe_avail, api.display_date_for_certificate(self.course)) + maybe_avail = self.course.certificate_available_date if uses_avail_date else self.certificate.modified_date + self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course, self.certificate)) + self.assertEqual(maybe_avail, api.display_date_for_certificate(self.course, self.certificate)) # With a future available date, they each return a different date - self.course.certificate_available_date = datetime.max - self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course)) - self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course)) + self.course.certificate_available_date = datetime.max.replace(tzinfo=pytz.UTC) + maybe_avail = self.course.certificate_available_date if uses_avail_date else self.certificate.modified_date + self.assertEqual(maybe_avail, api.available_date_for_certificate(self.course, self.certificate)) + self.assertEqual(self.certificate.modified_date, api.display_date_for_certificate(self.course, self.certificate)) diff --git a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py index 3366f002d9..02994e0916 100644 --- a/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py +++ b/openedx/core/djangoapps/programs/tasks/v1/tests/test_tasks.py @@ -2,10 +2,12 @@ Tests for programs celery tasks. """ import json -from datetime import datetime +from datetime import datetime, timedelta import ddt import httpretty import mock +import pytz +from waffle.testutils import override_switch from celery.exceptions import MaxRetriesExceededError from django.conf import settings from django.test import override_settings, TestCase @@ -15,6 +17,7 @@ from edx_rest_api_client.client import EdxRestApiClient from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin +from openedx.core.djangoapps.certificates.config import waffle from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin from openedx.core.djangoapps.programs.tasks.v1 import tasks @@ -406,6 +409,7 @@ class PostCourseCertificateTestCase(TestCase): @skip_unless_lms @mock.patch(TASKS_MODULE + '.post_course_certificate') @override_settings(CREDENTIALS_SERVICE_USERNAME='test-service-username') +@override_switch(waffle.WAFFLE_NAMESPACE + '.' + waffle.AUTO_CERTIFICATE_GENERATION, True) class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): """ Test the award_course_certificate celery task @@ -414,7 +418,7 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): def setUp(self): super(AwardCourseCertificatesTestCase, self).setUp() - self.available_date = datetime.datetime.max + self.available_date = datetime.now(pytz.UTC) + timedelta(days=1) self.course = CourseOverviewFactory.create( self_paced=True, # Any option to allow the certificate to be viewable for the course certificate_available_date=self.available_date, @@ -442,6 +446,18 @@ class AwardCourseCertificatesTestCase(CredentialsApiConfigMixin, TestCase): call_args, _ = mock_post_course_certificate.call_args self.assertEqual(call_args[1], self.student.username) self.assertEqual(call_args[2], self.certificate) + self.assertEqual(call_args[3], self.certificate.modified_date) + + def test_award_course_certificates_available_date(self, mock_post_course_certificate): + """ + Tests the API POST method is called with available date when the course is not self paced + """ + self.course.self_paced = False + self.course.save() + tasks.award_course_certificate.delay(self.student.username, str(self.course.id)).get() + call_args, _ = mock_post_course_certificate.call_args + self.assertEqual(call_args[1], self.student.username) + self.assertEqual(call_args[2], self.certificate) self.assertEqual(call_args[3], self.available_date) def test_award_course_cert_not_called_if_disabled(self, mock_post_course_certificate):