Limit lists to a certain number of entities #2961
Conversation
richard-cox
commented
Sep 6, 2018
•
richard-cox
commented
- Allow a paginated action to define a maximum number of entities it should fetch. If this is exceeded do not depaginate response (fetch all pages in one go)
- In the list component show a custom warning message when there are too many results
- Allow user to reduce results by filtering server side
- Wire in above to app wall
- To test update GetAllApplications 'flattenPaginationMax' to 10
- fixes Scalability: Application Wall #2895
- Allow max results value to be stored and depagination aborted if exceeded - Show custom warning message when there are too many results - Allow user to reduce results by filtering - Wire in above to app wall - To test update GetAllApplications 'flattenPaginationMax' to 10
|
Hey richard-cox! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
Codecov Report
@@ Coverage Diff @@
## v2-master #2961 +/- ##
=============================================
+ Coverage 70.74% 70.87% +0.13%
=============================================
Files 644 653 +9
Lines 28485 28862 +377
Branches 6464 6564 +100
=============================================
+ Hits 20152 20457 +305
- Misses 8333 8405 +72 |
- Improve efficiency when setting cf/org/space params - Fixed some typos, removed unrequired params
KlapTrap
added
needs attention
|
Bug:
|
- Previous solution was to avoid app requests when we knew we already had them all (in org with all 50 apps --> select space in org --> avoid fetching all apps in space and use previous list - There were many issues with this approach - Fall back on making a new app request whenever the filter changes
richard-cox
added
ready for review
and removed
needs attention
|
@richard-cox can you merge master into this PR? I'll review once that's done. |
- Bump 'maxedResults` observable down from list to data source - Bump dependend sort and filter observables down as well
- Don't fire reset action.. which previously kicked off new fetch list request - Now fetch list request is kicked off via filter change (only for maxed case) - Ensure we set q params correctly when using SetParams - Don't update the state via obj ref (see `newParams.q = qChanges`) - Set should replace all of q (including removing any old values) - NOTE - This does not solve the issue where the list is removed and shown again on filter change - This occurrs because the page$ observable executes the filters on the stale page list before executing again on proper page list - There's no clear way to block this without getting hacky (pagination section has not marked anything as busy)
|
@KlapTrap I've fixed points 1-3. Point 4 I've addressed (it now doesn't clear the list) however there's still a visible blip when changing a filter. This is due to the page$ observable returning an empty list.. due to the filter pipeline executing using the new filter values on the old page. I could fix this but it starts to get hacky (having something in the pagination object to block the page$ observable until cleared by a successful call?). Point 5 might be quite tricky to implement. I think at this stage, given how old this PR is and the amount of stacked PRs on top, we capture this as something to address separately. |
richard-cox
added
ready for review
and removed
needs attention
src/frontend/app/shared/components/list/data-sources-controllers/list-data-source.ts
Outdated
Show resolved
Hide resolved
KlapTrap
added
needs attention
- Always hide filter if maxed results is reached - when we're maxed (and page contents are invalid) don't execute local filtering on them
richard-cox
added
ready for review
and removed
needs attention
There was a problem hiding this comment.
There is another list loading indicator issue (seems related to the previous one):
- Go to the space application list (or any other list).
- Refresh the page to ensure there is no cached data.
- Hit the list refresh button, the top loading indicator shows rather than the refresh button spinning. It works once you've refreshed before or change page etc.
- This doesn't happen on master.
KlapTrap
added
needs attention
|
@KlapTrap Apart from the issue above, is this alright to merge? Just wary that you're away next week |
|
Another issue :'(. On an empty list (in my example is was a application instance list) when you hit refresh the empty list message disappears while the list fetches. I think we need some e2e tests covering these loading combinations (using current master as reference) as they're easy to break and easy to overlook. |
|
If you're happy after fixing these issues, feel free to merge this in while i'm away to unblock all the things. |
|
After playing a fair amount of wackamole I've created #3305 to fix the remaining loading indicator issues. It needs a bit more of a rework and we need to unblock other issues. Once we've got those merged I'll come back and fix. |
