Feat 191 #192
Merged
Feat 191 #192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request introduces several modifications across multiple classes in the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
BE/error/src/main/java/com/example/demo/schedule/application/dto/SemesterScheduleResponse.java (1)
14-18: Add validation annotations and documentationThe class would benefit from:
- Documentation explaining its purpose and field formats
- Validation annotations to ensure data integrity
Add documentation and validation:
+import javax.validation.constraints.NotBlank; +import javax.validation.constraints.NotNull; + +/** + * Response DTO for semester schedule information. + */ @Getter @AllArgsConstructor @NoArgsConstructor @Builder public class SemesterScheduleResponse { + @NotBlank(message = "Event name cannot be blank") private String eventName; + @NotNull(message = "Start date cannot be null") private LocalDateTime eventStartDate; + @NotNull(message = "End date cannot be null") private LocalDateTime eventEndDate; }BE/error/src/main/java/com/example/demo/schedule/application/service/ScheduleService.java (1)
Line range hint
65-69: Fix typo in method return typeThe return type contains a typo:
SpecificScheduleResopnseshould beSpecificScheduleResponseApply this fix:
- public SpecificScheduleResopnse getSpecificSchedule(final Long eventId) { + public SpecificScheduleResponse getSpecificSchedule(final Long eventId) { ScheduleModel model = domainService.findSchedule(eventId); String filterColor = scheduleJpaRepository.findFilterColor(model.getFilterId()); return responseConverter.from(model, filterColor); }BE/error/src/main/java/com/example/demo/schedule/presentation/ScheduleController.java (1)
Line range hint
82-91: Review scheduled task implementationThe
sendSlackMessagemethod:
- Is both a scheduled task and an HTTP endpoint
- Lacks error handling for Slack API calls
Consider separating the scheduled task from the HTTP endpoint and adding proper error handling.
BE/error/src/main/java/com/example/demo/schedule/domain/model/converter/ScheduleResponseConverter.java (1)
74-80: Consider adding null checks and consistent date formatting.The date conversion might need additional safety measures:
- Add null checks for entity and its fields
- Use the existing
FORMATTERfield for consistent date formatting across the classConsider this implementation:
private SemesterScheduleResponse buildSemesterResponse(ScheduleEntity entity) { + if (entity == null) { + return null; + } return SemesterScheduleResponse.builder() - .eventName(entity.getEventName()) - .eventStartDate(entity.getEventStartDate().toString()) - .eventEndDate(entity.getEventEndDate().toString()) + .eventName(entity.getEventName()) + .eventStartDate(entity.getEventStartDate() != null ? + entity.getEventStartDate().format(FORMATTER) : null) + .eventEndDate(entity.getEventEndDate() != null ? + entity.getEventEndDate().format(FORMATTER) : null) .build(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
BE/error/src/main/java/com/example/demo/filter/persistence/FilterEntity.java(0 hunks)BE/error/src/main/java/com/example/demo/schedule/application/dto/SemesterScheduleResponse.java(1 hunks)BE/error/src/main/java/com/example/demo/schedule/application/service/ScheduleService.java(1 hunks)BE/error/src/main/java/com/example/demo/schedule/domain/model/converter/ScheduleResponseConverter.java(2 hunks)BE/error/src/main/java/com/example/demo/schedule/infrastructure/persistence/ScheduleJpaRepository.java(2 hunks)BE/error/src/main/java/com/example/demo/schedule/presentation/ScheduleController.java(1 hunks)
💤 Files with no reviewable changes (1)
- BE/error/src/main/java/com/example/demo/filter/persistence/FilterEntity.java
🔇 Additional comments (2)
BE/error/src/main/java/com/example/demo/schedule/domain/model/converter/ScheduleResponseConverter.java (1)
68-72: LGTM! Clean implementation of the converter method.
The stream operation is implemented correctly with good separation of concerns.
BE/error/src/main/java/com/example/demo/schedule/infrastructure/persistence/ScheduleJpaRepository.java (1)
36-65: Verify the impact of pagination on the frontend.
The addition of pagination might require changes in the frontend to handle partial data loading.
Let's check for any frontend components that might be affected:
Comment on lines
+15
to
+17
| private String eventName; | ||
| private String eventStartDate; | ||
| private String eventEndDate; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using proper date types instead of Strings for date fields
Using String for date fields (eventStartDate and eventEndDate) can lead to:
- Inconsistent date formats
- Difficulty in date comparisons and calculations
- Potential parsing errors
Consider using LocalDateTime or LocalDate:
public class SemesterScheduleResponse {
private String eventName;
- private String eventStartDate;
- private String eventEndDate;
+ private LocalDateTime eventStartDate;
+ private LocalDateTime eventEndDate;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private String eventName; | |
| private String eventStartDate; | |
| private String eventEndDate; | |
| private String eventName; | |
| private LocalDateTime eventStartDate; | |
| private LocalDateTime eventEndDate; |
Comment on lines
+76
to
+80
| public List<SemesterScheduleResponse> getSemesterSchedule() { | ||
| LocalDateTime requestNow = LocalDateTime.now(); | ||
| List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow); | ||
| return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and documentation for getSemesterSchedule
The method lacks:
- Error handling for empty results
- Documentation explaining the business logic
- Separation of concerns (entity to DTO conversion should be in converter)
Consider this improvement:
+ /**
+ * Retrieves public schedules for the current semester.
+ * @return List of semester schedules, empty list if none found
+ */
public List<SemesterScheduleResponse> getSemesterSchedule() {
LocalDateTime requestNow = LocalDateTime.now();
List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow);
+ if (semesterScheduleResponses.isEmpty()) {
+ // Consider if this should throw a custom exception or return empty list
+ return Collections.emptyList();
+ }
return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public List<SemesterScheduleResponse> getSemesterSchedule() { | |
| LocalDateTime requestNow = LocalDateTime.now(); | |
| List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow); | |
| return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses); | |
| } | |
| /** | |
| * Retrieves public schedules for the current semester. | |
| * @return List of semester schedules, empty list if none found | |
| */ | |
| public List<SemesterScheduleResponse> getSemesterSchedule() { | |
| LocalDateTime requestNow = LocalDateTime.now(); | |
| List<ScheduleEntity> semesterScheduleResponses = scheduleJpaRepository.findOneSemesterPublicSchedule(requestNow); | |
| if (semesterScheduleResponses.isEmpty()) { | |
| // Consider if this should throw a custom exception or return empty list | |
| return Collections.emptyList(); | |
| } | |
| return responseConverter.toSemesterScheduleResponse(semesterScheduleResponses); | |
| } |
Comment on lines
+77
to
+81
| @GetMapping("semester") | ||
| public ApiResponse<SuccessBody<List<SemesterScheduleResponse>>> getSemesterSchedule() { | ||
| List<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule(); | ||
| return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.GET); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add API documentation and consider pagination
The endpoint would benefit from:
- Swagger/OpenAPI documentation
- Pagination support for large result sets
Consider these improvements:
+ /**
+ * Retrieves public schedules for the current semester.
+ * @return List of semester schedules wrapped in ApiResponse
+ */
+ @Operation(summary = "Get semester schedule", description = "Retrieves all public schedules for the current semester")
+ @ApiResponse(responseCode = "200", description = "Successfully retrieved semester schedule")
@GetMapping("semester")
- public ApiResponse<SuccessBody<List<SemesterScheduleResponse>>> getSemesterSchedule() {
+ public ApiResponse<SuccessBody<Page<SemesterScheduleResponse>>> getSemesterSchedule(
+ @RequestParam(defaultValue = "0") int page,
+ @RequestParam(defaultValue = "20") int size) {
- List<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule();
+ Page<SemesterScheduleResponse> response = scheduleService.getSemesterSchedule(PageRequest.of(page, size));
return ApiResponseGenerator.success(response, HttpStatus.OK, MessageCode.GET);
}Committable suggestion skipped: line range outside the PR's diff.
Comment on lines
+36
to
+65
| public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) { | ||
| int month = requestDate.getMonthValue(); | ||
| int year = requestDate.getYear(); | ||
|
|
||
| LocalDateTime startDate; | ||
| LocalDateTime endDate; | ||
|
|
||
| if (month >= 2 && month <= 7) { | ||
| // 2월 ~ 7월 | ||
| startDate = LocalDateTime.of(year, 2, 1, 0, 0); | ||
| endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59); | ||
| } else if (month >= 8 && month <= 12) { | ||
| // 8월 ~ 다음해 1월 | ||
| startDate = LocalDateTime.of(year, 8, 1, 0, 0); | ||
| endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59); | ||
| } else { // month == 1 | ||
| // 전년도 8월 ~ 해당 연도 1월 | ||
| startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0); | ||
| endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59); | ||
| } | ||
|
|
||
| String jpql = "SELECT s FROM ScheduleEntity s " + | ||
| "WHERE s.eventStartDate BETWEEN :startDate AND :endDate"; | ||
|
|
||
| TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class); | ||
| query.setParameter("startDate", startDate); | ||
| query.setParameter("endDate", endDate); | ||
|
|
||
| return query.getResultList(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed for robustness and performance.
The implementation has the following concerns:
- Missing filter for public schedules
- No pagination for potentially large result sets
- Hardcoded date ranges could be constants
- No sorting specified
Consider this improved implementation:
+ private static final int FIRST_SEMESTER_START_MONTH = 2;
+ private static final int FIRST_SEMESTER_END_MONTH = 7;
+ private static final int SECOND_SEMESTER_START_MONTH = 8;
public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) {
int month = requestDate.getMonthValue();
int year = requestDate.getYear();
LocalDateTime startDate;
LocalDateTime endDate;
- if (month >= 2 && month <= 7) {
+ if (month >= FIRST_SEMESTER_START_MONTH && month <= FIRST_SEMESTER_END_MONTH) {
// 2월 ~ 7월
- startDate = LocalDateTime.of(year, 2, 1, 0, 0);
- endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59);
+ startDate = LocalDateTime.of(year, FIRST_SEMESTER_START_MONTH, 1, 0, 0);
+ endDate = LocalDateTime.of(year, FIRST_SEMESTER_END_MONTH, 31, 23, 59, 59);
- } else if (month >= 8 && month <= 12) {
+ } else if (month >= SECOND_SEMESTER_START_MONTH && month <= 12) {
// 8월 ~ 다음해 1월
- startDate = LocalDateTime.of(year, 8, 1, 0, 0);
+ startDate = LocalDateTime.of(year, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59);
} else { // month == 1
// 전년도 8월 ~ 해당 연도 1월
- startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0);
+ startDate = LocalDateTime.of(year - 1, SECOND_SEMESTER_START_MONTH, 1, 0, 0);
endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59);
}
String jpql = "SELECT s FROM ScheduleEntity s " +
- "WHERE s.eventStartDate BETWEEN :startDate AND :endDate";
+ "WHERE s.scheduleType = :scheduleType " +
+ "AND s.eventStartDate BETWEEN :startDate AND :endDate " +
+ "ORDER BY s.eventStartDate ASC";
TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class);
+ query.setParameter("scheduleType", "PUBLIC");
query.setParameter("startDate", startDate);
query.setParameter("endDate", endDate);
+ query.setMaxResults(1000); // Add pagination to prevent memory issues
return query.getResultList();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) { | |
| int month = requestDate.getMonthValue(); | |
| int year = requestDate.getYear(); | |
| LocalDateTime startDate; | |
| LocalDateTime endDate; | |
| if (month >= 2 && month <= 7) { | |
| // 2월 ~ 7월 | |
| startDate = LocalDateTime.of(year, 2, 1, 0, 0); | |
| endDate = LocalDateTime.of(year, 7, 31, 23, 59, 59); | |
| } else if (month >= 8 && month <= 12) { | |
| // 8월 ~ 다음해 1월 | |
| startDate = LocalDateTime.of(year, 8, 1, 0, 0); | |
| endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59); | |
| } else { // month == 1 | |
| // 전년도 8월 ~ 해당 연도 1월 | |
| startDate = LocalDateTime.of(year - 1, 8, 1, 0, 0); | |
| endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59); | |
| } | |
| String jpql = "SELECT s FROM ScheduleEntity s " + | |
| "WHERE s.eventStartDate BETWEEN :startDate AND :endDate"; | |
| TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class); | |
| query.setParameter("startDate", startDate); | |
| query.setParameter("endDate", endDate); | |
| return query.getResultList(); | |
| } | |
| private static final int FIRST_SEMESTER_START_MONTH = 2; | |
| private static final int FIRST_SEMESTER_END_MONTH = 7; | |
| private static final int SECOND_SEMESTER_START_MONTH = 8; | |
| public List<ScheduleEntity> findOneSemesterPublicSchedule(LocalDateTime requestDate) { | |
| int month = requestDate.getMonthValue(); | |
| int year = requestDate.getYear(); | |
| LocalDateTime startDate; | |
| LocalDateTime endDate; | |
| if (month >= FIRST_SEMESTER_START_MONTH && month <= FIRST_SEMESTER_END_MONTH) { | |
| // 2월 ~ 7월 | |
| startDate = LocalDateTime.of(year, FIRST_SEMESTER_START_MONTH, 1, 0, 0); | |
| endDate = LocalDateTime.of(year, FIRST_SEMESTER_END_MONTH, 31, 23, 59, 59); | |
| } else if (month >= SECOND_SEMESTER_START_MONTH && month <= 12) { | |
| // 8월 ~ 다음해 1월 | |
| startDate = LocalDateTime.of(year, SECOND_SEMESTER_START_MONTH, 1, 0, 0); | |
| endDate = LocalDateTime.of(year + 1, 1, 31, 23, 59, 59); | |
| } else { // month == 1 | |
| // 전년도 8월 ~ 해당 연도 1월 | |
| startDate = LocalDateTime.of(year - 1, SECOND_SEMESTER_START_MONTH, 1, 0, 0); | |
| endDate = LocalDateTime.of(year, 1, 31, 23, 59, 59); | |
| } | |
| String jpql = "SELECT s FROM ScheduleEntity s " + | |
| "WHERE s.scheduleType = :scheduleType " + | |
| "AND s.eventStartDate BETWEEN :startDate AND :endDate " + | |
| "ORDER BY s.eventStartDate ASC"; | |
| TypedQuery<ScheduleEntity> query = em.createQuery(jpql, ScheduleEntity.class); | |
| query.setParameter("scheduleType", "PUBLIC"); | |
| query.setParameter("startDate", startDate); | |
| query.setParameter("endDate", endDate); | |
| query.setMaxResults(1000); // Add pagination to prevent memory issues | |
| return query.getResultList(); | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
/api/calendar/semester.Bug Fixes
Improvements