diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 63ab1f045b..7c370a17f4 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -20,12 +20,15 @@ from lms.djangoapps.certificates.models import ( ) from lms.djangoapps.certificates.queue import XQueueCertInterface from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate -from lms.djangoapps.certificates.utils import emit_certificate_event, has_html_certificates_enabled +from lms.djangoapps.certificates.utils import ( + emit_certificate_event, + has_html_certificates_enabled_from_course_overview +) from lms.djangoapps.grades.api import CourseGradeFactory -from lms.djangoapps.instructor.access import list_with_level +from lms.djangoapps.instructor.access import list_with_level_from_course_key from lms.djangoapps.verify_student.services import IDVerificationService +from openedx.core.djangoapps.content.course_overviews.api import get_course_overview from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag -from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -190,12 +193,11 @@ def _can_generate_v2_certificate(user, course_key): log.info(f'{course_key} is a CCX course. Certificate cannot be generated for {user.id}.') return False - course = _get_course(course_key) - if _is_beta_tester(user, course): + if _is_beta_tester(user, course_key): log.info(f'{user.id} is a beta tester in {course_key}. Certificate cannot be generated.') return False - if not _has_passing_grade(user, course): + if not _has_passing_grade(user, course_key): log.info(f'{user.id} does not have a passing grade in {course_key}. Certificate cannot be generated.') return False @@ -236,8 +238,8 @@ def _can_generate_certificate_common(user, course_key): if not _can_generate_certificate_for_status(user, course_key): return False - course = _get_course(course_key) - if not has_html_certificates_enabled(course): + course_overview = get_course_overview(course_key) + if not has_html_certificates_enabled_from_course_overview(course_overview): log.info(f'{course_key} does not have HTML certificates enabled. Certificate cannot be generated for ' f'{user.id}.') return False @@ -274,8 +276,7 @@ def _set_v2_cert_status(user, course_key): if status is not None: return status - course = _get_course(course_key) - course_grade = _get_course_grade(user, course) + course_grade = _get_course_grade(user, course_key) if not course_grade.passed: if cert is None: cert = GeneratedCertificate.objects.create(user=user, course_id=course_key) @@ -320,8 +321,7 @@ def _can_set_allowlist_cert_status(user, course_key): if not is_on_certificate_allowlist(user, course_key): return False - course = _get_course(course_key) - return _can_set_cert_status_common(user, course_key, course) + return _can_set_cert_status_common(user, course_key) def _can_set_v2_cert_status(user, course_key): @@ -334,14 +334,13 @@ def _can_set_v2_cert_status(user, course_key): if _is_ccx_course(course_key): return False - course = _get_course(course_key) - if _is_beta_tester(user, course): + if _is_beta_tester(user, course_key): return False - return _can_set_cert_status_common(user, course_key, course) + return _can_set_cert_status_common(user, course_key) -def _can_set_cert_status_common(user, course_key, course): +def _can_set_cert_status_common(user, course_key): """ Determine whether we can set a custom (non-downloadable) cert status """ @@ -355,7 +354,8 @@ def _can_set_cert_status_common(user, course_key, course): if not modes_api.is_eligible_for_certificate(enrollment_mode): return False - if not has_html_certificates_enabled(course): + course_overview = get_course_overview(course_key) + if not has_html_certificates_enabled_from_course_overview(course_overview): return False return True @@ -413,11 +413,11 @@ def _can_generate_certificate_for_status(user, course_key): return True -def _is_beta_tester(user, course): +def _is_beta_tester(user, course_key): """ Check if the user is a beta tester in this course run """ - beta_testers_queryset = list_with_level(course, 'beta') + beta_testers_queryset = list_with_level_from_course_key(course_key, 'beta') return beta_testers_queryset.filter(username=user.username).exists() @@ -428,19 +428,19 @@ def _is_ccx_course(course_key): return hasattr(course_key, 'ccx') -def _has_passing_grade(user, course): +def _has_passing_grade(user, course_key): """ Check if the user has a passing grade in this course run """ - course_grade = _get_course_grade(user, course) + course_grade = _get_course_grade(user, course_key) return course_grade.passed -def _get_course_grade(user, course): +def _get_course_grade(user, course_key): """ Get the user's course grade in this course run """ - return CourseGradeFactory().read(user, course) + return CourseGradeFactory().read(user, course_key=course_key) def _is_cert_downloadable(user, course_key): @@ -458,13 +458,6 @@ def _is_cert_downloadable(user, course_key): return True -def _get_course(course_key): - """ - Get the course from the course key - """ - return modulestore().get_course(course_key, depth=0) - - def generate_user_certificates(student, course_key, course=None, insecure=False, generation_mode='batch', forced_grade=None): """ @@ -499,21 +492,17 @@ def generate_user_certificates(student, course_key, course=None, insecure=False, f'{student.id}.') return generate_certificate_task(student, course_key) - if not course: - course = modulestore().get_course(course_key, depth=0) - - beta_testers_queryset = list_with_level(course, 'beta') - + beta_testers_queryset = list_with_level_from_course_key(course_key, 'beta') if beta_testers_queryset.filter(username=student.username): - message = 'Cancelling course certificate generation for user [{}] against course [{}], user is a Beta Tester.' - log.info(message.format(student.username, course_key)) + log.info(f"Canceling Certificate Generation task for user {student.id} : {course_key}. User is a Beta Tester.") return xqueue = XQueueCertInterface() if insecure: xqueue.use_https = False - generate_pdf = not has_html_certificates_enabled(course) + course_overview = get_course_overview(course_key) + generate_pdf = not has_html_certificates_enabled_from_course_overview(course_overview) cert = xqueue.add_cert( student, @@ -523,8 +512,7 @@ def generate_user_certificates(student, course_key, course=None, insecure=False, forced_grade=forced_grade ) - message = 'Queued Certificate Generation task for {user} : {course}' - log.info(message.format(user=student.id, course=course_key)) + log.info(f"Queued Certificate Generation task for {student.id} : {course_key}") # If cert_status is not present in certificate valid_statuses (for example unverified) then # add_cert returns None and raises AttributeError while accessing cert attributes. @@ -565,22 +553,18 @@ def regenerate_user_certificates(student, course_key, course=None, insecure - (Boolean) """ if can_generate_certificate_task(student, course_key): - log.info(f'{course_key} is using V2 certificates. Attempt will be made to regenerate a V2 certificate for ' - f'user {student.id}.') + log.info(f"{course_key} is using V2 certificates. Attempt will be made to regenerate a V2 certificate for " + f"user {student.id}.") return generate_certificate_task(student, course_key) xqueue = XQueueCertInterface() if insecure: xqueue.use_https = False - if not course: - course = modulestore().get_course(course_key, depth=0) - - generate_pdf = not has_html_certificates_enabled(course) - log.info( - "Started regenerating certificates for user %s in course %s with generate_pdf status: %s", - student.username, str(course_key), generate_pdf - ) + course_overview = get_course_overview(course_key) + generate_pdf = not has_html_certificates_enabled_from_course_overview(course_overview) + log.info(f"Started regenerating certificates for user {student.id} in course {course_key} with generate_pdf " + f"status: {generate_pdf}.") xqueue.regen_cert( student, diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index f0af0343dd..40485771fd 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -38,7 +38,7 @@ BETA_TESTER_METHOD = 'lms.djangoapps.certificates.generation_handler._is_beta_te CCX_COURSE_METHOD = 'lms.djangoapps.certificates.generation_handler._is_ccx_course' ID_VERIFIED_METHOD = 'lms.djangoapps.verify_student.services.IDVerificationService.user_is_verified' PASSING_GRADE_METHOD = 'lms.djangoapps.certificates.generation_handler._has_passing_grade' -WEB_CERTS_METHOD = 'lms.djangoapps.certificates.generation_handler.has_html_certificates_enabled' +WEB_CERTS_METHOD = 'lms.djangoapps.certificates.generation_handler.has_html_certificates_enabled_from_course_overview' @mock.patch(ID_VERIFIED_METHOD, mock.Mock(return_value=True)) diff --git a/lms/djangoapps/certificates/tests/test_utils.py b/lms/djangoapps/certificates/tests/test_utils.py new file mode 100644 index 0000000000..4fad425945 --- /dev/null +++ b/lms/djangoapps/certificates/tests/test_utils.py @@ -0,0 +1,45 @@ +""" +Tests for Certificates app utility functions +""" +from unittest.mock import patch + +from django.test import TestCase + +from lms.djangoapps.certificates.utils import has_html_certificates_enabled_from_course_overview +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory + + +class CertificateUtilityTests(TestCase): + """ + Tests for course certificate utility functions + """ + def setUp(self): + super().setUp() + self.course_overview = CourseOverviewFactory.create() + + @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': False}) + def test_has_html_certificates_enabled_from_course_overview_cert_html_view_disabled(self): + """ + Test to ensure we return the correct value when the `CERTIFICATES_HTML_VIEW` setting is disabled. + """ + assert not has_html_certificates_enabled_from_course_overview(self.course_overview) + + @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) + def test_has_html_certificates_enabled_from_course_overview_enabled(self): + """ + Test to ensure we return the correct value when the HTML certificates are enabled in a course-run. + """ + self.course_overview.cert_html_view_enabled = True + self.course_overview.save() + + assert has_html_certificates_enabled_from_course_overview(self.course_overview) + + @patch.dict('django.conf.settings.FEATURES', {'CERTIFICATES_HTML_VIEW': True}) + def test_has_html_certificates_enabled_from_course_overview_disabled(self): + """ + Test to ensure we return the correct value when the HTML certificates are disabled in a course-run. + """ + self.course_overview.cert_html_view_enabled = False + self.course_overview.save() + + assert not has_html_certificates_enabled_from_course_overview(self.course_overview) diff --git a/lms/djangoapps/certificates/utils.py b/lms/djangoapps/certificates/utils.py index a9fc06c1ac..83bf9181e1 100644 --- a/lms/djangoapps/certificates/utils.py +++ b/lms/djangoapps/certificates/utils.py @@ -69,6 +69,19 @@ def has_html_certificates_enabled(course): return course.cert_html_view_enabled +def has_html_certificates_enabled_from_course_overview(course_overview): + """ + Returns True if HTML certificates are enabled + + This is a companion function to the `has_html_certificates_enabled` function. We are in the process of refactoring + and removing the `Certificates` apps dependence on `modulestore`. These functions will be consolidated at a later + date. Progress is being tracked in MICROBA-1178. + """ + if not settings.FEATURES.get('CERTIFICATES_HTML_VIEW', False): + return False + return course_overview.cert_html_view_enabled + + def _certificate_html_url(uuid): """ Returns uuid based certificate URL. diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index ece469ea63..15df31bc51 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -44,6 +44,22 @@ def list_with_level(course, level): return ROLES[level](course.id).users_with_role() +def list_with_level_from_course_key(course_key, level): + """ + List users who have 'level' access. + + The 'level' value can be 'instructor', 'staff', or 'beta' for standard courses. + + It is possible for other levels to be defined specific to the course. If there is no group for that level we return + an empty list. + + This is a companion function to the `list_with_level` function. We are in the process of refactoring and removing + the `Certificates` apps dependence on `modulestore`. These functions will be consolidated at a later date. Progress + is being tracked in MICROBA-1178. + """ + return ROLES[level](course_key).users_with_role() + + def allow_access(course, user, level, send_email=True): """ Allow user access to course modification. diff --git a/lms/djangoapps/instructor/tests/test_access.py b/lms/djangoapps/instructor/tests/test_access.py index 961a4d7203..8d99ad8f95 100644 --- a/lms/djangoapps/instructor/tests/test_access.py +++ b/lms/djangoapps/instructor/tests/test_access.py @@ -7,7 +7,13 @@ import pytest from common.djangoapps.student.roles import CourseBetaTesterRole, CourseCcxCoachRole, CourseStaffRole from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.instructor.access import allow_access, list_with_level, revoke_access, update_forum_role +from lms.djangoapps.instructor.access import ( + allow_access, + list_with_level, + list_with_level_from_course_key, + revoke_access, + update_forum_role +) from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_MODERATOR, Role from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -32,11 +38,15 @@ class TestInstructorAccessList(SharedModuleStoreTestCase): def test_list_instructors(self): instructors = list_with_level(self.course, 'instructor') + instructors_alternative = list_with_level_from_course_key(self.course.id, 'instructor') assert set(instructors) == set(self.instructors) + assert set(instructors_alternative) == set(self.instructors) def test_list_beta(self): beta_testers = list_with_level(self.course, 'beta') + beta_testers_alternative = list_with_level_from_course_key(self.course.id, 'beta') assert set(beta_testers) == set(self.beta_testers) + assert set(beta_testers_alternative) == set(self.beta_testers) class TestInstructorAccessAllow(EmailTemplateTagMixin, SharedModuleStoreTestCase): diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 8252aba89c..04ca0fea41 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -1902,9 +1902,10 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll in which he/she is a beta-tester. """ with LogCapture() as capture: - message = 'Cancelling course certificate generation for user [{}] against course [{}], ' \ - 'user is a Beta Tester.' - message = message.format(self.beta_tester.username, self.course.id) + message = ( + f'Canceling Certificate Generation task for user {self.beta_tester.id} : {self.course.id}. User is a ' + 'Beta Tester.' + ) generate_user_certificates(self.beta_tester, self.course.id, self.course) capture.check_present(('lms.djangoapps.certificates.generation_handler', 'INFO', message)) diff --git a/openedx/core/djangoapps/content/course_overviews/api.py b/openedx/core/djangoapps/content/course_overviews/api.py index 9cc1203781..032ebc6396 100644 --- a/openedx/core/djangoapps/content/course_overviews/api.py +++ b/openedx/core/djangoapps/content/course_overviews/api.py @@ -2,12 +2,18 @@ CourseOverview internal api """ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview - from openedx.core.djangoapps.content.course_overviews.serializers import ( CourseOverviewBaseSerializer, ) +def get_course_overview(course_id): + """ + Retrieve and return course overview data for the provided course id. + """ + return CourseOverview.get_from_id(course_id) + + def get_course_overviews(course_ids): """ Return course_overview data for a given list of opaque_key course_ids. diff --git a/openedx/core/djangoapps/content/course_overviews/tests/test_api.py b/openedx/core/djangoapps/content/course_overviews/tests/test_api.py index 7017020c7d..dbb37fddf0 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests/test_api.py +++ b/openedx/core/djangoapps/content/course_overviews/tests/test_api.py @@ -3,7 +3,7 @@ course_overview api tests """ from django.test import TestCase -from openedx.core.djangoapps.content.course_overviews.api import get_course_overviews +from openedx.core.djangoapps.content.course_overviews.api import get_course_overview, get_course_overviews from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from ..models import CourseOverview @@ -19,6 +19,14 @@ class TestCourseOverviewsApi(TestCase): for _ in range(3): CourseOverviewFactory.create() + def test_get_course_overview(self): + """ + Test for `get_course_overview` function to retrieve a single course overview. + """ + course_overview = CourseOverviewFactory.create() + retrieved_course_overview = get_course_overview(course_overview.id) + assert course_overview.id == retrieved_course_overview.id + def test_get_course_overviews(self): """ get_course_overviews should return the expected CourseOverview data