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

Add played filter to beatmap listing #3051

Merged
merged 12 commits into from Apr 23, 2018

Conversation

2 participants
@notbakaneko
Copy link
Contributor

notbakaneko commented Apr 20, 2018

fixes #2846

@@ -111,6 +112,19 @@ private function addModeFilter($query)
}
}
private function addPlayedFilter($query)
{
if ($this->params->user === null || !$this->params->user->isSupporter()) {

This comment has been minimized.

@nanaya

nanaya Apr 23, 2018

Collaborator

shouldn't this be part of params ?_? There's already similar check for different param in BeatmapsetSearchRequestParams. Or more like it's already checked there? What's this doing here then...

private function getPlayedBeatmapIds()
{
$unionQuery = null;
foreach ($this->getSelectedModes() as $mode) {

This comment has been minimized.

@nanaya

nanaya Apr 23, 2018

Collaborator

the other thing this function is copied from can be updated to use this getSelectedModes as well

namespace App\Libraries\Search;
abstract class PlayedState

This comment has been minimized.

@nanaya

nanaya Apr 23, 2018

Collaborator

maybe part of BeatmapsetSearchParams

@@ -59,6 +59,8 @@ public function __construct(Request $request, ?User $user = null)
explode('.', $request['r'] ?? null),
$validRanks
);
$this->playedState = get_int($request['p']) ?? PlayedState::ALL;

This comment has been minimized.

@nanaya

nanaya Apr 23, 2018

Collaborator

Not sure if it's intended to set default for isSupporter explicitly to this or should be left whatever the class default is.

}
if ($this->params->playedState === PlayedState::PLAYED) {
$query->filter(['terms' => ['difficulties.beatmap_id' => $this->getPlayedBeatmapIds()]]);

This comment has been minimized.

@nanaya

nanaya Apr 23, 2018

Collaborator

I wonder if it's faster filtering by this or beatmapset_id (as done in the other filter).

This comment has been minimized.

@notbakaneko

notbakaneko Apr 23, 2018

Author Contributor

seems to be slightly faster just filtering by beatmap_id instead of running the extra query to get the beatmapset_ids, so we should probably make another PR to update the other one

notbakaneko added some commits Apr 23, 2018

rename playedState to playedFilter;
since param signifies presence of filtering
change querystring param.
'p' is not very informative..
@nanaya

nanaya approved these changes Apr 23, 2018

@nanaya nanaya merged commit 9e0aae5 into ppy:master Apr 23, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@notbakaneko notbakaneko deleted the notbakaneko:feature/beatmap-listing-unplayed-filter-rebase branch Apr 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.