Implementing a new EventResponseDTO with field rearrangement #8082
Conversation
…entdto # Conflicts: # core/src/main/java/greencity/controller/EventController.java
…939/update-eventdto
WalkthroughThis pull request introduces enhancements to the event management system by adding a new version of the event retrieval endpoint ( Changes
Possibly related PRs
Suggested Reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
service-api/src/main/java/greencity/dto/event/EventInformationDto.java (1)
11-15: Consider using more specific Lombok annotations.While
@Datais convenient, it provides more functionality than typically needed for a DTO. Consider replacing it with more specific annotations:@Builder @NoArgsConstructor @AllArgsConstructor -@Data +@Getter +@ToStringThis provides better encapsulation and prevents unintended side effects from the generated
equals,hashCode, andsettermethods.service-api/src/main/java/greencity/dto/event/EventResponseDto.java (2)
23-23: Standardize boolean field naming conventionThere's an inconsistency in boolean field naming:
- Some use
isprefix in field name (isOpen)- Others don't (
isSubscribed,isFavorite,isOrganizedByFriendare marked with @JsonProperty but field names don't match)Consider standardizing to either:
- private Boolean isOpen; + @JsonProperty("isOpen") + private boolean open; - @JsonProperty("isSubscribed") - private boolean isSubscribed; + @JsonProperty("isSubscribed") + private boolean subscribed; - @JsonProperty("isFavorite") - private boolean isFavorite; + @JsonProperty("isFavorite") + private boolean favorite; - @JsonProperty("isOrganizedByFriend") - private boolean isOrganizedByFriend; + @JsonProperty("isOrganizedByFriend") + private boolean organizedByFriend;Also applies to: 37-42, 51-53
25-26: Document the @max constraintsThe @max constraints on lists are good, but would benefit from documentation explaining the business rules behind these limits.
+ /** + * Maximum of 7 dates allowed per event to maintain UI consistency + * and prevent information overload. + */ @Max(7) private List<EventDateInformationDto> dates; + /** + * Maximum of 4 additional images allowed per event to optimize + * page load times and storage usage. + */ @Nullable @Max(4) private List<String> additionalImages;Also applies to: 31-33
service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java (2)
34-45: Enhance test coverage with edge casesThe current test verifies basic conversion, but consider adding tests for:
- Empty lists (additionalImages, dates)
- Null fields
- Maximum list size validation
Example test case:
@Test void convertWithNullFieldsTest() { Event event = getEvent(); event.setAdditionalImages(null); event.setDates(null); EventResponseDto result = mapper.convert(event); assertNull(result.getAdditionalImages()); assertNull(result.getDates()); }
47-53: Add test for false relevance caseThe current test only verifies the true case for isRelevant. Add a test for when an event should be marked as not relevant.
Example test case:
@Test void isRelevantFieldFalseTest() { Event event = getEvent(); event.setDates(List.of()); // Empty dates list should make event irrelevant EventResponseDto result = mapper.convert(event); assertFalse(result.getIsRelevant()); }service/src/test/java/greencity/utils/EventUtilsTest.java (1)
11-11: Enhance test readability with @DisplayName annotations.Consider adding @DisplayName annotations to improve test documentation and readability in test reports.
+@DisplayName("EventUtils Test") class EventUtilsTest {service/src/main/java/greencity/mapping/events/EventResponseDtoMapper.java (1)
27-30: Consider externalizing the MAX_ADDITIONAL_IMAGES constant.Move this configuration value to application properties for easier maintenance and environment-specific customization.
- private static final int MAX_ADDITIONAL_IMAGES = 4; + @Value("${event.max-additional-images:4}") + private int maxAdditionalImages;service-api/src/main/java/greencity/service/EventService.java (1)
49-55: Enhance method documentation.Add documentation for the principal parameter to maintain consistency with other method documentation.
/** * Method for getting Event instance. * * @param eventId - event id. + * @param principal - the currently authenticated user * @return {@link EventResponseDto} instance. */ EventResponseDto getEventV2(Long eventId, Principal principal);core/src/test/java/greencity/controller/EventControllerTest.java (1)
761-780: Good test coverage for the happy path!The test properly verifies the response status, content, and service interaction.
Consider adding a test case for when the service throws a
NotFoundException, similar to other controller tests in this file:@Test @SneakyThrows void getEventV2NotFoundTest() { Long eventId = 1L; when(eventService.getEventV2(eventId, principal)) .thenThrow(new NotFoundException("Event not found")); mockMvc.perform(get(EVENTS_CONTROLLER_LINK + "/v2/{eventId}", eventId) .principal(principal)) .andExpect(status().isNotFound()); }core/src/main/java/greencity/controller/EventController.java (1)
169-189: Clean implementation with thorough documentation!The new endpoint follows the established patterns and includes comprehensive OpenAPI documentation.
Consider enhancing the operation summary to clarify the differences between v1 and v2 endpoints:
- @Operation(summary = "Get the event") + @Operation(summary = "Get the event (v2 - includes additional event information)")service/src/main/java/greencity/service/EventServiceImpl.java (1)
246-258: Consider adding error handling for user mapping.While the implementation looks good overall, consider adding error handling for the case where the user mapping fails. This could happen if the user from the Principal doesn't exist in the system.
@Override public EventResponseDto getEventV2(Long eventId, Principal principal) { Event event = eventRepo.findById(eventId) .orElseThrow(() -> new NotFoundException(ErrorMessage.EVENT_NOT_FOUND)); if (principal != null) { - User currentUser = modelMapper.map(restClient.findByEmail(principal.getName()), User.class); + UserVO userVO = restClient.findByEmail(principal.getName()); + if (userVO == null) { + throw new NotFoundException(ErrorMessage.USER_NOT_FOUND_BY_EMAIL + principal.getName()); + } + User currentUser = modelMapper.map(userVO, User.class); return buildEventResponseDto(event, currentUser.getId()); } return modelMapper.map(event, EventResponseDto.class); }service/src/test/java/greencity/service/EventServiceImplTest.java (1)
544-588: Consider adding tests for error scenarios.While the current tests cover the happy paths well, consider adding tests for the following scenarios:
- Event not found
- User not found when mapping from Principal
- ModelMapper throws an exception during mapping
Example test for the "Event not found" scenario:
@Test void getEventV2ThrowsNotFoundExceptionWhenEventNotFound() { when(eventRepo.findById(anyLong())).thenReturn(Optional.empty()); assertThrows(NotFoundException.class, () -> eventService.getEventV2(1L, null), "Should throw NotFoundException when event is not found"); verify(eventRepo).findById(1L); verifyNoInteractions(modelMapper); }core/src/test/java/greencity/ModelUtils.java (3)
600-614: Consider adding validation for the test data.The basic event information setup looks good. However, consider adding edge cases or boundary values for testing purposes. For example:
- Very long title/description
- Special characters in the title
- Empty tags list
615-635: Enhance date-time test coverage.The date-time and location setup is well-structured. Consider adding test data variations:
- Multiple dates for the same event
- Different time zones
- Edge cases for coordinates (e.g., boundary values)
dates(List.of( EventDateInformationDto.builder() .startDate(ZonedDateTime.of(2025, 12, 26, 12, 30, 0, 0, ZoneOffset.UTC)) .finishDate(ZonedDateTime.of(2025, 12, 26, 21, 59, 0, 0, ZoneOffset.UTC)) .onlineLink("www.testlink.com") .coordinates(AddressDto.builder() // existing coordinates .build()) .build(), + // Add a second date with different time zone + EventDateInformationDto.builder() + .startDate(ZonedDateTime.of(2025, 12, 27, 12, 30, 0, 0, ZoneOffset.of("+02:00"))) + .finishDate(ZonedDateTime.of(2025, 12, 27, 21, 59, 0, 0, ZoneOffset.of("+02:00"))) + .coordinates(AddressDto.builder() + .latitude(-90.0) // South Pole + .longitude(180.0) // International Date Line + .build()) + .build() ))
636-648: Consider adding null value test cases.The additional event details are well-defined. However, consider adding test cases for:
- Null images
- Zero likes/dislikes
- Negative ratings
This would help ensure the application handles edge cases gracefully.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
core/src/main/java/greencity/config/SecurityConfig.java(1 hunks)core/src/main/java/greencity/controller/EventController.java(3 hunks)core/src/test/java/greencity/ModelUtils.java(4 hunks)core/src/test/java/greencity/controller/EventControllerTest.java(2 hunks)service-api/src/main/java/greencity/dto/event/EventDateInformationDto.java(1 hunks)service-api/src/main/java/greencity/dto/event/EventInformationDto.java(1 hunks)service-api/src/main/java/greencity/dto/event/EventResponseDto.java(1 hunks)service-api/src/main/java/greencity/service/EventService.java(2 hunks)service/src/main/java/greencity/mapping/events/EventDtoMapper.java(4 hunks)service/src/main/java/greencity/mapping/events/EventResponseDtoMapper.java(1 hunks)service/src/main/java/greencity/service/EventServiceImpl.java(4 hunks)service/src/main/java/greencity/utils/EventUtils.java(1 hunks)service/src/test/java/greencity/ModelUtils.java(2 hunks)service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java(1 hunks)service/src/test/java/greencity/service/EventServiceImplTest.java(2 hunks)service/src/test/java/greencity/utils/EventUtilsTest.java(1 hunks)
🔇 Additional comments (13)
service-api/src/main/java/greencity/dto/event/EventInformationDto.java (2)
1-10: Clean and well-organized imports!Good choice using Jakarta EE validation constraints for bean validation.
15-20: Verify integration with EventResponseDto.Since this DTO is used within
EventResponseDto, ensure that the validation constraints are properly cascaded.service-api/src/main/java/greencity/dto/event/EventDateInformationDto.java (1)
16-17: Review circular dependency between DTOsThere appears to be a circular dependency between
EventDateInformationDtoandEventResponseDto. This could lead to infinite recursion during JSON serialization/deserialization.Consider breaking this circular reference by:
- Creating a simplified version of EventResponseDto for this use case
- Using only the event ID instead of the full DTO
- Adding
@JsonIgnoreor custom serialization handlingWould you like me to provide an example implementation for any of these approaches?
service/src/test/java/greencity/utils/EventUtilsTest.java (1)
11-47: Well-structured test coverage for isRelevant!The tests effectively cover essential scenarios:
- Null input handling
- Empty list handling
- Past events
- Mixed past/future events
- Edge case with current time
service/src/main/java/greencity/mapping/events/EventDtoMapper.java (2)
53-53: Good refactoring to use EventUtils!Moving the isRelevant logic to a utility class improves code reusability and maintainability.
82-82: Consistent use of EventUtils for rate calculation!Good practice to centralize the rate calculation logic in the utility class.
core/src/main/java/greencity/config/SecurityConfig.java (1)
196-196: LGTM! The security configuration for the new endpoint is properly integrated.The new endpoint
/events/v2/{eventId}is correctly added to the list of permitted endpoints, maintaining consistency with other event-related endpoints.core/src/test/java/greencity/controller/EventControllerTest.java (1)
782-789: Good negative test case!The test properly verifies that invalid IDs are rejected and the service is not called.
service/src/main/java/greencity/service/EventServiceImpl.java (1)
16-16: Import looks good!The new import for
EventResponseDtois correctly placed within the event DTO imports section.service/src/test/java/greencity/service/EventServiceImplTest.java (1)
16-16: Import looks good!The new import for
EventResponseDtois correctly placed within the event DTO imports section.service/src/test/java/greencity/ModelUtils.java (2)
3438-3466: Well-structured test data builder with comprehensive field coverage.The implementation provides a good foundation for testing with realistic data. The use of the builder pattern and clear field organization makes the code maintainable.
A few suggestions to enhance the test data:
- Consider adding edge cases like null fields
- Add comments explaining any non-obvious field values or relationships
- Consider parameterizing the method to allow customization of key fields
public static EventResponseDto getEventResponseDto() { + // Create event response with future dates to ensure test relevance return EventResponseDto.builder() .id(1L) .eventInformation(EventInformationDto.builder() .title("Title") - .description("New Test Event") + .description("New Test Event") // Consider adding longer text to test UI wrapping .tags(List.of(TagUaEnDto.builder() .id(2L) .nameUa("Соціальний") .nameEn("Social") .build())) .build()) .organizer(EventAuthorDto.builder().id(1L).name("Test").build()) .creationDate(LocalDate.of(2025, 1, 10)) .isOpen(true) .dates(List.of( EventDateInformationDto.builder() .startDate(ZonedDateTime.of(2025, 12, 26, 12, 30, 0, 0, ZoneOffset.UTC)) .finishDate(ZonedDateTime.of(2025, 12, 26, 21, 59, 0, 0, ZoneOffset.UTC)) .onlineLink("www.link.com") .coordinates(getAddressDtoCorrect()).build())) .type(EventType.OFFLINE) .isRelevant(true) .countComments(1) .isSubscribed(false) .isFavorite(false) .isOrganizedByFriend(false) .build(); }
3468-3473: Clean utility method with focused responsibility.The method follows good practices:
- Clear method name that describes its purpose
- Uses builder pattern for object creation
- Takes only required parameters
Consider adding validation for the date parameters:
public static EventDateLocation createEventDateLocation(ZonedDateTime start, ZonedDateTime finish) { + if (start != null && finish != null && start.isAfter(finish)) { + throw new IllegalArgumentException("Start date must be before finish date"); + } return EventDateLocation.builder() .startDate(start) .finishDate(finish) .build(); }core/src/test/java/greencity/ModelUtils.java (1)
21-21: LGTM! Well-organized imports.The new imports are correctly placed and organized, specifically importing the necessary DTOs for the new event response implementation.
Also applies to: 24-25
| private String title; | ||
| private String description; | ||
| @NotEmpty | ||
| private List<TagUaEnDto> tags; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation constraints to prevent potential security issues.
The title and description fields should have appropriate validation constraints to prevent security issues and ensure data quality:
public class EventInformationDto {
+ @NotBlank
+ @Size(min = 1, max = 255)
private String title;
+ @NotBlank
+ @Size(max = 5000)
private String description;
@NotEmpty
+ @Size(max = 100)
private List<TagUaEnDto> tags;
}This ensures:
- Title is not blank and has a reasonable length
- Description is not blank and has a maximum length
- Tags list has a reasonable size limit
📝 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.
| private String title; | |
| private String description; | |
| @NotEmpty | |
| private List<TagUaEnDto> tags; | |
| } | |
| @NotBlank | |
| @Size(min = 1, max = 255) | |
| private String title; | |
| @NotBlank | |
| @Size(max = 5000) | |
| private String description; | |
| @NotEmpty | |
| @Size(max = 100) | |
| private List<TagUaEnDto> tags; | |
| } |
| public static double calculateEventRate(List<EventGrade> eventGrades) { | ||
| return eventGrades.stream() | ||
| .mapToInt(EventGrade::getGrade) | ||
| .average() | ||
| .orElse(0.0); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check for eventGrades parameter
The method should handle null input gracefully to prevent NullPointerException.
public static double calculateEventRate(List<EventGrade> eventGrades) {
+ if (eventGrades == null) {
+ return 0.0;
+ }
return eventGrades.stream()
.mapToInt(EventGrade::getGrade)
.average()
.orElse(0.0);
}📝 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 static double calculateEventRate(List<EventGrade> eventGrades) { | |
| return eventGrades.stream() | |
| .mapToInt(EventGrade::getGrade) | |
| .average() | |
| .orElse(0.0); | |
| } | |
| public static double calculateEventRate(List<EventGrade> eventGrades) { | |
| if (eventGrades == null) { | |
| return 0.0; | |
| } | |
| return eventGrades.stream() | |
| .mapToInt(EventGrade::getGrade) | |
| .average() | |
| .orElse(0.0); | |
| } |
| public static boolean isRelevant(List<EventDateLocation> dates) { | ||
| if (dates == null || dates.isEmpty()) { | ||
| return false; | ||
| } | ||
| return dates.getLast().getFinishDate().isAfter(ZonedDateTime.now()) | ||
| || dates.getLast().getFinishDate().isEqual(ZonedDateTime.now()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider backwards compatibility for Java versions
The getLast() method was introduced in Java 21. Consider using an alternative approach for broader compatibility:
- return dates.getLast().getFinishDate().isAfter(ZonedDateTime.now())
- || dates.getLast().getFinishDate().isEqual(ZonedDateTime.now());
+ EventDateLocation lastDate = dates.get(dates.size() - 1);
+ ZonedDateTime now = ZonedDateTime.now();
+ return lastDate.getFinishDate().isAfter(now)
+ || lastDate.getFinishDate().isEqual(now);📝 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 static boolean isRelevant(List<EventDateLocation> dates) { | |
| if (dates == null || dates.isEmpty()) { | |
| return false; | |
| } | |
| return dates.getLast().getFinishDate().isAfter(ZonedDateTime.now()) | |
| || dates.getLast().getFinishDate().isEqual(ZonedDateTime.now()); | |
| } | |
| public static boolean isRelevant(List<EventDateLocation> dates) { | |
| if (dates == null || dates.isEmpty()) { | |
| return false; | |
| } | |
| EventDateLocation lastDate = dates.get(dates.size() - 1); | |
| ZonedDateTime now = ZonedDateTime.now(); | |
| return lastDate.getFinishDate().isAfter(now) | |
| || lastDate.getFinishDate().isEqual(now); | |
| } |
| @Test | ||
| void calculateEventRateReturnsCorrectAverageTest() { | ||
| List<EventGrade> grades = List.of( | ||
| EventGrade.builder().grade(4).build(), | ||
| EventGrade.builder().grade(5).build(), | ||
| EventGrade.builder().grade(3).build()); | ||
| assertEquals(4.0, EventUtils.calculateEventRate(grades)); | ||
| } | ||
|
|
||
| @Test | ||
| void calculateEventRateReturnsSingleValueForOneGradeTest() { | ||
| List<EventGrade> grades = List.of(EventGrade.builder().grade(5).build()); | ||
| assertEquals(5.0, EventUtils.calculateEventRate(grades)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for calculateEventRate.
Consider adding these test cases:
- Empty list scenario
- Null input handling
- Invalid grade values (negative or exceeding maximum)
+ @Test
+ void calculateEventRateReturnsZeroForEmptyListTest() {
+ assertEquals(0.0, EventUtils.calculateEventRate(List.of()));
+ }
+
+ @Test
+ void calculateEventRateHandlesNullInputTest() {
+ assertEquals(0.0, EventUtils.calculateEventRate(null));
+ }
+
+ @Test
+ void calculateEventRateHandlesInvalidGradesTest() {
+ List<EventGrade> grades = List.of(
+ EventGrade.builder().grade(-1).build(),
+ EventGrade.builder().grade(6).build());
+ assertEquals(0.0, EventUtils.calculateEventRate(grades));
+ }📝 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.
| @Test | |
| void calculateEventRateReturnsCorrectAverageTest() { | |
| List<EventGrade> grades = List.of( | |
| EventGrade.builder().grade(4).build(), | |
| EventGrade.builder().grade(5).build(), | |
| EventGrade.builder().grade(3).build()); | |
| assertEquals(4.0, EventUtils.calculateEventRate(grades)); | |
| } | |
| @Test | |
| void calculateEventRateReturnsSingleValueForOneGradeTest() { | |
| List<EventGrade> grades = List.of(EventGrade.builder().grade(5).build()); | |
| assertEquals(5.0, EventUtils.calculateEventRate(grades)); | |
| } | |
| @Test | |
| void calculateEventRateReturnsCorrectAverageTest() { | |
| List<EventGrade> grades = List.of( | |
| EventGrade.builder().grade(4).build(), | |
| EventGrade.builder().grade(5).build(), | |
| EventGrade.builder().grade(3).build()); | |
| assertEquals(4.0, EventUtils.calculateEventRate(grades)); | |
| } | |
| @Test | |
| void calculateEventRateReturnsSingleValueForOneGradeTest() { | |
| List<EventGrade> grades = List.of(EventGrade.builder().grade(5).build()); | |
| assertEquals(5.0, EventUtils.calculateEventRate(grades)); | |
| } | |
| @Test | |
| void calculateEventRateReturnsZeroForEmptyListTest() { | |
| assertEquals(0.0, EventUtils.calculateEventRate(List.of())); | |
| } | |
| @Test | |
| void calculateEventRateHandlesNullInputTest() { | |
| assertEquals(0.0, EventUtils.calculateEventRate(null)); | |
| } | |
| @Test | |
| void calculateEventRateHandlesInvalidGradesTest() { | |
| List<EventGrade> grades = List.of( | |
| EventGrade.builder().grade(-1).build(), | |
| EventGrade.builder().grade(6).build()); | |
| assertEquals(0.0, EventUtils.calculateEventRate(grades)); | |
| } |
| .coordinates(AddressDto.builder() | ||
| .latitude(date.getAddress().getLatitude()) | ||
| .longitude(date.getAddress().getLongitude()) | ||
| .streetEn(date.getAddress().getStreetEn()) | ||
| .streetUa(date.getAddress().getStreetUa()) | ||
| .houseNumber(date.getAddress().getHouseNumber()) | ||
| .cityEn(date.getAddress().getCityEn()) | ||
| .cityUa(date.getAddress().getCityUa()) | ||
| .regionEn(date.getAddress().getRegionEn()) | ||
| .regionUa(date.getAddress().getRegionUa()) | ||
| .countryEn(date.getAddress().getCountryEn()) | ||
| .countryUa(date.getAddress().getCountryUa()) | ||
| .formattedAddressEn(date.getAddress().getFormattedAddressEn()) | ||
| .formattedAddressUa(date.getAddress().getFormattedAddressUa()) | ||
| .build()) |
There was a problem hiding this comment.
Add null check for address to prevent NPE.
The current implementation assumes the address is never null. Add null safety checks:
- .coordinates(AddressDto.builder()
- .latitude(date.getAddress().getLatitude())
- .longitude(date.getAddress().getLongitude())
+ .coordinates(date.getAddress() != null ? AddressDto.builder()
+ .latitude(date.getAddress().getLatitude())
+ .longitude(date.getAddress().getLongitude())
// ... other fields ...
- .build())
+ .build() : null)Committable suggestion skipped: line range outside the PR's diff.
| private EventResponseDto buildEventResponseDto(Event event, Long userId) { | ||
| EventResponseDto eventResponseDto = modelMapper.map(event, EventResponseDto.class); | ||
| Integer currentUserGrade = event.getEventGrades() | ||
| .stream() | ||
| .filter(g -> g.getUser() != null && g.getUser().getId().equals(userId)) | ||
| .map(EventGrade::getGrade) | ||
| .findFirst() | ||
| .orElse(null); | ||
| setFollowers(List.of(), userId); | ||
| setSubscribes(List.of(), userId); | ||
| eventResponseDto.setCurrentUserGrade(currentUserGrade); | ||
|
|
||
| return eventResponseDto; | ||
| } |
There was a problem hiding this comment.
Fix the usage of empty lists in setFollowers and setSubscribes.
The current implementation passes empty lists to setFollowers and setSubscribes, which differs from the pattern in buildEventDto where singleton lists are used. This might lead to inconsistent behavior.
private EventResponseDto buildEventResponseDto(Event event, Long userId) {
EventResponseDto eventResponseDto = modelMapper.map(event, EventResponseDto.class);
Integer currentUserGrade = event.getEventGrades()
.stream()
.filter(g -> g.getUser() != null && g.getUser().getId().equals(userId))
.map(EventGrade::getGrade)
.findFirst()
.orElse(null);
- setFollowers(List.of(), userId);
- setSubscribes(List.of(), userId);
+ setFollowers(List.of(eventResponseDto), userId);
+ setSubscribes(List.of(eventResponseDto), userId);
eventResponseDto.setCurrentUserGrade(currentUserGrade);
return eventResponseDto;
}📝 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.
| private EventResponseDto buildEventResponseDto(Event event, Long userId) { | |
| EventResponseDto eventResponseDto = modelMapper.map(event, EventResponseDto.class); | |
| Integer currentUserGrade = event.getEventGrades() | |
| .stream() | |
| .filter(g -> g.getUser() != null && g.getUser().getId().equals(userId)) | |
| .map(EventGrade::getGrade) | |
| .findFirst() | |
| .orElse(null); | |
| setFollowers(List.of(), userId); | |
| setSubscribes(List.of(), userId); | |
| eventResponseDto.setCurrentUserGrade(currentUserGrade); | |
| return eventResponseDto; | |
| } | |
| private EventResponseDto buildEventResponseDto(Event event, Long userId) { | |
| EventResponseDto eventResponseDto = modelMapper.map(event, EventResponseDto.class); | |
| Integer currentUserGrade = event.getEventGrades() | |
| .stream() | |
| .filter(g -> g.getUser() != null && g.getUser().getId().equals(userId)) | |
| .map(EventGrade::getGrade) | |
| .findFirst() | |
| .orElse(null); | |
| setFollowers(List.of(eventResponseDto), userId); | |
| setSubscribes(List.of(eventResponseDto), userId); | |
| eventResponseDto.setCurrentUserGrade(currentUserGrade); | |
| return eventResponseDto; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java (2)
Line range hint
1-400: Enhance test coverage with edge cases and error scenariosThe test suite provides good coverage for happy paths, but could benefit from additional test cases:
Invalid input validation:
- Malformed JSON
- Invalid dates (e.g., end date before start date)
- Missing required fields
Error handling:
- Concurrent modification scenarios
- Resource not found cases
- Permission denied scenarios
Pagination edge cases:
- Empty result sets
- Last page scenarios
- Maximum page size limits
Here's an example test for invalid input:
@Test @SneakyThrows void testCreateEventWithInvalidJson() { String invalidJson = "{\"title\":\"test\",\"invalidField\":true}"; MockMultipartFile addEventDtoRequestJSON = new MockMultipartFile("addEventDtoRequest", "", "application/json", invalidJson.getBytes()); mockMvc.perform(multipart(MANAGEMENT_EVENTS_LINK) .file(addEventDtoRequestJSON) .principal(() -> "user")) .andExpect(status().isBadRequest()); }
Line range hint
237-307: Consider refactoring long test methods for better maintainabilityThe
getEventTestmethod is quite long and tests multiple aspects. Consider breaking it down into smaller, focused test methods:
- Test date formatting separately
- Test image handling separately
- Test attender handling separately
Here's an example of how to split the test:
@Test @SneakyThrows void testEventDateFormatting() { Long eventId = 1L; EventDto eventDto = createEventDtoWithDates(); // Extract to helper method when(eventService.getEvent(eventId, principal)).thenReturn(eventDto); mockMvc.perform(get(MANAGEMENT_EVENTS_LINK + "/{eventId}", eventId).principal(principal)) .andExpect(status().isOk()) .andExpect(model().attribute("formattedDate", "Jan 1, 2025 - Jan 2, 2025")); } @Test @SneakyThrows void testEventImageHandling() { Long eventId = 1L; EventDto eventDto = createEventDtoWithImages(); // Extract to helper method when(eventService.getEvent(eventId, principal)).thenReturn(eventDto); mockMvc.perform(get(MANAGEMENT_EVENTS_LINK + "/{eventId}", eventId).principal(principal)) .andExpect(status().isOk()) .andExpect(model().attribute("imageUrls", List.of("title-image.jpg", "image1.jpg", "image2.jpg"))); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/test/java/greencity/webcontroller/ManagementEventControllerTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| @@ -78,6 +78,7 @@ class ManagementEventControllerTest { | |||
|
|
|||
| @BeforeEach | |||
| void setUp() { | |||
| Locale.setDefault(Locale.ENGLISH); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using per-test locale configuration instead of global default
Setting the default locale globally can affect other tests running in parallel and may mask locale-specific issues. Consider these alternatives:
- Use explicit locales in each test method where needed
- Create a test configuration class for locale settings
- Use
@BeforeEachto reset locale after each test
Here's an example of using explicit locales:
- Locale.setDefault(Locale.ENGLISH);
+ // Remove global locale setting and instead use in specific tests:
+ // Example for a test method:
+ mockMvc.perform(get(MANAGEMENT_EVENTS_LINK)
+ .locale(Locale.ENGLISH) // Set locale per request
+ ...)Or if global setting is necessary, ensure cleanup:
@BeforeEach
void setUp() {
- Locale.setDefault(Locale.ENGLISH);
+ Locale defaultLocale = Locale.getDefault();
+ Locale.setDefault(Locale.ENGLISH);
this.mockMvc = MockMvcBuilders.standaloneSetup(managementEventController)
.setCustomArgumentResolvers(new PageableHandlerMethodArgumentResolver())
.setValidator(mockValidator)
.build();
+
+ // Register cleanup callback
+ // Note: Requires JUnit 5.8.0 or later
+ cleanupCallback(() -> Locale.setDefault(defaultLocale));
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/greencity/ModelUtils.java (1)
598-647: Well-structured test data, with room for improvement.The implementation provides comprehensive test data with realistic values. However, consider these improvements:
- Extract commonly used test values into constants
- Break down the builder into helper methods for better reusability
Here's a suggested refactor:
+ private static final String TEST_ONLINE_LINK = "www.testlink.com"; + private static final String TEST_IMAGE_URL = "https://test.png"; + private static final double TEST_LATITUDE = 50.44628775288652; + private static final double TEST_LONGITUDE = 30.49364829378446; + private static AddressDto createTestAddress() { + return AddressDto.builder() + .latitude(TEST_LATITUDE) + .longitude(TEST_LONGITUDE) + .streetEn("Halytska Square") + .streetUa("Галицька площа") + // ... rest of the address fields + .build(); + } + private static EventDateInformationDto createTestEventDate() { + return EventDateInformationDto.builder() + .startDate(ZonedDateTime.of(2025, 12, 26, 12, 30, 0, 0, ZoneOffset.UTC)) + .finishDate(ZonedDateTime.of(2025, 12, 26, 21, 59, 0, 0, ZoneOffset.UTC)) + .onlineLink(TEST_ONLINE_LINK) + .coordinates(createTestAddress()) + .build(); + } public static EventResponseDto getEventResponseDto() { return EventResponseDto.builder() .id(1L) .eventInformation(EventInformationDto.builder() .title("Test Event") .description("New Test Event") .tags(List.of(TagUaEnDto.builder() .id(2L) .nameUa("Соціальний") .nameEn("Social") .build())) .build()) .organizer(EventAuthorDto.builder().id(1L).name("Test").email("[email protected]").build()) .creationDate(LocalDate.of(2025, 1, 10)) .isOpen(true) - .dates(List.of( - EventDateInformationDto.builder() - .startDate(ZonedDateTime.of(2025, 12, 26, 12, 30, 0, 0, ZoneOffset.UTC)) - .finishDate(ZonedDateTime.of(2025, 12, 26, 21, 59, 0, 0, ZoneOffset.UTC)) - .onlineLink("www.testlink.com") - .coordinates(AddressDto.builder() - .latitude(50.44628775288652) - .longitude(30.49364829378446) - // ... rest of the address fields - .build()) - .build())) + .dates(List.of(createTestEventDate())) - .titleImage("https://test.png") + .titleImage(TEST_IMAGE_URL) // ... rest of the builder .build(); }This refactoring:
- Improves reusability of test data components
- Makes the code more maintainable
- Reduces duplication
- Makes it easier to modify test values in one place
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/test/java/greencity/ModelUtils.java(3 hunks)service/src/main/java/greencity/utils/EventUtils.java(1 hunks)service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- service/src/test/java/greencity/mapping/events/EventResponseDtoMapperTest.java
- service/src/main/java/greencity/utils/EventUtils.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
core/src/test/java/greencity/ModelUtils.java (1)
21-21: LGTM! Clean import organization.The new imports for event-related DTOs are properly organized and grouped with other DTO imports.
Also applies to: 24-25
|
GreenCity PR
Issue Link 📋
#7939
Changed
*added a new EventResponseDTO with field rearrangement
*Added a new endpoint "events/v2/{eventId}" to retreive Event using new dto
*Added tests
PR Checklist Forms
(to be filled out by PR submitter)
developbranch.NEED_REVIEWandREADY_FOR_REVIEWlabels are added.Summary by CodeRabbit
New Features
/v2/{eventId})Improvements
Bug Fixes
Tests
getEventV2functionality and event utilitiesEventResponseDtoMapperandEventServiceImplChores