Skip to content

[BD-27] [TNL-8046] Add module meta and handler for external_tools links#55

Merged
alangsto merged 3 commits into
openedx:masterfrom
open-craft:arjun/bb-3973-canvas-lti
May 3, 2021
Merged

[BD-27] [TNL-8046] Add module meta and handler for external_tools links#55
alangsto merged 3 commits into
openedx:masterfrom
open-craft:arjun/bb-3973-canvas-lti

Conversation

@arjunsinghy96

@arjunsinghy96 arjunsinghy96 commented Apr 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Adds module meta parser and handler for external_tools
links for canvas flavoured CC. Also adds a parameter ----passport-file to add LTI passports in policy.json

Related Tickets

  1. TNL-8046
  2. BB-3973

Testing Instructions

Testing Canvas External Tools Support

  1. On master branch, Run cc2olx on a Canvas CC with external tools
  2. Import the converted course in studio
  3. Verify that the external tools content is converted to MISSING CONTENT
  4. Checkout to this branch.
  5. Convert the same Canvas CC course again.
  6. Import the new converted course in studio
  7. Verify that the external tools content is now visible. (LTI link might fail to load because of missing consumer key and secret)

Testing --passport-file parameter

  1. Checkout to this branch
  2. Convert a CC course with LTI components.
  3. Verify that policy.json has lti_consumer in advanced_modules
  4. Verify that policy.json has lit_passports with default values for all LTI tools in course.
  5. Create a passport csv file with consumer_id, consumer_key and consumer_secret fields
  6. Convert the CC course again with --passport-file [FILE_PATH] argument.
  7. Verify that policy.json now has values from passport file in lti_passports.

Reviewers

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 3, 2021
@openedx-webhooks

openedx-webhooks commented Apr 3, 2021 •
edited
Loading

Copy link
Copy Markdown

Thanks for the pull request, @arjunsinghy96! I've created BLENDED-817 to keep track of it in Jira. More details are on the BD-27 project page.

When this pull request is ready, tag your edX technical lead.

@arjunsinghy96 arjunsinghy96 force-pushed the arjun/bb-3973-canvas-lti branch from 88e4d21 to b20764d Compare April 3, 2021 12:08
@natabene

natabene commented Apr 5, 2021

Copy link
Copy Markdown

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

@arjunsinghy96 arjunsinghy96 force-pushed the arjun/bb-3973-canvas-lti branch 2 times, most recently from 8ac33c6 to dd47461 Compare April 12, 2021 13:06
@natabene natabene changed the title WIP: [TNL-8046] Add module meta and handler for external_tools links [BD-27] WIP: [TNL-8046] Add module meta and handler for external_tools links Apr 12, 2021
@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program and removed open-source-contribution PR author is not from Axim or 2U labels Apr 12, 2021
@kaizoku

kaizoku commented Apr 19, 2021

Copy link
Copy Markdown
Contributor

This is looking good so far @arjunsinghy96. It's filling in the correct data in generated OLX for the LTI components specified in module_meta.xml and the IMSCC course content. The tests look good too.

Do we still plan to add LTI passport support in this PR, or will that be a separate one?

@arjunsinghy96

arjunsinghy96 commented Apr 19, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the review @kaizoku. 🙂
I believe adding LTI passport support in this PR makes sense as I have changed the olx.policy method to include lti related polices. I will add a method to pull LTI_passports from a csv.

@arjunsinghy96 arjunsinghy96 force-pushed the arjun/bb-3973-canvas-lti branch from 0891025 to 5e0ad58 Compare April 26, 2021 12:28
@arjunsinghy96 arjunsinghy96 changed the title [BD-27] WIP: [TNL-8046] Add module meta and handler for external_tools links [BD-27] [TNL-8046] Add module meta and handler for external_tools links Apr 26, 2021
@openedx-webhooks openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Apr 26, 2021
@arjunsinghy96 arjunsinghy96 force-pushed the arjun/bb-3973-canvas-lti branch from 5e0ad58 to 564209f Compare May 3, 2021 12:42
@arjunsinghy96
Add module meta and handler for external_tools links
68cecd4 
Adds module meta parser and handler for external_tools
links for canvas flavoured cc
@arjunsinghy96
Add lti_id to lti_consumer elements
72dc2ef
@arjunsinghy96
Add cli argument for lti-passport file
da95743 
Adds a cli argumet -p or --passport-file which
can be used to pass lti passports.
Also adds support to process and use the file
@arjunsinghy96 arjunsinghy96 force-pushed the arjun/bb-3973-canvas-lti branch from 564209f to da95743 Compare May 3, 2021 12:43
@arjunsinghy96

arjunsinghy96 commented May 3, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the approval @alangsto 🙂 . I have squashed and cleaned the commits. Feel free to move forward with this.

@alangsto

alangsto commented May 3, 2021

Copy link
Copy Markdown
Contributor

Thanks @arjunsinghy96!

@alangsto alangsto merged commit 5f5ec37 into openedx:master May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@alangsto alangsto alangsto approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@arjunsinghy96 @openedx-webhooks @natabene @kaizoku @alangsto