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

Tree viewset for retrieving nested, paginated views of topic trees #8138

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jun 2, 2021

Summary

  • To accommodate the nested, paginated topic tree display as outlined in designs for this feature: Implement categorical and primary metadata search kolibri-ecosystem#7 this PR creates an API endpoint that allows for subsections of a topic tree to be queried in a single request, with the parent node, the paginated children, and the paginated grand children.
  • The children and the children of each child are returned as a pagination object, containing the results Array, and more which is either null or an object with the parameters that can be passed to the same endpoint in order to query for more results.
  • This PR also updates the ContentNodeResource to allow for simple access to the tree endpoint without having to define a new resource, and with a simplified caching mechanism that works across the TreeViewset and ContentNodeViewset requests.

References

Fixes #8108

Contributes to the solution for #7525

I did a dummy implementation using the current topics page as a PoC of using this API, an animation is seen here, but I did not commit the changes for this:
viewmore

Reviewer guidance

Does the API and its tests make sense?
Are the changes to the ContentNodeResource a good model for handling read only resources in the future?


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jun 2, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Jun 2, 2021
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Just a couple requests for comments in the contentNode.js otherwise it looks good to me.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

EDIT: - With this branch off of this PR's branch checked out I saw completely solid response times and payloads 👍


I'm also getting 30+ second loading times and every payload coming back is precisely 16.7MB.

I can't even view the response from the server because it freezes the browser tab.

image

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Just a couple of minor details. Overall it looks great

kolibri/core/content/api.py Show resolved Hide resolved
kolibri/core/content/test/test_content_app.py Show resolved Hide resolved
@rtibbles rtibbles dismissed nucleogenesis’s stale review June 15, 2021 15:40

Comments addressed.

@rtibbles rtibbles force-pushed the a_cow_a_cloud_a_tree_milton_keynes branch from 8d56499 to 5b2de43 Compare June 15, 2021 23:23
Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Thank you, I see it good to go.

@rtibbles rtibbles merged commit 0c00b9c into learningequality:develop Jun 21, 2021
@rtibbles rtibbles deleted the a_cow_a_cloud_a_tree_milton_keynes branch June 21, 2021 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create API endpoint(s) for nested, paginated topic tree data retrieval
3 participants