Skip to content

240: Create new filters param on previous leaderboard endpoint #909

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.

Sign up for GitHub

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

Open
MalihaT111 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

240: Create new filters param on previous leaderboard endpoint #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions src/main/java/org/patinanetwork/codebloom/api/leaderboard/LeaderboardController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import jakarta.servlet.http.HttpServletRequest;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import org.patinanetwork.codebloom.common.components.LeaderboardManager;
import org.patinanetwork.codebloom.common.db.models.leaderboard.Leaderboard;
import org.patinanetwork.codebloom.common.db.models.user.UserWithScore;
Expand Down Expand Up @@ -93,6 +94,9 @@ public ResponseEntity<ApiResponder<LeaderboardDto>> getLeaderboardMetadataByLead
})
public ResponseEntity<ApiResponder<Page<Indexed<UserWithScoreDto>>>> getLeaderboardUsersById(
@PathVariable final String leaderboardId,
@Parameter(description = "Comma-separated list of active filters.", example = "nyu,hunter,globalIndex")
@RequestParam(required = false)
final Set<String> filters,
@Parameter(description = "Page index", example = "1") @RequestParam(required = false, defaultValue = "1")
final int page,
@Parameter(description = "Page size (maximum of " + MAX_LEADERBOARD_PAGE_SIZE)
Expand Down Expand Up @@ -133,29 +137,30 @@ public ResponseEntity<ApiResponder<Page<Indexed<UserWithScoreDto>>>> getLeaderbo
final boolean globalIndex,
final HttpServletRequest request) {
Comment thread
MalihaT111 marked this conversation as resolved.
FakeLag.sleep(800);

final int parsedPageSize = Math.min(pageSize, MAX_LEADERBOARD_PAGE_SIZE);
final Set<String> activeFilters = filters != null ? filters : Set.of();
final boolean useFilters = !activeFilters.isEmpty();
final boolean globalIndexNew = useFilters ? activeFilters.contains("globalIndex") : globalIndex;

LeaderboardFilterOptions options = LeaderboardFilterOptions.builder()
final LeaderboardFilterOptions options = LeaderboardFilterOptions.builder()
.page(page)
.pageSize(parsedPageSize)
.query(query)
.patina(patina)
.hunter(hunter)
.nyu(nyu)
.baruch(baruch)
.rpi(rpi)
.gwc(gwc)
.sbu(sbu)
.ccny(ccny)
.columbia(columbia)
.cornell(cornell)
.bmcc(bmcc)
.mhcplusplus(mhcplusplus)
.patina(useFilters ? activeFilters.contains("patina") : patina)
.hunter(useFilters ? activeFilters.contains("hunter") : hunter)
.nyu(useFilters ? activeFilters.contains("nyu") : nyu)
.baruch(useFilters ? activeFilters.contains("baruch") : baruch)
.rpi(useFilters ? activeFilters.contains("rpi") : rpi)
.gwc(useFilters ? activeFilters.contains("gwc") : gwc)
.sbu(useFilters ? activeFilters.contains("sbu") : sbu)
.ccny(useFilters ? activeFilters.contains("ccny") : ccny)
.columbia(useFilters ? activeFilters.contains("columbia") : columbia)
.cornell(useFilters ? activeFilters.contains("cornell") : cornell)
.bmcc(useFilters ? activeFilters.contains("bmcc") : bmcc)
.mhcplusplus(useFilters ? activeFilters.contains("mhcplusplus") : mhcplusplus)
.build();
Comment thread
MalihaT111 marked this conversation as resolved.
Comment on lines +145 to 161
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot Apr 24, 2026

Choose a reason for hiding this comment

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

Suggestion: The new conditional logic for setting each filter in the LeaderboardFilterOptions builder is highly repetitive. Encapsulating this logic in a private helper method would significantly improve readability and maintainability, especially if more filters are added or the logic changes. This aligns with the "Abstraction / Code Structure" guideline. [general, importance: 7]

Suggested change
final LeaderboardFilterOptions options = LeaderboardFilterOptions.builder()
.page(page)
.pageSize(parsedPageSize)
.query(query)
.patina(patina)
.hunter(hunter)
.nyu(nyu)
.baruch(baruch)
.rpi(rpi)
.gwc(gwc)
.sbu(sbu)
.ccny(ccny)
.columbia(columbia)
.cornell(cornell)
.bmcc(bmcc)
.mhcplusplus(mhcplusplus)
.patina(useFilters ? activeFilters.contains("patina") : patina)
.hunter(useFilters ? activeFilters.contains("hunter") : hunter)
.nyu(useFilters ? activeFilters.contains("nyu") : nyu)
.baruch(useFilters ? activeFilters.contains("baruch") : baruch)
.rpi(useFilters ? activeFilters.contains("rpi") : rpi)
.gwc(useFilters ? activeFilters.contains("gwc") : gwc)
.sbu(useFilters ? activeFilters.contains("sbu") : sbu)
.ccny(useFilters ? activeFilters.contains("ccny") : ccny)
.columbia(useFilters ? activeFilters.contains("columbia") : columbia)
.cornell(useFilters ? activeFilters.contains("cornell") : cornell)
.bmcc(useFilters ? activeFilters.contains("bmcc") : bmcc)
.mhcplusplus(useFilters ? activeFilters.contains("mhcplusplus") : mhcplusplus)
.build();
final LeaderboardFilterOptions options = LeaderboardFilterOptions.builder()
.page(page)
.pageSize(parsedPageSize)
.query(query)
.patina(getFilterValue(useFilters, activeFilters, "patina", patina))
.hunter(getFilterValue(useFilters, activeFilters, "hunter", hunter))
.nyu(getFilterValue(useFilters, activeFilters, "nyu", nyu))
.baruch(getFilterValue(useFilters, activeFilters, "baruch", baruch))
.rpi(getFilterValue(useFilters, activeFilters, "rpi", rpi))
.gwc(getFilterValue(useFilters, activeFilters, "gwc", gwc))
.sbu(getFilterValue(useFilters, activeFilters, "sbu", sbu))
.ccny(getFilterValue(useFilters, activeFilters, "ccny", ccny))
.columbia(getFilterValue(useFilters, activeFilters, "columbia", columbia))
.cornell(getFilterValue(useFilters, activeFilters, "cornell", cornell))
.bmcc(getFilterValue(useFilters, activeFilters, "bmcc", bmcc))
.mhcplusplus(getFilterValue(useFilters, activeFilters, "mhcplusplus", mhcplusplus))
.build();
// Add this private helper method to the LeaderboardController class:
private boolean getFilterValue(boolean useFilters, Set<String> activeFilters, String filterName, boolean defaultValue) {
return useFilters ? activeFilters.contains(filterName) : defaultValue;
}


Page<Indexed<UserWithScoreDto>> createdPage =
leaderboardManager.getLeaderboardUsers(leaderboardId, options, globalIndex);
leaderboardManager.getLeaderboardUsers(leaderboardId, options, globalIndexNew);
Comment thread
MalihaT111 marked this conversation as resolved.
Comment thread
MalihaT111 marked this conversation as resolved.
Comment thread
MalihaT111 marked this conversation as resolved.

return ResponseEntity.ok().body(ApiResponder.success("All leaderboards found!", createdPage));
}
Expand Down
94 changes: 94 additions & 0 deletions src/test/java/org/patinanetwork/codebloom/api/leaderboard/LeaderboardControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import java.time.LocalDateTime;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
Expand Down Expand Up @@ -54,6 +55,7 @@ void setup() {
void testGetLeaderboardUsersByIdMhcPlusPlus() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
null,
PAGE,
PAGE_SIZE,
"",
Expand Down Expand Up @@ -135,6 +137,7 @@ void throws404WhenNotFound() {
void capsPageSize() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
null,
1,
999,
"",
Expand Down Expand Up @@ -164,6 +167,7 @@ void capsPageSize() {
void passesAllFilters() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
null,
2,
10,
"tahmid",
Expand Down Expand Up @@ -203,6 +207,96 @@ void passesAllFilters() {
assertTrue(opts.isMhcplusplus());
}

@Test
@DisplayName("uses filters Set when provided, ignoring individual boolean params")
void usesFiltersSetWhenProvided() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
Set.of("nyu", "hunter"),
PAGE,
PAGE_SIZE,
"",
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
null);

ArgumentCaptor<LeaderboardFilterOptions> captor = ArgumentCaptor.forClass(LeaderboardFilterOptions.class);
verify(leaderboardManager).getLeaderboardUsers(eq(LEADERBOARD_ID), captor.capture(), eq(false));
var opts = captor.getValue();
assertTrue(opts.isNyu());
assertTrue(opts.isHunter());
assertFalse(opts.isPatina());
}

@Test
@DisplayName("sets globalIndex from filters Set when it contains globalIndex")
void setsGlobalIndexFromFiltersSet() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
Set.of("nyu", "globalIndex"),
PAGE,
PAGE_SIZE,
"",
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
null);

ArgumentCaptor<LeaderboardFilterOptions> captor = ArgumentCaptor.forClass(LeaderboardFilterOptions.class);
verify(leaderboardManager).getLeaderboardUsers(eq(LEADERBOARD_ID), captor.capture(), eq(true));
assertTrue(captor.getValue().isNyu());
}

@Test
@DisplayName("falls back to boolean params when filters Set is empty")
void fallsBackToBooleansWhenFiltersEmpty() {
leaderboardController.getLeaderboardUsersById(
LEADERBOARD_ID,
Set.of(),
PAGE,
PAGE_SIZE,
"",
true,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
false,
null);

ArgumentCaptor<LeaderboardFilterOptions> captor = ArgumentCaptor.forClass(LeaderboardFilterOptions.class);
verify(leaderboardManager).getLeaderboardUsers(eq(LEADERBOARD_ID), captor.capture(), eq(false));
assertTrue(captor.getValue().isPatina());
}

@Test
@DisplayName("returns metadata for the most recent leaderboard")
void returnsCurrentMetadata() {
Expand Down
Loading