feat: Allow optionally passing PII in course LTI tab [BD-38] [BB-3899] [TNL-8104] by xitij2000 · Pull Request #26982 · openedx/openedx-platform

xitij2000 · 2021-03-12T12:34:10Z

Adds the ability to pass PII to an LTI provider in the LTI course tab if the LtiConfiguration associated with that tab is configured to allow that.

Jira task: https://openedx.atlassian.net/browse/TNL-8104

Dependencies: None

Sandbox URL:

LMS: https://pr26982.sandbox.opencraft.hosting/
Studio: https://studio.pr26982.sandbox.opencraft.hosting/

Merge deadline: "None"

Testing instructions:

Set up a new course with an LTI-based discussion provider.
In the LtiConfiguration for this provider set the lti_config to:
{"ask_to_send_username":true,"ask_to_send_email":true}
Check and see if it's passing the data to the LTI provider.

Reviewers

(OpenCraft internal reviewer's GitHub username goes here)
edX reviewer[s] TBD

openedx-webhooks · 2021-03-12T12:34:15Z

Thanks for the pull request, @xitij2000! I've created BLENDED-792 to keep track of it in Jira.

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

davidjoy · 2021-03-16T16:42:35Z

Are ask_to_send_username and ask_to_send_email two new fields we're adding to LtiConfiguration? I'm wondering if we want these to be phrased more as authorizations or allowances. can_send_username or something like that, perhaps? I'm not sure if there's precedent for a naming scheme for this elsewhere in the codebase.

@davidjoy These are the terms used by the current LTI XBLock. I was going for familiarity here, but since they have different UIs perhaps it isn't that important. I will change this to what you suggested.

Yeah, I'm not a huge fan of the ask_to_ prefix; I initially thought this would prompt the user for consent.

That said, it may be even worse to refer to the variable by multiple different names.

Given the original key has existed in the xblock for >5 years now, I think I'd be begrudgingly inclined to leave it as-is 🤷

@stvstnfrd The field name is internal to the XBlock, and the display name is "Request user's username" the XBlock does actually ask for permission before sending this data.

Perhaps we can use more accurate terminology internally while using familiar names in the UI?

What's the current stance on naming these variables?

The current internal naming is to be consistent with the current XBlock field names. This might not be the best idea since these are used differently here.
Perhaps the names should drop the ask and just be send_username and send_email.

sure, that would clean up a little.

stvstnfrd · 2021-03-17T06:42:01Z

Yeah, I'm not a huge fan of the ask_to_ prefix; I initially thought this would prompt the user for consent.

That said, it may be even worse to refer to the variable by multiple different names.

Given the original key has existed in the xblock for >5 years now, I think I'd be begrudgingly inclined to leave it as-is 🤷

marcotuts · 2021-04-30T16:06:55Z

What is the next step on this PR?

marcotuts · 2021-04-30T16:08:25Z

jenkins run quality

xitij2000 · 2021-05-03T06:33:52Z

@marcotuts While this was being worked on I created an ADR to decide how to handle the flag situation since it wasn't straightforward. Since that ADR was approved, I will have a task this sprint to implement the suggestions from the ADR, and use the flag the way it's described in the ADR

arch-bom-gocd-alerts · 2021-06-11T18:02:34Z

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 kshitij/lti-pii-toggle && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

awaisdar001

Just a few nits -- otherwise this PR looks good to me.

awaisdar001 · 2021-06-15T07:53:22Z

What's the current stance on naming these variables?

awaisdar001

@xitij2000 This PR seems outdated or blocked on some other PR.
The reason I say that is because there is no model "CourseAllowPIISharingInLTIFlag" in specified import location (edx:master). at the moment.

Thoughts?

xitij2000 · 2021-06-25T11:31:23Z

@awaisdar001 Yes, it is currently blocked on this PR: openedx/xblock-lti-consumer#172

That needs to be updated to add the new changelog, bump up the version to 3 etc. Then this PR needs to be updated to use new new flag, remove the prefixes etc.

I don't have time in the current sprint, but will look into next sprint starting next tuesday.

xitij2000 · 2021-07-01T05:20:36Z

jenkins run all

AhtishamShahid

There is lti-consumer-xblock dependency at this line. https://github.com/edx/edx-platform/blob/36319bda486394f049f006898c5a920a09211c33/requirements/edx/base.in#L112
And changes in .txt files for dependency seem to be pointing towards the branch, normally we use tags for that.

xitij2000 · 2021-07-05T13:18:44Z

There is lti-consumer-xblock dependency at this line.

https://github.com/edx/edx-platform/blob/36319bda486394f049f006898c5a920a09211c33/requirements/edx/base.in#L112

And changes in .txt files for dependency seem to be pointing towards the branch, normally we use tags for that.

This is a temporary change. Currenly the version of xblock-lti-consumer this needs hasn't been merged or tagged yet: openedx/xblock-lti-consumer#172

So while that PR is still unmerged, and the release is untagged, this PR needs to use the branch name. Once a release is tagged, I'll run make upgrade and commit that.

saadyousafarbi

Have added some comments!
If this is ready, can we have a sandbox for this so we can test the functionality thoroughly before moving forward!
Thank you!

saadyousafarbi · 2021-07-05T13:15:27Z

are there any other fields other than pii_share_username and pii_share_email starting with pii_ for this check?

Nope, but the idea is that if there are any in the future, this will cover it.

awaisdar001

Looks good, but updatexblock-lti-consumer requirement before merging.

xitij2000 · 2021-07-07T10:03:33Z

Looks good, but updatexblock-lti-consumer requirement before merging.

I've manually updated the dependency.

awaisdar001 · 2021-07-08T06:41:18Z

jenkins run all

asadazam93 · 2021-07-08T12:29:09Z

@xitij2000 the checks are failing here. Can you take a look?

awaisdar001 · 2021-07-08T12:30:25Z

@xitij2000 the checks are failing here. Can you take a look?

Thats because pypi is not updated to 3.0.0 :(
I have filed an SRE ticket for that

saadyousafarbi · 2021-07-08T12:32:01Z

@xitij2000 please share the sandbox for this PR once the build pipelines are fixed and test pass! Thank you.

awaisdar001 · 2021-07-08T16:06:32Z

jenkins run all

edx-status-bot · 2021-07-08T16:50:33Z

Your PR has finished running tests. There were no failures.

asadazam93 · 2021-07-09T06:53:48Z

@xitij2000 is this PR ready to merge? Should I merge it?

xitij2000 · 2021-07-09T07:58:02Z

@asadazam93 Yes. It's good to go for my side.

edx-pipeline-bot · 2021-07-09T14:23:14Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2021-07-09T14:57:39Z

EdX Release Notice: This PR has been deployed to the production environment.

This reverts commit 666a50d

…26982)"" This reverts commit 2b70093

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Mar 12, 2021

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from 11433cd to bfdaba8 Compare March 16, 2021 08:28

xitij2000 changed the title ~~[BD-03] Allow optionally passing PII in course LTI tab~~ [BD-03] [BB-3899] Allow optionally passing PII in course LTI tab Mar 16, 2021

davidjoy reviewed Mar 16, 2021

View reviewed changes

Comment thread openedx/features/lti_course_tab/tab.py Outdated

davidjoy reviewed Mar 16, 2021

View reviewed changes

stvstnfrd reviewed Mar 17, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from 8337f57 to 59ded7b Compare March 17, 2021 09:51

Agrendalath requested changes Apr 15, 2021

View reviewed changes

Comment thread openedx/features/lti_course_tab/tab.py Outdated

xitij2000 changed the title ~~[BD-03] [BB-3899] Allow optionally passing PII in course LTI tab~~ feat: Allow optionally passing PII in course LTI tab [BD-38] [BB-3899] May 21, 2021

xitij2000 changed the title ~~feat: Allow optionally passing PII in course LTI tab [BD-38] [BB-3899]~~ feat: Allow optionally passing PII in course LTI tab [BD-38] [BB-3899] [TNL-8104] Jun 10, 2021

awaisdar001 reviewed Jun 15, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from 33379b9 to dad2973 Compare June 16, 2021 11:29

awaisdar001 suggested changes Jun 25, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/lti-pii-toggle branch 2 times, most recently from 1b41608 to 26bbacc Compare July 1, 2021 04:05

awaisdar001 assigned awaisdar001, AhtishamShahid and saadyousafarbi Jul 5, 2021

AhtishamShahid requested changes Jul 5, 2021

View reviewed changes

saadyousafarbi reviewed Jul 5, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from 31272d2 to 9d2a087 Compare July 6, 2021 06:25

xitij2000 force-pushed the kshitij/lti-pii-toggle branch 3 times, most recently from ae6113b to 3708721 Compare July 6, 2021 12:55

xitij2000 mentioned this pull request Jul 6, 2021

refactor: Rename CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag and use it for LTI1.3 [BD-38] [BB-3899] [TNL-8104] openedx/xblock-lti-consumer#172

Merged

awaisdar001 approved these changes Jul 7, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from 3708721 to d989432 Compare July 7, 2021 10:03

feat: allow optionally passing PII in course LTI tab

68321a0

xitij2000 force-pushed the kshitij/lti-pii-toggle branch from d989432 to 68321a0 Compare July 8, 2021 14:07

asadazam93 merged commit 666a50d into openedx:master Jul 9, 2021

openedx-webhooks added merged and removed needs triage labels Jul 9, 2021

robrap added a commit that referenced this pull request Jul 9, 2021

Revert "feat: allow optionally passing PII in course LTI tab (#26982)"

2b70093

This reverts commit 666a50d

robrap added a commit that referenced this pull request Jul 9, 2021

Revert "Revert "feat: allow optionally passing PII in course LTI tab (#…

516fd8a

…26982)"" This reverts commit 2b70093

Agrendalath deleted the kshitij/lti-pii-toggle branch July 13, 2021 19:46

Agrendalath mentioned this pull request Mar 31, 2022

[SE-4860] feat: backport Celery upgrade open-craft/openedx-platform#464

Merged

Uh oh!

Conversation

xitij2000 commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marcotuts commented Apr 30, 2021

Uh oh!

marcotuts commented Apr 30, 2021

Uh oh!

xitij2000 commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arch-bom-gocd-alerts commented Jun 11, 2021

Uh oh!

awaisdar001 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awaisdar001 left a comment

Choose a reason for hiding this comment

Uh oh!

xitij2000 commented Jun 25, 2021

Uh oh!

xitij2000 commented Jul 1, 2021

Uh oh!

AhtishamShahid left a comment

Choose a reason for hiding this comment

Uh oh!

xitij2000 commented Jul 5, 2021

Uh oh!

saadyousafarbi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awaisdar001 left a comment

Choose a reason for hiding this comment

Uh oh!

xitij2000 commented Jul 7, 2021

Uh oh!

awaisdar001 commented Jul 8, 2021

Uh oh!

asadazam93 commented Jul 8, 2021

Uh oh!

awaisdar001 commented Jul 8, 2021

Uh oh!

saadyousafarbi commented Jul 8, 2021

Uh oh!

xitij2000 commented Mar 12, 2021 •

edited

Loading

openedx-webhooks commented Mar 12, 2021 •

edited

Loading

xitij2000 commented May 3, 2021 •

edited

Loading