[BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality #254

tecoholic · 2022-06-03T05:51:11Z

Description

The goal of the PR is to decouple the LTI 1.3 functionality from the XBlock and move the functionality to the Django Plugin, while keeping backward compatibility with the XBlock Handlers. The main consideration is that, this does not break any existing integrations. So all the endpoints and XBlock handlers are made backward compatible.

Changes introduced by the refactor

The functionality of LTI 1.3 Launch Handler is moved from the XBlock to the Django plugin
The functionality of the Access Token endpoint is moved from XBlock to the Django plugin
A new URL format using the LtiConfiguration ID is introduced for the Access Token endpoint and is used when a LTI Consumer is configured without a location allowing LTI integrations to be created without the XBlock context.
A new URL format using the LtiConfiguration ID is introduced for the Keyset Endpoint and is used with the location of the XBlock is not available in the configuration.

Testing instructions

Setup an LTI Consumer block (using test tools like Saltire or IMS Global Reference Tool)
Verify the LTI launches are working as expected.
Install the PR branch in the test system pip install git+https:/open-craft/xblock-lti-consumer.git@tecoholic/BB-5559-lti1p3-refactor. If you are using devstack, install it in both lms and studio containers.
Restart the services as necessary.
Ensure that all the LTI Blocks are working as before.

openedx-webhooks · 2022-06-03T05:51:17Z

Thanks for the pull request, @tecoholic! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2022-06-03T08:53:35Z

Codecov Report

Merging #254 (5dda795) into master (06c08bd) will decrease coverage by 0.46%.
The diff coverage is 94.37%.

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   97.01%   96.55%   -0.47%     
==========================================
  Files          71       71              
  Lines        5162     5252      +90     
==========================================
+ Hits         5008     5071      +63     
- Misses        154      181      +27

Flag	Coverage Δ
unittests	`96.55% <94.37%> (-0.47%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lti_consumer/utils.py	`90.90% <ø> (ø)`
lti_consumer/plugin/compat.py	`40.21% <28.57%> (-0.96%)`	⬇️
lti_consumer/models.py	`90.74% <61.53%> (-1.57%)`	⬇️
lti_consumer/tests/unit/test_lti_xblock.py	`98.93% <68.96%> (-1.07%)`	⬇️
lti_consumer/__init__.py	`100.00% <100.00%> (ø)`
lti_consumer/api.py	`96.92% <100.00%> (-1.52%)`	⬇️
lti_consumer/lti_xblock.py	`93.82% <100.00%> (-1.40%)`	⬇️
lti_consumer/plugin/views.py	`97.89% <100.00%> (+0.79%)`	⬆️
lti_consumer/tests/unit/plugin/test_views.py	`100.00% <100.00%> (ø)`
lti_consumer/tests/unit/test_api.py	`100.00% <100.00%> (ø)`
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

natabene · 2022-06-06T13:45:30Z

@tecoholic Thank you for the contribution, please let me know once this is ready for our review.

giovannicimolin · 2022-06-10T18:46:35Z

@tecoholic

@giovannicimolin Once we move the Access Token logic to the Django view, do we need to keep this XBlock Handler here? I do not find any internal reference to this handler except from the Django view. As that is no longer required, we can completely get rid of this handler here right?
I wanted to confirm this with you before I started rewriting the related unittests. Kindly let me know if there is any reason to keep this.
Don't mind the redirect stub I have put in here - its just a placeholder.

Unfortunately, we can't, this is used by external tools - and there's already a lot of them configured on edX.org.
We need to keep backwards capability here (just as a passthrough view, minimal code).

Studio should only display the new URL from now onwards.

giovannicimolin

@tecoholic Woah, great progress here!

You'll also need to update the URLs on https:/openedx/xblock-lti-consumer/blob/master/lti_consumer/utils.py#L56 to point to the new endpoints and update references to those.

lti_consumer/lti_xblock.py

lti_consumer/plugin/views.py

lti_consumer/tests/unit/test_lti_xblock.py

lti_consumer/plugin/compat.py

lti_consumer/lti_xblock.py

lti_consumer/plugin/views.py

lti_consumer/lti_xblock.py

tecoholic · 2022-07-06T14:15:40Z

NOTE: The lti_config_id in the URLs are supposed to be UUID but it got mis-implemented here, I think. I guess this wasn't caught early on because the regex ([-\w]+) applies to both UUID and integer ids. Maybe this was intentional. Anyway, this means, the new URLs can use UUIDs and prevent primary key from leaking as originally intended.

Zacharis278 · 2022-07-06T15:09:34Z

Hi @tecoholic. This is great, we also have some very similar work queued up to start doing LTI1.3 launches outside of the XBlock. (including #260) I believe @giovannicimolin is out for the better part of the month but I'd like to sync up so we aren't duplicating effort. Who is the best person to reach out to?

tecoholic · 2022-07-06T15:52:49Z

@Zacharis278 Hi! Looks like #260 is a very similar to this. 👀 You are right, @giovannicimolin is going to be unavailable for the month. @Agrendalath is handling the reviews in the meantime. Maybe he can help.

Agrendalath · 2022-07-11T11:23:55Z

lti_consumer/api.py

+ # set to CONFIG_ON_XBLOCK
+ if config_changed or conf.config_store == conf.CONFIG_ON_XBLOCK:
+ conf.lti_config.update(block_config)
+ conf.config_store = conf.CONFIG_ON_DB


Do we also need to set config_type to 'external'? I see that _get_lti_config is checking this, not _get_lti_config.

Ah. I have not considered the external case. For now we can simply return the config without any changes in the get_or_update_lti_config (Line 91). Adding

if block.config_type == "external": return conf

lti_consumer/lti_xblock.py

lti_consumer/plugin/views.py

lti_consumer/tests/unit/test_api.py

lti_consumer/signals.py

tecoholic · 2022-07-21T14:47:26Z

Original content removed

Never mind. I figured out the solution literally 5 mins after posting the comment. I think articulating the design challenge helped see the problem clearly. :) Sorry for the noise.

tecoholic · 2022-08-04T11:55:04Z

@giovannicimolin I have removed the overengineered parts, incorporated your feedback about the logging, rebased the code to master. Kindly give this another go.

MichaelRoytman

I'll be building off this, so I'm reviewing to better understand your work. I left a few small thoughts.

MichaelRoytman · 2022-08-04T19:41:10Z

lti_consumer/lti_xblock.py

@@ -710,6 +700,14 @@ def editable_fields(self):
 if not is_external_config_filter_enabled:
 noneditable_fields.append('external_config')

+ if self.lti_version == 'lti_1p3' and is_database_config_enabled and self.config_type == 'database':


I had something very similar in #260. I removed it in #266, because I believe that the Javascript should be responsible for showing or hiding fields. If this method doesn't return the LTI 1.3 fields when the config_type is database, then the Javascript cannot show the LTI 1.3 fields when the config_type is changed to XBlock in the modal.

Is this leftover code from #260 that should be removed? Or is it necessary?

@MichaelRoytman I believe this would be leftover code from #260. I am not sure why rebasing hasn't updated it. Maybe a conflict line. Let me update it to the latest code.

@tecoholic This is still showing up for me :)

MichaelRoytman · 2022-08-04T20:42:48Z

lti_consumer/lti_xblock.py

- )
+ # Asserting that the consumer can be created. This makes sure that the LtiConfiguration
+ # object exists before calling the Django View
+ assert self._get_lti_consumer()


Does this need exception handling similar to what you have in launch_gate_endpoint?

The original code doesn't have any error handling around this line. So I assume it is safe to call. We can add one here if it is required.

lti_consumer/plugin/views.py

giovannicimolin

Launches are working! @tecoholic Nice job!

I left a few comments in the PR though, we can address the remaining issues next sprint with smaller PRs.

giovannicimolin · 2022-08-08T19:29:15Z

lti_consumer/plugin/views.py

+ if usage_id:
+ lti_config = LtiConfiguration.objects.get(location=UsageKey.from_string(usage_id))
+ elif lti_config_id:
+ lti_config = LtiConfiguration.objects.get(config_id=lti_config_id)


If none of these blocks execute, you'll have a NameError: name 'lti_config' is not defined, which is unhandled by the try-except block below.

But wouldn't the Django's URL resolver take care this path and return a 404? The two paths that lead to this function are

'lti_consumer/v1/public_keysets/<uuid:lti_config_id>' f'lti_consumer/v1/public_keysets/{settings.USAGE_ID_PATTERN}$'

lti_consumer/plugin/views.py

giovannicimolin · 2022-08-08T19:31:11Z

lti_consumer/plugin/views.py

 """
+ usage_id = request.GET.get('login_hint')


This PR is getting too big - we'll have to change the way we give the context to the LTI consumer on a follow-up PR. Let's get these endpoints working on django with this PR first.

giovannicimolin · 2022-08-08T19:40:37Z

lti_consumer/plugin/views.py

+ if lti_config_id:
+ lti_config = LtiConfiguration.objects.get(config_id=lti_config_id)
+ else:
+ usage_key = UsageKey.from_string(usage_id)
+ lti_config = LtiConfiguration.objects.get(location=usage_key)
+ except LtiConfiguration.DoesNotExist as exc:
+ log.warning("Error getting the LTI configuration with id %r: %s", lti_config_id, exc)
+ raise Http404 from exc


Add an extra case for the uncaught path here, and improve log output.

Suggested change

if lti_config_id:

lti_config = LtiConfiguration.objects.get(config_id=lti_config_id)

else:

usage_key = UsageKey.from_string(usage_id)

lti_config = LtiConfiguration.objects.get(location=usage_key)

except LtiConfiguration.DoesNotExist as exc:

log.warning("Error getting the LTI configuration with id %r: %s", lti_config_id, exc)

raise Http404 from exc

if lti_config_id:

lti_config = LtiConfiguration.objects.get(config_id=lti_config_id)

elif usage_id:

usage_key = UsageKey.from_string(usage_id)

lti_config = LtiConfiguration.objects.get(location=usage_key)

else:

raise ValueError("Either `lti_config` or `usage_id` need to be set for this function to work")

except (LtiConfiguration.DoesNotExist, ValueError) as exc:

log.warning("Error getting the LTI configuration with id %r: %s", lti_config_id, exc, exc_info=True)

raise Http404 from exc

Same question as above? The Djanog's URL resolver would reject calls without either lti_config_id or usage_id.

lti_consumer/lti_xblock.py

giovannicimolin · 2022-08-08T19:57:32Z

lti_consumer/lti_xblock.py

@@ -710,6 +700,14 @@ def editable_fields(self):
 if not is_external_config_filter_enabled:
 noneditable_fields.append('external_config')

+ if self.lti_version == 'lti_1p3' and is_database_config_enabled and self.config_type == 'database':


@tecoholic This is still showing up for me :)

Zacharis278 · 2022-08-10T20:49:04Z

Hey guys, wanted to check in on where we stand with this PR. We've got some work lined up that would require doing a 1.3 launch without an xblock context so this would be great to have in master since there's some big changes here, even if we haven't totally solved the context/login_hint solution in launch_gate_endpoint().

More than happy to collaborate on how to solve that in a future PR since it's key to our needs as well!

The LTI 1.3 Launch callback handler is removed from the XBlock as the logic has been moved to its corresponding Django View. The associated tests are also moved from the XBlock testcase to the Django View TestCase.

tecoholic · 2022-08-11T06:11:43Z

@Zacharis278 I have made the changes to address the comments from @giovannicimolin. I think it is close to being merged.

giovannicimolin · 2022-08-12T19:49:13Z

@Zacharis278 This is in the final stages of review.
I've started an issue (#273) so we can all discuss the launch context problem and get re-usability and standalone launches working.

giovannicimolin · 2022-08-12T21:00:04Z

@tecoholic I'm almost done with the review - just wanted to test the grade passback functionality, but I don't have any tools with that capability set up locally.

Other than that, launches and DL launches are working - just want to make sure the other services work too.

giovannicimolin

👍

I tested this:
I set up the following tools and checked that they still work as expected:
IMS Reference implementation (https://lti-ri.imsglobal.org/)
H5P.com (https://opencraft.h5p.com/)
Locally, with IMS's PHP tool reference implementation.

I checked that launches are working as expected ✔️
I checked that Grades are being sent back to the LMS ✔️
I checked that Deep linking configuration is still working ✔️

I read through the code
~~I checked for accessibility issues~~ NA
Includes documentation

@tecoholic Good to go, I've checked that everything keeps working with this new rework.

Before this is merged, can you add a version bump?
We should go to __version__ = '4.4.0' with this change.

tecoholic · 2022-08-16T14:39:04Z

@giovannicimolin Great. Thank you for testing it so thoroughly. I have updated bumped the version to 4.4.0

…ndpoints from BB-5559 (#254)

Zacharis278

Also run through a basic reference tool launch with this. Looks great!

openedx-webhooks · 2022-08-17T18:20:47Z

@tecoholic 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jun 3, 2022

tecoholic marked this pull request as draft June 3, 2022 05:51

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch from 198068d to 0e7c50c Compare June 3, 2022 08:48

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch 2 times, most recently from 61c2444 to ae42e75 Compare June 3, 2022 09:37

giovannicimolin suggested changes Jun 10, 2022

View reviewed changes

lti_consumer/lti_xblock.py Outdated Show resolved Hide resolved

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

lti_consumer/tests/unit/test_lti_xblock.py Show resolved Hide resolved

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

openedx-webhooks removed the needs triage label Jun 16, 2022

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch 2 times, most recently from 310a2e2 to 60a24e6 Compare June 22, 2022 13:57

tecoholic changed the title ~~LTI 1.3 Refactoring~~ [BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality Jun 23, 2022

tecoholic marked this pull request as ready for review June 23, 2022 05:43

Agrendalath reviewed Jun 26, 2022

View reviewed changes

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch from d4ff48b to c7a8545 Compare July 6, 2022 15:15

Zacharis278 mentioned this pull request Jul 7, 2022

feat: Add core LTI 1.3 and LTI Advantage configuration to LTIConfiguration model and support XBlock using the database #260

Merged

Agrendalath reviewed Jul 11, 2022

View reviewed changes

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

lti_consumer/tests/unit/test_api.py Show resolved Hide resolved

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch from 2f7e77b to 3976c2b Compare July 14, 2022 10:49

giovannicimolin reviewed Jul 19, 2022

View reviewed changes

lti_consumer/signals.py Outdated Show resolved Hide resolved

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch 2 times, most recently from b85f2da to 718f18f Compare July 20, 2022 13:49

Zacharis278 mentioned this pull request Jul 20, 2022

Preliminary Discovery: LTI Directory openedx/platform-roadmap#126

Closed

3 tasks

tecoholic added 2 commits August 4, 2022 11:28

chore: move lti launch tracking from xblock to django view

78e4e7f

refactor: remove config syncing between xblock & models

9c2e441

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch from ed4e4b3 to 9c2e441 Compare August 4, 2022 07:31

refactor: improve logging and fix quality issues

1684785

tecoholic force-pushed the tecoholic/BB-5559-lti1p3-refactor branch from c4adc1e to 1684785 Compare August 4, 2022 11:54

MichaelRoytman reviewed Aug 4, 2022

View reviewed changes

giovannicimolin suggested changes Aug 8, 2022

View reviewed changes

giovannicimolin added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 10, 2022

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 11, 2022

refactor: remove launch_callback_handler from XBlock

c71a68a

The LTI 1.3 Launch callback handler is removed from the XBlock as the logic has been moved to its corresponding Django View. The associated tests are also moved from the XBlock testcase to the Django View TestCase.

giovannicimolin mentioned this pull request Aug 12, 2022

Remove XBlock location dependencies from LTI 1.3 launches #273

Open

giovannicimolin approved these changes Aug 15, 2022

View reviewed changes

chore: version bump

c89887d

MichaelRoytman added a commit that referenced this pull request Aug 16, 2022

feat: include tecoholic's changes to access_token and public_keyset e…

d541bac

…ndpoints from BB-5559 (#254)

MichaelRoytman mentioned this pull request Aug 16, 2022

[MST-1473] Proctoring Service Launch Implementation #275

Closed

MichaelRoytman added a commit that referenced this pull request Aug 16, 2022

feat: include tecoholic's changes to access_token and public_keyset e…

9cf62a6

…ndpoints from BB-5559 (#254)

MichaelRoytman mentioned this pull request Aug 16, 2022

[MST-1473] Proctoring Service Launch edx/edx-exams#32

Merged

6 tasks

Zacharis278 approved these changes Aug 17, 2022

View reviewed changes

docs: adds changelog for version 4.4.0

5dda795

giovannicimolin merged commit ec43c30 into openedx:master Aug 17, 2022

giovannicimolin deleted the tecoholic/BB-5559-lti1p3-refactor branch August 17, 2022 18:20

giovannicimolin restored the tecoholic/BB-5559-lti1p3-refactor branch August 17, 2022 18:20

MichaelRoytman mentioned this pull request Aug 22, 2022

feat: decouple LTI handlers from XBlock and improve LTI error handling for unauthenticated users openedx/edx-platform#30884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality #254

[BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality #254

tecoholic commented Jun 3, 2022 •

edited

Loading

openedx-webhooks commented Jun 3, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading

natabene commented Jun 6, 2022

giovannicimolin commented Jun 10, 2022

giovannicimolin left a comment

tecoholic commented Jul 6, 2022

Zacharis278 commented Jul 6, 2022

tecoholic commented Jul 6, 2022

Agrendalath Jul 11, 2022

tecoholic Jul 21, 2022

tecoholic commented Jul 21, 2022 •

edited

Loading

tecoholic commented Aug 4, 2022

MichaelRoytman left a comment

MichaelRoytman Aug 4, 2022

tecoholic Aug 5, 2022

giovannicimolin Aug 8, 2022

MichaelRoytman Aug 4, 2022

tecoholic Aug 5, 2022

giovannicimolin left a comment

giovannicimolin Aug 8, 2022

tecoholic Aug 11, 2022 •

edited

Loading

giovannicimolin Aug 8, 2022

giovannicimolin Aug 8, 2022

tecoholic Aug 11, 2022

giovannicimolin Aug 8, 2022

Zacharis278 commented Aug 10, 2022

tecoholic commented Aug 11, 2022

giovannicimolin commented Aug 12, 2022 •

edited

Loading

giovannicimolin commented Aug 12, 2022

giovannicimolin left a comment

tecoholic commented Aug 16, 2022

Zacharis278 left a comment

openedx-webhooks commented Aug 17, 2022

[BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality #254

[BB-5559] Decouple LTI 1.3 from LTI Consumer XBlock functionality #254

Conversation

tecoholic commented Jun 3, 2022 • edited Loading

Description

Changes introduced by the refactor

Testing instructions

openedx-webhooks commented Jun 3, 2022 • edited Loading

codecov bot commented Jun 3, 2022 • edited Loading

Codecov Report

natabene commented Jun 6, 2022

giovannicimolin commented Jun 10, 2022

giovannicimolin left a comment

Choose a reason for hiding this comment

tecoholic commented Jul 6, 2022

Zacharis278 commented Jul 6, 2022

tecoholic commented Jul 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tecoholic commented Jul 21, 2022 • edited Loading

tecoholic commented Aug 4, 2022

MichaelRoytman 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

tecoholic Aug 11, 2022 • edited Loading

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

Zacharis278 commented Aug 10, 2022

tecoholic commented Aug 11, 2022

giovannicimolin commented Aug 12, 2022 • edited Loading

giovannicimolin commented Aug 12, 2022

giovannicimolin left a comment

Choose a reason for hiding this comment

tecoholic commented Aug 16, 2022

Zacharis278 left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Aug 17, 2022

tecoholic commented Jun 3, 2022 •

edited

Loading

openedx-webhooks commented Jun 3, 2022 •

edited

Loading

codecov bot commented Jun 3, 2022 •

edited

Loading

tecoholic commented Jul 21, 2022 •

edited

Loading

tecoholic Aug 11, 2022 •

edited

Loading

giovannicimolin commented Aug 12, 2022 •

edited

Loading