Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
88 changes: 48 additions & 40 deletions lms/djangoapps/course_api/blocks/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,47 +11,55 @@

def filter_discussion_xblocks_from_response(response, course_key):
"""
Removes discussion xblocks if discussion provider is openedx
Removes discussion xblocks if discussion provider is openedx.
"""
configuration = DiscussionsConfiguration.get(context_key=course_key)
provider = configuration.provider_type
if provider == Provider.OPEN_EDX:
# Finding ids of discussion xblocks
if isinstance(response.data, ReturnList):
discussion_xblocks = [
value.get('id') for value in response.data if value.get('type') == 'discussion'
]
else:
discussion_xblocks = [
key for key, value in response.data.get('blocks', {}).items()
if value.get('type') == 'discussion'
]
# Filtering discussion xblocks keys from blocks
if isinstance(response.data, ReturnList):
filtered_blocks = {
value.get('id'): value
for value in response.data
if value.get('type') != 'discussion'
}
else:
filtered_blocks = {
key: value
for key, value in response.data.get('blocks', {}).items()
if value.get('type') != 'discussion'
}
# Removing reference of discussion xblocks from unit
Copy link
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
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
else:
response.data['blocks'] = filtered_blocks

if provider != Provider.OPEN_EDX:
return response

is_list_response = isinstance(response.data, ReturnList)

# Find discussion xblock IDs
if is_list_response:
Copy link
Contributor

Choose a reason for hiding this comment

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

[Nit, for the future, not necessary for merge]: There's so much logic branched off of is_list_response conditionals in this function that it probably makes sense to make separate methods for the list and dict versions of this call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Noted for the future, thank you!

discussion_xblocks = [
block.get('id') for block in response.data
if block.get('type') == 'discussion'
]
else:
discussion_xblocks = [
key for key, value in response.data.get('blocks', {}).items()
if value.get('type') == 'discussion'
]

# Filter out discussion blocks
if is_list_response:
filtered_blocks = [
block for block in response.data
if block.get('type') != 'discussion'
]
else:
filtered_blocks = {
key: value for key, value in response.data.get('blocks', {}).items()
if value.get('type') != 'discussion'
}

# Remove references to discussion xblocks
# These references needs to be removed because they no longer exist
blocks_iterable = filtered_blocks if is_list_response else filtered_blocks.values()
for block_data in blocks_iterable:
for key in ['descendants', 'children']:
if key in block_data:
block_data[key] = [
descendant for descendant in block_data[key]
if descendant not in discussion_xblocks
]

# Update response
if is_list_response:
response.data = ReturnList(filtered_blocks, serializer=None)
else:
response.data['blocks'] = filtered_blocks

return response
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
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
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
Loading