From cf3a6c16d6f98eb850ebca94ee2ea0d616242eff Mon Sep 17 00:00:00 2001 From: Christie Rice <8483753+crice100@users.noreply.github.com> Date: Tue, 10 Aug 2021 09:06:14 -0400 Subject: [PATCH] fix: Stop showing course certificate buttons to beta testers (#28416) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Beta testers can’t earn course certificates, so they should not see a “Request Certificate” button or other info describing how they can earn a cert. MICROBA-992 --- common/djangoapps/student/helpers.py | 9 ++++ common/djangoapps/student/tests/tests.py | 43 ++++++++++++++++++- .../certificates/generation_handler.py | 14 ++---- .../tests/test_generation_handler.py | 2 +- lms/djangoapps/courseware/tests/test_views.py | 8 ++-- lms/djangoapps/instructor/access.py | 8 ++++ .../instructor/tests/test_access.py | 7 +++ openedx/core/djangoapps/certificates/api.py | 17 +++++++- .../djangoapps/certificates/tests/test_api.py | 38 ++++++++++++++++ 9 files changed, 127 insertions(+), 19 deletions(-) diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 49a58f6083..855b6e7df2 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -44,6 +44,7 @@ from lms.djangoapps.certificates.api import ( ) from lms.djangoapps.certificates.data import CertificateStatuses from lms.djangoapps.grades.api import CourseGradeFactory +from lms.djangoapps.instructor import access from lms.djangoapps.verify_student.models import VerificationDeadline from lms.djangoapps.verify_student.services import IDVerificationService from lms.djangoapps.verify_student.utils import is_verification_expiring_soon, verification_for_datetime @@ -465,6 +466,10 @@ def _cert_info(user, enrollment, cert_status): """ Implements the logic for cert_info -- split out for testing. + TODO: replace with a method that lives in the certificates app and combines this logic with + openedx.core.djangoapps.certificates.api.can_show_certificate_message and + lms.djangoapps.courseware.views.get_cert_data + Arguments: user (User): A user. enrollment (CourseEnrollment): A course enrollment. @@ -526,6 +531,10 @@ def _cert_info(user, enrollment, cert_status): if not CourseMode.is_eligible_for_certificate(enrollment.mode, status=status): return default_info + if course_overview and access.is_beta_tester(user, course_overview.id): + # Beta testers are not eligible for a course certificate + return default_info + status_dict = { 'status': status, 'mode': cert_status.get('mode', None), diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 3870bd393c..71a0c51335 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -54,6 +54,8 @@ from xmodule.data import CertificatesDisplayBehaviors log = logging.getLogger(__name__) +BETA_TESTER_METHOD = 'common.djangoapps.student.helpers.access.is_beta_tester' + @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @ddt.ddt @@ -169,6 +171,45 @@ class CourseEndingTest(ModuleStoreTestCase): assert _cert_info(user, enrollment3, cert_status) == {'status': 'processing', 'show_survey_button': False, 'can_unenroll': True} + def test_cert_info_beta_tester(self): + user = UserFactory.create() + course = CourseOverviewFactory.create() + mode = CourseMode.VERIFIED + grade = '0.67' + status = CertificateStatuses.downloadable + cert = GeneratedCertificateFactory.create( + user=user, + course_id=course.id, + status=status, + mode=mode + ) + enrollment = CourseEnrollmentFactory(user=user, course_id=course.id, mode=mode) + + cert_status = { + 'status': status, + 'grade': grade, + 'download_url': cert.download_url, + 'mode': mode, + 'uuid': 'blah', + } + with patch(BETA_TESTER_METHOD, return_value=False): + assert _cert_info(user, enrollment, cert_status) == { + 'status': status, + 'download_url': cert.download_url, + 'show_survey_button': False, + 'grade': grade, + 'mode': mode, + 'linked_in_url': None, + 'can_unenroll': False + } + + with patch(BETA_TESTER_METHOD, return_value=True): + assert _cert_info(user, enrollment, cert_status) == { + 'status': 'processing', + 'show_survey_button': False, + 'can_unenroll': True + } + @ddt.data( (0.70, 0.60), (0.60, 0.70), @@ -823,7 +864,7 @@ class EnrollInCourseTest(EnrollmentEventTestMixin, CacheIsolationTestCase): assert CourseEnrollment.is_enrolled(user, course_id) self.assert_no_events_were_emitted() - # Now deactive + # Now deactivate enrollment.deactivate() assert not CourseEnrollment.is_enrolled(user, course_id) self.assert_unenrollment_event_was_emitted(user, course_id, course, enrollment) diff --git a/lms/djangoapps/certificates/generation_handler.py b/lms/djangoapps/certificates/generation_handler.py index 39a01eff6b..d475986099 100644 --- a/lms/djangoapps/certificates/generation_handler.py +++ b/lms/djangoapps/certificates/generation_handler.py @@ -19,7 +19,7 @@ from lms.djangoapps.certificates.models import ( from lms.djangoapps.certificates.tasks import CERTIFICATE_DELAY_SECONDS, generate_certificate from lms.djangoapps.certificates.utils import has_html_certificates_enabled from lms.djangoapps.grades.api import CourseGradeFactory -from lms.djangoapps.instructor.access import list_with_level +from lms.djangoapps.instructor.access import is_beta_tester from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.content.course_overviews.api import get_course_overview_or_none @@ -131,7 +131,7 @@ def _can_generate_regular_certificate(user, course_key, enrollment_mode, course_ log.info(f'{course_key} is a CCX course. Certificate cannot be generated for {user.id}.') return False - if _is_beta_tester(user, course_key): + 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 @@ -268,7 +268,7 @@ def _can_set_regular_cert_status(user, course_key, enrollment_mode): if _is_ccx_course(course_key): return False - if _is_beta_tester(user, course_key): + if is_beta_tester(user, course_key): return False return _can_set_cert_status_common(user, course_key, enrollment_mode) @@ -324,14 +324,6 @@ def _can_generate_certificate_for_status(user, course_key, enrollment_mode): return True -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_key, 'beta') - return beta_testers_queryset.filter(username=user.username).exists() - - def _is_ccx_course(course_key): """ Check if the course is a CCX (custom edX course) diff --git a/lms/djangoapps/certificates/tests/test_generation_handler.py b/lms/djangoapps/certificates/tests/test_generation_handler.py index a369279b59..6b32042b6b 100644 --- a/lms/djangoapps/certificates/tests/test_generation_handler.py +++ b/lms/djangoapps/certificates/tests/test_generation_handler.py @@ -32,7 +32,7 @@ from xmodule.modulestore.tests.factories import CourseFactory log = logging.getLogger(__name__) -BETA_TESTER_METHOD = 'lms.djangoapps.certificates.generation_handler._is_beta_tester' +BETA_TESTER_METHOD = 'lms.djangoapps.certificates.generation_handler.is_beta_tester' COURSE_OVERVIEW_METHOD = 'lms.djangoapps.certificates.generation_handler.get_course_overview_or_none' CCX_COURSE_METHOD = 'lms.djangoapps.certificates.generation_handler._is_ccx_course' GET_GRADE_METHOD = 'lms.djangoapps.certificates.generation_handler._get_course_grade' diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index e5fc05696d..0e1462671c 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1545,8 +1545,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, "Download Your Certificate") @ddt.data( - (True, 54), - (False, 54), + (True, 55), + (False, 55), ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1559,8 +1559,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 62, 45), - (True, 54, 39) + (False, 63, 46), + (True, 55, 40) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index d1f8010171..85f659d6d0 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -112,3 +112,11 @@ def update_forum_role(course_id, user, rolename, action): role.users.remove(user) else: raise ValueError(f"unrecognized action '{action}'") + + +def is_beta_tester(user, course_id): + """ + Returns True if the user is a beta tester in this course, and False if not + """ + beta_testers_queryset = list_with_level(course_id, 'beta') + return beta_testers_queryset.filter(username=user.username).exists() diff --git a/lms/djangoapps/instructor/tests/test_access.py b/lms/djangoapps/instructor/tests/test_access.py index f061066949..3307b12418 100644 --- a/lms/djangoapps/instructor/tests/test_access.py +++ b/lms/djangoapps/instructor/tests/test_access.py @@ -10,6 +10,7 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.instructor.access import ( allow_access, list_with_level, + is_beta_tester, revoke_access, update_forum_role ) @@ -47,6 +48,12 @@ class TestInstructorAccessList(SharedModuleStoreTestCase): assert set(beta_testers) == set(self.beta_testers) assert set(beta_testers_alternative) == set(self.beta_testers) + def test_is_beta(self): + beta_tester = self.beta_testers[0] + user = UserFactory.create() + assert is_beta_tester(beta_tester, self.course.id) + assert not is_beta_tester(user, self.course.id) + class TestInstructorAccessAllow(EmailTemplateTagMixin, SharedModuleStoreTestCase): """ Test access allow. """ diff --git a/openedx/core/djangoapps/certificates/api.py b/openedx/core/djangoapps/certificates/api.py index e31dbe939a..889ec2c863 100644 --- a/openedx/core/djangoapps/certificates/api.py +++ b/openedx/core/djangoapps/certificates/api.py @@ -11,6 +11,7 @@ from django.conf import settings from lms.djangoapps.certificates import api as certs_api from lms.djangoapps.certificates.data import CertificateStatuses +from lms.djangoapps.instructor import access from openedx.core.djangoapps.certificates.config import waffle from common.djangoapps.student.models import CourseEnrollment from xmodule.data import CertificatesDisplayBehaviors @@ -34,16 +35,18 @@ def can_show_certificate_message(course, student, course_grade, certificates_ena """ Returns True if a course certificate message can be shown """ - is_allowlisted = certs_api.is_on_allowlist(student, course.id) auto_cert_gen_enabled = auto_certificate_generation_enabled() has_active_enrollment = CourseEnrollment.is_enrolled(student, course.id) certificates_are_viewable = certs_api.certificates_viewable_for_course(course) + is_beta_tester = access.is_beta_tester(student, course.id) + has_passed_or_is_allowlisted = _has_passed_or_is_allowlisted(course, student, course_grade) return ( (auto_cert_gen_enabled or certificates_enabled_for_course) and has_active_enrollment and certificates_are_viewable and - (course_grade.passed or is_allowlisted) + has_passed_or_is_allowlisted and + (not is_beta_tester) ) @@ -95,3 +98,13 @@ def display_date_for_certificate(course, certificate): def is_valid_pdf_certificate(cert_data): return cert_data.cert_status == CertificateStatuses.downloadable and cert_data.download_url + + +def _has_passed_or_is_allowlisted(course, student, course_grade): + """ + Returns True if the student has passed this course run, or is on the allowlist for this course run + """ + is_allowlisted = certs_api.is_on_allowlist(student, course.id) + has_passed = course_grade and course_grade.passed + + return has_passed or is_allowlisted diff --git a/openedx/core/djangoapps/certificates/tests/test_api.py b/openedx/core/djangoapps/certificates/tests/test_api.py index 0e5b13b61c..37d0091c2e 100644 --- a/openedx/core/djangoapps/certificates/tests/test_api.py +++ b/openedx/core/djangoapps/certificates/tests/test_api.py @@ -7,14 +7,22 @@ from datetime import datetime import ddt import pytz from django.test import TestCase +from unittest.mock import patch from edx_toggles.toggles import LegacyWaffleSwitch from edx_toggles.toggles.testutils import override_waffle_switch +from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory from openedx.core.djangoapps.certificates import api from openedx.core.djangoapps.certificates.config import waffle as certs_waffle from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from xmodule.data import CertificatesDisplayBehaviors +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + + +BETA_TESTER_METHOD = 'openedx.core.djangoapps.certificates.api.access.is_beta_tester' +CERTS_VIEWABLE_METHOD = 'openedx.core.djangoapps.certificates.api.certs_api.certificates_viewable_for_course' +PASSED_OR_ALLOWLISTED_METHOD = 'openedx.core.djangoapps.certificates.api._has_passed_or_is_allowlisted' # TODO: Copied from lms.djangoapps.certificates.models, @@ -147,3 +155,33 @@ class CertificatesApiTestCase(TestCase): maybe_avail = self.course.certificate_available_date if uses_avail_date else self.certificate.modified_date assert maybe_avail == api.available_date_for_certificate(self.course, self.certificate) assert self.certificate.modified_date == api.display_date_for_certificate(self.course, self.certificate) + + +@ddt.ddt +class CertificatesMessagingTestCase(ModuleStoreTestCase): + """ + API tests for certificate messaging + """ + def setUp(self): + super().setUp() + self.course = CourseOverviewFactory.create() + self.course_run_key = self.course.id + self.user = UserFactory.create() + self.enrollment = CourseEnrollmentFactory( + user=self.user, + course_id=self.course_run_key, + is_active=True, + mode=CourseMode.VERIFIED, + ) + + def test_beta_tester(self): + grade = None + certs_enabled = True + + with patch(PASSED_OR_ALLOWLISTED_METHOD, return_value=True): + with patch(CERTS_VIEWABLE_METHOD, return_value=True): + with patch(BETA_TESTER_METHOD, return_value=False): + assert api.can_show_certificate_message(self.course, self.user, grade, certs_enabled) + + with patch(BETA_TESTER_METHOD, return_value=True): + assert not api.can_show_certificate_message(self.course, self.user, grade, certs_enabled)