refactor: downstream entity links api [FC-0076]#36311
refactor: downstream entity links api [FC-0076]#36311ChrisChV merged 13 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
fcd903e to
901c1d0
Compare
3dcc942 to
7c2f62f
Compare
pomegranited
left a comment
There was a problem hiding this comment.
Works great @navinkarkera , and the renaming makes sense :) One optional nit, but 👍
- I tested this using the test instructions from openedx/frontend-app-authoring#1699
- I read through the code
-
I checked for accessibility issues by using my keyboard to navigate -
Includes documentation -
User-facing strings are extracted for translation
| return Response({ | ||
| 'next': self.page.next_page_number() if self.page.has_next() else None, | ||
| 'previous': self.page.previous_page_number() if self.page.has_previous() else None, | ||
| 'count': self.page.paginator.count, | ||
| 'num_pages': self.page.paginator.num_pages, | ||
| 'current_page': self.page.number, | ||
| 'results': data | ||
| }) |
There was a problem hiding this comment.
nit: can't you just do this?
| return Response({ | |
| 'next': self.page.next_page_number() if self.page.has_next() else None, | |
| 'previous': self.page.previous_page_number() if self.page.has_previous() else None, | |
| 'count': self.page.paginator.count, | |
| 'num_pages': self.page.paginator.num_pages, | |
| 'current_page': self.page.number, | |
| 'results': data | |
| }) | |
| return super().get_paginated_response(data, *args, **kwargs) |
There was a problem hiding this comment.
@pomegranited I wanted to replace next and previous links by page number, but I understand that it was not clear. Updated here: 77cac91
There was a problem hiding this comment.
Huh ok.. that makes the returned data non-standard for a paginated DRF view, but your update at least makes this clearer.
There was a problem hiding this comment.
I wanted to make it work in useInfiniteQuery which expects next or previous page number instead of links.
There was a problem hiding this comment.
To keep the standard, maybe instead of replacing next and previous, you can add the page numbers as new values: next_num_page and previous_num_page. What do you think?
7c2f62f to
77cac91
Compare
ChrisChV
left a comment
There was a problem hiding this comment.
Looks good! With some nits
77cac91 to
b99e1dc
Compare
b99e1dc to
a4af946
Compare
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
Thanks! |
Refactors downstream links API to handle multiple filters using a single API. Also adds a new route to return summary of library links for a given course.
Refactors downstream links API to handle multiple filters using a single API. Also adds a new route to return summary of library links for a given course.
Refactors downstream links API to handle multiple filters using a single API. Also adds a new route to return summary of library links for a given course.
Refactors downstream links API to handle multiple filters using a single API. Also adds a new route to return summary of library links for a given course.
Description
Refactors downstream links API to handle multiple filters using a single API. Also adds a new route to return summary of library links for a given course.
Useful information to include:
Supporting information
Private-ref: FAL-4007Also addresses following comments
Testing instructions
See openedx/frontend-app-authoring#1699 for instructions
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.