[SE-4235] Add missing parameter processors code #150

pkulkark · 2021-03-24T11:12:43Z

Somewhere between versions 2.0.0 and 2.1.0, the code to handle external parameter processors was dropped. This PR adds it back. Without this, external processors that are specified in the parameter processors settings is not called.

Jira Tickets: OSPR-5686

Sandbox URL: TBD

Testing Instructions:

Install this branch on your xblock runtime environment (for example xblock-sdk).
Test the parameter processors from tahoe-lti as per the test instructions provided
Verify that the external parameter processors are being called correctly.

Reviewers

@gabor-boros
edX reviewer TBD

openedx-webhooks · 2021-03-24T11:12:48Z

Thanks for the pull request, @pkulkark! I've created OSPR-5686 to keep track of it in JIRA, where we prioritize reviews. 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:

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.

coveralls · 2021-03-24T11:16:18Z

Coverage remained the same at 0.0% when pulling 1926299 on open-craft:pooja/fix-parameter-processor-logic into a5ca2cd on edx:master.

natabene · 2021-03-24T14:28:30Z

@pkulkark Thank you for your contribution. Please let me know once it is ready for our review.

pkulkark · 2021-03-26T11:53:36Z

@natabene This is ready for edX's review.

giovannicimolin

@pkulkark @gabor-boros I left some comments in the code :)

lti_consumer/lti_1p1/consumer.py

pkulkark · 2021-05-26T14:18:11Z

Thanks @giovannicimolin. I've made the changes as per your suggestions. Could you take a look and let me know if it looks good?

giovannicimolin · 2021-05-26T14:40:55Z

@pkulkark Code changes look good, were you able to test these changes and make sure they work as expected?

pkulkark · 2021-05-26T14:50:56Z

@giovannicimolin Yup, I tested it on this sandbox. Everything works as expected.

giovannicimolin

👍

I tested this:

I checked the sandbox instance using the staff user.
Checked data from additional parameter processors were passed to the LTI tool.

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

@pkulkark Thanks for the fix, this was missed when implementing LTI 1.1 support outside XBlocks for the Discussions epic.

nedbat · 2021-06-11T19:41:59Z

@pkulkark Is this ready for a final review?

pkulkark · 2021-06-14T09:49:48Z

@nedbat Yes, this is good for a final review.

lti_consumer/lti_xblock.py

lti_consumer/lti_1p1/consumer.py

giovannicimolin · 2021-08-06T12:48:02Z

@pkulkark I just realized that squash & merge is not enabled for this repo. Can you squash your commits down to a single one following OEP-51?

I can merge after that.

This Adds back the parameter processors code logic that was missed in a recent refactor.

giovannicimolin · 2021-08-06T13:16:20Z

@sarina @nedbat Merging this as all comments have been addressed.

This PR updates the lti-consumer-xblock version to a new commit that contains the backported fix from openedx/xblock-lti-consumer#150

louis-s-29 · 2021-08-12T08:30:40Z

@pkulkark @giovannicimolin - Hi, currently testing out the new xblock-lti-consumer 3.0.3 and tahoe-lti.

Per the "how to test" instructions of tahoe-lti, I was wondering if this PR + Tahoe-lti will work only for Lti1.1 or also Lti1.3?

pkulkark · 2021-08-12T09:35:07Z

@louis-s-29 Tahoe-lti only supports LTI 1.1 as far as I know.

giovannicimolin · 2021-08-13T14:29:55Z

@louis-s-29 Parameter processors are currently not implemented in LTI 1.3, but they can be plugged in the LTI 1.3 launch handler along with the custom parameter support.

Upon a quick check, the parameter processor code seems to be generic enough so that it isn't too complex to add support for on LTI 1.3. If you are going to work on adding support, let me know, I can help/guide you through the implementation process.

The previous lti-consumer-xblock commit hash was pointing to a code drift branch which was backporting openedx/xblock-lti-consumer#150 to tag `2.4.0` Unfortunately, `config_id` duplicate errors started showing for one of the clients. Accordingly, we had to update the xblock to the latest version which inculded that fix and still supported koa (`2.10.1`). However, openedx/xblock-lti-consumer#150 was not merged until tag `3.0.3`. Thefore, we created a custom tag, `2.10.1-opencraft` which includes the backported change on top of tag `2.10.1`

The previous lti-consumer-xblock commit hash was pointing to a code drift branch which was backporting openedx/xblock-lti-consumer#150 to tag `2.4.0` Unfortunately, `config_id` duplicate errors started showing for one of the clients. Accordingly, we had to update the xblock to the latest version which inculded that fix and still supported koa (`2.10.1`). However, openedx/xblock-lti-consumer#150 was not merged until tag `3.0.3`. Thefore, we created a custom tag, `2.10.1-opencraft` which includes the backported change on top of tag `2.10.1` (cherry picked from commit 85884f3)

This PR updates the lti-consumer-xblock version to a new commit that contains the backported fix from openedx/xblock-lti-consumer#150 (cherry picked from commit 9612bad)

The previous lti-consumer-xblock commit hash was pointing to a code drift branch which was backporting openedx/xblock-lti-consumer#150 to tag `2.4.0` Unfortunately, `config_id` duplicate errors started showing for one of the clients. Accordingly, we had to update the xblock to the latest version which inculded that fix and still supported koa (`2.10.1`). However, openedx/xblock-lti-consumer#150 was not merged until tag `3.0.3`. Thefore, we created a custom tag, `2.10.1-opencraft` which includes the backported change on top of tag `2.10.1` (cherry picked from commit 85884f3)

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Mar 24, 2021

pkulkark force-pushed the pooja/fix-parameter-processor-logic branch from db253da to 3c1a549 Compare March 24, 2021 11:24

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

gabor-boros approved these changes Mar 26, 2021

View reviewed changes

openedx-webhooks added engineering review awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. engineering review labels Mar 26, 2021

giovannicimolin suggested changes Apr 27, 2021

View reviewed changes

lti_consumer/lti_1p1/consumer.py Outdated Show resolved Hide resolved

lti_consumer/lti_1p1/consumer.py Outdated Show resolved Hide resolved

openedx-webhooks added changes requested and removed awaiting prioritization labels Apr 28, 2021

natabene added the cc-reviewer label Apr 28, 2021

OmarIthawi mentioned this pull request May 25, 2021

Tahoe LTI customizations seem to have no effect on an Tutor installation of Open edX appsembler/tahoe-lti#11

Closed

giovannicimolin approved these changes May 27, 2021

View reviewed changes

openedx-webhooks added engineering review and removed changes requested labels Jun 15, 2021

nedbat suggested changes Jun 23, 2021

View reviewed changes

lti_consumer/lti_xblock.py Outdated Show resolved Hide resolved

lti_consumer/lti_1p1/consumer.py Outdated Show resolved Hide resolved

openedx-webhooks added changes requested and removed engineering review labels Jun 23, 2021

Fix: Add missing parameter processors code

1926299

This Adds back the parameter processors code logic that was missed in a recent refactor.

pkulkark force-pushed the pooja/fix-parameter-processor-logic branch from ed0b23f to 1926299 Compare August 6, 2021 13:11

giovannicimolin merged commit 41d9dd8 into openedx:master Aug 6, 2021

giovannicimolin deleted the pooja/fix-parameter-processor-logic branch August 6, 2021 13:16

openedx-webhooks added merged and removed changes requested labels Aug 6, 2021

shimulch restored the pooja/fix-parameter-processor-logic branch August 7, 2021 12:41

shimulch deleted the pooja/fix-parameter-processor-logic branch August 7, 2021 13:01

pkulkark added a commit to open-craft/edx-platform that referenced this pull request Aug 9, 2021

fix: update lti-consumer-xblock to new commit

36b55c6

This PR updates the lti-consumer-xblock version to a new commit that contains the backported fix from openedx/xblock-lti-consumer#150

pkulkark mentioned this pull request Aug 9, 2021

[SE-4235] Update lti-consumer-xblock to new commit open-craft/edx-platform#384

Merged

pkulkark added a commit to open-craft/edx-platform that referenced this pull request Aug 9, 2021

fix: update lti-consumer-xblock to new commit (#384)

9612bad

This PR updates the lti-consumer-xblock version to a new commit that contains the backported fix from openedx/xblock-lti-consumer#150

This was referenced Aug 13, 2021

Support custom parameter processors on LTI 1.3 #191

Open

Parameter substitution is not working on LTI 1.1 launches #179

Open

nizarmah mentioned this pull request Sep 16, 2021

fix: update lti-consumer-xblock to 2.10.1 open-craft/edx-platform#400

Merged

pkulkark mentioned this pull request May 20, 2022

[BB-6046] Prepare opencraft-release/maple.3 branch for edx-platform open-craft/edx-platform#471

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SE-4235] Add missing parameter processors code #150

[SE-4235] Add missing parameter processors code #150

pkulkark commented Mar 24, 2021 •

edited

Loading

openedx-webhooks commented Mar 24, 2021

coveralls commented Mar 24, 2021 •

edited

Loading

natabene commented Mar 24, 2021

pkulkark commented Mar 26, 2021

giovannicimolin left a comment

pkulkark commented May 26, 2021

giovannicimolin commented May 26, 2021

pkulkark commented May 26, 2021

giovannicimolin left a comment

nedbat commented Jun 11, 2021

pkulkark commented Jun 14, 2021

giovannicimolin commented Aug 6, 2021

giovannicimolin commented Aug 6, 2021

louis-s-29 commented Aug 12, 2021

pkulkark commented Aug 12, 2021

giovannicimolin commented Aug 13, 2021

[SE-4235] Add missing parameter processors code #150

[SE-4235] Add missing parameter processors code #150

Conversation

pkulkark commented Mar 24, 2021 • edited Loading

openedx-webhooks commented Mar 24, 2021

coveralls commented Mar 24, 2021 • edited Loading

natabene commented Mar 24, 2021

pkulkark commented Mar 26, 2021

giovannicimolin left a comment

Choose a reason for hiding this comment

pkulkark commented May 26, 2021

giovannicimolin commented May 26, 2021

pkulkark commented May 26, 2021

giovannicimolin left a comment

Choose a reason for hiding this comment

nedbat commented Jun 11, 2021

pkulkark commented Jun 14, 2021

giovannicimolin commented Aug 6, 2021

giovannicimolin commented Aug 6, 2021

louis-s-29 commented Aug 12, 2021

pkulkark commented Aug 12, 2021

giovannicimolin commented Aug 13, 2021

pkulkark commented Mar 24, 2021 •

edited

Loading

coveralls commented Mar 24, 2021 •

edited

Loading