-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: [AXIMST-416] add sequence_ids to container response #2498
Conversation
cms/djangoapps/contentstore/utils.py
Outdated
@@ -1948,6 +1976,7 @@ def get_container_handler_context(request, usage_key, course, xblock): # pylint | |||
# Status of the user's clipboard, exactly as would be returned from the "GET clipboard" REST API. | |||
'user_clipboard': user_clipboard, | |||
'is_fullwidth_content': is_library_xblock, | |||
'sequence_ids': sequence_ids, |
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.
[nit] rename to aggregated_sequence_ids
everywhere
Because of we need whole sequences in course, not just depending to this unit
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.
Agree, the name can be more specific like suggested
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.
Agree on renaming, I would go with course_sequence_ids
cms/djangoapps/contentstore/utils.py
Outdated
|
||
course_structure = _course_outline_json(request, course) |
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.
[nit] I think we need to move computing of sequence_ids
to cms/djangoapps/contentstore/rest_api/v1/views/vertical_block.py
We don't need to have sequence_ids
in the shared utils context.
But definition get_sequence_usage_keys
leave here.
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 would keep it inside the util function in order to have data collection incapsulated
@monteri please pay attention that some quality checks were failed |
cms/djangoapps/contentstore/utils.py
Outdated
|
||
course_structure = _course_outline_json(request, course) |
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.
Instead of creating JSON representation of a course, with all additional keys for each xblock, we should work directly with course
object to save computational resources and make it faster.
Please see example for iterating over the course's sections (chapters) https://github.com/openedx/edx-platform/blob/bca34c0993aa7004b86cbf318ea55fd3a0ca5d95/cms/djangoapps/contentstore/utils.py#L1005, so you can collect subsections (sequence) IDs
cms/djangoapps/contentstore/utils.py
Outdated
sections = course.get_children() | ||
for section in sections: | ||
subsections = section.get_children() | ||
for subsection in subsections: |
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.
Can we use list comprehensions here, everything else is good!
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.
LGTM
* feat: add sequence_ids to container response * fix: PR comments fix * fix: update --------- Co-authored-by: monteri <lansevermore>
* feat: add sequence_ids to container response * fix: PR comments fix * fix: update --------- Co-authored-by: monteri <lansevermore>
Adding
sequence_ids
to the response