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
refactor: Rename CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag and use it for LTI1.3 [BD-38] [BB-3899] [TNL-8104] #172
Conversation
Thanks for the pull request, @xitij2000! I've created BLENDED-894 to keep track of it in Jira. When this pull request is ready, tag your edX technical lead. |
a7d8393
to
498bc54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few typos.
@giovannicimolin any concerns? |
@xitij2000 Can you please create a TNL ticket for this and add a detailed description? |
@xitij2000 How do you plan to backfill data? |
I'm not sure I follow. What data needs to be backfilled? |
You are changing the name of the model. How do you plan to fill data from the previous model into the new model? |
@asadazam93 Changing the name of a model doesn't do anything to the data. The table is renamed, so the data is retained. In ths case, since the table name is also set, it won't be renamed either. |
4c8d008
to
67160c5
Compare
…ingInLTIFlag and use it for LTI1.3 This commit renames the CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag since the aim is to expand its scope to all LTI-related PII sharing. It also removes the current LTI1.3 waffle flag for PII sharing.
67160c5
to
6fb8679
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 👍
@xitij2000 This PR is still missing a TNL ticket. Can you please create one and add to the description? Let us merge the PR after the ticket is created |
@asadazam93 I've added the ticket number to the PR title. |
Thanks, @xitij2000 can this be merged? |
@awaisdar001 Yes, however https://github.com/edx/edx-platform/pull/26982 will need to be adjusted right after, so I would prefer if we do this during early IST hours. |
@xitij2000 Let us merge both PRs during our meeting tomorrow morning? |
That might not be possible. After this PR is merged, it would still be nice to run the edx-platform tests to make sure nothing is broken. That would take some time. |
we can merge this PR in standup -- and secondly we should get in touch on Slack when the other PR in edx-platform needs to be merged. |
This commit renames the CourseEditLTIFieldsEnabledFlag to CourseAllowPIISharingInLTIFlag since the aim is to expand its scope to all LTI-related PII sharing. It also removes the current LTI1.3 waffle flag for PII sharing.
TNL-8104