refactor: Move CourseEditLTIFieldsEnabledFlag to xblock-lti-consumer [BD-38] [TNL-8104] #27529
Conversation
openedx-webhooks
added
blended
|
Thanks for the pull request, @xitij2000! I've created BLENDED-837 to keep track of it in Jira. When this pull request is ready, tag your edX technical lead. |
common/djangoapps/student/migrations/0043_remove_userprofile_allow_certificate.py
Outdated
Show resolved
Hide resolved
requirements/edx/base.in
Outdated
| @@ -107,7 +107,6 @@ icalendar # .ics generator, used by calendar_sync | |||
| ipaddress # Ip network support for Embargo feature | |||
| jsonfield2 # Django model field for validated JSON; used in several apps | |||
| laboratory # Library for testing that code refactors/infrastructure changes produce identical results | |||
| lti-consumer-xblock | |||
There was a problem hiding this comment.
Can we add a TODO to port that back in after the other PR is merged?
| # Since this model only needs to be moved, we just want to | ||
| # indicate to Django that the model is no longer here without | ||
| # actually deleting any data from the database. | ||
| migrations.SeparateDatabaseAndState( |
There was a problem hiding this comment.
I am not entirely sure what's happening here in this migration. Can you provide some details?
There was a problem hiding this comment.
Django migrations work on two levels. One is the internal state/representation of the models, and the other is what is applied to the database.
For instance, when you remove a field and generate migrations, Django will go through all previous migrations and build an internal representation of what the model should be; then it will look at the current state of the model, and see that a field was removed. This will generate a RemoveField operation in the next migration file. This will cause Django to update its internal representation of the model, so it won't ask you to generate the migration again and again. When you run the migration it will also apply this change to the database.
What the SeparateDatabaseAndState operation does is to separate the action Django performs in python and the action it performs in the database.
Here simply deleting the model will also delete the data in the database, but we don't want that, we want to keep the data in the database, but still indicate to Django that this model is no more. So here we are telling it to delete the model from the django state, but not apply that to the database, which is why the delete operation is applied in state_operations but no in database_operations.
xitij2000
changed the title
There was a problem hiding this comment.
@xitij2000, I've tested this and it works well. I've left some comments on both PRs. Also, could you please add the reason for this change to this PR? I see that the other one contains "This allows using the flag from both lms and studio." explanation. It would be good to see it here, too (in the description and in the commit) so that we will have this preserved once it's merged.
common/djangoapps/student/migrations/0043_remove_userprofile_allow_certificate.py
Outdated
Show resolved
Hide resolved
xitij2000
force-pushed
the
kshitij/bb-3899/pii-flag
branch
2 times, most recently
from
cb69149
to
7c7b2d8
Compare
There was a problem hiding this comment.
👍
- I tested this: tested that this works as expected locally
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: n/a
- I made sure any change in configuration variables is reflected in the corresponding client's
configuration-securerepository: n/a
xitij2000
changed the title
|
📣 💥 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: 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). |
xitij2000
force-pushed
the
kshitij/bb-3899/pii-flag
branch
5 times, most recently
from
0a5ce7e
to
e7ff30b
Compare
xitij2000
force-pushed
the
kshitij/bb-3899/pii-flag
branch
from
e7ff30b
to
c12ed45
Compare
|
Your PR has finished running tests. There were no failures. |
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production. |
|
EdX Release Notice: This PR has been deployed to the production environment. |
This moves the CourseEditLTIFieldsEnabledFlag to xblock-lti-consumer. An intended side-effect of this is that this model is now available in the LMS in additon to Studio.
The other part of this refactor is in this PR: openedx/xblock-lti-consumer#159
This PR should include a version bump of xblock-lti-consumer that includes the change from that PR, but that requires that PR to merge first.
It partially addresses EDUCATOR-121.
JIRA tickets: EDUCATOR-121
Dependencies: openedx/xblock-lti-consumer#159
Sandbox URL: TBD - sandbox is being provisioned.
Merge deadline: "None"
Testing instructions:
Reviewers
Settings