Skip to content
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

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

monteri
Copy link
Collaborator

@monteri monteri commented Jan 25, 2024

Adding sequence_ids to the response

@monteri monteri changed the title feat: [AXIMST-417] add sequence_ids to container response feat: [AXIMST-416] add sequence_ids to container response Jan 25, 2024
@@ -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,
Copy link

@ruzniaievdm ruzniaievdm Jan 25, 2024

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

Copy link
Collaborator

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

Copy link
Collaborator Author

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


course_structure = _course_outline_json(request, course)
Copy link

@ruzniaievdm ruzniaievdm Jan 25, 2024

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.

Copy link
Collaborator Author

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

@ruzniaievdm
Copy link

@monteri please pay attention that some quality checks were failed


course_structure = _course_outline_json(request, course)
Copy link
Collaborator

@GlugovGrGlib GlugovGrGlib Jan 25, 2024

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

sections = course.get_children()
for section in sections:
subsections = section.get_children()
for subsection in subsections:
Copy link
Collaborator

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!

Copy link
Collaborator

@GlugovGrGlib GlugovGrGlib left a comment

Choose a reason for hiding this comment

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

LGTM

@monteri monteri merged this pull request into ts-develop Jan 26, 2024
45 checks passed
monteri added a commit that referenced this pull request Jan 30, 2024
* feat: add sequence_ids to container response

* fix: PR comments fix

* fix: update

---------

Co-authored-by: monteri <lansevermore>
ruzniaievdm pushed a commit that referenced this pull request Feb 22, 2024
* feat: add sequence_ids to container response

* fix: PR comments fix

* fix: update

---------

Co-authored-by: monteri <lansevermore>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants