Skip to content
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

Merged
merged 21 commits into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lti_consumer/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -16,3 +16,4 @@ class LtiConfigurationAdmin(admin.ModelAdmin):

admin.site.register(LtiConfiguration, LtiConfigurationAdmin)
admin.site.register(LtiAgsLineItem)
admin.site.register(LtiAgsScore)
4 changes: 4 additions & 0 deletions lti_consumer/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,7 @@ class LTIConsumerApp(AppConfig):
}
}
}

def ready(self):
# pylint: disable=unused-import,import-outside-toplevel
import lti_consumer.signals
19 changes: 13 additions & 6 deletions lti_consumer/lti_1p3/ags.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class LtiAgs:
def __init__(
self,
lineitems_url,
lineitem_url=None,
allow_creating_lineitems=True,
results_service_enabled=True,
scores_service_enabled=True
Expand All @@ -37,6 +38,8 @@ def __init__(
# Lineitems urls
self.lineitems_url = lineitems_url

self.lineitem_url = lineitem_url

shimulch marked this conversation as resolved.
Show resolved Hide resolved
def get_available_scopes(self):
"""
Retrieves list of available token scopes in this instance.
Expand All @@ -60,11 +63,15 @@ def get_lti_ags_launch_claim(self):
"""
Returns LTI AGS Claim to be injected in the LTI launch message.
"""
ags_claim = {
"https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": {
"scope": self.get_available_scopes(),
"lineitems": self.lineitems_url,
}

claim_values = {
"scope": self.get_available_scopes(),
"lineitems": self.lineitems_url,
}

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

return {
"https://purl.imsglobal.org/spec/lti-ags/claim/endpoint": claim_values,
}
6 changes: 5 additions & 1 deletion lti_consumer/lti_1p3/consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -475,16 +475,20 @@ def lti_ags(self):
def enable_ags(
self,
lineitems_url,
lineitem_url=None,
allow_programatic_grade_interaction=False
):
"""
Enable LTI Advantage Assignments and Grades Service.

This will include the LTI AGS Claim in the LTI message
and set up the required class.
"""

self.ags = LtiAgs(
lineitems_url=lineitems_url,
allow_creating_lineitems=True,
lineitem_url=lineitem_url,
allow_creating_lineitems=allow_programatic_grade_interaction,
results_service_enabled=True,
scores_service_enabled=True
)
Expand Down
6 changes: 3 additions & 3 deletions lti_consumer/lti_1p3/tests/test_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -647,11 +647,11 @@ def test_enable_ags(self):
{
'https://purl.imsglobal.org/spec/lti-ags/claim/endpoint': {
'scope': [
'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem',
'https://purl.imsglobal.org/spec/lti-ags/scope/lineitem.readonly',
'https://purl.imsglobal.org/spec/lti-ags/scope/result.readonly',
'https://purl.imsglobal.org/spec/lti-ags/scope/score'
'https://purl.imsglobal.org/spec/lti-ags/scope/score',
],
'lineitems': 'http://example.com/lineitems'
'lineitems': 'http://example.com/lineitems',
}
}
)
28 changes: 26 additions & 2 deletions lti_consumer/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
# LTI 1.3
from lti_consumer.lti_1p3.consumer import LtiAdvantageConsumer
from lti_consumer.plugin import compat
from lti_consumer.utils import get_lms_base, get_lti_ags_lineitems_url
from lti_consumer.utils import (
get_lms_base,
get_lti_ags_lineitems_url,
)


class LtiConfiguration(models.Model):
Expand Down Expand Up @@ -129,8 +132,29 @@ def _get_lti_1p3_consumer(self):

# Check if enabled and setup LTI-AGS
if self.block.has_score:

default_values = {
'resource_id': self.block.location,
'score_maximum': self.block.weight,
'label': self.block.display_name
}

if hasattr(self.block, 'start'):
default_values['start_date_time'] = self.block.start

if hasattr(self.block, 'due'):
default_values['end_date_time'] = self.block.due

# create LineItem if there is none for current lti configuration
lineitem, _ = LtiAgsLineItem.objects.get_or_create(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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_configuration=self,
resource_link_id=self.block.location,
defaults=default_values
)

consumer.enable_ags(
lineitems_url=get_lti_ags_lineitems_url(self.id)
lineitems_url=get_lti_ags_lineitems_url(self.id),
lineitem_url=get_lti_ags_lineitems_url(self.id, lineitem.id)
)

return consumer
Expand Down
67 changes: 56 additions & 11 deletions lti_consumer/plugin/compat.py
Original file line number Diff line number Diff line change
@@ -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):
Expand Down Expand Up @@ -30,22 +32,65 @@ def load_block_as_anonymous_user(location):
handler.
"""
# pylint: disable=import-error,import-outside-toplevel
from crum import impersonate
from django.contrib.auth.models import AnonymousUser
from xmodule.modulestore.django import modulestore
from lms.djangoapps.courseware.module_render import get_module_for_descriptor_internal

# Retrieve descriptor from modulestore
descriptor = modulestore().get_item(location)

# Load block, attaching it to AnonymousUser
get_module_for_descriptor_internal(
user=AnonymousUser(),
descriptor=descriptor,
student_data=None,
course_id=location.course_key,
track_function=None,
xqueue_callback_url_prefix="",
request_token="",
)
# ensure `crum.get_current_user` returns AnonymousUser. It returns None when outside
# of request scope which causes error during block loading.
user = AnonymousUser()
with impersonate(user):
# Load block, attaching it to AnonymousUser
get_module_for_descriptor_internal(
user=user,
descriptor=descriptor,
student_data=None,
course_id=location.course_key,
track_function=None,
xqueue_callback_url_prefix="",
request_token="",
)

return descriptor


def get_user_from_external_user_id(external_user_id):
"""
Import ExternalId model and find user by external_user_id
"""
# pylint: disable=import-error,import-outside-toplevel
from openedx.core.djangoapps.external_user_ids.models import ExternalId
try:
external_id = ExternalId.objects.get(
external_user_id=external_user_id,
external_id_type__name='lti'
)
return external_id.user
except ExternalId.DoesNotExist as exception:
raise LtiError('Invalid User') from exception
except ValidationError as exception:
raise LtiError('Invalid userID') from exception

return descriptor

def publish_grade(block, user, score, possible, only_if_higher=False, score_deleted=None, comment=None):
"""
Import grades signals and publishes score by triggering SCORE_PUBLISHED signal.
"""
# pylint: disable=import-error,import-outside-toplevel
from lms.djangoapps.grades.api import signals as grades_signals

# publish score
grades_signals.SCORE_PUBLISHED.send(
sender=None,
block=block,
user=user,
raw_earned=score,
raw_possible=possible,
only_if_higher=only_if_higher,
score_deleted=score_deleted,
grader_response=comment
)
29 changes: 29 additions & 0 deletions lti_consumer/signals.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
"""
LTI Consumer related Signal handlers
"""

from django.db.models.signals import post_save
from django.dispatch import receiver

from lti_consumer.models import LtiAgsScore
from lti_consumer.plugin import compat


@receiver(post_save, sender=LtiAgsScore, dispatch_uid='publish_grade_on_score_update')
def publish_grade_on_score_update(sender, instance, **kwargs): # pylint: disable=unused-argument
"""
Publish grade to xblock whenever score saved/updated and its grading_progress is set to FullyGraded.
"""
if instance.grading_progress == LtiAgsScore.FULLY_GRADED:
block = compat.load_block_as_anonymous_user(instance.line_item.resource_link_id)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

if not block.is_past_due() or block.accept_grades_past_due:
user = compat.get_user_from_external_user_id(instance.user_id)
# check if score_given is larger than score_maximum
score = instance.score_given if instance.score_given < instance.score_maximum else instance.score_maximum
compat.publish_grade(
block,
user,
score,
instance.score_maximum,
comment=instance.comment
)
Loading