fix: Course blocks API with param return_type=list - #34424
Conversation
|
Thanks for the pull request, @qasimgulzar! 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. |
9c359e1 to
e24ab0f
Compare
7927443 to
6d567ba
Compare
… discussion is enabled
6d567ba to
73a6958
Compare
|
This PR is now ready for review. Cc @vunguyen-dmt who reported the initial issue #34379 |
|
@openedx/2u-aperture could you please help reviewing this PR? |
|
Hi @qasimgulzar, This particular bit of code lies outside of Aperture's ownership/maintenance load, nor does the team have any experience or knowledge of the functionality in the I believe you might be looking for folks from the Aurora team at 2U, who look to be maintainers of this functionality (sorry for any confusion!) |
|
@justinhynes thank you very much for you response, let me try to find someone relevant to this PR. |
feanil
left a comment
There was a problem hiding this comment.
Generally looks good to me other than this, @qasimgulzar do you want to put the imports back as they were? I think the rest of this looks good to land.
| from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider | ||
| from xmodule.modulestore.tests.django_utils import \ | ||
| SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order | ||
| from xmodule.modulestore.tests.factories import ( # lint-amnesty, pylint: disable=wrong-import-order | ||
| BlockFactory, | ||
| ToyCourseFactory |
There was a problem hiding this comment.
This seems like a weird formatting change, why remove the parens on the first entry and add them to the last one?
There was a problem hiding this comment.
@feanil I had run iSort on this file but yes it seems weird with / I have manually updated the line and added parentheses. Other than this there were few unused imports do you want me to put those back as well?
There was a problem hiding this comment.
I think dropping the unused imports is great. This is fine to leave in as-is if this is just weird isort behavior.
|
@qasimgulzar 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Discussion: edx (New).
Issue Link
Endpoint: {LMS_HOST}/api/courses/v1/blocks/?course_id={COURSE_ID}&return_type=list
Root cause
When
return_typeis set to list in querystring the api endpoint is crashing because it is callingdictmethod onReturnListtype instance to get blocks.Solution
I have added a type check before fetching blocks if it is getting
ReturnListI am treating it as list. otherwise following the old path.Stacktrace