-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch A few nits here and there, but this is going in a good direction.
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch resource_link_id
is missing and should be set to the problem's location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
lti_consumer/utils.py
Outdated
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lti_consumer/models.py
Outdated
@@ -296,3 +315,17 @@ def __str__(self): | |||
|
|||
class Meta: | |||
unique_together = (('line_item', 'user_id'),) | |||
|
|||
|
|||
@receiver(post_save, sender=LtiAgsScore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
9e38038
to
c68b0f6
Compare
lti_consumer/lti_1p3/ags.py
Outdated
@@ -18,7 +18,8 @@ class LtiAgs: | |||
""" | |||
def __init__( | |||
self, | |||
lineitems_url, | |||
lineitems_url=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch This is always required. Only the lineitem_url
is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed code, will do some testing now.
lti_consumer/lti_1p3/consumer.py
Outdated
@@ -474,17 +474,21 @@ def lti_ags(self): | |||
|
|||
def enable_ags( | |||
self, | |||
lineitems_url, | |||
lineitems_url=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now required.
@@ -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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…t date, updated tests.
73c7234
to
978e7f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor code nits.
'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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Throughout:) please add a trailing comma after the last item to avoid excess diffs like we see here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
else: | ||
url = base_ags_url | ||
|
||
return url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be easier to read as:
url = "{lms_base}/api/...."
if lineitem_id:
url += "/" + lineitem_id
return url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored based on the mentioned approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
lti_consumer/lti_1p3/ags.py
Outdated
|
||
if self.lineitem_url: | ||
claim_values["lineitem"] = self.lineitem_url | ||
|
||
ags_claim = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shimulch Minor nitpickinig: just return the dict instead of assigning it to a variable.
ags_claim = { | |
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
lti_consumer/signals.py
Outdated
compat.publish_grade( | ||
block, | ||
user, | ||
instance.score_given, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@giovannicimolin Checked this condition and added a test for it.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution works and given it is present in the official documentation, I don't see a reason not to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go 👍
- I tested this on my master devstack.
- I read through the code.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution works and given it is present in the official documentation, I don't see a reason not to use it.
lti_consumer/plugin/compat.py
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change still makes sense? Unless I misunderstood the code, it always impersonates the AnonymousUser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viadanna, you're right. Fixed the misleading docstring.
I've tested this with the IMS reference implementation tool, and it looks good! |
@giovannicimolin, should the new version be |
@shimulch Makes sense :) |
@giovannicimolin @nedbat version upgrade PR has been created at #123 |
This PR adds support for the declarative grading model for LTI consumer.
JIRA tickets:
Discussions:Dependencies: None
Screenshots:Sandbox URL:Merge deadline: ASAP
Testing instructions:
Scored = True
and Set aweight
value./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
).Author notes and concerns:
User
to be null when loaded bydjango-crum
module. As per documentation here it returnsuser=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
Settings