Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lms/djangoapps/course_api/blocks/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ def test_filter_discussion_xblocks(self, is_openedx_provider, return_type):
def blocks_has_discussion_xblock(blocks):
if isinstance(blocks, ReturnList):
for value in blocks:
if value.get('type') == 'discussion':
if value['type'] == 'discussion':

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why you are making this change? I understand that the new version will throw an exception, but wanted to understand your thinking.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is not needed. Reverted back to avoid confusion.

return True
else:
for key, value in blocks.items():
Expand Down
38 changes: 25 additions & 13 deletions lms/djangoapps/course_api/blocks/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ def filter_discussion_xblocks_from_response(response, course_key):
]
# Filtering discussion xblocks keys from blocks
if isinstance(response.data, ReturnList):
filtered_blocks = {
value.get('id'): value
filtered_blocks = [
value
for value in response.data
if value.get('type') != 'discussion'
}
]
else:
filtered_blocks = {
key: value
Expand All @@ -41,17 +41,29 @@ def filter_discussion_xblocks_from_response(response, course_key):
}
# Removing reference of discussion xblocks from unit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this comment still relevant as it explains why this is being done?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this comment can still be useful, I re-added it.

# These references needs to be removed because they no longer exist
for _, block_data in filtered_blocks.items():
for key in ['descendants', 'children']:
descendants = block_data.get(key, [])
if descendants:
descendants = [
descendant for descendant in descendants
if descendant not in discussion_xblocks
]
block_data[key] = descendants
if isinstance(response.data, ReturnList):
response.data = filtered_blocks
for block_data in filtered_blocks:
_put_xblock_descendants(block_data, discussion_xblocks)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very unclear way to update objects in list.
Perhaps it would be better to check isinstance(response.data, ReturnList) and, based on that, simply choose whether to iterate over filtered_blocks or filtered_blocks.items() without moving the xblock descendants logic into a separate function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this refactoring

else:
for _, block_data in filtered_blocks.items():
_put_xblock_descendants(block_data, discussion_xblocks)

if isinstance(response.data, ReturnList):
response.data = ReturnList(filtered_blocks, serializer=None)
else:
response.data['blocks'] = filtered_blocks
return response


def _put_xblock_descendants(block_data, discussion_xblocks):
"""
Put descendants into xblock data if they existed before filtering.
"""
for key in ['descendants', 'children']:
descendants = block_data.get(key, [])
if descendants:
descendants = [
descendant for descendant in descendants
if descendant not in discussion_xblocks
]
block_data[key] = descendants
4 changes: 3 additions & 1 deletion lms/djangoapps/course_api/blocks/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,12 +330,14 @@ def list(self, request, hide_access_denials=False): # pylint: disable=arguments

if course_block.get('type') == 'course':
root = course_block['id']
else:
root = str(course_usage_key)
else:
root = response.data['root']
course_blocks = response.data['blocks']

if not root:
raise ValueError(f"Unable to find course block in {course_key_string}")
raise ValidationError(f"Unable to find course block in '{course_key_string}'")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use ValueError?

  • ValueError is a low-level Python exception, used for bad internal logic or type coercion.

  • Raising it in an API view will usually result in an unhandled 500 error unless you catch it manually.

  • It does not generate a proper HTTP response, which violates RESTful API principles.

So, raise ValueError in Django API view - it is bad idea. We can raise ValidationError and receive 400 status with readable error massage on frontend side instead of 500 status code on server.


recurse_mark_complete(root, course_blocks)
return response
Expand Down