-
Notifications
You must be signed in to change notification settings - Fork 4.2k
refactor: convert module term to block [BD-13]
#31475
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: convert module term to block [BD-13]
#31475
Conversation
|
Thanks for the pull request, @0x29a! When this pull request is ready, tag your edX technical lead. |
fbadbab to
87c7a2c
Compare
ef5305d to
2ece590
Compare
module term to blockmodule term to block
Agrendalath
left a comment
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.
@0x29a, as always, fantastic work! I still need to test some parts (I will continue tomorrow), but I'm posting my review to unblock you. Everything looks reasonable to me.
This PR has to be merged simultaneously with: (...) edx-sga
Fortunately, integration tests are not part of the CI , so this is not a blocker - we can merge mitodl/edx-sga#333 at its own pace.
Some ugettext_lazy-wrapped strings were changed. Should I re-generate *.po files and update translations, or this is done automatically by some regular job, like dependencies upgrades?
These files will be re-generated by the transifex bot. We can remove all translation changes from this PR. Please see this example change and the follow-up update.
requirements/edx/base.txt
Outdated
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.
Just leaving an open comment to remember about changing this:
-
requirements/edx/base.txt -
requirements/edx/development.txt -
requirements/edx/testing.txt
b34c122 to
df30178
Compare
|
Thank you for the thorough review, @Agrendalath! I think I addressed all comments. Triggering the sandbox update now.
Got it, thanks for the explanation. I left file rename changes in |
Agrendalath
left a comment
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.
👍
- I tested this: checked for regressions
- 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
df30178 to
9e26985
Compare
9e26985 to
fc6060c
Compare
|
@ormsbee, just a friendly reminder about this PR. |
ormsbee
left a comment
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.
@Agrendalath, @0x29a : This is fantastic. I'm sorry for not getting back to you earlier. For things like this, feel free to merge without blocking on me–just as long as you coordinate with @jristau1984 on the window. Please do block on me if you're making a breaking change, model change, or database migration.
Thank you.
Also, removed unused `_has_access_xmodule` methid from `lms/djangoapps/courseware/access.py`.
Also, removed `_iter_scorable_xmodules` method from `lms/djangoapps/grades/transformer.py` file.
fc6060c to
e576478
Compare
module term to blockmodule term to block [BD-13]
|
@0x29a 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
This makes me so happy :) Thanks for getting this done! |
|
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. |
Description
Note: for now, the PR depends on and includes #31384, so use this link to see and review the actual diff.This PR is the continuation of our previous effort to unify XBlock naming across the platform. This time, each
moduleoccurrence reviewed and changed toblockif it satisfied these criteria:*.py,*.rst, or*.mdfile.moduletoblockalongsidemodule_id, which can't be renamed as it's the part of the CCX API. Same aboutmodule_dataandmodule_state_key.This regex has been used to filter out the larger part of false-positive occurrences:
Basically, it excludes
modulestoreandfrom xmodule.<something> import <something>from the search results.The following table shows the distribution by files:
item->block, includingcms/djangoapps/contentstore/views/item.pyfile.module_scoreandmodule_completion, as they're used together, andmodule_scoreis the LTI consumer block field. Didn't touchmodule_idbecause of the same reason.StudentModuleand anything related, as this would generate Django migration. 2)StudentModule's fields. Also, removed unused_has_access_xmodulefromaccess.pyfile.module state, orXModulein in the context ofStudentModule.moduleterm is extensively used in the REST API._iter_scorable_xmodulesfromtransformer.pyfile.module_state_key, as it'sStudentModule's field, didn't renamedelete_module, as it's an API part.Supporting information
This PR has to be merged simultaneously with:
Testing instructions
Use the sandbox to perform general testing (courseware, course tabs, activation emails, course authoring, course exporting/importing, etc):
staff@example.com;edx)Questions
Some
ugettext_lazy-wrapped strings were changed. Should I re-generate*.pofiles and update translations, or this is done automatically by some regular job, like dependencies upgrades?Private-ref: BB-6926