[BD-24] [TNL-7661] [BB-3172] LTI Improvements - Use declarative grading model on XBlock launch #116

shimulch · 2020-11-04T16:41:21Z

This PR adds support for the declarative grading model for LTI consumer.

JIRA tickets:

https://openedx.atlassian.net/browse/TNL-7661

Discussions:

Dependencies: None

Screenshots:

Sandbox URL:

Merge deadline: ASAP

Testing instructions:

Make sure lti_consumer installed as per README.rst
Setup a new LTI block with Scored = True and Set a weight value.
Test if the LTI launch is successful.
Go to admin /admin/lti_consumer/ltiagslineitem/ URL and confirm that a Line Item has been generated automatically with correct LTI Configuration, resource id, label, and Maximum Score (weight).
Generate a JWT token -
- Go to www.jwt.io
- select RS-256
- Fill in both the private and public keys section in the bottom right (use public & private key used during LTI block setup)
- Set the header and payload as follows

# HEADER:
{
  "alg": "RS256",
  "typ": "JWT",
  "kid": "123"
}
# PAYLOAD:
{
  "sub": "1234567890",
  "name": "John Doe",
  "admin": true,
  "iat": 1516239022
}

Use JWT to get an OIDC token

curl --request POST \
  --url 'http://localhost:18000/api/lti_consumer/v1/token/<USAGE_KEY>' \
  --header 'Content-Type: application/x-www-form-urlencoded' \
  --data grant_type=client_credentials \
  --data 'scope=https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readonly https://purl.imsglobal.org/spec/lti-ags/scope/score https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly' \
  --data client_assertion_type=urn:ietf:params:oauth:client-assertion-type:jwt-bearer \
  --data client_assertion=<JWT_TOKEN>

Publish score

curl --request POST \
  --url http://localhost:18000/api/lti_consumer/v1/lti/1/lti-ags/1/scores \
  --header 'Authorization: Bearer <OIDC_TOKEN>' \
  --header 'Content-Type: application/vnd.ims.lis.v1.score+json' \
  --data '{"timestamp": "2020-09-27T18:54:36.736+00:00","scoreGiven" : 86,"scoreMaximum" : 100,"comment" : "This is exceptional work.","activityProgress" : "Completed","gradingProgress": "FullyGraded","userId" : "<EXTERNAL_ID>"}'

The published score should be visible in the Progress tab of the course.

Author notes and concerns:

The signal seems to be called from out of request scope causing User to be null when loaded by django-crum module. As per documentation here it returns user=None when not in request scope. I couldn't figure out why xblock signal is outside the request scope, when some other places seems to be working fine like here.

Anyhow, I fixed the issue by impersonating as AnonymousUser here. But I'm not sure if this is the right way to do it.

Reviewers

@giovannicimolin
edX reviewer[s] TBD

~~Settings~~

openedx-webhooks · 2020-11-04T16:41:27Z

Thanks for the pull request, @shimulch! I've created BLENDED-651 to keep track of it in Jira. More details are on the BD-24 project page.

When this pull request is ready, tag your edX technical lead.

giovannicimolin

@shimulch A few nits here and there, but this is going in a good direction.

lti_consumer/lti_1p3/ags.py

lti_consumer/lti_1p3/consumer.py

giovannicimolin · 2020-11-05T12:40:29Z

lti_consumer/models.py

@@ -132,8 +139,20 @@ 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(


This is missing a few parameters. Check https://github.com/edx/xblock-lti-consumer/blob/master/docs/decisions/0003-lti-1p3-score-linking.rst#declarative-grade-handling for details on all attributes.

I've added lti_configuration, resource_id, label, score_maximum (weight). I didn't find others in attributes. Let me know if I've missed.

There's start_date_time and end_date_time which might be available on some cases.
You can check if the problem is past due by looking at https://github.com/edx/xblock-lti-consumer/blob/bcdcf218096d63b88a8d8c56e027c0910814c381/lti_consumer/lti_xblock.py#L828

@shimulch resource_link_id is missing and should be set to the problem's location.

@giovannicimolin I've put resource_link_id as same as self.block.location. Also checked if start and due is set. If set then saved as start_date_time and end_date_time accordingly.

giovannicimolin · 2020-11-05T12:41:51Z

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):


Instead of this, you can re-use the function above and add an optional parameter lineitem_id which appends / + the id at the end of the URL.

giovannicimolin · 2020-11-05T12:44:19Z

lti_consumer/models.py

@@ -296,3 +315,17 @@ def __str__(self):

    class Meta:
        unique_together = (('line_item', 'user_id'),)
+
+
+@receiver(post_save, sender=LtiAgsScore)


Create a signals.py file and move this code there.

Don't forget to import the signals on app.py in the ready method, otherwise they won't work properly in the platform.

Here I've added a receiver and didn't create any custom signal so I don't have to add it to the app.py ready method.

lti_consumer/models.py

coveralls · 2020-11-08T18:48:59Z

Coverage decreased (-98.9%) to 0.0% when pulling 9e38038 on open-craft:shimulch/bb-3172-declarative-grading into 5fc16b3 on edx:master.

coveralls · 2020-11-08T18:49:00Z

Coverage decreased (-0.5%) to 98.279% when pulling a9edcdb on open-craft:shimulch/bb-3172-declarative-grading into 3a54508 on edx:master.

giovannicimolin · 2020-11-10T12:29:01Z

lti_consumer/lti_1p3/ags.py

@@ -18,7 +18,8 @@ class LtiAgs:
    """
    def __init__(
        self,
-        lineitems_url,
+        lineitems_url=None,


@shimulch This is always required. Only the lineitem_url is optional.

giovannicimolin

Reviewed code, will do some testing now.

giovannicimolin · 2020-11-10T12:29:25Z

lti_consumer/lti_1p3/consumer.py

@@ -474,17 +474,21 @@ def lti_ags(self):

    def enable_ags(
        self,
-        lineitems_url,
+        lineitems_url=None,


This one is required.

Now required.

giovannicimolin · 2020-11-10T12:34:54Z

lti_consumer/models.py

@@ -132,8 +139,20 @@ 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(


There's start_date_time and end_date_time which might be available on some cases.
You can check if the problem is past due by looking at https://github.com/edx/xblock-lti-consumer/blob/bcdcf218096d63b88a8d8c56e027c0910814c381/lti_consumer/lti_xblock.py#L828

lti_consumer/utils.py

lti_consumer/lti_1p3/ags.py

lti_consumer/plugin/compat.py

…ble_ags method

…t date, updated tests.

giovannicimolin

Minor code nits.

lti_consumer/signals.py

lti_consumer/tests/unit/test_utils.py

nedbat · 2020-11-13T17:08:35Z

lti_consumer/tests/unit/test_models.py

                        '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'


(Throughout:) please add a trailing comma after the last item to avoid excess diffs like we see here.

I've added trailing comma to all related lists to this PR. I didn't go through other lists as that will make a lot of unrelated diff in this PR.

nedbat · 2020-11-13T17:17:20Z

lti_consumer/utils.py

+    else:
+        url = base_ags_url
+
+    return url


This would be easier to read as:

url = "{lms_base}/api/...." if lineitem_id: url += "/" + lineitem_id return url

Refactored based on the mentioned approach.

giovannicimolin

@shimulch I was hoping to approve this, but the code doesn't seem to be working anymore after the last revisions. Can you investigate and fix the issue?
This is a high priority ticket and needs to be merged as soon as possible to be shipped in the Koa release.

Also, you'll need a new reviewer here since I'll be off in the upcoming sprint.

giovannicimolin · 2020-11-14T21:16:50Z

lti_consumer/lti_1p3/ags.py

+
+        if self.lineitem_url:
+            claim_values["lineitem"] = self.lineitem_url
+
        ags_claim = {


@shimulch Minor nitpickinig: just return the dict instead of assigning it to a variable.

Suggested change

ags_claim = {

return {

lti_consumer/lti_1p3/ags.py

giovannicimolin · 2020-11-14T21:36:56Z

lti_consumer/signals.py

+            compat.publish_grade(
+                block,
+                user,
+                instance.score_given,


@shimulch Just noticed this: score_given can be higher than score_maximum on LTI Scores, but the same isn't true in the LMS.
Can you adjust this so that if given > maximum then the value passed to this function is capped at maximum?

@giovannicimolin Checked this condition and added a test for it.

giovannicimolin · 2020-11-14T21:52:06Z

lti_consumer/signals.py

+    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)


This is not working for me, I'm getting a 500 error when doing the request:

File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/query.py", line 422, in create obj.save(force_insert=True, using=self.db) File "/edx/src/xblock-lti-consumer/lti_consumer/models.py", line 308, in save super().save(*args, **kwargs) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py", line 743, in save self.save_base(using=using, force_insert=force_insert, File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/db/models/base.py", line 791, in save_base post_save.send( File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 173, in send return [ File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/django/dispatch/dispatcher.py", line 174, in <listcomp> (receiver, receiver(signal=self, sender=sender, **named)) File "/edx/src/xblock-lti-consumer/lti_consumer/signals.py", line 18, in publish_grade_on_score_update block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id) File "/edx/src/xblock-lti-consumer/lti_consumer/plugin/compat.py", line 43, in load_block_as_anonymous_user get_module_for_descriptor_internal( File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/module_render.py", line 874, in get_module_for_descriptor_internal (system, student_data) = get_module_system_for_user( File "/edx/app/edxapp/edx-platform/lms/djangoapps/courseware/module_render.py", line 768, in get_module_system_for_user field_data = DateLookupFieldData(descriptor._field_data, course_id, user) # pylint: disable=protected-access File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/edx_when/field_data.py", line 51, in __init__ self._load_dates(course_id, user, use_cached=use_cached) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/edx_when/field_data.py", line 59, in _load_dates for (location, field), date in api.get_dates_for_course(course_id, user, use_cached=use_cached).items(): File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/edx_when/api.py", line 142, in get_dates_for_course allow_relative_dates = _are_relative_dates_enabled(course_id) File "/edx/app/edxapp/venvs/edxapp/lib/python3.8/site-packages/edx_when/api.py", line 57, in _are_relative_dates_enabled return RELATIVE_DATES_FLAG.is_enabled(course_key) File "/edx/app/edxapp/edx-platform/lms/djangoapps/experiments/flags.py", line 253, in is_enabled return self.get_bucket(course_key) != 0 File "/edx/app/edxapp/edx-platform/lms/djangoapps/experiments/flags.py", line 165, in get_bucket if not hasattr(request, 'user') or not request.user.id: AttributeError: 'NoneType' object has no attribute 'id' [14/Nov/2020 21:50:29] "POST /api/lti_consumer/v1/lti/1/lti-ags/1/scores HTTP/1.1" 500 126762

Loading the block here (on signals.py) is erroring out for me. Can you check what's causing this?

@giovannicimolin, It seems to be causing from django-crum module. As per documentation here it returns user=None when not in request scope. I couldn't figure out why xblock signal is outside the request scope, when some other places seems to be working fine like here.

Anyhow, I fixed the issue by impersonating as AnonymousUser here. But I'm not sure if this is the right way to do it. Adding this to Author's concern section.

This solution works and given it is present in the official documentation, I don't see a reason not to use it.

viadanna

Good to go 👍

I tested this on my master devstack.
I read through the code.

viadanna · 2020-11-17T17:56:09Z

lti_consumer/signals.py

+    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)


This solution works and given it is present in the official documentation, I don't see a reason not to use it.

viadanna · 2020-11-17T17:57:18Z

lti_consumer/plugin/compat.py

@@ -23,29 +25,72 @@ 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.


Does this change still makes sense? Unless I misunderstood the code, it always impersonates the AnonymousUser.

@viadanna, you're right. Fixed the misleading docstring.

shimulch · 2020-11-17T18:12:07Z

@viadanna, Thanks for reviewing & approving the PR.

@nedbat, It's ready for review.

nedbat · 2020-11-20T14:56:25Z

I've tested this with the IMS reference implementation tool, and it looks good!

giovannicimolin · 2020-11-26T17:56:23Z

@shimulch @nedbat I don't see the version bump in this PR, we still need a tag for this to get released. @shimulch Can you create a version bump PR? CC @viadanna

shimulch · 2020-11-27T09:01:29Z

@giovannicimolin, should the new version be 2.4.0? This seems like new feature support than a bug.

giovannicimolin · 2020-11-27T13:55:31Z

@shimulch Makes sense :)

shimulch · 2020-11-28T04:27:51Z

@giovannicimolin @nedbat version upgrade PR has been created at #123

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 4, 2020

giovannicimolin suggested changes Nov 5, 2020

View reviewed changes

giovannicimolin mentioned this pull request Nov 5, 2020

[BD-24] [TNL-7661] [BB-3172] LTI Improvements - Load XBlock properly #117

Merged

2 tasks

shimulch marked this pull request as ready for review November 9, 2020 11:00

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Nov 9, 2020

shimulch force-pushed the shimulch/bb-3172-declarative-grading branch from 9e38038 to c68b0f6 Compare November 9, 2020 11:07

giovannicimolin reviewed Nov 10, 2020

View reviewed changes

giovannicimolin suggested changes Nov 10, 2020

View reviewed changes

giovannicimolin reviewed Nov 10, 2020

View reviewed changes

lti_consumer/lti_1p3/ags.py Show resolved Hide resolved

giovannicimolin reviewed Nov 10, 2020

View reviewed changes

lti_consumer/plugin/compat.py Outdated Show resolved Hide resolved

giovannicimolin reviewed Nov 10, 2020

View reviewed changes

lti_consumer/plugin/compat.py Outdated Show resolved Hide resolved

shimulch added 13 commits November 12, 2020 00:09

create default LineItem, WIP grade save

b6bff61

add score to django admin

0f51530

WIP: find user and save grade to xblock

89a0a17

boolean pragramatic grade interaction flag and optional params in ena…

dbe4dd8

…ble_ags method

Submit grades using grade signals

0ede3f3

lineitem urls should be optional

4a3175b

lineitem is now readonly in declarative method

f414206

test grade_submit called properly

2616d43

quality issue

74772e1

raise LTIError

45c8ad2

moved listener to signal.py, refactored models.py, added due and star…

e93a29f

…t date, updated tests.

use load_block_as_anonymous_user and remove load_block, refactor tests

30e3286

refactor test to fix quality issue

978e7f8

shimulch force-pushed the shimulch/bb-3172-declarative-grading branch from 73c7234 to 978e7f8 Compare November 11, 2020 18:45

make lineitems_url required

476d2d3

giovannicimolin suggested changes Nov 13, 2020

View reviewed changes

lti_consumer/signals.py Outdated Show resolved Hide resolved

lti_consumer/tests/unit/test_utils.py Outdated Show resolved Hide resolved

shimulch added 2 commits November 13, 2020 22:13

refactor tests, accept_grades_past_due on check

bdc3f53

test accept_grades_past_due

404091a

nedbat reviewed Nov 13, 2020

View reviewed changes

shimulch added 2 commits November 13, 2020 23:25

add comma to last items

1530cad

refactor get_lti_ags_lineitems_url

b54c193

giovannicimolin suggested changes Nov 14, 2020

View reviewed changes

shimulch added 2 commits November 15, 2020 15:29

make sure crum returns user and not None

09c1e0a

nitpicks & use maximum score when given score is larger than maximum

6538c36

viadanna approved these changes Nov 17, 2020

View reviewed changes

fix docstring of load_block_as_anonymous_user

a9edcdb

nedbat approved these changes Nov 20, 2020

View reviewed changes

nedbat merged commit 8b72fb9 into openedx:master Nov 20, 2020

openedx-webhooks added merged and removed needs triage labels Nov 20, 2020

shimulch deleted the shimulch/bb-3172-declarative-grading branch November 20, 2020 15:14

shimulch mentioned this pull request Nov 28, 2020

[BD-24] [TNL-7661] [BB-3172] Bump up the version #123

Merged

2 tasks

[BD-24] [TNL-7661] [BB-3172] LTI Improvements - Use declarative grading model on XBlock launch #116

[BD-24] [TNL-7661] [BB-3172] LTI Improvements - Use declarative grading model on XBlock launch #116

Conversation

shimulch commented Nov 4, 2020 • edited

openedx-webhooks commented Nov 4, 2020 • edited

giovannicimolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimulch Nov 8, 2020 • edited

Choose a reason for hiding this comment

coveralls commented Nov 8, 2020

coveralls commented Nov 8, 2020 • edited

Choose a reason for hiding this comment

giovannicimolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannicimolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannicimolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viadanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shimulch commented Nov 17, 2020

nedbat commented Nov 20, 2020

giovannicimolin commented Nov 26, 2020

shimulch commented Nov 27, 2020

giovannicimolin commented Nov 27, 2020

shimulch commented Nov 28, 2020

shimulch commented Nov 4, 2020 •

edited

openedx-webhooks commented Nov 4, 2020 •

edited

shimulch Nov 8, 2020 •

edited

coveralls commented Nov 8, 2020 •

edited