diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 2935ea3343..c664966eb8 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -212,6 +212,25 @@ class EligibleCertificateManager(models.Manager): ) +class EligibleAvailableCertificateManager(EligibleCertificateManager): + """ + A manager for `GeneratedCertificate` models that automatically + filters out ineligible certs and any linked to nonexistent courses. + + Adds to the super class filtering ot also exclude certificates for + courses that do not have a corresponding CourseOverview. + """ + + def get_queryset(self): + """ + Return a queryset for `GeneratedCertificate` models, filtering out + ineligible certificates and any linked to nonexistent courses. + """ + return super(EligibleAvailableCertificateManager, self).get_queryset().filter( + course_id__in=list(CourseOverview.objects.values_list('id', flat=True)) + ) + + class GeneratedCertificate(models.Model): """ Base model for generated certificates @@ -228,6 +247,10 @@ class GeneratedCertificate(models.Model): # preference to the default `objects` manager in most cases. eligible_certificates = EligibleCertificateManager() + # Only returns eligible certificates for courses that have an + # associated CourseOverview + eligible_available_certificates = EligibleAvailableCertificateManager() + # Normal object manager, which should only be used when ineligible # certificates (i.e. new audit certs) should be included in the # results. Django requires us to explicitly declare this. diff --git a/lms/djangoapps/certificates/tests/test_models.py b/lms/djangoapps/certificates/tests/test_models.py index 3387595ddc..3998403911 100644 --- a/lms/djangoapps/certificates/tests/test_models.py +++ b/lms/djangoapps/certificates/tests/test_models.py @@ -7,7 +7,7 @@ from django.core.exceptions import ValidationError from django.core.files.uploadedfile import SimpleUploadedFile from django.test import TestCase from django.test.utils import override_settings -from opaque_keys.edx.locator import CourseLocator +from opaque_keys.edx.locator import CourseLocator, CourseKey from path import Path as path from lms.djangoapps.certificates.models import ( @@ -22,6 +22,7 @@ from lms.djangoapps.certificates.models import ( ) from lms.djangoapps.certificates.tests.factories import CertificateInvalidationFactory, GeneratedCertificateFactory from lms.djangoapps.instructor_task.tests.factories import InstructorTaskFactory +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from student.tests.factories import AdminFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -200,37 +201,50 @@ class EligibleCertificateManagerTest(SharedModuleStoreTestCase): out ineligible certs. """ - @classmethod - def setUpClass(cls): - super(EligibleCertificateManagerTest, cls).setUpClass() - cls.courses = (CourseFactory(), CourseFactory()) - def setUp(self): super(EligibleCertificateManagerTest, self).setUp() self.user = UserFactory() + + self.course1 = CourseOverviewFactory() + self.course2 = CourseOverviewFactory( + id=CourseKey.from_string('{}a'.format(self.course1.id)) + ) + self.eligible_cert = GeneratedCertificateFactory.create( status=CertificateStatuses.downloadable, user=self.user, - course_id=self.courses[0].id + course_id=self.course1.id ) self.ineligible_cert = GeneratedCertificateFactory.create( status=CertificateStatuses.audit_passing, user=self.user, - course_id=self.courses[1].id + course_id=self.course2.id ) def test_filter_ineligible_certificates(self): """ - Verify that the EligibleCertificateManager filters out + Verify that the EligibleAvailableCertificateManager filters out certificates marked as ineligible, and that the default object manager for GeneratedCertificate does not filter them out. """ - self.assertEqual(list(GeneratedCertificate.eligible_certificates.filter(user=self.user)), [self.eligible_cert]) + self.assertEqual(list( + GeneratedCertificate.eligible_available_certificates.filter(user=self.user)), [self.eligible_cert] + ) self.assertEqual( list(GeneratedCertificate.objects.filter(user=self.user)), [self.eligible_cert, self.ineligible_cert] ) + def test_filter_certificates_for_nonexistent_courses(self): + """ + Verify that the EligibleAvailableCertificateManager filters out + certificates for courses with no CourseOverview. + """ + self.course1.delete() + self.assertFalse(GeneratedCertificate.eligible_available_certificates.filter( + user=self.user) + ) + @ddt.ddt class TestCertificateGenerationHistory(TestCase): diff --git a/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py b/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py index 91a9daf016..3da9ec5c9a 100644 --- a/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py +++ b/openedx/core/djangoapps/programs/management/commands/backpopulate_program_credentials.py @@ -111,5 +111,7 @@ class Command(BaseCommand): query = status_query & course_run_query - username_dicts = GeneratedCertificate.eligible_certificates.filter(query).values('user__username').distinct() + username_dicts = GeneratedCertificate.eligible_available_certificates.filter( + query + ).values('user__username').distinct() self.usernames = [d['user__username'] for d in username_dicts] diff --git a/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py b/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py index 44f20cceae..7397c70563 100644 --- a/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py +++ b/openedx/core/djangoapps/programs/tests/test_backpopulate_program_credentials.py @@ -8,6 +8,7 @@ from lms.djangoapps.certificates.models import CertificateStatuses from course_modes.models import CourseMode from lms.djangoapps.certificates.api import MODES from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory +from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.catalog.tests.factories import ( generate_course_run_key, ProgramFactory, @@ -15,6 +16,7 @@ from openedx.core.djangoapps.catalog.tests.factories import ( CourseRunFactory, ) from openedx.core.djangoapps.catalog.tests.mixins import CatalogIntegrationMixin +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.credentials.tests.mixins import CredentialsApiConfigMixin from openedx.core.djangolib.testing.utils import skip_unless_lms from student.tests.factories import UserFactory @@ -28,7 +30,9 @@ COMMAND_MODULE = 'openedx.core.djangoapps.programs.management.commands.backpopul @skip_unless_lms class BackpopulateProgramCredentialsTests(CatalogIntegrationMixin, CredentialsApiConfigMixin, TestCase): """Tests for the backpopulate_program_credentials management command.""" - course_run_key, alternate_course_run_key = (generate_course_run_key() for __ in range(2)) + course_run_key, alternate_course_run_key, course_run_key_no_course_overview = ( + generate_course_run_key() for __ in range(3) + ) # Constants for the _get_programs_data hierarchy types used in test_flatten() SEPARATE_PROGRAMS = 'separate_programs' SEPARATE_COURSES = 'separate_courses' @@ -40,6 +44,10 @@ class BackpopulateProgramCredentialsTests(CatalogIntegrationMixin, CredentialsAp self.alice = UserFactory() self.bob = UserFactory() + # We need CourseOverview instances to exist for the test courses + CourseOverviewFactory(id=CourseKey.from_string(self.course_run_key)) + CourseOverviewFactory(id=CourseKey.from_string(self.alternate_course_run_key)) + # Disable certification to prevent the task from being triggered when # setting up test data (i.e., certificates with a passing status), thereby # skewing mock call counts. @@ -299,6 +307,41 @@ class BackpopulateProgramCredentialsTests(CatalogIntegrationMixin, CredentialsAp mock_task.assert_called_once_with(self.alice.username) + def test_handle_no_course_overview(self, mock_task, mock_get_programs): + """ + Verify that the task is not enqueued for a user whose only certificate + is for a course with no CourseOverview. + """ + data = [ + ProgramFactory( + courses=[ + CourseFactory(course_runs=[ + CourseRunFactory(key=self.course_run_key), + CourseRunFactory(key=self.course_run_key_no_course_overview), + ]), + ] + ), + ] + mock_get_programs.return_value = data + + GeneratedCertificateFactory( + user=self.alice, + course_id=self.course_run_key, + mode=MODES.verified, + status=CertificateStatuses.downloadable, + ) + GeneratedCertificateFactory( + user=self.bob, + course_id=self.course_run_key_no_course_overview, + mode=MODES.verified, + status=CertificateStatuses.downloadable, + ) + + call_command('backpopulate_program_credentials', commit=True) + + mock_task.assert_called_once_with(self.alice.username) + mock_task.assert_not_called(self.bob.username) + @mock.patch(COMMAND_MODULE + '.logger.exception') def test_handle_enqueue_failure(self, mock_log, mock_task, mock_get_programs): """Verify that failure to enqueue a task doesn't halt execution.""" diff --git a/openedx/core/djangoapps/programs/utils.py b/openedx/core/djangoapps/programs/utils.py index 8ed354dd79..766c6908dd 100644 --- a/openedx/core/djangoapps/programs/utils.py +++ b/openedx/core/djangoapps/programs/utils.py @@ -285,7 +285,7 @@ class ProgramProgressMeter(object): Returns a dict of {uuid_string: available_datetime} """ # Query for all user certs up front, for performance reasons (rather than querying per course run). - user_certificates = GeneratedCertificate.eligible_certificates.filter(user=self.user) + user_certificates = GeneratedCertificate.eligible_available_certificates.filter(user=self.user) certificates_by_run = {cert.course_id: cert for cert in user_certificates} completed = {}