diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index aae2a3cc8c..5685044722 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -52,6 +52,7 @@ from certificates.models import GeneratedCertificate from course_modes.models import CourseMode from courseware.models import DynamicUpgradeDeadlineConfiguration, CourseDynamicUpgradeDeadlineConfiguration from enrollment.api import _default_course_mode + from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.schedules.models import ScheduleConfig from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -62,6 +63,8 @@ from util.milestones_helpers import is_entrance_exams_enabled from util.model_utils import emit_field_changed_events, get_changed_fields_dict from util.query import use_read_replica_if_available +from .signals.signals import ENROLLMENT_TRACK_UPDATED + UNENROLL_DONE = Signal(providing_args=["course_enrollment", "skip_refund"]) ENROLL_STATUS_CHANGE = Signal(providing_args=["event", "user", "course_id", "mode", "cost", "currency"]) REFUND_ORDER = Signal(providing_args=["course_enrollment"]) @@ -1191,6 +1194,7 @@ class CourseEnrollment(models.Model): # Only emit mode change events when the user's enrollment # mode has changed from its previous setting self.emit_event(EVENT_NAME_ENROLLMENT_MODE_CHANGED) + ENROLLMENT_TRACK_UPDATED.send(sender=None, user=self.user, course_key=self.course_id) def send_signal(self, event, cost=None, currency=None): """ diff --git a/common/djangoapps/student/signals/__init__.py b/common/djangoapps/student/signals/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/common/djangoapps/student/signals/signals.py b/common/djangoapps/student/signals/signals.py new file mode 100644 index 0000000000..7f280b98b0 --- /dev/null +++ b/common/djangoapps/student/signals/signals.py @@ -0,0 +1,6 @@ +""" +Enrollment track related signals. +""" +from django.dispatch import Signal + +ENROLLMENT_TRACK_UPDATED = Signal(providing_args=['user', 'course_key']) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 07defdf246..7ffefa44e2 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -11,8 +11,10 @@ from xblock.scorable import ScorableXBlockMixin, Score from courseware.model_data import get_score, set_score from eventtracking import tracker from lms.djangoapps.instructor_task.tasks_helper.module_state import GRADES_OVERRIDE_EVENT_TYPE +from openedx.core.djangoapps.course_groups.signals.signals import COHORT_MEMBERSHIP_UPDATED from openedx.core.lib.grade_utils import is_score_higher_or_equal from student.models import user_by_anonymous_id +from student.signals.signals import ENROLLMENT_TRACK_UPDATED from submissions.models import score_reset, score_set from track.event_transaction_utils import ( create_new_event_transaction_id, @@ -22,6 +24,7 @@ from track.event_transaction_utils import ( ) from util.date_utils import to_timestamp +from ..config.waffle import waffle, WRITE_ONLY_IF_ENGAGED from ..constants import ScoreDatabaseTableEnum from ..new.course_grade_factory import CourseGradeFactory from ..scores import weighted_score @@ -31,7 +34,7 @@ from .signals import ( PROBLEM_WEIGHTED_SCORE_CHANGED, SCORE_PUBLISHED, SUBSECTION_SCORE_CHANGED, - SUBSECTION_OVERRIDE_CHANGED + SUBSECTION_OVERRIDE_CHANGED, ) log = getLogger(__name__) @@ -237,13 +240,28 @@ def enqueue_subsection_update(sender, **kwargs): # pylint: disable=unused-argum @receiver(SUBSECTION_SCORE_CHANGED) -def recalculate_course_grade(sender, course, course_structure, user, **kwargs): # pylint: disable=unused-argument +def recalculate_course_grade_only(sender, course, course_structure, user, **kwargs): # pylint: disable=unused-argument """ - Updates a saved course grade. + Updates a saved course grade, but does not update the subsection + grades the user has in this course. """ CourseGradeFactory().update(user, course=course, course_structure=course_structure) +@receiver(ENROLLMENT_TRACK_UPDATED) +@receiver(COHORT_MEMBERSHIP_UPDATED) +def force_recalculate_course_and_subsection_grades(sender, user, course_key, **kwargs): + """ + Updates a saved course grade, forcing the subsection grades + from which it is calculated to update along the way. + + Does not create a grade if the user has never attempted a problem, + even if the WRITE_ONLY_IF_ENGAGED waffle switch is off. + """ + if waffle().is_enabled(WRITE_ONLY_IF_ENGAGED) or CourseGradeFactory().read(user, course_key=course_key): + CourseGradeFactory().update(user=user, course_key=course_key, force_update_subsections=True) + + def _emit_event(kwargs): """ Emits a problem submitted event only if there is no current event diff --git a/lms/djangoapps/grades/tests/test_signals.py b/lms/djangoapps/grades/tests/test_signals.py index f28282125d..ffc6877690 100644 --- a/lms/djangoapps/grades/tests/test_signals.py +++ b/lms/djangoapps/grades/tests/test_signals.py @@ -1,7 +1,7 @@ """ Tests for the score change signals defined in the courseware models module. """ - +import itertools import re from datetime import datetime @@ -10,15 +10,20 @@ import pytz from django.test import TestCase from mock import MagicMock, patch +from opaque_keys.edx.locations import CourseLocator +from openedx.core.djangoapps.course_groups.signals.signals import COHORT_MEMBERSHIP_UPDATED +from student.signals.signals import ENROLLMENT_TRACK_UPDATED +from student.tests.factories import UserFactory from submissions.models import score_reset, score_set from util.date_utils import to_timestamp +from ..config.waffle import waffle, WRITE_ONLY_IF_ENGAGED from ..constants import ScoreDatabaseTableEnum from ..signals.handlers import ( disconnect_submissions_signal_receiver, problem_raw_score_changed_handler, submissions_score_reset_handler, - submissions_score_set_handler + submissions_score_set_handler, ) from ..signals.signals import PROBLEM_RAW_SCORE_CHANGED @@ -251,3 +256,32 @@ class ScoreChangedSignalRelayTest(TestCase): with self.assertRaises(ValueError): with disconnect_submissions_signal_receiver(PROBLEM_RAW_SCORE_CHANGED): pass + + +@ddt.ddt +class RecalculateUserGradeSignalsTest(TestCase): + def setUp(self): + super(RecalculateUserGradeSignalsTest, self).setUp() + self.user = UserFactory() + self.course_key = CourseLocator("test_org", "test_course_num", "test_run") + + @patch('lms.djangoapps.grades.signals.handlers.CourseGradeFactory.update') + @patch('lms.djangoapps.grades.signals.handlers.CourseGradeFactory.read') + @ddt.data(*itertools.product((COHORT_MEMBERSHIP_UPDATED, ENROLLMENT_TRACK_UPDATED), (True, False), (True, False))) + @ddt.unpack + def test_recalculate_on_signal(self, signal, write_only_if_engaged, has_grade, read_mock, update_mock): + """ + Tests the grades handler for signals that trigger regrading. + The handler should call CourseGradeFactory.update() with the + args below, *except* if the WRITE_ONLY_IF_ENGAGED waffle flag + is inactive and the user does not have a grade. + """ + if not has_grade: + read_mock.return_value = None + with waffle().override(WRITE_ONLY_IF_ENGAGED, active=write_only_if_engaged): + signal.send(sender=None, user=self.user, course_key=self.course_key) + + if not write_only_if_engaged and not has_grade: + update_mock.assert_not_called() + else: + update_mock.assert_called_with(course_key=self.course_key, user=self.user, force_update_subsections=True) diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 3a1713b68e..2a676f5791 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -28,6 +28,7 @@ from .models import ( CourseUserGroupPartitionGroup, UnregisteredLearnerCohortAssignments ) +from .signals.signals import COHORT_MEMBERSHIP_UPDATED log = logging.getLogger(__name__) @@ -424,7 +425,9 @@ def remove_user_from_cohort(cohort, username_or_email): try: membership = CohortMembership.objects.get(course_user_group=cohort, user=user) + course_key = membership.course_id membership.delete() + COHORT_MEMBERSHIP_UPDATED.send(sender=None, user=user, course_key=course_key) except CohortMembership.DoesNotExist: raise ValueError("User {} was not present in cohort {}".format(username_or_email, cohort)) @@ -454,7 +457,7 @@ def add_user_to_cohort(cohort, username_or_email): membership = CohortMembership(course_user_group=cohort, user=user) membership.save() # This will handle both cases, creation and updating, of a CohortMembership for this user. - + COHORT_MEMBERSHIP_UPDATED.send(sender=None, user=user, course_key=membership.course_id) tracker.emit( "edx.cohort.user_add_requested", { diff --git a/openedx/core/djangoapps/course_groups/signals/__init__.py b/openedx/core/djangoapps/course_groups/signals/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openedx/core/djangoapps/course_groups/signals/signals.py b/openedx/core/djangoapps/course_groups/signals/signals.py new file mode 100644 index 0000000000..e373f20417 --- /dev/null +++ b/openedx/core/djangoapps/course_groups/signals/signals.py @@ -0,0 +1,6 @@ +""" +Cohorts related signals. +""" +from django.dispatch import Signal + +COHORT_MEMBERSHIP_UPDATED = Signal(providing_args=['user', 'course_key']) diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index 65a2c1c717..cee543f396 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -591,7 +591,8 @@ class TestCohorts(ModuleStoreTestCase): ) @patch("openedx.core.djangoapps.course_groups.cohorts.tracker") - def test_add_user_to_cohort(self, mock_tracker): + @patch("openedx.core.djangoapps.course_groups.cohorts.COHORT_MEMBERSHIP_UPDATED") + def test_add_user_to_cohort(self, mock_signal, mock_tracker): """ Make sure cohorts.add_user_to_cohort() properly adds a user to a cohort and handles errors. @@ -603,6 +604,10 @@ class TestCohorts(ModuleStoreTestCase): first_cohort = CohortFactory(course_id=course.id, name="FirstCohort") second_cohort = CohortFactory(course_id=course.id, name="SecondCohort") + def check_and_reset_signal(): + mock_signal.send.assert_called_with(sender=None, user=course_user, course_key=self.toy_course_key) + mock_signal.reset_mock() + # Success cases # We shouldn't get back a previous cohort, since the user wasn't in one self.assertEqual( @@ -619,6 +624,8 @@ class TestCohorts(ModuleStoreTestCase): "previous_cohort_name": None, } ) + check_and_reset_signal() + # Should get (user, previous_cohort_name) when moved from one cohort to # another self.assertEqual( @@ -635,6 +642,8 @@ class TestCohorts(ModuleStoreTestCase): "previous_cohort_name": first_cohort.name, } ) + check_and_reset_signal() + # Should preregister email address for a cohort if an email address # not associated with a user is added (user, previous_cohort, prereg) = cohorts.add_user_to_cohort(first_cohort, "new_email@example.com") @@ -650,6 +659,7 @@ class TestCohorts(ModuleStoreTestCase): "cohort_name": first_cohort.name, } ) + # Error cases # Should get ValueError if user already in cohort self.assertRaises( diff --git a/openedx/core/djangoapps/credit/tests/test_api.py b/openedx/core/djangoapps/credit/tests/test_api.py index ee57fbe99c..7d103ae1fc 100644 --- a/openedx/core/djangoapps/credit/tests/test_api.py +++ b/openedx/core/djangoapps/credit/tests/test_api.py @@ -158,7 +158,8 @@ class CreditApiTestBase(ModuleStoreTestCase): def setUp(self, **kwargs): super(CreditApiTestBase, self).setUp() - self.course_key = CourseKey.from_string("edX/DemoX/Demo_Course") + self.course = CourseFactory.create(org="edx", course="DemoX", run="Demo_Course") + self.course_key = self.course.id def add_credit_course(self, course_key=None, enabled=True): """Mark the course as a credit """ @@ -631,8 +632,6 @@ class CreditRequirementApiTests(CreditApiTestBase): # Configure a course with two credit requirements self.add_credit_course() user = self.create_and_enroll_user(username=self.USER_INFO['username'], password=self.USER_INFO['password']) - CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') - requirements = [ { "namespace": "grade", @@ -664,7 +663,7 @@ class CreditRequirementApiTests(CreditApiTestBase): self.assertFalse(api.is_user_eligible_for_credit(user.username, self.course_key)) # Satisfy the other requirement - with self.assertNumQueries(24): + with self.assertNumQueries(23): api.set_credit_requirement_status( user, self.course_key, @@ -822,7 +821,6 @@ class CreditRequirementApiTests(CreditApiTestBase): # Configure a course with two credit requirements self.add_credit_course() user = self.create_and_enroll_user(username=self.USER_INFO['username'], password=self.USER_INFO['password']) - CourseFactory.create(org='edX', number='DemoX', display_name='Demo_Course') requirements = [ { "namespace": "grade",