-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[BD-13][BB-6927] Unify xblock naming descriptor #31492
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
[BD-13][BB-6927] Unify xblock naming descriptor #31492
Conversation
|
Thanks for the pull request, @pkulkark! When this pull request is ready, tag your edX technical lead. |
687bbab to
c4a958f
Compare
openedx/core/djangoapps/content/course_overviews/tests/test_course_overviews.py
Outdated
Show resolved
Hide resolved
e990fae to
3b0d4d8
Compare
da81b5f to
1eced71
Compare
8bf474c to
1be8649
Compare
a54a149 to
aa81a75
Compare
aa81a75 to
3d8ee15
Compare
|
@Agrendalath is the plan to rebase this after #31472 is released or would there be value in doing so now? I notice there are some failing unit tests, not sure if that will be addressed by the rebase. |
|
@e0d, both PRs change lots of files, so each rebase on #31472 is complex and time-consuming. We need to keep #31472 up-to-date because it is possible that changes introduced to
Yes, these should be gone after the final rebase. |
3d8ee15 to
e91bd8b
Compare
|
@ormsbee, I plan to finish my last pass of review and fixes tomorrow. Most of the diffs are trivial, so to simplify things I would like to get your opinion on changes in the following files:
If you would also be able to review a small refactor in openedx/completion#231 (related to this PR), it would be perfect. |
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: did regression checks 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
|
@e0d, this is good to go from my end. Edit: I'll autosquash the commits before merging this. |
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: I reviewed the changes in the files you mentioned, and that looks good to me. Also reviewed and merged completion API PR.
|
Thank you, @ormsbee! If you could create a release from the tag for the completion PR, that would be all we need to merge this and close this project. |
|
Weird, I did tag it, but I don't see it properly showing up on PyPI. I wonder if I messed that up somehow? I'll take a look. |
|
@ormsbee the release workflow triggers on publish so you will need to use the GH releases UI rather than just creating a tag. Fortunately, you can make a GH release from an existing tag. |
|
Thanks @kdmccormick! |
808f867 to
bc0f796
Compare
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
…_task Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
Co-authored-by: Agrendalath <piotr@surowiec.it>
bc0f796 to
e1fb866
Compare
|
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. |
1 similar comment
|
EdX Release Notice: This PR has been deployed to the production environment. |
Description
Note: This PR depends on and includes the changes from #31475, which in turn depends on #31384. Use this link to see the actual diff.This PR is the continuation of our previous effort to unify XBlock naming across the platform. This time, each
descriptoroccurrence reviewed and changed toblockif it satisfied these criteria:*.py,*.rst, or*.mdfile.Supporting Information
This PR should be merged along with:[BD-13][BB-6927] Rename descriptor to block xblock-lti-consumer#322 (closed, not needed)Testing Instructions
Use the sandbox to perform general testing (courseware, course tabs, activation emails, course authoring, course exporting/importing, etc):
staff@example.com;edx)