Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #27501 from edx/jhynes/microba-1178_modulestore

Browse Source 
refactor: reduce certificate django apps dependence on the modulestore (generation_handler)
This commit is contained in:
Justin Hynes
2021-05-04 13:03:15 -04:00
committed by GitHub
parent b807b7c3d1 5db4fd5bb2
commit 6313c1ba67
9 changed files with 140 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

84
lms/djangoapps/certificates/generation_handler.py
View File

@@ -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,

2
lms/djangoapps/certificates/tests/test_generation_handler.py
View File

@@ -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))

45
lms/djangoapps/certificates/tests/test_utils.py Normal file
View File

@@ -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)

13
lms/djangoapps/certificates/utils.py
View File

@@ -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.

16
lms/djangoapps/instructor/access.py
View File

@@ -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.

12
lms/djangoapps/instructor/tests/test_access.py
View File

@@ -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):

7
lms/djangoapps/instructor/tests/test_api.py
View File

@@ -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))

8
openedx/core/djangoapps/content/course_overviews/api.py
View File

@@ -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.

10
openedx/core/djangoapps/content/course_overviews/tests/test_api.py
View File

@@ -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