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

Fix 'show more' links on user profiles not disappearing after loading the last batch of data #1594

Merged
merged 24 commits into from Dec 21, 2017

Conversation

4 participants
@LiquidPL
Contributor

LiquidPL commented Oct 10, 2017

This happened specifically when the length of last fetched data was equal to the amount of elements loaded with each fetch.

@nekodex

This comment has been minimized.

Show comment
Hide comment
@nekodex

nekodex Oct 19, 2017

Collaborator

The queries required to get those counts aren't really optimal and don't really work at scale (which is the main reason why I didn't do this initially)... probably a better idea to fetch n+1 results each time (while only displaying n results) and check that way

Collaborator

nekodex commented Oct 19, 2017

The queries required to get those counts aren't really optimal and don't really work at scale (which is the main reason why I didn't do this initially)... probably a better idea to fetch n+1 results each time (while only displaying n results) and check that way

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Nov 7, 2017

Member

@nekodex what was the consensus on this? i remember you looking into an alternative solution at some point

Member

peppy commented Nov 7, 2017

@nekodex what was the consensus on this? i remember you looking into an alternative solution at some point

@nekodex

This comment has been minimized.

Show comment
Hide comment
@nekodex

nekodex Nov 7, 2017

Collaborator

@peppy Read the comment directly above yours?

Collaborator

nekodex commented Nov 7, 2017

@peppy Read the comment directly above yours?

@peppy

This comment has been minimized.

Show comment
Hide comment
@peppy

peppy Nov 7, 2017

Member

Ok, just checking no work was done elsewhere to resolve this one since my memory was vague.

Member

peppy commented Nov 7, 2017

Ok, just checking no work was done elsewhere to resolve this one since my memory was vague.

LiquidPL added some commits Nov 21, 2017

Rework pagination for scores
Fetches one more element than the amount shown, sets hasMore to true if
amount fetched is larger than perPage
@LiquidPL

This comment has been minimized.

Show comment
Hide comment
@LiquidPL

LiquidPL Nov 21, 2017

Contributor

Reworked this to do the n+1 fetch for scores, as we seem to have accurate counts for everything else.

Contributor

LiquidPL commented Nov 21, 2017

Reworked this to do the n+1 fetch for scores, as we seem to have accurate counts for everything else.

LiquidPL added some commits Nov 22, 2017

@nanaya

This comment has been minimized.

Show comment
Hide comment
@nanaya

nanaya Nov 22, 2017

Collaborator

This probably a bit simpler (not quite tested) https://0paste.com/18191.txt

  • moves responsibility of determining per page to each component in view (already done but not passed to backend for some reason)
  • moves responsibility of figuring out if there are more items to the fetch function (fetch more and cut result here)
  • controller dutifully does what it's being told to do (with exception of allowing one extra maxResult) (not sure if last part is needed)
Collaborator

nanaya commented Nov 22, 2017

This probably a bit simpler (not quite tested) https://0paste.com/18191.txt

  • moves responsibility of determining per page to each component in view (already done but not passed to backend for some reason)
  • moves responsibility of figuring out if there are more items to the fetch function (fetch more and cut result here)
  • controller dutifully does what it's being told to do (with exception of allowing one extra maxResult) (not sure if last part is needed)
@LiquidPL

This comment has been minimized.

Show comment
Hide comment
@LiquidPL

LiquidPL Dec 4, 2017

Contributor

What commit did you base this patch on? I can't really seem to apply it to anything.

Contributor

LiquidPL commented Dec 4, 2017

What commit did you base this patch on? I can't really seem to apply it to anything.

@nanaya

This comment has been minimized.

Show comment
Hide comment
@nanaya

nanaya Dec 5, 2017

Collaborator

master.

Collaborator

nanaya commented Dec 5, 2017

master.

@nanaya

This comment has been minimized.

Show comment
Hide comment
@nanaya

nanaya Dec 8, 2017

Collaborator

Another one unifying per page setting and more per page graveyards (maybe). https://0paste.com/19224.txt

Collaborator

nanaya commented Dec 8, 2017

Another one unifying per page setting and more per page graveyards (maybe). https://0paste.com/19224.txt

LiquidPL added some commits Dec 8, 2017

'favourite' => $this->favouriteBeatmapsets($user),
'unranked' => $this->unrankedBeatmapsets($user),
'graveyard' => $this->graveyardBeatmapsets($user),
'most_played' => $this->mostPlayedBeatmapsets($user, $perPage['beatmapPlaycounts'] + 1),

This comment has been minimized.

@peppy

peppy Dec 20, 2017

Member

probably worth adding a comment on why the + 1 is necessary, for future explorers of this code.

@peppy

peppy Dec 20, 2017

Member

probably worth adding a comment on why the + 1 is necessary, for future explorers of this code.

@nekodex nekodex merged commit f5bab3b into ppy:master Dec 21, 2017

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment