From d5d83acfd5cb4fa886f2dd870fbc42e5a19beea4 Mon Sep 17 00:00:00 2001
From: Giovanni Cimolin da Silva <giovannicimolin@gmail.com>
Date: Fri, 26 Mar 2021 14:49:00 -0300
Subject: [PATCH] feat: Improve grade publishing handling on LTI 1.3
This commit improves the functionality of the grade publishing method. Improvements:
* Doesn't publish grade to LMS if this isn't a graded block.
* Doesn't try to publish grades when LineItem is not linked (`resourceLinkId` is empty)
* Improves logging and properly logs exceptions
---
lti_consumer/signals.py | 59 ++++++++++---
.../tests/unit/plugin/test_views_lti_ags.py | 84 ++++++++++++-------
2 files changed, 100 insertions(+), 43 deletions(-)
diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py
index 83f242f..b239349 100644
--- a/lti_consumer/signals.py
+++ b/lti_consumer/signals.py
@@ -1,6 +1,7 @@
"""
LTI Consumer related Signal handlers
"""
+import logging
from django.db.models.signals import post_save
from django.dispatch import receiver
@@ -9,21 +10,55 @@ from lti_consumer.models import LtiAgsScore
from lti_consumer.plugin import compat
+log = logging.getLogger(__name__)
+
+
@receiver(post_save, sender=LtiAgsScore, dispatch_uid='publish_grade_on_score_update')
def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disable=unused-argument
"""
Publish grade to xblock whenever score saved/updated and its grading_progress is set to FullyGraded.
+
+ This method DOES NOT WORK on Studio, since it relies on APIs only available and configured
+ in the LMS. Trying to trigger this signal from Studio (from the Django-admin interface, for example)
+ throw an exception.
"""
- if instance.grading_progress == LtiAgsScore.FULLY_GRADED:
- block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id)
- if not block.is_past_due() or block.accept_grades_past_due:
- user = compat.get_user_from_external_user_id(instance.user_id)
- # check if score_given is larger than score_maximum
- score = instance.score_given if instance.score_given < instance.score_maximum else instance.score_maximum
- compat.publish_grade(
- block,
- user,
- score,
- instance.score_maximum,
- comment=instance.comment,
+ # Before starting to publish grades to the LMS, check that:
+ # 1. The grade being submitted in the final one - `FullyGraded`
+ # 2. This LineItem is linked to a LMS grade - the `LtiResouceLinkId` field is set
+ # 3. There's a valid grade in this score - `scoreGiven` is set
+ if instance.grading_progress == LtiAgsScore.FULLY_GRADED \
+ and instance.line_item.resource_link_id \
+ and instance.score_given:
+ try:
+ # Load block using LMS APIs and check if the block is graded and still accept grades.
+ block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id)
+ if block.has_score and (not block.is_past_due() or block.accept_grades_past_due):
+ # Map external ID to platform user
+ user = compat.get_user_from_external_user_id(instance.user_id)
+
+ # The LTI AGS spec allow tools to send grades higher than score maximum, so
+ # we have to cap the score sent to the gradebook to the maximum allowed value.
+ # Also, this is an normalized score ranging from 0 to 1.
+ score = min(instance.score_given, instance.score_maximum) / instance.score_maximum
+
+ # Set module score using XBlock custom method to do so.
+ # This saves the score on both the XBlock's K/V store as well as in
+ # the LMS database.
+ log.info(
+ "Publishing LTI grade from block %s to LMS. User: %s (score: %s)",
+ block.location,
+ user,
+ score,
+ )
+ block.set_user_module_score(user, score, block.max_score(), instance.comment)
+
+ # This is a catch all exception to catch and log any issues related to loading the block
+ # from the modulestore and other LMS API calls
+ except Exception as exc:
+ log.exception(
+ "Error while publishing score %r to block %s to LMS: %s",
+ instance,
+ instance.line_item.resource_link_id,
+ exc,
)
+ raise exc
diff --git a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py
index 791c677..cff1caf 100644
--- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py
+++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py
@@ -422,44 +422,63 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
@ddt.data(
LtiAgsScore.PENDING,
LtiAgsScore.PENDING_MANUAL,
- LtiAgsScore.FULLY_GRADED,
LtiAgsScore.FAILED,
LtiAgsScore.NOT_READY,
)
- def test_xblock_grade_publish_on_score_save(self, grading_progress):
+ def test_grade_publish_not_called_when_pending(self, grading_progress):
"""
- Test on LtiAgsScore save, if gradingProgress is Fully Graded then xblock grade should be submitted.
+ Check that the grade is not submmitted to LMS if the status is a pending one.
"""
-
+ # Set xblock attribute and make score request
+ self.xblock.has_score = True
self._post_lti_score({
"gradingProgress": grading_progress,
})
- if grading_progress == LtiAgsScore.FULLY_GRADED:
- score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id)
+ self._compat_mock.load_block_as_anonymous_user.assert_not_called()
+ self._compat_mock.get_user_from_external_user_id.assert_not_called()
- self._compat_mock.publish_grade.assert_called_once()
- self._compat_mock.get_user_from_external_user_id.assert_called_once()
- self._compat_mock.load_block_as_anonymous_user.assert_called_once()
+ def test_xblock_grade_publish_on_score_save(self):
+ """
+ Test that the grade is submitted when gradingProgress is `FullyGraded`.
+ """
+ # Set up LMS mocks
+ self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
+ self.xblock.set_user_module_score = Mock()
- call_args = self._compat_mock.publish_grade.call_args.args
- call_kwargs = self._compat_mock.publish_grade.call_args.kwargs
- self.assertEqual(call_args, (self.xblock, self._mock_user, score.score_given, score.score_maximum,))
- self.assertEqual(call_kwargs['comment'], score.comment)
- else:
- self._compat_mock.load_block_as_anonymous_user.assert_not_called()
- self._compat_mock.get_user_from_external_user_id.assert_not_called()
- self._compat_mock.publish_grade.assert_not_called()
+ # Set xblock attribute and make score request
+ self.xblock.has_score = True
+ self._post_lti_score({
+ "gradingProgress": "FullyGraded",
+ })
+
+ # Check if publish grade was called
+ self.xblock.set_user_module_score.assert_called_once()
+ self._compat_mock.get_user_from_external_user_id.assert_called_once()
+ self._compat_mock.load_block_as_anonymous_user.assert_called_once()
+
+ call_args = self.xblock.set_user_module_score.call_args.args
+ self.assertEqual(call_args, ('user_mock', 0.83, 1, 'This is exceptional work.'))
def test_grade_publish_score_bigger_than_maximum(self):
"""
Test when given score is bigger than maximum score.
"""
+ # Return block bypassing LMS API
+ self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
+ self.xblock.set_user_module_score = Mock()
+
+ # Set block as graded
+ self.xblock.has_score = True
+
+ # Post and retrieve score object
self._post_lti_score({
"scoreGiven": 110,
"scoreMaximum": 100,
+ "comment": "comment",
})
- score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id)
# Check that the function was called as expected
# and that the correct variables were passed as arguments
@@ -490,35 +509,38 @@ class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase):
"""
Test grade publish after due date. Grade shouldn't publish
"""
+ self.xblock.set_user_module_score = Mock()
timezone_patcher.now.return_value = timezone.now() + timedelta(days=30)
self._post_lti_score()
+ # Check that the block wasn't set if due date is past
self._compat_mock.load_block_as_anonymous_user.assert_called_once()
-
self._compat_mock.get_user_from_external_user_id.assert_not_called()
- self._compat_mock.publish_grade.assert_not_called()
+ self.xblock.set_user_module_score.assert_not_called()
@patch('lti_consumer.lti_xblock.timezone')
def test_xblock_grade_publish_accept_passed_due_date(self, timezone_patcher):
"""
Test grade publish after due date when accept_grades_past_due is True. Grade should publish.
"""
- xblock_attrs = {
- 'accept_grades_past_due': True,
- }
- xblock_attrs.update(self.xblock_attributes)
- xblock = make_xblock('lti_consumer', LtiConsumerXBlock, xblock_attrs)
- self._compat_mock.load_block_as_anonymous_user.return_value = xblock
+ # Return block bypassing LMS API
+ self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock
+ self._compat_mock.get_user_from_external_user_id.return_value = 'user_mock'
+ self.xblock.set_user_module_score = Mock()
- timezone_patcher.now.return_value = timezone.now() + timedelta(days=30)
+ # Change block attribute
+ self.xblock.has_score = True
+ self.xblock.accept_grades_past_due = True
+ # Try sending grade after due date
+ timezone_patcher.now.return_value = timezone.now() + timedelta(days=30)
self._post_lti_score()
- self._compat_mock.load_block_as_anonymous_user.assert_called_once()
-
- self._compat_mock.get_user_from_external_user_id.assert_not_called()
- self._compat_mock.publish_grade.assert_not_called()
+ # Check that the grade is published
+ self.xblock.set_user_module_score.assert_called_once()
+ call_args = self.xblock.set_user_module_score.call_args.args
+ self.assertEqual(call_args, ('user_mock', 0.83, 1, 'This is exceptional work.'))
def test_create_multiple_scores_with_multiple_users(self):
"""
--
GitLab