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

fix: LTI 1.3 extra claims and custom parameters #392

Conversation

kuipumu
Copy link
Contributor

@kuipumu kuipumu commented Jul 10, 2023

Description

This PR adds a feature that allows the LTI consumer XBlock to send custom parameters (including dynamic custom parameters) and extra claims to LTI 1.3 launches.

Type of Change

  • Add LtiConsumerXBlock get_lti_1p3_custom_parameters method.
  • Modify LtiConsumerXBlock get_lti_1p3_launch_data method.
  • Include tests for all added and modified code.

Testing:

  1. Add LTI custom params template setting:
# Here we set the value to any module and function
# that can be imported and returns a str.
LTI_CUSTOM_PARAM_TEMPLATES = {
    'test_param': 'django.utils.html:escape'
}
  1. Add a LTI processors to XBLOCK_SETTINGS (Example: https://github.com/appsembler/tahoe-lti).
  2. Create a course and add a LTI 1.3 consumer XBlock.
  3. Add custom parameters to the XBlock settings.
["test=test", "test_param=${test_param}"] 
  1. Enable the "Send extra parameters" setting.
  2. Go to the live course and execute an LTI 1.3 launch.
  3. The custom parameters and extra claims should be present on the https://purl.imsglobal.org/spec/lti/claim/custom claim.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 10, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jul 10, 2023

Thanks for the pull request, @kuipumu! 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.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 11, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (3de9bcb) 97.91% compared to head (2c04bcc) 97.94%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #392      +/-   ##
==========================================
+ Coverage   97.91%   97.94%   +0.02%     
==========================================
  Files          77       77              
  Lines        6475     6555      +80     
==========================================
+ Hits         6340     6420      +80     
  Misses        135      135              
Flag Coverage Δ
unittests 97.94% <100.00%> (+0.02%) ⬆️

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

Files Changed Coverage Δ
lti_consumer/__init__.py 100.00% <100.00%> (ø)
lti_consumer/data.py 100.00% <100.00%> (ø)
lti_consumer/lti_xblock.py 96.62% <100.00%> (+0.13%) ⬆️
lti_consumer/plugin/views.py 97.94% <100.00%> (+0.01%) ⬆️
lti_consumer/tests/unit/plugin/test_views.py 100.00% <100.00%> (ø)
lti_consumer/tests/unit/test_lti_xblock.py 99.25% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 12, 2023
@e0d
Copy link
Contributor

e0d commented Jul 14, 2023

@kuipumu It looks like there's a coverage decrease that needs attention.

@kuipumu
Copy link
Contributor Author

kuipumu commented Jul 16, 2023

@e0d I just added a commit to include a test to cover the missing lines, please let me know if there is any other improvement that should be added to the tests

@mphilbrick211
Copy link

Hi @openedx/masters-devs-cosmonauts! Would someone be able to please review/merge this for us? Thank you!

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 2, 2023
@zacharis278
Copy link
Contributor

@kuipumu This looks good but I think we should include some documentation on the new custom params. At minimum adding similar setup instructions to the /docs folder in this repo would be a great help. It's a small feature but I wouldn't want knowledge of how to use this to get lost in just the PR description here.

@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 11, 2023

@zacharis278 I was about to add documentation about this feature but I noticed there is already documentation about these features on the docs/developing.rst document, do you think there is anything else about the feature that should be remarked on that document?, the feature is still the same, this PR only includes a fix for these features to work on LTI 1.3, this also includes a minor improvement to the validation of the custom parameters field on the XBlock.

@zacharis278
Copy link
Contributor

ahh thankyou I hadn't noticed it was already there. That seems like to wrong doc for this to me but that's not your problem.

@mphilbrick211 mphilbrick211 removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Aug 17, 2023
@zacharis278
Copy link
Contributor

@kuipumu this does look good can you add the init.py and changelog entries for the new version? With those added I can merge this in.

@kuipumu kuipumu force-pushed the kuipumu/lti_1p3_custom_parameters_fix branch from b8b3728 to 2c04bcc Compare August 22, 2023 15:44
@kuipumu
Copy link
Contributor Author

kuipumu commented Aug 22, 2023

@zacharis278 I just updated the changelog and the version of the package.

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

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

tested locally and this is working as expected 👍

Thanks!

@zacharis278 zacharis278 merged commit d3686bf into openedx:master Aug 23, 2023
6 checks passed
@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants