Create a runtime service for answer_available function
#33424
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @steff456! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
There was a problem hiding this comment.
@steff456 It is a good start, but I don't think it's quiet it.
First of all, testing instructions should usually provide manual steps which can be performed to test the changes. It can be challenging to do for the refactoring PRs, since the goal is for the behavior to stay the same. I would suggest, creating a few problem blocks through the studio in the demo course, set different ShowAnswer values, and test that all of the behave the same before the refactor and after. Then you can add these steps, so that the review can both - see if the testing steps are sufficient, and try them himself to validate the changes.
Second, is that I can see that David mentioned that it should be refactored into a runtime service:
We should refactor this into a runtime service that will return a simple True/False for this question, so that we can get consistent behavior without other XBlock authors having to think about it.
To be frank, I'm not super familiar with the internals of the XBlocks. However, I know that the way that XBlocks can access services from XBlock runtime is via self.runtime.service(.... I grepped for def service\( and can see it in a few places, but the most notable is in openedx/core/djangoapps/xblock/runtime/runtime.py on line 231. You'll be able to see that this method returns many different services, and my intuition suggests that the new service should be implemented in the similar way as those, and returned from that method. Don't be afraid to let David know on the issue that you're working on it, and you can as well ask him what he thinks about this approach. IIRC, he knows the internals of the XBlocks well.
mphilbrick211
added
needs test run
e0d
added
the
waiting on author
There was a problem hiding this comment.
As David commented here, it seems like these services need to be added in other places as well.
This is looking much closer to the final solution. A few other things are:
- Can you please sign the CLA mentioned in this comment?
- I see that Ed has pushed a commit, probably to trigger the pipelines. As you can see some of the pipelines that ran are failing, including some code linting, commit message linting and some unittests that haven't been updated with the changes. Can you please make sure you address those?
mphilbrick211
added
needs test run
There was a problem hiding this comment.
First of all, I can see that some of the tests are failing. Since the github actions are blocked, you will need to run the tests locally. You can do that by going to the devstack directory, then running make lms-shell and inside the shell you can run pytest -vvv -p no:randomly xmodule to run the tests for xmodule. You should run other tests as well by removing xmodule or changing it to a path , but that might take a lot of time, so do it only once you think that most of the issues have been addressed.
You will at the very least need to add @XBlock.wants('show_correctness') and @XBlock.wants('show_answer') decorators to the XBlock classes that use the new services (only add where appropriate), like ProblemBlock. In other tests the runtime doesn't return the service, so you probably need to add it to the xmodule/tests/__init__.py to get_test_system and prepare_block_runtime functions.. This will fixed most, but not all tests that are failing.
You would also want to run some linters. You can do in the similar way, by calling paver run_pep8 and paver run_pylint it should catch most issues.
Secondly, I think the tests that are testing the new services, but are still located in the xmodule/tests/test_graders.py should be moved to the appropriate xmodule/tests/test_services.py.
The PR description is still missing the testing instructions.
Lastly, can you please rebase your branch onto master instead of merging the master branch into your branch. This should make commit history cleaner. You should make sure that your commit messages follow the conventional commits format. You can also squash your commits into one or several, if more appropriate.
openedx/features/personalized_learner_schedules/show_answer/tests/test_show_answer_override.py
Outdated
Show resolved
Hide resolved
openedx/features/personalized_learner_schedules/show_answer/show_answer_field_override.py
Outdated
Show resolved
Hide resolved
|
@Cup0fCoffee thanks for the detailed review! I already ran locally all the tests and they are working as expected. I also checked the linting, pep8 doesn't flagged any issue, but there's a lot of unrelated linting errors coming from files that I didn't modified in this PR from the pylint check. Also, I already left the testing instructions in the details of this PR and made the rebase. Please let me know if there's anything else I need to check before this PR is ready! |
|
👍
|
mphilbrick211
removed
the
needs test run
mphilbrick211
added
the
needs test run
mphilbrick211
removed
the
needs test run
|
Sandbox deployment started. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting |
|
Sandbox update request received. Deployment will start soon. |
|
Sandbox deployment started. |
|
Sandbox update request received. Deployment will start soon. |
|
@steff456: Please let me know if you're blocked on anything else. Thank you! |
e0d
removed
the
needs test run
|
@steff456 there are some test failures. Can you have a look? I've added you to the triage group, so the tests should run automatically going forward. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5655162132-config.yml |
|
Sandbox deployment failed. Check failure logs here https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5658336475.log Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5658336475-config.yml Please check the settings and requirements and retry deployment by updating the pull request or posting a |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5659087411-config.yml |
|
@e0d thanks for adding me! @ormsbee @Agrendalath I already made the final changes. At the end I moved a lot of functions to the service as requested. Hope this PR is ready for a final review and hopefully approval :)! Let me know if there's anything else as always |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5659180797-config.yml |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5675582081-config.yml |
cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py
Outdated
Show resolved
Hide resolved
|
Hi @ormsbee! Thanks for the review, I just made the changes requested. Please note that the bugs I encountered will yield into failing tests. From what I understood from our previous discussion is that fixing those tests is out of scope for this PR, so please let me know what else is missing for having the approval and merge of this PR. |
|
Sandbox deployment successful. Sandbox LMS is available at pr-33424-139931.staging.do.opencraft.hosting Instance config : https://grove-stage-build-logs.nyc3.digitaloceanspaces.com/34602668-5716644611-config.yml |
|
@ormsbee I hope you had a great time during holidays! I wanted to ping just to know what else is needed for this PR to be approved? |
|
Thank you for the reminder. It looks good. I'll merge on Monday. |
|
Hi @ormsbee I'm pinging again to ask what else is needed from this PR to be approved, thanks! |
|
@steff456: Just squash your commits and you're good. I just cannot merge this and monitor it rolling out this week because of other time commitments. @Cup0fCoffee: If an OpenCraft CC wants to do so, please feel free. FYI to @connorhaugh: this is a refactoring how how capa problems generate various "show answer" statuses (refactor into a service). |
|
@ormsbee Thank you. I'll check with the team. |
|
@ormsbee FYI, Piotr agreed to merge it some time in the next two weeks. |
|
@steff456 @ormsbee @Agrendalath This PR should be closed in favor of #34320, because this PR was open from a fork that doesn't allow us to squash and rebase the commits. |
|
@steff456 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR fixes #33325
This PR,
answer_availableto reduce the logic shown for each XBlock authors.Supporting information
can_view_answer,can_view_correctness#33325Testing instructions
Content > Outlineand search theHomeworktaskshow answerbuttonDeadline
None