From b6bff61b330a3327ae39b68c167ceaa29c231537 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Wed, 4 Nov 2020 22:36:05 +0600 Subject: [PATCH 01/21] create default LineItem, WIP grade save --- lti_consumer/lti_1p3/ags.py | 3 +++ lti_consumer/lti_1p3/consumer.py | 8 +++++++- lti_consumer/models.py | 34 ++++++++++++++++++++++++++++++-- lti_consumer/utils.py | 14 +++++++++++++ 4 files changed, 56 insertions(+), 3 deletions(-) diff --git a/lti_consumer/lti_1p3/ags.py b/lti_consumer/lti_1p3/ags.py index 5fbb459b..fa06c045 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, 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. diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index 6cd2c8ab..224b9e5b 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, + ags_interaction_model='programmatic' ): """ Enable LTI Advantage Assignments and Grades Service. @@ -482,9 +484,13 @@ def enable_ags( This will include the LTI AGS Claim in the LTI message and set up the required class. """ + + allow_creating_lineitems = ags_interaction_model == 'programmatic' + self.ags = LtiAgs( lineitems_url=lineitems_url, - allow_creating_lineitems=True, + lineitem_url=lineitem_url, + allow_creating_lineitems=allow_creating_lineitems, results_service_enabled=True, scores_service_enabled=True ) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index eafe48f9..b89d34b5 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -2,6 +2,8 @@ LTI configuration and linking models. """ from django.db import models +from django.db.models.signals import post_save +from django.dispatch import receiver from django.core.validators import MinValueValidator from django.core.exceptions import ValidationError @@ -12,7 +14,11 @@ # 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, + get_lti_ags_lineitem_url, +) class LtiConfiguration(models.Model): @@ -129,8 +135,19 @@ def _get_lti_1p3_consumer(self): # Check if enabled and setup LTI-AGS if self.block.has_score: + + # create LineItem if there is none for current lti configuration + lineitem, _ = LtiAgsLineItem.objects.get_or_create( + lti_configuration=self, + defaults={ + 'score_maximum': 100, + 'label': 'Score' + } + ) + 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_lineitem_url(self.id, lineitem.id) ) return consumer @@ -293,3 +310,16 @@ def __str__(self): class Meta: unique_together = (('line_item', 'user_id'),) + + +@receiver(post_save, sender=LtiAgsScore) +def update_student_grade(sender, instance, **kwargs): + if instance.grading_progress == LtiAgsScore.FULLY_GRADED: + + # get lti config + lti_configuration = instance.line_item.lti_configuration + + # find user + # rebound user with xblock + # save grade to xblock + pass diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index b70ee93f..3fd451a5 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -76,3 +76,17 @@ def get_lti_ags_lineitems_url(lti_config_id): lms_base=get_lms_base(), lti_config_id=str(lti_config_id), ) + + +def get_lti_ags_lineitem_url(lti_config_id, lineitem_id): + """ + Return the LTI AGS LineItem endpoint + + :param lti_config_id: LTI configuration id + :param lineitem_id: Line Item instance id + """ + return "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-ags/{lineitem_id}".format( + lms_base=get_lms_base(), + lti_config_id=str(lti_config_id), + lineitem_id=str(lineitem_id) + ) From 0f515301240dc7483f35b1b1235080bdab3d40fc Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 5 Nov 2020 16:09:09 +0600 Subject: [PATCH 02/21] add score to django admin --- lti_consumer/admin.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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) From 89a0a17003fbdf3c2bacb1592c3f881ebbb78ef1 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 5 Nov 2020 16:09:36 +0600 Subject: [PATCH 03/21] WIP: find user and save grade to xblock --- lti_consumer/models.py | 11 +++++++---- lti_consumer/plugin/compat.py | 12 ++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index b89d34b5..4a829a61 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -19,6 +19,7 @@ get_lti_ags_lineitems_url, get_lti_ags_lineitem_url, ) +from lti_consumer.plugin.compat import get_user_from_external_user_id class LtiConfiguration(models.Model): @@ -139,6 +140,7 @@ def _get_lti_1p3_consumer(self): # create LineItem if there is none for current lti configuration lineitem, _ = LtiAgsLineItem.objects.get_or_create( lti_configuration=self, + resource_id=self.block.location, defaults={ 'score_maximum': 100, 'label': 'Score' @@ -317,9 +319,10 @@ def update_student_grade(sender, instance, **kwargs): if instance.grading_progress == LtiAgsScore.FULLY_GRADED: # get lti config - lti_configuration = instance.line_item.lti_configuration + lti_config = instance.line_item.lti_configuration # find user - # rebound user with xblock - # save grade to xblock - pass + user = get_user_from_external_user_id(instance.user_id) + + # save grade to xblock, this rebounds user with xblock internally + lti_config.block.set_user_module_score(user, instance.score_given, instance.score_maximum, instance.comment) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 8d22f02c..60181498 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -49,3 +49,15 @@ def load_block_as_anonymous_user(location): ) 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 + return ExternalId.objects.get( + external_user_id=external_user_id, + external_id_type__name='lti' + ).user From dbe4dd8e5597dc0b9c614b700e9c8a8893fdbf33 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Fri, 6 Nov 2020 23:48:07 +0600 Subject: [PATCH 04/21] boolean pragramatic grade interaction flag and optional params in enable_ags method --- lti_consumer/lti_1p3/consumer.py | 10 ++++------ lti_consumer/utils.py | 21 +++++++-------------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index 224b9e5b..eb994a40 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -474,9 +474,9 @@ def lti_ags(self): def enable_ags( self, - lineitems_url, - lineitem_url, - ags_interaction_model='programmatic' + lineitems_url=None, + lineitem_url=None, + allow_programatic_grade_interaction=False ): """ Enable LTI Advantage Assignments and Grades Service. @@ -485,12 +485,10 @@ def enable_ags( and set up the required class. """ - allow_creating_lineitems = ags_interaction_model == 'programmatic' - self.ags = LtiAgs( lineitems_url=lineitems_url, lineitem_url=lineitem_url, - allow_creating_lineitems=allow_creating_lineitems, + allow_creating_lineitems=allow_programatic_grade_interaction, results_service_enabled=True, scores_service_enabled=True ) diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index 3fd451a5..8cebd926 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -66,27 +66,20 @@ 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 = "{url}/{lineitem_id}".format(url=url, lineitem_id=lineitem_id) -def get_lti_ags_lineitem_url(lti_config_id, lineitem_id): - """ - Return the LTI AGS LineItem endpoint - - :param lti_config_id: LTI configuration id - :param lineitem_id: Line Item instance id - """ - return "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-ags/{lineitem_id}".format( - lms_base=get_lms_base(), - lti_config_id=str(lti_config_id), - lineitem_id=str(lineitem_id) - ) + return url From 0ede3f386bb9eebefbfb59982cd7efb21889965a Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Fri, 6 Nov 2020 23:48:36 +0600 Subject: [PATCH 05/21] Submit grades using grade signals --- lti_consumer/models.py | 23 +++++++++-------------- lti_consumer/plugin/compat.py | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 4a829a61..c9baf36b 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -17,9 +17,8 @@ from lti_consumer.utils import ( get_lms_base, get_lti_ags_lineitems_url, - get_lti_ags_lineitem_url, ) -from lti_consumer.plugin.compat import get_user_from_external_user_id +from lti_consumer.plugin.compat import submit_grade class LtiConfiguration(models.Model): @@ -142,14 +141,14 @@ def _get_lti_1p3_consumer(self): lti_configuration=self, resource_id=self.block.location, defaults={ - 'score_maximum': 100, - 'label': 'Score' + 'score_maximum': self.block.weight, + 'label': self.block.display_name } ) consumer.enable_ags( lineitems_url=get_lti_ags_lineitems_url(self.id), - lineitem_url=get_lti_ags_lineitem_url(self.id, lineitem.id) + lineitem_url=get_lti_ags_lineitems_url(self.id, lineitem.id) ) return consumer @@ -316,13 +315,9 @@ class Meta: @receiver(post_save, sender=LtiAgsScore) def update_student_grade(sender, instance, **kwargs): + """ + Submit grade to xblock whenever score saved/updated and its + grading_progress is set to FullyGraded. + """ if instance.grading_progress == LtiAgsScore.FULLY_GRADED: - - # get lti config - lti_config = instance.line_item.lti_configuration - - # find user - user = get_user_from_external_user_id(instance.user_id) - - # save grade to xblock, this rebounds user with xblock internally - lti_config.block.set_user_module_score(user, instance.score_given, instance.score_maximum, instance.comment) + submit_grade(instance) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 60181498..c4b703b7 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -61,3 +61,29 @@ def get_user_from_external_user_id(external_user_id): external_user_id=external_user_id, external_id_type__name='lti' ).user + + +def submit_grade(score): + """ + Import grades signals and submit grade given a LtiAgsScore instance. + """ + # pylint: disable=import-error,import-outside-toplevel + from lms.djangoapps.grades.api import signals as grades_signals + + # get lti config + lti_config = score.line_item.lti_configuration + + # find user + user = get_user_from_external_user_id(score.user_id) + + # publish score + grades_signals.SCORE_PUBLISHED.send( + sender=None, + block=lti_config.block, + user=user, + raw_earned=score.score_given, + raw_possible=score.score_maximum, + only_if_higher=False, + # score_deleted=event.get('score_deleted'), + grader_response=score.comment + ) From 4a3175bda02b1431129d3b4dfb7ae7d2ccc0777a Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Sun, 8 Nov 2020 23:42:03 +0600 Subject: [PATCH 06/21] lineitem urls should be optional --- lti_consumer/lti_1p3/ags.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lti_consumer/lti_1p3/ags.py b/lti_consumer/lti_1p3/ags.py index fa06c045..94593bfe 100644 --- a/lti_consumer/lti_1p3/ags.py +++ b/lti_consumer/lti_1p3/ags.py @@ -18,8 +18,8 @@ class LtiAgs: """ def __init__( self, - lineitems_url, - lineitem_url, + lineitems_url=None, + lineitem_url=None, allow_creating_lineitems=True, results_service_enabled=True, scores_service_enabled=True From f41420657c897574806ebaf63babd645e2dfdc74 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Sun, 8 Nov 2020 23:42:33 +0600 Subject: [PATCH 07/21] lineitem is now readonly in declarative method --- lti_consumer/lti_1p3/tests/test_consumer.py | 2 +- lti_consumer/tests/unit/test_models.py | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index dfae8e16..fac3d0d3 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -647,7 +647,7 @@ 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' ], diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 67dcfe3f..67e1dea0 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -99,7 +99,7 @@ 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', ], @@ -160,6 +160,13 @@ class TestLtiAgsScoreModel(TestCase): def setUp(self): super().setUp() + submit_grade_patcher = patch( + 'lti_consumer.models.submit_grade', + return_value=None + ) + self.addCleanup(submit_grade_patcher.stop) + self._submit_block_patch = submit_grade_patcher.start() + self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test' self.line_item = LtiAgsLineItem.objects.create( lti_configuration=None, From 2616d439c64bce7513c67213606a178d3844e4d7 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Mon, 9 Nov 2020 00:35:22 +0600 Subject: [PATCH 08/21] test grade_submit called properly --- .../tests/unit/plugin/test_views_lti_ags.py | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) 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..837a2246 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -67,6 +67,13 @@ def setUp(self): self.addCleanup(patcher.stop) self._lti_block_patch = patcher.start() + submit_grade_patcher = patch( + 'lti_consumer.models.submit_grade', + return_value=None + ) + self.addCleanup(submit_grade_patcher.stop) + self._submit_block_patch = submit_grade_patcher.start() + def _set_lti_token(self, scopes=None): """ Generates and sets a LTI Auth token in the request client. @@ -303,6 +310,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 +392,42 @@ def test_create_score(self): self.assertEqual(score.grading_progress, LtiAgsScore.FULLY_GRADED) self.assertEqual(score.user_id, self.primary_user_id) + @ddt.data( + LtiAgsScore.PENDING, + LtiAgsScore.PENDING_MANUAL, + LtiAgsScore.FULLY_GRADED, + LtiAgsScore.FAILED, + LtiAgsScore.NOT_READY + ) + def test_xblock_grade_submit_on_score_save(self, grading_progress): + """ + Test on LtiAgsScore save, if gradingProgress is Fully Graded then xblock grade should be submitted. + """ + self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') + + self.client.post( + self.scores_endpoint, + data=json.dumps({ + "timestamp": self.early_timestamp, + "scoreGiven": 83, + "scoreMaximum": 100, + "comment": "This is exceptional work.", + "activityProgress": LtiAgsScore.COMPLETED, + "gradingProgress": grading_progress, + "userId": self.primary_user_id + }), + content_type="application/vnd.ims.lis.v1.score+json", + ) + + if grading_progress == LtiAgsScore.FULLY_GRADED: + score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id) + self._submit_block_patch.assert_called_once() + call_args = self._submit_block_patch.call_args.args + self.assertEqual(len(call_args), 1) + self.assertEqual(call_args[0], score) + else: + self._submit_block_patch.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. From 74772e1545a02c26411e2dd7acf6245a7ff3e0bf Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Mon, 9 Nov 2020 00:45:52 +0600 Subject: [PATCH 09/21] quality issue --- lti_consumer/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/models.py b/lti_consumer/models.py index c9baf36b..90e0057a 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -314,7 +314,7 @@ class Meta: @receiver(post_save, sender=LtiAgsScore) -def update_student_grade(sender, instance, **kwargs): +def update_student_grade(sender, instance, **kwargs): # pylint: disable=unused-argument """ Submit grade to xblock whenever score saved/updated and its grading_progress is set to FullyGraded. From 45c8ad2c498b21560756811a976675d4aedd41f4 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Mon, 9 Nov 2020 17:02:23 +0600 Subject: [PATCH 10/21] raise LTIError --- lti_consumer/plugin/compat.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index c4b703b7..2dd196c3 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): @@ -57,10 +59,16 @@ def get_user_from_external_user_id(external_user_id): """ # pylint: disable=import-error,import-outside-toplevel from openedx.core.djangoapps.external_user_ids.models import ExternalId - return ExternalId.objects.get( - external_user_id=external_user_id, - external_id_type__name='lti' - ).user + 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 def submit_grade(score): From e93a29fd5020b1093d362ae2ca20640df95b76f6 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 12 Nov 2020 00:01:28 +0600 Subject: [PATCH 11/21] moved listener to signal.py, refactored models.py, added due and start date, updated tests. --- lti_consumer/apps.py | 4 ++ lti_consumer/lti_1p3/ags.py | 16 +++-- lti_consumer/models.py | 32 ++++----- lti_consumer/plugin/compat.py | 28 ++++---- lti_consumer/signals.py | 31 +++++++++ .../tests/unit/plugin/test_views_lti_ags.py | 68 ++++++++++++++++--- lti_consumer/tests/unit/test_models.py | 35 ++++++++-- lti_consumer/tests/unit/test_utils.py | 7 +- lti_consumer/utils.py | 6 +- 9 files changed, 170 insertions(+), 57 deletions(-) create mode 100644 lti_consumer/signals.py 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 94593bfe..2bfd42b6 100644 --- a/lti_consumer/lti_1p3/ags.py +++ b/lti_consumer/lti_1p3/ags.py @@ -18,7 +18,7 @@ class LtiAgs: """ def __init__( self, - lineitems_url=None, + lineitems_url, lineitem_url=None, allow_creating_lineitems=True, results_service_enabled=True, @@ -63,11 +63,17 @@ def get_lti_ags_launch_claim(self): """ Returns LTI AGS Claim to be injected in the LTI launch message. """ + + claim_values = { + "scope": self.get_available_scopes(), + "lineitems": self.lineitems_url, + } + + if self.lineitem_url: + claim_values["lineitem"] = self.lineitem_url + ags_claim = { - "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": { - "scope": self.get_available_scopes(), - "lineitems": self.lineitems_url, - } + "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values } return ags_claim diff --git a/lti_consumer/models.py b/lti_consumer/models.py index 90e0057a..31836f19 100644 --- a/lti_consumer/models.py +++ b/lti_consumer/models.py @@ -2,8 +2,6 @@ LTI configuration and linking models. """ from django.db import models -from django.db.models.signals import post_save -from django.dispatch import receiver from django.core.validators import MinValueValidator from django.core.exceptions import ValidationError @@ -18,7 +16,6 @@ get_lms_base, get_lti_ags_lineitems_url, ) -from lti_consumer.plugin.compat import submit_grade class LtiConfiguration(models.Model): @@ -136,14 +133,23 @@ 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_id=self.block.location, - defaults={ - 'score_maximum': self.block.weight, - 'label': self.block.display_name - } + resource_link_id=self.block.location, + defaults=default_values ) consumer.enable_ags( @@ -311,13 +317,3 @@ def __str__(self): class Meta: unique_together = (('line_item', 'user_id'),) - - -@receiver(post_save, sender=LtiAgsScore) -def update_student_grade(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Submit grade to xblock whenever score saved/updated and its - grading_progress is set to FullyGraded. - """ - if instance.grading_progress == LtiAgsScore.FULLY_GRADED: - submit_grade(instance) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 2dd196c3..869b7bdd 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -71,27 +71,27 @@ def get_user_from_external_user_id(external_user_id): raise LtiError('Invalid userID') from exception -def submit_grade(score): +def load_block(key): + # pylint: disable=import-outside-toplevel,import-error + from xmodule.modulestore.django import modulestore + return modulestore().get_item(key) + + +def publish_grade(block, user, score, possible, only_if_higher=False, score_deleted=None, comment=None): """ - Import grades signals and submit grade given a LtiAgsScore instance. + 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 - # get lti config - lti_config = score.line_item.lti_configuration - - # find user - user = get_user_from_external_user_id(score.user_id) - # publish score grades_signals.SCORE_PUBLISHED.send( sender=None, - block=lti_config.block, + block=block, user=user, - raw_earned=score.score_given, - raw_possible=score.score_maximum, - only_if_higher=False, - # score_deleted=event.get('score_deleted'), - grader_response=score.comment + 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..d51c7666 --- /dev/null +++ b/lti_consumer/signals.py @@ -0,0 +1,31 @@ +""" +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.compat import ( + publish_grade, + load_block, + get_user_from_external_user_id, +) + + +@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 = load_block(instance.line_item.resource_link_id) + if not block.is_past_due(): + user = get_user_from_external_user_id(instance.user_id) + publish_grade( + block, + user, + instance.score_given, + 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 837a2246..1760c4e1 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -2,7 +2,7 @@ Tests for LTI Advantage Assignments and Grades Service views. """ import json -from mock import patch, PropertyMock +from mock import patch, PropertyMock, MagicMock from Cryptodome.PublicKey import RSA import ddt @@ -50,6 +50,8 @@ def setUp(self): # Set dummy location so that UsageKey lookup is valid self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' + self.xblock.is_past_due = MagicMock(return_value=False) + # Create configuration self.lti_config = LtiConfiguration.objects.create( location=str(self.xblock.location), @@ -67,12 +69,26 @@ def setUp(self): self.addCleanup(patcher.stop) self._lti_block_patch = patcher.start() - submit_grade_patcher = patch( - 'lti_consumer.models.submit_grade', + publish_grade_patcher = patch( + 'lti_consumer.signals.publish_grade', + return_value=None + ) + self.addCleanup(publish_grade_patcher.stop) + self._publish_grade_patcher = publish_grade_patcher.start() + + load_block_patcher = patch( + 'lti_consumer.signals.load_block', + return_value=self.xblock + ) + self.addCleanup(load_block_patcher.stop) + self._load_block_patcher = load_block_patcher.start() + + get_user_from_external_user_id_patcher = patch( + 'lti_consumer.signals.get_user_from_external_user_id', return_value=None ) - self.addCleanup(submit_grade_patcher.stop) - self._submit_block_patch = submit_grade_patcher.start() + self.addCleanup(get_user_from_external_user_id_patcher.stop) + self._get_user_from_external_user_id_patcher = get_user_from_external_user_id_patcher.start() def _set_lti_token(self, scopes=None): """ @@ -399,7 +415,7 @@ def test_create_score(self): LtiAgsScore.FAILED, LtiAgsScore.NOT_READY ) - def test_xblock_grade_submit_on_score_save(self, grading_progress): + 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. """ @@ -421,12 +437,42 @@ def test_xblock_grade_submit_on_score_save(self, grading_progress): if grading_progress == LtiAgsScore.FULLY_GRADED: score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id) - self._submit_block_patch.assert_called_once() - call_args = self._submit_block_patch.call_args.args - self.assertEqual(len(call_args), 1) - self.assertEqual(call_args[0], score) + self._publish_grade_patcher.assert_called_once() + self._get_user_from_external_user_id_patcher.assert_called_once() + self._load_block_patcher.assert_called_once() + call_args = self._publish_grade_patcher.call_args.args + call_kwargs = self._publish_grade_patcher.call_args.kwargs + self.assertEqual(call_args, (self.xblock, None, score.score_given, score.score_maximum,)) + self.assertEqual(call_kwargs['comment'], score.comment) else: - self._submit_block_patch.assert_not_called() + self._load_block_patcher.assert_not_called() + self._get_user_from_external_user_id_patcher.assert_not_called() + self._publish_grade_patcher.assert_not_called() + + def test_xblock_grade_publish_passed_due_date(self): + """ + Test grade publish after due date. Grade shouldn't published + """ + self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') + + self.xblock.is_past_due = MagicMock(return_value=True) + self.client.post( + self.scores_endpoint, + data=json.dumps({ + "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 + }), + content_type="application/vnd.ims.lis.v1.score+json", + ) + + self._load_block_patcher.assert_called_once() + self._get_user_from_external_user_id_patcher.assert_not_called() + self._publish_grade_patcher.assert_not_called() def test_create_multiple_scores_with_multiple_users(self): """ diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 67e1dea0..382c68f1 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -5,7 +5,7 @@ from django.test.testcases import TestCase from jwkest.jwk import RSAKey -from mock import patch +from mock import patch, MagicMock from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiAgsScore @@ -103,7 +103,8 @@ def test_lti_consumer_ags_enabled(self): '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' } } ) @@ -160,12 +161,26 @@ class TestLtiAgsScoreModel(TestCase): def setUp(self): super().setUp() - submit_grade_patcher = patch( - 'lti_consumer.models.submit_grade', + publish_grade_patcher = patch( + 'lti_consumer.signals.publish_grade', return_value=None ) - self.addCleanup(submit_grade_patcher.stop) - self._submit_block_patch = submit_grade_patcher.start() + self.addCleanup(publish_grade_patcher.stop) + self._publish_grade_patcher = publish_grade_patcher.start() + + load_block_patcher = patch( + 'lti_consumer.signals.load_block', + return_value=make_xblock('lti_consumer', LtiConsumerXBlock, {}, MagicMock(return_value=False)) + ) + self.addCleanup(load_block_patcher.stop) + self._load_block_patcher = load_block_patcher.start() + + get_user_from_external_user_id_patcher = patch( + 'lti_consumer.signals.get_user_from_external_user_id', + return_value=None + ) + self.addCleanup(get_user_from_external_user_id_patcher.stop) + self._get_user_from_external_user_id_patcher = get_user_from_external_user_id_patcher.start() self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test' self.line_item = LtiAgsLineItem.objects.create( @@ -194,3 +209,11 @@ def test_repr(self): str(self.score), "LineItem 1: score 10.0 out of 100.0 - FullyGraded" ) + + def test_score_update_signal(self): + """ + Test score update signal connected + """ + self._load_block_patcher.assert_called_once() + self._get_user_from_external_user_id_patcher.assert_called_once() + self._publish_grade_patcher.assert_called_once() diff --git a/lti_consumer/tests/unit/test_utils.py b/lti_consumer/tests/unit/test_utils.py index da42a607..4a8b0f6e 100644 --- a/lti_consumer/tests/unit/test_utils.py +++ b/lti_consumer/tests/unit/test_utils.py @@ -11,7 +11,7 @@ FAKE_USER_ID = 'fake_user_id' -def make_xblock(xblock_name, xblock_cls, attributes): +def make_xblock(xblock_name, xblock_cls, attributes, is_past_due_patch=None): """ Helper to construct XBlock objects """ @@ -27,6 +27,11 @@ def make_xblock(xblock_name, xblock_cls, attributes): xblock.runtime = Mock( hostname='localhost', ) + + # mock is_past_due method + if is_past_due_patch: + xblock.is_past_due = is_past_due_patch + xblock.course_id = 'course-v1:edX+DemoX+Demo_Course' for key, value in attributes.items(): setattr(xblock, key, value) diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index 8cebd926..7bc698c0 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -74,12 +74,14 @@ def get_lti_ags_lineitems_url(lti_config_id, lineitem_id=None): :param lineitem_id: LTI Line Item id. Single line item if given an id, otherwise returns list url """ - url = "{lms_base}/api/lti_consumer/v1/lti/{lti_config_id}/lti-ags".format( + base_ags_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 = "{url}/{lineitem_id}".format(url=url, lineitem_id=lineitem_id) + url = "{url}/{lineitem_id}".format(url=base_ags_url, lineitem_id=lineitem_id) + else: + url = base_ags_url return url From 30e3286cef400d75e03d01ece36d273619fae703 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 12 Nov 2020 00:39:50 +0600 Subject: [PATCH 12/21] use load_block_as_anonymous_user and remove load_block, refactor tests --- lti_consumer/plugin/compat.py | 6 -- lti_consumer/signals.py | 12 ++-- .../tests/unit/plugin/test_views_lti_ags.py | 61 ++++++++----------- lti_consumer/tests/unit/test_models.py | 33 ++-------- 4 files changed, 34 insertions(+), 78 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 869b7bdd..3ec29a85 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -71,12 +71,6 @@ def get_user_from_external_user_id(external_user_id): raise LtiError('Invalid userID') from exception -def load_block(key): - # pylint: disable=import-outside-toplevel,import-error - from xmodule.modulestore.django import modulestore - return modulestore().get_item(key) - - 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. diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py index d51c7666..f0684505 100644 --- a/lti_consumer/signals.py +++ b/lti_consumer/signals.py @@ -6,11 +6,7 @@ from django.dispatch import receiver from lti_consumer.models import LtiAgsScore -from lti_consumer.plugin.compat import ( - publish_grade, - load_block, - get_user_from_external_user_id, -) +from lti_consumer.plugin import compat @receiver(post_save, sender=LtiAgsScore, dispatch_uid='publish_grade_on_score_update') @@ -19,10 +15,10 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl Publish grade to xblock whenever score saved/updated and its grading_progress is set to FullyGraded. """ if instance.grading_progress == LtiAgsScore.FULLY_GRADED: - block = load_block(instance.line_item.resource_link_id) + block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id) if not block.is_past_due(): - user = get_user_from_external_user_id(instance.user_id) - publish_grade( + user = compat.get_user_from_external_user_id(instance.user_id) + compat.publish_grade( block, user, instance.score_given, 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 1760c4e1..5718ed5f 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -2,7 +2,7 @@ Tests for LTI Advantage Assignments and Grades Service views. """ import json -from mock import patch, PropertyMock, MagicMock +from mock import patch, PropertyMock, MagicMock, Mock from Cryptodome.PublicKey import RSA import ddt @@ -45,13 +45,13 @@ def setUp(self): # allow using signing methods and make testing easier. 'lti_1p3_tool_public_key': self.public_key, } - self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) + self.xblock = make_xblock( + 'lti_consumer', LtiConsumerXBlock, self.xblock_attributes, MagicMock(return_value=False) + ) # Set dummy location so that UsageKey lookup is valid self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' - self.xblock.is_past_due = MagicMock(return_value=False) - # Create configuration self.lti_config = LtiConfiguration.objects.create( location=str(self.xblock.location), @@ -69,26 +69,12 @@ def setUp(self): self.addCleanup(patcher.stop) self._lti_block_patch = patcher.start() - publish_grade_patcher = patch( - 'lti_consumer.signals.publish_grade', - return_value=None - ) - self.addCleanup(publish_grade_patcher.stop) - self._publish_grade_patcher = publish_grade_patcher.start() - - load_block_patcher = patch( - 'lti_consumer.signals.load_block', - return_value=self.xblock - ) - self.addCleanup(load_block_patcher.stop) - self._load_block_patcher = load_block_patcher.start() - - get_user_from_external_user_id_patcher = patch( - 'lti_consumer.signals.get_user_from_external_user_id', - return_value=None - ) - self.addCleanup(get_user_from_external_user_id_patcher.stop) - self._get_user_from_external_user_id_patcher = get_user_from_external_user_id_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): """ @@ -437,17 +423,19 @@ def test_xblock_grade_publish_on_score_save(self, grading_progress): if grading_progress == LtiAgsScore.FULLY_GRADED: score = LtiAgsScore.objects.get(line_item=self.line_item, user_id=self.primary_user_id) - self._publish_grade_patcher.assert_called_once() - self._get_user_from_external_user_id_patcher.assert_called_once() - self._load_block_patcher.assert_called_once() - call_args = self._publish_grade_patcher.call_args.args - call_kwargs = self._publish_grade_patcher.call_args.kwargs - self.assertEqual(call_args, (self.xblock, None, score.score_given, score.score_maximum,)) + + 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._load_block_patcher.assert_not_called() - self._get_user_from_external_user_id_patcher.assert_not_called() - self._publish_grade_patcher.assert_not_called() + 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_xblock_grade_publish_passed_due_date(self): """ @@ -470,9 +458,10 @@ def test_xblock_grade_publish_passed_due_date(self): content_type="application/vnd.ims.lis.v1.score+json", ) - self._load_block_patcher.assert_called_once() - self._get_user_from_external_user_id_patcher.assert_not_called() - self._publish_grade_patcher.assert_not_called() + 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): """ diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 382c68f1..6cffd195 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -158,29 +158,14 @@ class TestLtiAgsScoreModel(TestCase): """ Unit tests for LtiAgsScore model methods. """ - def setUp(self): - super().setUp() - publish_grade_patcher = patch( - 'lti_consumer.signals.publish_grade', - return_value=None - ) - self.addCleanup(publish_grade_patcher.stop) - self._publish_grade_patcher = publish_grade_patcher.start() - - load_block_patcher = patch( - 'lti_consumer.signals.load_block', - return_value=make_xblock('lti_consumer', LtiConsumerXBlock, {}, MagicMock(return_value=False)) - ) - self.addCleanup(load_block_patcher.stop) - self._load_block_patcher = load_block_patcher.start() + @patch("lti_consumer.signals.compat") + def setUp(self, compat_mock): + super().setUp() - get_user_from_external_user_id_patcher = patch( - 'lti_consumer.signals.get_user_from_external_user_id', - return_value=None + compat_mock.load_block_as_anonymous_user.return_value = make_xblock( + 'lti_consumer', LtiConsumerXBlock, {}, MagicMock(return_value=False) ) - self.addCleanup(get_user_from_external_user_id_patcher.stop) - self._get_user_from_external_user_id_patcher = get_user_from_external_user_id_patcher.start() self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test' self.line_item = LtiAgsLineItem.objects.create( @@ -209,11 +194,3 @@ def test_repr(self): str(self.score), "LineItem 1: score 10.0 out of 100.0 - FullyGraded" ) - - def test_score_update_signal(self): - """ - Test score update signal connected - """ - self._load_block_patcher.assert_called_once() - self._get_user_from_external_user_id_patcher.assert_called_once() - self._publish_grade_patcher.assert_called_once() From 978e7f8e9d73d61d250db43b6a0e7e57eba50cb3 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 12 Nov 2020 00:44:58 +0600 Subject: [PATCH 13/21] refactor test to fix quality issue --- lti_consumer/tests/unit/test_models.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 6cffd195..1e48a2ad 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -159,11 +159,14 @@ class TestLtiAgsScoreModel(TestCase): Unit tests for LtiAgsScore model methods. """ - @patch("lti_consumer.signals.compat") - def setUp(self, compat_mock): + def setUp(self): super().setUp() - compat_mock.load_block_as_anonymous_user.return_value = make_xblock( + # 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, {}, MagicMock(return_value=False) ) From 476d2d37b2655d4e390b5e73dc2b3e4adae878c7 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Thu, 12 Nov 2020 01:15:51 +0600 Subject: [PATCH 14/21] make lineitems_url required --- lti_consumer/lti_1p3/consumer.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/lti_1p3/consumer.py b/lti_consumer/lti_1p3/consumer.py index eb994a40..f85d9ccd 100644 --- a/lti_consumer/lti_1p3/consumer.py +++ b/lti_consumer/lti_1p3/consumer.py @@ -474,7 +474,7 @@ def lti_ags(self): def enable_ags( self, - lineitems_url=None, + lineitems_url, lineitem_url=None, allow_programatic_grade_interaction=False ): From bdc3f53364c3361c4ea3139e7b4175033b5ee3c8 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Fri, 13 Nov 2020 22:13:06 +0600 Subject: [PATCH 15/21] refactor tests, accept_grades_past_due on check --- lti_consumer/signals.py | 2 +- .../tests/unit/plugin/test_views_lti_ags.py | 77 ++++++++++--------- lti_consumer/tests/unit/test_models.py | 9 ++- lti_consumer/tests/unit/test_utils.py | 7 +- 4 files changed, 51 insertions(+), 44 deletions(-) diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py index f0684505..2d8409f8 100644 --- a/lti_consumer/signals.py +++ b/lti_consumer/signals.py @@ -16,7 +16,7 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl """ 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(): + if not block.is_past_due() or block.accept_grades_past_due: user = compat.get_user_from_external_user_id(instance.user_id) compat.publish_grade( block, 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 5718ed5f..b88f9d12 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, MagicMock, Mock +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,10 +46,13 @@ 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, MagicMock(return_value=False) - ) + self.xblock = make_xblock('lti_consumer', LtiConsumerXBlock, self.xblock_attributes) # Set dummy location so that UsageKey lookup is valid self.xblock.location = 'block-v1:course+test+2020+type@problem+block@test' @@ -394,6 +399,31 @@ 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, @@ -405,21 +435,10 @@ 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._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') - self.client.post( - self.scores_endpoint, - data=json.dumps({ - "timestamp": self.early_timestamp, - "scoreGiven": 83, - "scoreMaximum": 100, - "comment": "This is exceptional work.", - "activityProgress": LtiAgsScore.COMPLETED, - "gradingProgress": grading_progress, - "userId": self.primary_user_id - }), - content_type="application/vnd.ims.lis.v1.score+json", - ) + 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) @@ -437,26 +456,14 @@ def test_xblock_grade_publish_on_score_save(self, grading_progress): self._compat_mock.get_user_from_external_user_id.assert_not_called() self._compat_mock.publish_grade.assert_not_called() - def test_xblock_grade_publish_passed_due_date(self): + @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 published + Test grade publish after due date. Grade shouldn't publish """ - self._set_lti_token('https://purl.imsglobal.org/spec/lti-ags/scope/score') + timezone_patcher.now.return_value = timezone.now() + timedelta(days=30) - self.xblock.is_past_due = MagicMock(return_value=True) - self.client.post( - self.scores_endpoint, - data=json.dumps({ - "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 - }), - content_type="application/vnd.ims.lis.v1.score+json", - ) + self._post_lti_score() self._compat_mock.load_block_as_anonymous_user.assert_called_once() diff --git a/lti_consumer/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 1e48a2ad..7113f8a0 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -1,11 +1,13 @@ """ 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 -from mock import patch, MagicMock +from mock import patch from lti_consumer.lti_xblock import LtiConsumerXBlock from lti_consumer.models import LtiAgsLineItem, LtiConfiguration, LtiAgsScore @@ -167,7 +169,10 @@ def setUp(self): 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, {}, MagicMock(return_value=False) + 'lti_consumer', LtiConsumerXBlock, { + 'due': timezone.now(), + 'graceperiod': timedelta(days=2) + } ) self.dummy_location = 'block-v1:course+test+2020+type@problem+block@test' diff --git a/lti_consumer/tests/unit/test_utils.py b/lti_consumer/tests/unit/test_utils.py index 4a8b0f6e..da42a607 100644 --- a/lti_consumer/tests/unit/test_utils.py +++ b/lti_consumer/tests/unit/test_utils.py @@ -11,7 +11,7 @@ FAKE_USER_ID = 'fake_user_id' -def make_xblock(xblock_name, xblock_cls, attributes, is_past_due_patch=None): +def make_xblock(xblock_name, xblock_cls, attributes): """ Helper to construct XBlock objects """ @@ -27,11 +27,6 @@ def make_xblock(xblock_name, xblock_cls, attributes, is_past_due_patch=None): xblock.runtime = Mock( hostname='localhost', ) - - # mock is_past_due method - if is_past_due_patch: - xblock.is_past_due = is_past_due_patch - xblock.course_id = 'course-v1:edX+DemoX+Demo_Course' for key, value in attributes.items(): setattr(xblock, key, value) From 404091ae604dddf9ba66afb2e56f835e0b447511 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Fri, 13 Nov 2020 22:37:42 +0600 Subject: [PATCH 16/21] test accept_grades_past_due --- .../tests/unit/plugin/test_views_lti_ags.py | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 b88f9d12..dc308f42 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -470,6 +470,27 @@ def test_xblock_grade_publish_passed_due_date(self, timezone_patcher): 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. From 1530cadea9d042051d1b1927ef25349f6f8ce0a0 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Fri, 13 Nov 2020 23:25:06 +0600 Subject: [PATCH 17/21] add comma to last items --- lti_consumer/lti_1p3/ags.py | 2 +- lti_consumer/lti_1p3/tests/test_consumer.py | 4 ++-- lti_consumer/tests/unit/test_models.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lti_consumer/lti_1p3/ags.py b/lti_consumer/lti_1p3/ags.py index 2bfd42b6..046d6485 100644 --- a/lti_consumer/lti_1p3/ags.py +++ b/lti_consumer/lti_1p3/ags.py @@ -73,7 +73,7 @@ def get_lti_ags_launch_claim(self): claim_values["lineitem"] = self.lineitem_url ags_claim = { - "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values + "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values, } return ags_claim diff --git a/lti_consumer/lti_1p3/tests/test_consumer.py b/lti_consumer/lti_1p3/tests/test_consumer.py index fac3d0d3..b0a40b16 100644 --- a/lti_consumer/lti_1p3/tests/test_consumer.py +++ b/lti_consumer/lti_1p3/tests/test_consumer.py @@ -649,9 +649,9 @@ def test_enable_ags(self): 'scope': [ '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/tests/unit/test_models.py b/lti_consumer/tests/unit/test_models.py index 7113f8a0..d1fc03da 100644 --- a/lti_consumer/tests/unit/test_models.py +++ b/lti_consumer/tests/unit/test_models.py @@ -106,7 +106,7 @@ def test_lti_consumer_ags_enabled(self): 'https://purl.imsglobal.org/spec/lti-ags/scope/score', ], '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' + 'lineitem': 'https://example.com/api/lti_consumer/v1/lti/2/lti-ags/1', } } ) @@ -171,7 +171,7 @@ def setUp(self): self._compat_mock.load_block_as_anonymous_user.return_value = make_xblock( 'lti_consumer', LtiConsumerXBlock, { 'due': timezone.now(), - 'graceperiod': timedelta(days=2) + 'graceperiod': timedelta(days=2), } ) From b54c193df1d580f47166aafe9ee06538b2f4dfe1 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Sat, 14 Nov 2020 00:01:45 +0600 Subject: [PATCH 18/21] refactor get_lti_ags_lineitems_url --- lti_consumer/utils.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lti_consumer/utils.py b/lti_consumer/utils.py index 7bc698c0..bbce7853 100644 --- a/lti_consumer/utils.py +++ b/lti_consumer/utils.py @@ -74,14 +74,13 @@ def get_lti_ags_lineitems_url(lti_config_id, lineitem_id=None): :param lineitem_id: LTI Line Item id. Single line item if given an id, otherwise returns list url """ - base_ags_url = "{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 = "{url}/{lineitem_id}".format(url=base_ags_url, lineitem_id=lineitem_id) - else: - url = base_ags_url + url += "/" + str(lineitem_id) return url From 09c1e0a310ac5a1971dc1177334086762bee5971 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Sun, 15 Nov 2020 15:29:08 +0600 Subject: [PATCH 19/21] make sure crum returns user and not None --- lti_consumer/plugin/compat.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index 3ec29a85..b7564e00 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -25,13 +25,14 @@ def run_xblock_handler_noauth(*args, **kwargs): def load_block_as_anonymous_user(location): """ - Load a block as anonymous user. + Load a block as an user if given, else anonymous user. This uses a few internal courseware methods to retrieve the descriptor and bind an AnonymousUser to it, in a similar fashion as a `noauth` XBlock 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 @@ -39,18 +40,22 @@ 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 + return descriptor def get_user_from_external_user_id(external_user_id): From 6538c36e089fb41077ebb32dde3c0ff16e6bc631 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Sun, 15 Nov 2020 15:57:04 +0600 Subject: [PATCH 20/21] nitpicks & use maximum score when given score is larger than maximum --- lti_consumer/lti_1p3/ags.py | 4 +--- lti_consumer/signals.py | 4 +++- .../tests/unit/plugin/test_views_lti_ags.py | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/lti_consumer/lti_1p3/ags.py b/lti_consumer/lti_1p3/ags.py index 046d6485..9be67e3a 100644 --- a/lti_consumer/lti_1p3/ags.py +++ b/lti_consumer/lti_1p3/ags.py @@ -72,8 +72,6 @@ def get_lti_ags_launch_claim(self): if self.lineitem_url: claim_values["lineitem"] = self.lineitem_url - ags_claim = { + return { "https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values, } - - return ags_claim diff --git a/lti_consumer/signals.py b/lti_consumer/signals.py index 2d8409f8..453a12b5 100644 --- a/lti_consumer/signals.py +++ b/lti_consumer/signals.py @@ -18,10 +18,12 @@ def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disabl 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, - instance.score_given, + 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 dc308f42..cd34e415 100644 --- a/lti_consumer/tests/unit/plugin/test_views_lti_ags.py +++ b/lti_consumer/tests/unit/plugin/test_views_lti_ags.py @@ -456,6 +456,23 @@ def test_xblock_grade_publish_on_score_save(self, grading_progress): 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): """ From a9edcdb0f594dae476bcc37c80f1a2ec778b44e9 Mon Sep 17 00:00:00 2001 From: Shimul Chowdhury Date: Wed, 18 Nov 2020 00:07:36 +0600 Subject: [PATCH 21/21] fix docstring of load_block_as_anonymous_user --- lti_consumer/plugin/compat.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lti_consumer/plugin/compat.py b/lti_consumer/plugin/compat.py index b7564e00..0c3d0f14 100644 --- a/lti_consumer/plugin/compat.py +++ b/lti_consumer/plugin/compat.py @@ -25,7 +25,7 @@ def run_xblock_handler_noauth(*args, **kwargs): def load_block_as_anonymous_user(location): """ - Load a block as an user if given, else anonymous user. + Load a block as anonymous user. This uses a few internal courseware methods to retrieve the descriptor and bind an AnonymousUser to it, in a similar fashion as a `noauth` XBlock