[BE-Feat] 논의 생성 기능 구현 #57
Conversation
|
|
||
| private final DiscussionService discussionService; | ||
|
|
||
| @PostMapping("/create") |
There was a problem hiding this comment.
p2: Post가 이미 만든다는 의미가 있어서 /create는 빼는 건 어떤가요??
| public ResponseEntity<CreateDiscussionResponse> createDiscussion( | ||
| @RequestBody CreateDiscussionRequest request) { | ||
| CreateDiscussionResponse response = discussionService.createDiscussion(request); | ||
| return ResponseEntity.ok(response); |
There was a problem hiding this comment.
p3: 201 created로 하는 것도 좋아보입니다.
| .orElseThrow(() -> new RuntimeException("사용자를 찾을 수 없습니다.")); | ||
| } | ||
|
|
||
| private String calculateTimeLeft(LocalDate deadline) { |
|
|
||
| @PostMapping("/create") | ||
| public ResponseEntity<CreateDiscussionResponse> createDiscussion( | ||
| @RequestBody CreateDiscussionRequest request) { |
| discussionParticipantRepository.save(participant); | ||
|
|
||
| // 공유 링크 생성 (도메인 주소 설정 필요) | ||
| String shareableLink = "localhost:8080/api/v1/discussion/invite/" + discussion.getId(); |
There was a problem hiding this comment.
p2: 이 링크는 backend api url를 사용하면 안 됩니다.
기존의 isModifiable 삭제 후 enum으로 state 관리
WalkthroughThis pull request introduces discussion functionality by adding a new controller and service along with related DTO records for discussion creation. The discussion entity has been updated to use enumerated types for meeting methods and statuses. The participant entity now uses a boolean flag to indicate the host instead of a role enum. Additionally, two new enums have been added, and test classes for both the controller and service have been implemented to ensure correct behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as DiscussionController
participant S as DiscussionService
participant DR as DiscussionRepository
participant DPR as DiscussionParticipantRepository
participant UR as UserRepository
U->>C: POST /api/v1/discussion/ (CreateDiscussionRequest)
C->>S: createDiscussion(request)
S->>DR: Save Discussion entity
S->>UR: Retrieve current user
S->>DPR: Save DiscussionParticipant (isHost=true)
S->>S: Generate shareable link & calculate time left
S->>C: Return CreateDiscussionResponse
C->>U: HTTP 200 OK with response
Poem
✨ Finishing Touches
🪧 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: 5
♻️ Duplicate comments (1)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java (1)
50-51: 🛠️ Refactor suggestionUse configuration for domain URL.
As noted in the past review, hardcoding the backend API URL is not recommended. Consider using configuration properties.
+@Value("${app.domain.url}") +private String domainUrl; -String shareableLink = "localhost:8080/api/v1/discussion/invite/" + discussion.getId(); +String shareableLink = domainUrl + "/discussion/invite/" + discussion.getId();
🧹 Nitpick comments (7)
backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java (1)
61-62: Consider validating location based on meeting method.If
MeetingMethod.ONLINErequires a link orMeetingMethod.OFFLINErequires a physical address, you might add validation to ensure consistency.backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java (1)
17-22: Add API documentation using OpenAPI annotations.Document the API endpoint to improve API discoverability and maintainability.
Add OpenAPI annotations:
+ @Operation(summary = "Create a new discussion") + @ApiResponse(responseCode = "201", description = "Discussion created successfully") + @ApiResponse(responseCode = "400", description = "Invalid request parameters") @PostMapping("/") public ResponseEntity<CreateDiscussionResponse> createDiscussion( @RequestBody @Valid CreateDiscussionRequest request) { CreateDiscussionResponse response = discussionService.createDiscussion(request); - return ResponseEntity.ok(response); //TODO: 201 created로 변경 + return ResponseEntity.status(HttpStatus.CREATED).body(response); }backend/src/main/java/endolphin/backend/domain/discussion/entity/DiscussionParticipant.java (1)
20-22: Add index on discussion_id foreign key.Add an index to improve query performance when filtering participants by discussion.
@ManyToOne(fetch = FetchType.LAZY) - @JoinColumn(name = "discussion_id", nullable = false) + @JoinColumn(name = "discussion_id", nullable = false) + @Index(name = "idx_discussion_participant_discussion_id") private Discussion discussion;backend/src/test/java/endolphin/backend/domain/discussion/DiscussionControllerTest.java (1)
65-66: Use dynamic dates in test data.Replace hardcoded dates with dynamic ones to prevent test failures in the future.
- LocalDate.of(2025, 2, 10), - LocalDate.of(2025, 2, 15), + request.dateRangeStart(), + request.dateRangeEnd(),backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java (2)
65-70: Improve error handling with custom exception.While this method is marked for deletion, it should use a custom exception for better error handling until removed.
+public class UserNotFoundException extends RuntimeException { + public UserNotFoundException(Long userId) { + super("User not found with ID: " + userId); + } +} private User getCurrentUser() { Long userId = UserContext.get().userId(); return userRepository.findById(userId) - .orElseThrow(() -> new RuntimeException("사용자를 찾을 수 없습니다.")); + .orElseThrow(() -> new UserNotFoundException(userId)); }
72-88: Consider i18n and flexibility improvements.The method could benefit from:
- Externalized strings for internationalization
- Configurable end-of-day time
+@Value("${app.deadline.end-of-day:23:59:59}") +private String deadlineEndTime; +@Autowired +private MessageSource messageSource; private String calculateTimeLeft(LocalDate deadline) { LocalDateTime now = LocalDateTime.now(); - LocalDateTime deadlineDateTime = deadline.atTime(23, 59, 59); + LocalDateTime deadlineDateTime = deadline.atTime(LocalTime.parse(deadlineEndTime)); Duration duration = Duration.between(now, deadlineDateTime); long days = duration.toDays(); long hours = duration.toHours() % 24; long minutes = duration.toMinutes() % 60; if (days > 0) { - return "마감까지 " + days + "일"; + return messageSource.getMessage("time.left.days", new Object[]{days}, LocaleContextHolder.getLocale()); } else if (hours > 0) { - return "마감까지 " + hours + "시간"; + return messageSource.getMessage("time.left.hours", new Object[]{hours}, LocaleContextHolder.getLocale()); } else { - return "마감시간까지 " + minutes + "분"; + return messageSource.getMessage("time.left.minutes", new Object[]{minutes}, LocaleContextHolder.getLocale()); } }backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java (1)
71-75: Verify saved entity attributes.The test should verify that the Discussion entity is saved with correct attributes from the request.
given(discussionRepository.save(any(Discussion.class))).willAnswer(invocation -> { Discussion d = invocation.getArgument(0); + assertThat(d.getTitle()).isEqualTo("Test Discussion"); + assertThat(d.getDateRangeStart()).isEqualTo(LocalDate.of(2025, 2, 10)); + // ... verify other attributes ... ReflectionTestUtils.setField(d, "id", 100L); return d; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/DiscussionService.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/dto/CreateDiscussionRequest.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/dto/CreateDiscussionResponse.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java(2 hunks)backend/src/main/java/endolphin/backend/domain/discussion/entity/DiscussionParticipant.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/enums/DiscussionStatus.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/enums/MeetingMethod.java(1 hunks)backend/src/test/java/endolphin/backend/domain/discussion/DiscussionControllerTest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/discussion/DiscussionServiceTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/main/java/endolphin/backend/domain/discussion/enums/MeetingMethod.java
🔇 Additional comments (6)
backend/src/main/java/endolphin/backend/domain/discussion/entity/Discussion.java (4)
3-4: Enums successfully imported for type safety.These imports bring in the newly introduced
DiscussionStatusandMeetingMethodenums, enhancing clarity and preventing invalid inputs. No issues found.
44-45: Appropriate use of @Enumerated(EnumType.STRING).Storing enum names rather than ordinal positions helps preserve readability and maintain database compatibility even if the enum ordering changes.
53-56: Verify correctness of column default vs. enum constant.The default column value
'ONGOING'matches a corresponding constant inDiscussionStatus. Ensure the DB default is in sync with the enum to avoid runtime or migration issues.Run a quick check in your DB or schema migration scripts to confirm
'ONGOING'matches exactly the enum constant name.
70-71: New fields are correctly assigned in the constructor.Straightforward assignment of
meetingMethodandlocationensures the discussion entity is fully initialized.backend/src/main/java/endolphin/backend/domain/discussion/enums/DiscussionStatus.java (1)
1-5: Enumeration provides clear discussion states.Defining
UPCOMING,ONGOING, andFINISHEDis a clean approach for representing discussion lifecycles.backend/src/main/java/endolphin/backend/domain/discussion/dto/CreateDiscussionResponse.java (1)
1-17: Record structure is concise and readable.Using a Java record for response data is a straightforward choice for an immutable DTO. Fields appear relevant and clearly named.
| MeetingMethod meetingMethod, | ||
| String location, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add conditional validation for location field.
When the meeting method is OFFLINE, the location field should be required.
Consider using a custom validator:
@Target({TYPE})
@Retention(RUNTIME)
@Constraint(validatedBy = LocationValidator.class)
public @interface ValidLocation {
String message() default "Location is required for offline meetings";
Class<?>[] groups() default {};
Class<? extends Payload>[] payload() default {};
}
public class LocationValidator implements ConstraintValidator<ValidLocation, CreateDiscussionRequest> {
@Override
public boolean isValid(CreateDiscussionRequest request, ConstraintValidatorContext context) {
if (request.meetingMethod() == MeetingMethod.OFFLINE) {
return request.location() != null && !request.location().trim().isEmpty();
}
return true;
}
}Then apply it to the record:
+@ValidLocation
public record CreateDiscussionRequest(| @NotNull LocalDate dateRangeEnd, | ||
| @NotNull LocalTime timeRangeStart, | ||
| @NotNull LocalTime timeRangeEnd, | ||
| @NotNull Integer duration, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds validation for duration.
The duration field should have minimum and maximum bounds to ensure reasonable meeting lengths.
Add validation constraints:
- @NotNull Integer duration,
+ @NotNull @Min(15) @Max(480) Integer duration,📝 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.
| @NotNull Integer duration, | |
| @NotNull @Min(15) @Max(480) Integer duration, |
| @DisplayName("create discusison API 테스트") | ||
| @Test | ||
| public void testCreateDiscussionEndpoint() throws Exception { | ||
| // given: 요청 DTO 생성 | ||
| CreateDiscussionRequest request = new CreateDiscussionRequest( | ||
| "Test Discussion", | ||
| LocalDate.now().plusDays(2), | ||
| LocalDate.now().plusDays(7), | ||
| LocalTime.of(9, 0), | ||
| LocalTime.of(18, 0), | ||
| 60, | ||
| MeetingMethod.ONLINE, | ||
| null, | ||
| LocalDate.now().plusDays(10) | ||
| ); | ||
|
|
||
| // given: 서비스가 반환할 응답 DTO 생성 | ||
| CreateDiscussionResponse response = new CreateDiscussionResponse( | ||
| 100L, | ||
| "Test Discussion", | ||
| LocalDate.of(2025, 2, 10), | ||
| LocalDate.of(2025, 2, 15), | ||
| MeetingMethod.ONLINE, | ||
| null, | ||
| 60, | ||
| "localhost:8080/api/v1/discussion/invite/100", | ||
| "마감까지 10일" | ||
| ); | ||
|
|
||
| given(discussionService.createDiscussion(request)).willReturn(response); | ||
|
|
||
| // when & then | ||
| mockMvc.perform(post("/api/v1/discussion/") | ||
| .contentType("application/json") | ||
| .content(objectMapper.writeValueAsString(request))) | ||
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.id").value(100)) | ||
| .andExpect(jsonPath("$.title").value("Test Discussion")) | ||
| .andExpect( | ||
| jsonPath("$.shareableLink").value("localhost:8080/api/v1/discussion/invite/100")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for validation errors and edge cases.
The current test only covers the happy path. Add tests for:
- Invalid request parameters
- Missing required fields
- Invalid date ranges (end before start)
- Invalid time ranges
- Invalid duration
- Missing location for offline meetings
Example test case:
@Test
@DisplayName("날짜 범위가 잘못된 경우 400 응답")
public void testCreateDiscussionWithInvalidDateRange() throws Exception {
CreateDiscussionRequest request = new CreateDiscussionRequest(
"Test Discussion",
LocalDate.now().plusDays(7), // end before start
LocalDate.now().plusDays(2),
LocalTime.of(9, 0),
LocalTime.of(18, 0),
60,
MeetingMethod.ONLINE,
null,
LocalDate.now().plusDays(10)
);
mockMvc.perform(post("/api/v1/discussion/")
.contentType("application/json")
.content(objectMapper.writeValueAsString(request)))
.andExpect(status().isBadRequest());
}| public CreateDiscussionResponse createDiscussion(CreateDiscussionRequest request) { | ||
|
|
||
| Discussion discussion = Discussion.builder() | ||
| .title(request.title()) | ||
| .dateStart(request.dateRangeStart()) | ||
| .dateEnd(request.dateRangeEnd()) | ||
| .timeStart(request.timeRangeStart()) | ||
| .timeEnd(request.timeRangeEnd()) | ||
| .duration(request.duration()) | ||
| .deadline(request.deadline()) | ||
| .meetingMethod(request.meetingMethod()) | ||
| .location(request.location()) | ||
| .build(); | ||
|
|
||
| discussionRepository.save(discussion); | ||
|
|
||
| User currentUser = getCurrentUser(); | ||
| DiscussionParticipant participant = DiscussionParticipant.builder() | ||
| .discussion(discussion) | ||
| .user(currentUser) | ||
| .isHost(true) | ||
| .build(); | ||
| discussionParticipantRepository.save(participant); | ||
|
|
||
| //TODO: 도메인 주소 설정 | ||
| String shareableLink = "localhost:8080/api/v1/discussion/invite/" + discussion.getId(); | ||
|
|
||
| return new CreateDiscussionResponse( | ||
| discussion.getId(), | ||
| discussion.getTitle(), | ||
| discussion.getDateRangeStart(), | ||
| discussion.getDateRangeEnd(), | ||
| discussion.getMeetingMethod(), | ||
| discussion.getLocation(), | ||
| discussion.getDuration(), | ||
| shareableLink, | ||
| calculateTimeLeft(discussion.getDeadline()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and error handling.
The method should include:
- Additional validation for date/time ranges and duration
- Error handling for repository operations
public CreateDiscussionResponse createDiscussion(CreateDiscussionRequest request) {
+ validateDiscussionRequest(request);
+ try {
Discussion discussion = Discussion.builder()
// ... existing builder code ...
.build();
- discussionRepository.save(discussion);
+ discussion = discussionRepository.save(discussion);
// ... rest of the method ...
+ } catch (Exception e) {
+ throw new RuntimeException("Failed to create discussion", e);
+ }
}
+private void validateDiscussionRequest(CreateDiscussionRequest request) {
+ if (request.dateRangeEnd().isBefore(request.dateRangeStart())) {
+ throw new IllegalArgumentException("End date cannot be before start date");
+ }
+ if (request.timeRangeEnd().isBefore(request.timeRangeStart())) {
+ throw new IllegalArgumentException("End time cannot be before start time");
+ }
+ if (request.duration() <= 0) {
+ throw new IllegalArgumentException("Duration must be positive");
+ }
+}📝 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.
| public CreateDiscussionResponse createDiscussion(CreateDiscussionRequest request) { | |
| Discussion discussion = Discussion.builder() | |
| .title(request.title()) | |
| .dateStart(request.dateRangeStart()) | |
| .dateEnd(request.dateRangeEnd()) | |
| .timeStart(request.timeRangeStart()) | |
| .timeEnd(request.timeRangeEnd()) | |
| .duration(request.duration()) | |
| .deadline(request.deadline()) | |
| .meetingMethod(request.meetingMethod()) | |
| .location(request.location()) | |
| .build(); | |
| discussionRepository.save(discussion); | |
| User currentUser = getCurrentUser(); | |
| DiscussionParticipant participant = DiscussionParticipant.builder() | |
| .discussion(discussion) | |
| .user(currentUser) | |
| .isHost(true) | |
| .build(); | |
| discussionParticipantRepository.save(participant); | |
| //TODO: 도메인 주소 설정 | |
| String shareableLink = "localhost:8080/api/v1/discussion/invite/" + discussion.getId(); | |
| return new CreateDiscussionResponse( | |
| discussion.getId(), | |
| discussion.getTitle(), | |
| discussion.getDateRangeStart(), | |
| discussion.getDateRangeEnd(), | |
| discussion.getMeetingMethod(), | |
| discussion.getLocation(), | |
| discussion.getDuration(), | |
| shareableLink, | |
| calculateTimeLeft(discussion.getDeadline()) | |
| ); | |
| } | |
| public CreateDiscussionResponse createDiscussion(CreateDiscussionRequest request) { | |
| validateDiscussionRequest(request); | |
| try { | |
| Discussion discussion = Discussion.builder() | |
| .title(request.title()) | |
| .dateStart(request.dateRangeStart()) | |
| .dateEnd(request.dateRangeEnd()) | |
| .timeStart(request.timeRangeStart()) | |
| .timeEnd(request.timeRangeEnd()) | |
| .duration(request.duration()) | |
| .deadline(request.deadline()) | |
| .meetingMethod(request.meetingMethod()) | |
| .location(request.location()) | |
| .build(); | |
| discussion = discussionRepository.save(discussion); | |
| User currentUser = getCurrentUser(); | |
| DiscussionParticipant participant = DiscussionParticipant.builder() | |
| .discussion(discussion) | |
| .user(currentUser) | |
| .isHost(true) | |
| .build(); | |
| discussionParticipantRepository.save(participant); | |
| //TODO: 도메인 주소 설정 | |
| String shareableLink = "localhost:8080/api/v1/discussion/invite/" + discussion.getId(); | |
| return new CreateDiscussionResponse( | |
| discussion.getId(), | |
| discussion.getTitle(), | |
| discussion.getDateRangeStart(), | |
| discussion.getDateRangeEnd(), | |
| discussion.getMeetingMethod(), | |
| discussion.getLocation(), | |
| discussion.getDuration(), | |
| shareableLink, | |
| calculateTimeLeft(discussion.getDeadline()) | |
| ); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Failed to create discussion", e); | |
| } | |
| } | |
| private void validateDiscussionRequest(CreateDiscussionRequest request) { | |
| if (request.dateRangeEnd().isBefore(request.dateRangeStart())) { | |
| throw new IllegalArgumentException("End date cannot be before start date"); | |
| } | |
| if (request.timeRangeEnd().isBefore(request.timeRangeStart())) { | |
| throw new IllegalArgumentException("End time cannot be before start time"); | |
| } | |
| if (request.duration() <= 0) { | |
| throw new IllegalArgumentException("Duration must be positive"); | |
| } | |
| } |
| @DisplayName("모든 필드가 채워진 논의 생성 request 테스트") | ||
| @Test | ||
| public void testCreateDiscussion_withDeadlineProvided() { | ||
| // given: 모든 필드가 채워진 요청 (deadline 제공) | ||
| CreateDiscussionRequest request = new CreateDiscussionRequest( | ||
| "Test Discussion", | ||
| LocalDate.of(2025, 2, 10), | ||
| LocalDate.of(2025, 2, 15), | ||
| LocalTime.of(9, 0), | ||
| LocalTime.of(18, 0), | ||
| 60, | ||
| MeetingMethod.OFFLINE, | ||
| "Test Location", | ||
| LocalDate.of(2025, 2, 20) | ||
| ); | ||
|
|
||
| given(discussionRepository.save(any(Discussion.class))).willAnswer(invocation -> { | ||
| Discussion d = invocation.getArgument(0); | ||
| ReflectionTestUtils.setField(d, "id", 100L); | ||
| return d; | ||
| }); | ||
|
|
||
| User user = new User(); | ||
| ReflectionTestUtils.setField(user, "id", 1L); | ||
|
|
||
| given(userRepository.findById(1L)).willReturn(Optional.of(user)); | ||
|
|
||
| // when | ||
| CreateDiscussionResponse response = discussionService.createDiscussion(request); | ||
|
|
||
| // then | ||
| assertThat(response).isNotNull(); | ||
| assertThat(response.id()).isEqualTo(100L); | ||
| assertThat(response.title()).isEqualTo("Test Discussion"); | ||
| assertThat(response.dateRangeStart()).isEqualTo(LocalDate.of(2025, 2, 10)); | ||
| assertThat(response.dateRangeEnd()).isEqualTo(LocalDate.of(2025, 2, 15)); | ||
| assertThat(response.meetingMethod()).isEqualTo(MeetingMethod.OFFLINE); | ||
| assertThat(response.location()).isEqualTo("Test Location"); | ||
| assertThat(response.duration()).isEqualTo(60); | ||
| assertThat(response.shareableLink()).isEqualTo( | ||
| "localhost:8080/api/v1/discussion/invite/100"); | ||
| assertThat(response.timeLeft()).isNotNull(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with edge cases and error scenarios.
Current test only covers the happy path. Consider adding tests for:
- Invalid date/time ranges
- Participant creation verification
- User not found scenario
- Repository failures
Example test case:
@Test
@DisplayName("날짜 범위가 잘못된 경우 예외 발생")
public void testCreateDiscussion_withInvalidDateRange() {
// given
CreateDiscussionRequest request = new CreateDiscussionRequest(
"Test Discussion",
LocalDate.of(2025, 2, 15), // End date before start date
LocalDate.of(2025, 2, 10),
LocalTime.of(9, 0),
LocalTime.of(18, 0),
60,
MeetingMethod.OFFLINE,
"Test Location",
LocalDate.of(2025, 2, 20)
);
// when/then
assertThatThrownBy(() -> discussionService.createDiscussion(request))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("End date cannot be before start date");
}
| private final DiscussionService discussionService; | ||
|
|
||
| @PostMapping("/create") | ||
| @PostMapping("/") |
There was a problem hiding this comment.
p3: "/" 이거 빼는 게 좋아보여요. 이렇게 하면 /discussion/ 으로 매핑됩니다.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/src/test/java/endolphin/backend/domain/discussion/DiscussionControllerTest.java (2)
49-59: Use test data builder pattern for better maintainability.The test request creation uses hardcoded values directly in the constructor. Consider using a builder pattern or test fixture for better maintainability and readability.
public class TestFixtures { public static CreateDiscussionRequest.Builder defaultDiscussionRequest() { return CreateDiscussionRequest.builder() .title("Test Discussion") .startDate(LocalDate.now().plusDays(2)) .endDate(LocalDate.now().plusDays(7)) .startTime(LocalTime.of(9, 0)) .endTime(LocalTime.of(18, 0)) .duration(60) .meetingMethod(MeetingMethod.ONLINE) .location(null) .deadlineDate(LocalDate.now().plusDays(10)); } }
45-47: Add comprehensive test documentation.The test method lacks detailed documentation about the test scenario and expected behavior.
- @DisplayName("create discusison API 테스트") + @DisplayName("토론 생성 API - 정상 케이스") @Test - public void testCreateDiscussionEndpoint() throws Exception { + /** + * 토론 생성 API의 정상 동작을 검증하는 테스트 + * + * Given: + * - 유효한 토론 생성 요청 데이터 + * When: + * - POST /api/v1/discussion 엔드포인트 호출 + * Then: + * - 200 OK 응답 + * - 생성된 토론 정보가 올바르게 반환됨 + */ + public void should_create_discussion_successfully() throws Exception {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java(1 hunks)backend/src/main/java/endolphin/backend/domain/discussion/dto/CreateDiscussionRequest.java(1 hunks)backend/src/test/java/endolphin/backend/domain/discussion/DiscussionControllerTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/endolphin/backend/domain/discussion/dto/CreateDiscussionRequest.java
- backend/src/main/java/endolphin/backend/domain/discussion/DiscussionController.java
|
|
||
| private MockMvc mockMvc; | ||
|
|
||
| private ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant ObjectMapper initialization.
The ObjectMapper is initialized twice - once as a class field and again in the setUp method. Remove the redundant initialization.
- private ObjectMapper objectMapper = new ObjectMapper();
+ private ObjectMapper objectMapper;
@BeforeEach
public void setUp() {
mockMvc = MockMvcBuilders.standaloneSetup(discussionController).build();
- objectMapper = new ObjectMapper();
+ objectMapper = new ObjectMapper();
objectMapper.registerModule(new JavaTimeModule());
}Also applies to: 41-42
| .andExpect(status().isOk()) | ||
| .andExpect(jsonPath("$.id").value(100)) | ||
| .andExpect(jsonPath("$.title").value("Test Discussion")) | ||
| .andExpect( | ||
| jsonPath("$.shareableLink").value("localhost:8080/api/v1/discussion/invite/100")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify all response fields.
The test only verifies a subset of the response fields. Add assertions for all fields to ensure complete response validation.
.andExpect(status().isOk())
.andExpect(jsonPath("$.id").value(100))
.andExpect(jsonPath("$.title").value("Test Discussion"))
+ .andExpect(jsonPath("$.startDate").value("2025-02-10"))
+ .andExpect(jsonPath("$.endDate").value("2025-02-15"))
+ .andExpect(jsonPath("$.meetingMethod").value("ONLINE"))
+ .andExpect(jsonPath("$.location").isEmpty())
+ .andExpect(jsonPath("$.duration").value(60))
.andExpect(jsonPath("$.shareableLink").value("localhost:8080/api/v1/discussion/invite/100"))
+ .andExpect(jsonPath("$.deadlineMessage").value("마감까지 10일"));📝 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.
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.id").value(100)) | |
| .andExpect(jsonPath("$.title").value("Test Discussion")) | |
| .andExpect( | |
| jsonPath("$.shareableLink").value("localhost:8080/api/v1/discussion/invite/100")); | |
| .andExpect(status().isOk()) | |
| .andExpect(jsonPath("$.id").value(100)) | |
| .andExpect(jsonPath("$.title").value("Test Discussion")) | |
| .andExpect(jsonPath("$.startDate").value("2025-02-10")) | |
| .andExpect(jsonPath("$.endDate").value("2025-02-15")) | |
| .andExpect(jsonPath("$.meetingMethod").value("ONLINE")) | |
| .andExpect(jsonPath("$.location").isEmpty()) | |
| .andExpect(jsonPath("$.duration").value(60)) | |
| .andExpect(jsonPath("$.shareableLink").value("localhost:8080/api/v1/discussion/invite/100")) | |
| .andExpect(jsonPath("$.deadlineMessage").value("마감까지 10일")); |
| // @WebMvcTest나 스프링 컨텍스트 없이 standalone 방식으로 MockMvc 구성 | ||
| mockMvc = MockMvcBuilders.standaloneSetup(discussionController).build(); | ||
| objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance MockMvc configuration for better error handling.
The current MockMvc setup lacks exception handlers and content negotiation configuration, which are crucial for testing error responses.
- mockMvc = MockMvcBuilders.standaloneSetup(discussionController).build();
+ mockMvc = MockMvcBuilders.standaloneSetup(discussionController)
+ .setControllerAdvice(new GlobalExceptionHandler()) // Add your exception handler
+ .addFilter((request, response, chain) -> {
+ response.setCharacterEncoding("UTF-8");
+ chain.doFilter(request, response);
+ })
+ .build();📝 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.
| // @WebMvcTest나 스프링 컨텍스트 없이 standalone 방식으로 MockMvc 구성 | |
| mockMvc = MockMvcBuilders.standaloneSetup(discussionController).build(); | |
| objectMapper = new ObjectMapper(); | |
| // @WebMvcTest나 스프링 컨텍스트 없이 standalone 방식으로 MockMvc 구성 | |
| - mockMvc = MockMvcBuilders.standaloneSetup(discussionController).build(); | |
| + mockMvc = MockMvcBuilders.standaloneSetup(discussionController) | |
| + .setControllerAdvice(new GlobalExceptionHandler()) // Add your exception handler | |
| + .addFilter((request, response, chain) -> { | |
| + response.setCharacterEncoding("UTF-8"); | |
| + chain.doFilter(request, response); | |
| + }) | |
| + .build(); | |
| objectMapper = new ObjectMapper(); |
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
getCurrentUser()는 추후 삭제할 예정입니다.
도메인 관련 설정 추가 필요합니다.
Summary by CodeRabbit
New Features
Tests