Conversation
Available PR Commands
See: https://github.com/tahminator/codebloom/wiki/CI-Commands |
Title853: create /leaderboard/current endpoint PR TypeEnhancement Description
Diagram Walkthroughflowchart LR
AdminController["AdminController"] -- "Handles PUT /leaderboard/current" --> EditLeaderboardBody["EditLeaderboardBody (Validation)"];
EditLeaderboardBody -- "Validated data" --> AdminController;
AdminController -- "Updates Leaderboard via" --> LeaderboardSqlRepository["LeaderboardSqlRepository"];
LeaderboardSqlRepository -- "Persists changes" --> Database["Database"];
|
| Relevant files | |||||||
|---|---|---|---|---|---|---|---|
| Enhancement |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/main/java/org/patinanetwork/codebloom/api/admin/AdminController.java
Show resolved
Hide resolved
src/main/java/org/patinanetwork/codebloom/api/admin/body/EditLeaderboardBody.java
Outdated
Show resolved
Hide resolved
Title853: create /leaderboard/current endpoint PR TypeEnhancement Description
Diagram Walkthroughflowchart LR
AdminController -- "PUT /leaderboard/current" --> EditLeaderboardBody["Validate Request Body"]
EditLeaderboardBody --> LeaderboardRepository["Get & Update Leaderboard"]
LeaderboardRepository --> AdminController["Return Success"]
|
| Relevant files | |||
|---|---|---|---|
| Api endpoint |
| ||
| New feature |
| ||
| Database |
| ||
| Tests |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| leaderboardRepository.updateLeaderboard(updated); | ||
| }); | ||
|
|
||
| return ResponseEntity.ok().body(ApiResponder.success("Leaderboard updated successfully", Empty.of())); | ||
| } |
There was a problem hiding this comment.
Suggestion: The editCurrentLeaderboard endpoint currently returns a success response even if no current leaderboard is found to update. This misrepresents the operation's outcome and is an edge case that needs proper error handling. Add a check for currentLeaderboard.isEmpty() and return an appropriate NOT_FOUND response if no leaderboard exists. [possible issue, importance: 9]
| leaderboardRepository.updateLeaderboard(updated); | |
| }); | |
| return ResponseEntity.ok().body(ApiResponder.success("Leaderboard updated successfully", Empty.of())); | |
| } | |
| leaderboardRepository.updateLeaderboard(updated); | |
| }); | |
| if (currentLeaderboard.isEmpty()) { | |
| throw new ResponseStatusException(HttpStatus.NOT_FOUND, "No current leaderboard found to update."); | |
| } | |
| return ResponseEntity.ok().body(ApiResponder.success("Leaderboard updated successfully", Empty.of())); | |
| } |
| .deletedAt(lb.getDeletedAt()) | ||
| .createdAt(lb.getCreatedAt()) | ||
| .shouldExpireBy(Optional.ofNullable(shouldExpireBy).map(d -> d.toLocalDateTime())) | ||
| .syntaxHighlightingLanguage(Optional.of(editLeaderboardBody.getSyntaxHighlightingLanguage())) | ||
| .id(lb.getId()) | ||
| .build(); |
There was a problem hiding this comment.
Suggestion: Converting OffsetDateTime to LocalDateTime for shouldExpireBy might lead to loss of timezone information if the database or Leaderboard model expects OffsetDateTime. Ensure consistency by passing OffsetDateTime directly to the builder, or confirm that LocalDateTime is the intended and correctly handled type throughout the persistence layer. Additionally, use Optional.ofNullable() for syntaxHighlightingLanguage to safely handle potentially null values. [possible issue, importance: 8]
| .deletedAt(lb.getDeletedAt()) | |
| .createdAt(lb.getCreatedAt()) | |
| .shouldExpireBy(Optional.ofNullable(shouldExpireBy).map(d -> d.toLocalDateTime())) | |
| .syntaxHighlightingLanguage(Optional.of(editLeaderboardBody.getSyntaxHighlightingLanguage())) | |
| .id(lb.getId()) | |
| .build(); | |
| .deletedAt(lb.getDeletedAt()) | |
| .createdAt(lb.getCreatedAt()) | |
| .shouldExpireBy(Optional.ofNullable(shouldExpireBy)) // Pass OffsetDateTime directly | |
| .syntaxHighlightingLanguage(Optional.ofNullable(editLeaderboardBody.getSyntaxHighlightingLanguage())) // Use ofNullable for safety | |
| .id(lb.getId()) | |
| .build(); |
| var name = getName(); | ||
| var expire = getShouldExpireBy(); | ||
|
|
||
| if (Strings.isNullOrEmpty(name)) { | ||
| throw new ValidationException("Leaderboard name cannot be null or empty"); | ||
| } | ||
|
|
||
| if (name.length() == 1) { | ||
| throw new ValidationException("Leaderboard name cannot have only 1 character"); | ||
| } | ||
|
|
||
| if (name.length() > 512) { | ||
| throw new ValidationException("Leaderboard name cannot have more than 512 characters"); | ||
| } |
There was a problem hiding this comment.
Suggestion: The manual Strings.isNullOrEmpty(name) check is redundant as the @NotBlank annotation already handles null, empty, and whitespace-only strings. Remove this redundant check to streamline the validation logic. Consider adding validation for syntaxHighlightingLanguage if it has specific constraints or is a required field. [general, importance: 6]
| var name = getName(); | |
| var expire = getShouldExpireBy(); | |
| if (Strings.isNullOrEmpty(name)) { | |
| throw new ValidationException("Leaderboard name cannot be null or empty"); | |
| } | |
| if (name.length() == 1) { | |
| throw new ValidationException("Leaderboard name cannot have only 1 character"); | |
| } | |
| if (name.length() > 512) { | |
| throw new ValidationException("Leaderboard name cannot have more than 512 characters"); | |
| } | |
| var name = getName(); | |
| var expire = getShouldExpireBy(); | |
| // @NotBlank already handles null/empty/whitespace for 'name' | |
| if (name.length() == 1) { | |
| throw new ValidationException("Leaderboard name cannot have only 1 character"); | |
| } | |
| if (name.length() > 512) { | |
| throw new ValidationException("Leaderboard name cannot have more than 512 characters"); | |
| } | |
| // Add validation for syntaxHighlightingLanguage if required, e.g., using @NotBlank or a custom check. |
1 participant
853
Description of changes
Added /leaderboard/current endpoint in admin route to update current leaderboard
Checklist before review
Screenshots
Dev
Screen.Recording.2026-03-17.at.7.45.33.PM.mov
Staging