diff --git a/lti_consumer/admin.py b/lti_consumer/admin.py index a942a889..439f8310 100644 --- a/lti_consumer/admin.py +++ b/lti_consumer/admin.py @@ -2,7 +2,7 @@ Admin views for LTI related models. """ from django.contrib import admin -from lti_consumer.models import LtiAgsLineItem, LtiConfiguration +from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiAgsScore class LtiConfigurationAdmin(admin.ModelAdmin): @@ -16,3 +16,4 @@ class LtiConfigurationAdmin(admin.ModelAdmin): admin.site.register(LtiConfiguration, LtiConfigurationAdmin) admin.site.register(LtiAgsLineItem) +admin.site.register(LtiAgsScore) diff --git a/lti_consumer/apps.py b/lti_consumer/apps.py index f06d6d84..197d55e9 100644 --- a/lti_consumer/apps.py +++ b/lti_consumer/apps.py @@ -25,3 +25,7 @@ class LTIConsumerApp(AppConfig): } } } + + def ready(self): + # pylint: disable=unused-import,import-outside-toplevel + import lti_consumer.signals diff --git a/lti_consumer/lti_1p3/ags.py b/lti_consumer/lti_1p3/ags.py index 5fbb459b..9be67e3a 100644 --- a/lti_consumer/lti_1p3/ags.py +++ b/lti_consumer/lti_1p3/ags.py @@ -19,6 +19,7 @@ class LtiAgs: def __init__( self, lineitems_url, + lineitem_url=None, allow_creating_lineitems=True, results_service_enabled=True, scores_service_enabled=True @@ -37,6 +38,8 @@ def __init__( # Lineitems urls self.lineitems_url = lineitems_url + self.lineitem_url = lineitem_url + def get_available_scopes(self): """ Retrieves list of available token scopes in this instance. @@ -60,11 +63,15 @@ def get_lti_ags_launch_claim(self): """ Returns LTI AGS Claim to be injected in the LTI launch message. """ - ags_claim = { - "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": { - "scope": self.get_available_scopes(), - "lineitems": self.lineitems_url, - } + + claim_values = { + "scope": self.get_available_scopes(), + "lineitems": self.lineitems_url, } - return ags_claim + if self.lineitem_url: + claim_values["lineitem"] = self.lineitem_url + + return { + "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values, + } diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index 6cd2c8ab..f85d9ccd 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -475,6 +475,8 @@ def lti_ags(self): def enable_ags( self, lineitems_url, + lineitem_url=None, + allow_programatic_grade_interaction=False ): """ Enable LTI Advantage Assignments and Grades Service. @@ -482,9 +484,11 @@ def enable_ags( This will include the LTI AGS Claim in the LTI message and set up the required class. """ + self.ags = LtiAgs( lineitems_url=lineitems_url, - allow_creating_lineitems=True, + lineitem_url=lineitem_url, + allow_creating_lineitems=allow_programatic_grade_interaction, results_service_enabled=True, scores_service_enabled=True ) diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index dfae8e16..b0a40b16 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -647,11 +647,11 @@ def test_enable_ags(self): { 'https://purl.imsglobal.org/spec/lti-ags/claim/endpoint': { 'scope': [ - 'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem', + 'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readonly', 'https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly', - 'https://purl.imsglobal.org/spec/lti-ags/scope/score' + 'https://purl.imsglobal.org/spec/lti-ags/scope/score', ], - 'lineitems': 'http://example.com/lineitems' + 'lineitems': 'http://example.com/lineitems', } } ) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index eafe48f9..31836f19 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -12,7 +12,10 @@ # LTI 1.3 from lti_consumer.lti_1p3.consumer import LtiAdvantageConsumer from lti_consumer.plugin import compat -from lti_consumer.utils import get_lms_base, get_lti_ags_lineitems_url +from lti_consumer.utils import ( + get_lms_base, + get_lti_ags_lineitems_url, +) class LtiConfiguration(models.Model): @@ -129,8 +132,29 @@ def _get_lti_1p3_consumer(self): # Check if enabled and setup LTI-AGS if self.block.has_score: + + default_values = { + 'resource_id': self.block.location, + 'score_maximum': self.block.weight, + 'label': self.block.display_name + } + + if hasattr(self.block, 'start'): + default_values['start_date_time'] = self.block.start + + if hasattr(self.block, 'due'): + default_values['end_date_time'] = self.block.due + + # create LineItem if there is none for current lti configuration + lineitem, _ = LtiAgsLineItem.objects.get_or_create( + lti_configuration=self, + resource_link_id=self.block.location, + defaults=default_values + ) + consumer.enable_ags( - lineitems_url=get_lti_ags_lineitems_url(self.id) + lineitems_url=get_lti_ags_lineitems_url(self.id), + lineitem_url=get_lti_ags_lineitems_url(self.id, lineitem.id) ) return consumer diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 8d22f02c..0c3d0f14 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -1,6 +1,8 @@ """ Compatibility layer to isolate core-platform method calls from implementation. """ +from django.core.exceptions import ValidationError +from lti_consumer.exceptions import LtiError def run_xblock_handler(*args, **kwargs): @@ -30,6 +32,7 @@ def load_block_as_anonymous_user(location): handler. """ # pylint: disable=import-error,import-outside-toplevel + from crum import impersonate from django.contrib.auth.models import AnonymousUser from xmodule.modulestore.django import modulestore from lms.djangoapps.courseware.module_render import get_module_for_descriptor_internal @@ -37,15 +40,57 @@ def load_block_as_anonymous_user(location): # Retrieve descriptor from modulestore descriptor = modulestore().get_item(location) - # Load block, attaching it to AnonymousUser - get_module_for_descriptor_internal( - user=AnonymousUser(), - descriptor=descriptor, - student_data=None, - course_id=location.course_key, - track_function=None, - xqueue_callback_url_prefix="", - request_token="", - ) + # ensure `crum.get_current_user` returns AnonymousUser. It returns None when outside + # of request scope which causes error during block loading. + user = AnonymousUser() + with impersonate(user): + # Load block, attaching it to AnonymousUser + get_module_for_descriptor_internal( + user=user, + descriptor=descriptor, + student_data=None, + course_id=location.course_key, + track_function=None, + xqueue_callback_url_prefix="", + request_token="", + ) + + return descriptor + + +def get_user_from_external_user_id(external_user_id): + """ + Import ExternalId model and find user by external_user_id + """ + # pylint: disable=import-error,import-outside-toplevel + from openedx.core.djangoapps.external_user_ids.models import ExternalId + try: + external_id = ExternalId.objects.get( + external_user_id=external_user_id, + external_id_type__name='lti' + ) + return external_id.user + except ExternalId.DoesNotExist as exception: + raise LtiError('Invalid User') from exception + except ValidationError as exception: + raise LtiError('Invalid userID') from exception - return descriptor + +def publish_grade(block, user, score, possible, only_if_higher=False, score_deleted=None, comment=None): + """ + Import grades signals and publishes score by triggering SCORE_PUBLISHED signal. + """ + # pylint: disable=import-error,import-outside-toplevel + from lms.djangoapps.grades.api import signals as grades_signals + + # publish score + grades_signals.SCORE_PUBLISHED.send( + sender=None, + block=block, + user=user, + raw_earned=score, + raw_possible=possible, + only_if_higher=only_if_higher, + score_deleted=score_deleted, + grader_response=comment + ) diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py new file mode 100644 index 00000000..453a12b5 --- /dev/null +++ b/lti_consumer/signals.py @@ -0,0 +1,29 @@ +""" +LTI Consumer related Signal handlers +""" + +from django.db.models.signals import post_save +from django.dispatch import receiver + +from lti_consumer.models import LtiAgsScore +from lti_consumer.plugin import compat + + +@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. + """ + 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 + ) 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 e58484c3..cd34e415 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -2,11 +2,13 @@ Tests for LTI Advantage Assignments and Grades Service views. """ import json -from mock import patch, PropertyMock +from datetime import timedelta +from mock import patch, PropertyMock, Mock from Cryptodome.PublicKey import RSA import ddt from django.urls import reverse +from django.utils import timezone from jwkest.jwk import RSAKey from rest_framework.test import APITransactionTestCase @@ -44,6 +46,11 @@ def setUp(self): # Intentionally using the same key for tool key to # allow using signing methods and make testing easier. 'lti_1p3_tool_public_key': self.public_key, + + # xblock due date related attributes + 'due': timezone.now(), + 'graceperiod': timedelta(days=2), + 'accept_grades_past_due': False } self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) @@ -67,6 +74,13 @@ def setUp(self): self.addCleanup(patcher.stop) self._lti_block_patch = patcher.start() + self._mock_user = Mock() + compat_mock = patch("lti_consumer.signals.compat") + self.addCleanup(compat_mock.stop) + self._compat_mock = compat_mock.start() + self._compat_mock.get_user_from_external_user_id.return_value = self._mock_user + self._compat_mock.load_block_as_anonymous_user.return_value = self.xblock + def _set_lti_token(self, scopes=None): """ Generates and sets a LTI Auth token in the request client. @@ -303,6 +317,7 @@ def test_create_lineitem_invalid_resource_link_id(self): self.assertEqual(response.status_code, 400) +@ddt.ddt class LtiAgsViewSetScoresTests(LtiAgsLineItemViewSetTestCase): """ Test `LtiAgsLineItemViewset` Score Publishing requests/responses. @@ -384,6 +399,115 @@ def test_create_score(self): self.assertEqual(score.grading_progress, LtiAgsScore.FULLY_GRADED) self.assertEqual(score.user_id, self.primary_user_id) + def _post_lti_score(self, override_data=None): + """ + Helper method to post a LTI score + """ + self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') + + data = { + "timestamp": self.early_timestamp, + "scoreGiven": 83, + "scoreMaximum": 100, + "comment": "This is exceptional work.", + "activityProgress": LtiAgsScore.COMPLETED, + "gradingProgress": LtiAgsScore.FULLY_GRADED, + "userId": self.primary_user_id + } + + if override_data: + data.update(override_data) + + self.client.post( + self.scores_endpoint, + data=json.dumps(data), + content_type="application/vnd.ims.lis.v1.score+json", + ) + + @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): + """ + Test on LtiAgsScore save, if gradingProgress is Fully Graded then xblock grade should be submitted. + """ + + 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.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() + + 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() + + def test_grade_publish_score_bigger_than_maximum(self): + """ + Test when given score is bigger than maximum score. + """ + self._post_lti_score({ + "scoreGiven": 110, + "scoreMaximum": 100, + }) + score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id) + + self._compat_mock.publish_grade.assert_called_once() + + call_args = self._compat_mock.publish_grade.call_args.args + + # as score_given is larger than score_maximum, it should pass score_maximum as given score + self.assertEqual(call_args, (self.xblock, self._mock_user, score.score_maximum, score.score_maximum,)) + + @patch('lti_consumer.lti_xblock.timezone') + def test_xblock_grade_publish_passed_due_date(self, timezone_patcher): + """ + Test grade publish after due date. Grade shouldn't publish + """ + 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() + + @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 + + 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() + def test_create_multiple_scores_with_multiple_users(self): """ Test the LTI AGS LineItem Score Creation on the same LineItem for different users. diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 67dcfe3f..d1fc03da 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -1,7 +1,9 @@ """ Unit tests for LTI models. """ +from datetime import timedelta from Cryptodome.PublicKey import RSA +from django.utils import timezone from django.test.testcases import TestCase from jwkest.jwk import RSAKey @@ -99,11 +101,12 @@ def test_lti_consumer_ags_enabled(self): 'https://purl.imsglobal.org/spec/lti-ags/claim/endpoint': { 'scope': [ - 'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem', + 'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readonly', 'https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly', 'https://purl.imsglobal.org/spec/lti-ags/scope/score', ], - 'lineitems': 'https://example.com/api/lti_consumer/v1/lti/2/lti-ags' + 'lineitems': 'https://example.com/api/lti_consumer/v1/lti/2/lti-ags', + 'lineitem': 'https://example.com/api/lti_consumer/v1/lti/2/lti-ags/1', } } ) @@ -157,9 +160,21 @@ class TestLtiAgsScoreModel(TestCase): """ Unit tests for LtiAgsScore model methods. """ + def setUp(self): super().setUp() + # patch things related to LtiAgsScore post_save signal receiver + compat_mock = patch("lti_consumer.signals.compat") + self.addCleanup(compat_mock.stop) + self._compat_mock = compat_mock.start() + self._compat_mock.load_block_as_anonymous_user.return_value = make_xblock( + 'lti_consumer', LtiConsumerXBlock, { + 'due': timezone.now(), + 'graceperiod': timedelta(days=2), + } + ) + self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test' self.line_item = LtiAgsLineItem.objects.create( lti_configuration=None, diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index b70ee93f..bbce7853 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -66,13 +66,21 @@ def get_lms_lti_access_token_link(location): ) -def get_lti_ags_lineitems_url(lti_config_id): +def get_lti_ags_lineitems_url(lti_config_id, lineitem_id=None): """ Return the LTI AGS endpoint :param lti_config_id: LTI configuration id + :param lineitem_id: LTI Line Item id. Single line item if given an id, + otherwise returns list url """ - return "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-ags".format( + + url = "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-ags".format( lms_base=get_lms_base(), lti_config_id=str(lti_config_id), ) + + if lineitem_id: + url += "/" + str(lineitem_id) + + return url