diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index db539a8688..e03ba80d96 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -1440,7 +1440,7 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 45, 29), + (False, 45, 28), (True, 37, 24) ) @ddt.unpack diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index e1bf2d71e1..554a6c43d9 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -104,21 +104,21 @@ class TestCourseGradeFactory(GradeTestBase): with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5 - num_queries = 7 + num_queries = 6 with self.assertNumQueries(num_queries), mock_get_score(1, 4): grade_factory.update(self.request.user, self.course, force_update_subsections=False) with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - num_queries = 21 + num_queries = 20 with self.assertNumQueries(num_queries), mock_get_score(2, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) with self.assertNumQueries(2): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - num_queries = 24 + num_queries = 23 with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero grade_factory.update(self.request.user, self.course, force_update_subsections=True) diff --git a/openedx/core/djangoapps/credentials/docs/site_config.rst b/openedx/core/djangoapps/credentials/docs/site_config.rst new file mode 100644 index 0000000000..62788a223d --- /dev/null +++ b/openedx/core/djangoapps/credentials/docs/site_config.rst @@ -0,0 +1,15 @@ +Site Configuration +================== + +These are the site configuration flags that affect Credentials features. + +ENABLE_CREDENTIALS_RECORDS +-------------------------- + +Controls whether the LMS integrates with the Learner Records feature in Credentials. Specifically, this turns on some web buttons that link to a learner's record on the Credentials IDA and enables some data being passed to Credentials related to those records. + +If you have had this disabled and decide to enable it, you will need to backpopulate the data that was not sent while the feature was disabled. Look into running the ``notify_credentials`` management command. + +Note that Credentials has a similar site config option that you'll want to keep in sync with this setting. + +Default is ``True``. diff --git a/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py b/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py index 582c3506cc..5593cde24a 100644 --- a/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py +++ b/openedx/core/djangoapps/credentials/management/commands/notify_credentials.py @@ -26,6 +26,7 @@ from lms.djangoapps.certificates.models import GeneratedCertificate from lms.djangoapps.grades.models import PersistentCourseGrade from openedx.core.djangoapps.credentials.signals import handle_cert_change, send_grade_if_interesting from openedx.core.djangoapps.programs.signals import handle_course_cert_changed +from openedx.core.djangoapps.site_configuration.models import SiteConfiguration log = logging.getLogger(__name__) @@ -101,6 +102,11 @@ class Command(BaseCommand): action='store_true', help='Just show a preview of what would happen.', ) + parser.add_argument( + '--site', + default=None, + help="Site domain to notify for (if not specified, all sites are notified). Uses course_org_filter.", + ) parser.add_argument( '--courses', nargs='+', @@ -131,14 +137,20 @@ class Command(BaseCommand): def handle(self, *args, **options): log.info( - "notify_credentials starting, dry-run=%s, delay=%d seconds", + "notify_credentials starting, dry-run=%s, site=%s, delay=%d seconds", options['dry_run'], + options['site'], options['delay'] ) cert_filter_args = {} grade_filter_args = {} + try: + site_config = SiteConfiguration.objects.get(site__domain=options['site']) if options['site'] else None + except SiteConfiguration.DoesNotExist: + log.error('No site configuration found for site %s', options['site']) + if options['courses']: course_keys = self.get_course_keys(options['courses']) cert_filter_args['course_id__in'] = course_keys @@ -162,15 +174,22 @@ class Command(BaseCommand): if options['dry_run']: self.print_dry_run(certs, grades) else: - self.send_notifications(certs, grades, delay=options['delay'], page_size=options['page_size']) + self.send_notifications(certs, grades, + site_config=site_config, + delay=options['delay'], + page_size=options['page_size']) log.info('notify_credentials finished') - def send_notifications(self, certs, grades, delay=0, page_size=0): + def send_notifications(self, certs, grades, site_config=None, delay=0, page_size=0): """ Run actual handler commands for the provided certs and grades. """ # First, do certs for i, cert in paged_query(certs, delay, page_size): + if site_config and not site_config.has_org(cert.course_id.org): + log.info("Skipping credential changes %d for certificate %s", i, certstr(cert)) + continue + log.info( "Handling credential changes %d for certificate %s", i, certstr(cert), @@ -188,6 +207,10 @@ class Command(BaseCommand): # Then do grades for i, grade in paged_query(grades, delay, page_size): + if site_config and not site_config.has_org(grade.course_id.org): + log.info("Skipping grade changes %d for grade %s", i, gradestr(grade)) + continue + log.info( "Handling grade changes %d for grade %s", i, gradestr(grade), diff --git a/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py b/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py index 1d5dc7ee3d..3bc07303d3 100644 --- a/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py +++ b/openedx/core/djangoapps/credentials/management/commands/tests/test_notify_credentials.py @@ -14,6 +14,7 @@ from freezegun import freeze_time from lms.djangoapps.certificates.tests.factories import GeneratedCertificateFactory from lms.djangoapps.grades.models import PersistentCourseGrade +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory from openedx.core.djangolib.testing.utils import skip_unless_lms from student.tests.factories import UserFactory @@ -36,7 +37,7 @@ class TestNotifyCredentials(TestCase): with freeze_time(datetime(2017, 2, 1)): self.cert2 = GeneratedCertificateFactory(user=self.user, course_id='course-v1:edX+Test+2') with freeze_time(datetime(2017, 3, 1)): - self.cert3 = GeneratedCertificateFactory(user=self.user, course_id='course-v1:edX+Test+3') + self.cert3 = GeneratedCertificateFactory(user=self.user, course_id='course-v1:testX+Test+3') print('self.cert1.modified_date', self.cert1.modified_date) # No factory for these @@ -47,7 +48,7 @@ class TestNotifyCredentials(TestCase): self.grade2 = PersistentCourseGrade.objects.create(user_id=self.user.id, course_id='course-v1:edX+Test+2', percent_grade=1) with freeze_time(datetime(2017, 3, 1)): - self.grade3 = PersistentCourseGrade.objects.create(user_id=self.user.id, course_id='course-v1:edX+Test+3', + self.grade3 = PersistentCourseGrade.objects.create(user_id=self.user.id, course_id='course-v1:testX+Test+3', percent_grade=1) print('self.grade1.modified', self.grade1.modified) @@ -119,3 +120,14 @@ class TestNotifyCredentials(TestCase): reset_queries() call_command(Command(), '--start-date', '2017-01-01', '--page-size=2') self.assertEqual(len(connection.queries), baseline + 2) # one extra page query each for certs & grades + + @mock.patch(COMMAND_MODULE + '.send_grade_if_interesting') + @mock.patch(COMMAND_MODULE + '.handle_cert_change') + def test_site(self, mock_grade_interesting, mock_cert_change): + site_config = SiteConfigurationFactory.create( + values={'course_org_filter': ['testX']}, + ) + + call_command(Command(), '--site', site_config.site.domain, '--start-date', '2017-01-01') + self.assertEqual(mock_grade_interesting.call_count, 1) + self.assertEqual(mock_cert_change.call_count, 1) diff --git a/openedx/core/djangoapps/credentials/models.py b/openedx/core/djangoapps/credentials/models.py index c6c8aed36c..5630013d51 100644 --- a/openedx/core/djangoapps/credentials/models.py +++ b/openedx/core/djangoapps/credentials/models.py @@ -96,6 +96,9 @@ class CredentialsApiConfig(ConfigurationModel): # Temporarily disable this feature while we work on it if not STUDENT_RECORDS_FLAG.is_enabled(): return None + # Not every site wants the Learner Records feature, so we allow opting out. + if not helpers.get_value('ENABLE_LEARNER_RECORDS', True): + return None root = helpers.get_value('CREDENTIALS_PUBLIC_SERVICE_URL', settings.CREDENTIALS_PUBLIC_SERVICE_URL) return urljoin(root, '/records/') diff --git a/openedx/core/djangoapps/credentials/signals.py b/openedx/core/djangoapps/credentials/signals.py index c22d9f7fb3..461f6f514f 100644 --- a/openedx/core/djangoapps/credentials/signals.py +++ b/openedx/core/djangoapps/credentials/signals.py @@ -8,6 +8,8 @@ from django.contrib.sites.models import Site from lms.djangoapps.certificates.models import CertificateStatuses, GeneratedCertificate from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from openedx.core.djangoapps.catalog.utils import get_programs +from openedx.core.djangoapps.credentials.models import CredentialsApiConfig +from openedx.core.djangoapps.site_configuration import helpers from .tasks.v1.tasks import send_grade_to_credentials @@ -43,6 +45,14 @@ def is_course_run_in_a_program(course_run_key): def send_grade_if_interesting(user, course_run_key, mode, status, letter_grade, percent_grade): """ Checks if grade is interesting to Credentials and schedules a Celery task if so. """ + # Avoid scheduling new tasks if certification is disabled. (Grades are a part of the records/cert story) + if not CredentialsApiConfig.current().is_learner_issuance_enabled: + return + + # Avoid scheduling new tasks if learner records are disabled for this site. + if not helpers.get_value_for_org(course_run_key.org, 'ENABLE_LEARNER_RECORDS', True): + return + # Grab mode/status if we don't have them in hand if mode is None or status is None: try: diff --git a/openedx/core/djangoapps/credentials/tests/test_signals.py b/openedx/core/djangoapps/credentials/tests/test_signals.py index db39dac67f..6d1de48e12 100644 --- a/openedx/core/djangoapps/credentials/tests/test_signals.py +++ b/openedx/core/djangoapps/credentials/tests/test_signals.py @@ -10,7 +10,7 @@ from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from lms.djangoapps.grades.tests.utils import mock_passing_grade from openedx.core.djangoapps.catalog.tests.factories import CourseFactory, CourseRunFactory, ProgramFactory from openedx.core.djangoapps.credentials.signals import is_course_run_in_a_program, send_grade_if_interesting -from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory, SiteFactory from openedx.core.djangolib.testing.utils import skip_unless_lms from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -22,6 +22,11 @@ SIGNALS_MODULE = 'openedx.core.djangoapps.credentials.signals' @ddt.ddt @skip_unless_lms +@mock.patch( + 'openedx.core.djangoapps.credentials.models.CredentialsApiConfig.is_learner_issuance_enabled', + new_callable=mock.PropertyMock, + return_value=True, +) @mock.patch(SIGNALS_MODULE + '.send_grade_to_credentials') @mock.patch(SIGNALS_MODULE + '.is_course_run_in_a_program') class TestCredentialsSignalsSendGrade(TestCase): @@ -42,7 +47,7 @@ class TestCredentialsSignalsSendGrade(TestCase): ) @ddt.unpack def test_send_grade_if_right_cert(self, called, mode, status, mock_is_course_run_in_a_program, - mock_send_grade_to_credentials): + mock_send_grade_to_credentials, _mock_is_learner_issuance_enabled): mock_is_course_run_in_a_program.return_value = True # Test direct send @@ -60,19 +65,20 @@ class TestCredentialsSignalsSendGrade(TestCase): send_grade_if_interesting(self.user, self.key, None, None, 'A', 1.0) self.assertIs(mock_send_grade_to_credentials.delay.called, called) - def test_send_grade_missing_cert(self, _, mock_send_grade_to_credentials): + def test_send_grade_missing_cert(self, _, mock_send_grade_to_credentials, _mock_is_learner_issuance_enabled): send_grade_if_interesting(self.user, self.key, None, None, 'A', 1.0) self.assertFalse(mock_send_grade_to_credentials.delay.called) @ddt.data([True], [False]) @ddt.unpack def test_send_grade_if_in_a_program(self, in_program, mock_is_course_run_in_a_program, - mock_send_grade_to_credentials): + mock_send_grade_to_credentials, _mock_is_learner_issuance_enabled): mock_is_course_run_in_a_program.return_value = in_program send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', 'A', 1.0) self.assertIs(mock_send_grade_to_credentials.delay.called, in_program) - def test_send_grade_queries_grade(self, mock_is_course_run_in_a_program, mock_send_grade_to_credentials): + def test_send_grade_queries_grade(self, mock_is_course_run_in_a_program, mock_send_grade_to_credentials, + _mock_is_learner_issuance_enabled): mock_is_course_run_in_a_program.return_value = True with mock_passing_grade('B', 0.81): @@ -83,11 +89,36 @@ class TestCredentialsSignalsSendGrade(TestCase): mock_send_grade_to_credentials.delay.reset_mock() @mock.patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) - def test_send_grade_without_grade(self, mock_is_course_run_in_a_program, mock_send_grade_to_credentials): + def test_send_grade_without_grade(self, mock_is_course_run_in_a_program, mock_send_grade_to_credentials, + _mock_is_learner_issuance_enabled): mock_is_course_run_in_a_program.return_value = True send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) self.assertFalse(mock_send_grade_to_credentials.delay.called) + def test_send_grade_without_issuance_enabled(self, _mock_is_course_run_in_a_program, + mock_send_grade_to_credentials, mock_is_learner_issuance_enabled): + mock_is_learner_issuance_enabled.return_value = False + send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) + self.assertTrue(mock_is_learner_issuance_enabled.called) + self.assertFalse(mock_send_grade_to_credentials.delay.called) + + def test_send_grade_records_enabled(self, _mock_is_course_run_in_a_program, mock_send_grade_to_credentials, + _mock_is_learner_issuance_enabled): + site_config = SiteConfigurationFactory.create( + values={'course_org_filter': [self.key.org]}, + ) + + # Correctly sent + send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) + self.assertTrue(mock_send_grade_to_credentials.delay.called) + mock_send_grade_to_credentials.delay.reset_mock() + + # Correctly not sent + site_config.values['ENABLE_LEARNER_RECORDS'] = False + site_config.save() + send_grade_if_interesting(self.user, self.key, 'verified', 'downloadable', None, None) + self.assertFalse(mock_send_grade_to_credentials.delay.called) + @skip_unless_lms @mock.patch(SIGNALS_MODULE + '.get_programs') diff --git a/openedx/core/djangoapps/programs/signals.py b/openedx/core/djangoapps/programs/signals.py index ecbae88606..65d8a3749e 100644 --- a/openedx/core/djangoapps/programs/signals.py +++ b/openedx/core/djangoapps/programs/signals.py @@ -6,6 +6,7 @@ import logging from django.dispatch import receiver from openedx.core.djangoapps.signals.signals import COURSE_CERT_AWARDED, COURSE_CERT_CHANGED +from openedx.core.djangoapps.site_configuration import helpers LOGGER = logging.getLogger(__name__) @@ -84,6 +85,11 @@ def handle_course_cert_changed(sender, user, course_key, mode, status, **kwargs) if not CredentialsApiConfig.current().is_learner_issuance_enabled: return + # Avoid scheduling new tasks if learner records are disabled for this site (right now, course certs are only + # used for learner records -- when that changes, we can remove this bit and always send course certs). + if not helpers.get_value_for_org(course_key.org, 'ENABLE_LEARNER_RECORDS', True): + return + # schedule background task to process LOGGER.debug( 'handling COURSE_CERT_CHANGED: username=%s, course_key=%s, mode=%s, status=%s', diff --git a/openedx/core/djangoapps/programs/tests/test_signals.py b/openedx/core/djangoapps/programs/tests/test_signals.py index d187e4b0e9..4ba09c4a3e 100644 --- a/openedx/core/djangoapps/programs/tests/test_signals.py +++ b/openedx/core/djangoapps/programs/tests/test_signals.py @@ -6,14 +6,16 @@ from django.test import TestCase from nose.plugins.attrib import attr import mock +from opaque_keys.edx.keys import CourseKey from student.tests.factories import UserFactory from openedx.core.djangoapps.signals.signals import COURSE_CERT_AWARDED, COURSE_CERT_CHANGED from openedx.core.djangoapps.programs.signals import handle_course_cert_awarded, handle_course_cert_changed +from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory from openedx.core.djangolib.testing.utils import skip_unless_lms TEST_USERNAME = 'test-user' -TEST_COURSE_KEY = 'test-course' +TEST_COURSE_KEY = CourseKey.from_string('course-v1:edX+test_course+1') @attr(shard=2) @@ -92,6 +94,10 @@ class CertChangedReceiverTest(TestCase): Tests for the `handle_course_cert_changed` signal handler function. """ + def setUp(self): + super(CertChangedReceiverTest, self).setUp() + self.user = UserFactory.create(username=TEST_USERNAME) + @property def signal_kwargs(self): """ @@ -99,8 +105,8 @@ class CertChangedReceiverTest(TestCase): """ return dict( sender=self.__class__, - user=UserFactory.create(username=TEST_USERNAME), - course_key='test-course', + user=self.user, + course_key=TEST_COURSE_KEY, mode='test-mode', status='test-status', ) @@ -115,7 +121,7 @@ class CertChangedReceiverTest(TestCase): known to take place inside the function. """ COURSE_CERT_CHANGED.send(**self.signal_kwargs) - self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1) + self.assertEqual(mock_is_learner_issuance_enabled.call_count, 2) def test_credentials_disabled(self, mock_is_learner_issuance_enabled, mock_task): """ @@ -137,4 +143,22 @@ class CertChangedReceiverTest(TestCase): self.assertEqual(mock_is_learner_issuance_enabled.call_count, 1) self.assertEqual(mock_task.call_count, 1) - self.assertEqual(mock_task.call_args[0], (TEST_USERNAME, TEST_COURSE_KEY)) + self.assertEqual(mock_task.call_args[0], (TEST_USERNAME, str(TEST_COURSE_KEY))) + + def test_records_enabled(self, mock_is_learner_issuance_enabled, mock_task): + mock_is_learner_issuance_enabled.return_value = True + + site_config = SiteConfigurationFactory.create( + values={'course_org_filter': ['edX']}, + ) + + # Correctly sent + handle_course_cert_changed(**self.signal_kwargs) + self.assertTrue(mock_task.called) + mock_task.reset_mock() + + # Correctly not sent + site_config.values['ENABLE_LEARNER_RECORDS'] = False + site_config.save() + handle_course_cert_changed(**self.signal_kwargs) + self.assertFalse(mock_task.called)