Bug/7682/fix management page #7975
Conversation
…, eorking howers, or day fields is empty, and if the difference between the end time and start time is les then 30 minutes, add message ua and en
core/src/test/java/greencity/controller/PlaceControllerTest.java
Outdated
Show resolved
Hide resolved
core/src/main/resources/static/management/places/buttonsAJAX.js
Outdated
Show resolved
Hide resolved
| @@ -230,6 +230,25 @@ void saveTest() { | |||
| placeVO.getCategory().getName(), placeVO.getName()); | |||
| } | |||
|
|
|||
| @Test | |||
| void saveShouldSavePlaceSuccessfully() { | |||
There was a problem hiding this comment.
https://github.com/ita-social-projects/GreenCity/wiki/Naming-Conventions
please check our naming conventions
There was a problem hiding this comment.
please add "Test" to the end of test method name so that it was by out naming convention
WalkthroughThe pull request introduces comprehensive updates to the place management functionality in the GreenCity application. The changes span multiple components, including controllers, services, repositories, and frontend resources. Key modifications involve enhancing validation mechanisms, improving error handling, adding multilingual support for addresses, and refactoring the place update process. The updates focus on providing a more robust and user-friendly experience for managing place information. Changes
Sequence DiagramsequenceDiagram
participant User
participant Controller
participant Service
participant Repository
User->>Controller: Submit Place Update
Controller->>Controller: Validate Input
Controller->>Service: updateFromUI()
Service->>Service: Update Location
Service->>Service: Update Place Properties
Service->>Repository: Save Changes
Repository-->>Service: Confirmation
Service-->>Controller: Updated Place
Controller-->>User: Success Response
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (8)
🪧 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
🔭 Outside diff range comments (1)
core/src/main/resources/templates/core/management_places.html (1)
Line range hint
489-490: Secure the Google Maps API keyThe Google Maps API key is exposed in the template. Consider moving it to a secure configuration and implementing proper key restrictions.
The API key should be:
- Stored in environment variables or secure configuration
- Restricted to specific domains/IPs
- Limited to only necessary Google Maps APIs
🧹 Nitpick comments (19)
service/src/test/java/greencity/mapping/PhotoVOMapperTest.java (3)
25-42: Well-structured test for null field handling! Consider adding @DisplayName.The test thoroughly verifies the mapper's behavior with null fields. Good use of the builder pattern and explicit assertions.
Consider adding JUnit 5's @DisplayName for better test reporting:
@Test + @DisplayName("Should convert Photo with all null fields to PhotoVO with all null fields") void convertWithNullFieldsTest() {
44-62: Good test for partial field mapping! Consider extracting test data setup.The test effectively verifies partial field mapping. Good practice using meaningful test data ("partial_photo").
Consider extracting the test data setup to improve maintainability:
private Photo createPartialPhoto() { return greencity.entity.Photo.builder() .id(1L) .name("partial_photo") .build(); } private PhotoVO createExpectedPartialPhotoVO() { return PhotoVO.builder() .id(1L) .name("partial_photo") .commentId(null) .placeId(null) .userId(null) .build(); }Then use these methods in your test:
@Test + @DisplayName("Should correctly convert Photo with partial fields to PhotoVO") void convertWithPartialFieldsTest() { - var source = greencity.entity.Photo.builder()... - var expected = PhotoVO.builder()... + var source = createPartialPhoto(); + var expected = createExpectedPartialPhotoVO(); assertEquals(expected, mapper.convert(source)); }
25-62: Consider adding edge cases to strengthen test coverage.While the current tests cover basic null and partial field scenarios, consider adding tests for edge cases.
Here are some scenarios to consider:
- Test with empty strings vs null strings
- Test with special characters in the name field
- Test with maximum field lengths
- Test with minimum valid values for numeric fields
Would you like me to provide example implementations for these test cases?
service/src/main/java/greencity/mapping/PhotoVOMapper.java (1)
Line range hint
19-24: Consider enhancing method documentationThe current JavaDoc could be more descriptive about how null values are handled. This would help other developers understand the expected behavior.
Consider updating the documentation like this:
/** * Method convert {@link Photo} to {@link PhotoVO}. + * Handles null values gracefully for place, user, and comment relationships. * * @return {@link Photo} */service-api/src/main/java/greencity/service/PlaceService.java (2)
96-104: Enhance method documentation for better clarityThe documentation for
updateFromUIcould be more descriptive to help implementers understand:
- The relationship between this method and the existing
updatemethod- The validation rules for images (allowed formats, sizes, etc.)
- The role of the admin email parameter
Consider expanding the documentation like this:
/** * Method for updating from admin panel {@link PlaceVO}. * + * This method extends the base update functionality to support image uploads + * and admin-specific operations. It should be used exclusively for updates + * initiated from the admin UI. * * @param dto - dto for Place entity - * @param images - array of photos + * @param images - array of photos (supported formats: JPEG, PNG; max size: X MB) * @param email - admin user email + * @throws ValidationException if the images are invalid + * @throws UnauthorizedException if the email doesn't belong to an admin user * @return place {@link PlaceVO} */
96-104: Consider adding validation-related annotationsSince this method handles file uploads and admin authentication, consider adding relevant annotations to make the contract more explicit.
Add validation annotations to enforce constraints:
+ @PreAuthorize("hasRole('ADMIN')") + @Validated PlaceVO updateFromUI( + @Valid PlaceUpdateDto dto, + @Size(max = 10, message = "Maximum 10 images allowed") + @ContentType({"image/jpeg", "image/png"}) MultipartFile[] images, + @Email String email);core/src/main/java/greencity/webcontroller/ManagementPlacesController.java (1)
124-127: Enhance error handling consistencyWhile the validation pattern is good, consider adding specific error logging before returning the binding result. This would help with debugging validation failures in production.
if (!bindingResult.hasErrors()) { placeService.updateFromUI(placeUpdateDto, images, principal.getName()); +} else { + log.debug("Validation failed for place update: {}", + bindingResult.getAllErrors().stream() + .map(DefaultMessageSourceResolvable::getDefaultMessage) + .collect(Collectors.joining(", "))); }dao/src/main/java/greencity/repository/DiscountValuesRepo.java (1)
29-32: Well-structured query implementation with proper annotations!The enhanced implementation follows JPA best practices:
@Modifyingcorrectly indicates this is a modifying query@Transactionalensures data consistency- Parameter binding using
@Paramprevents SQL injection- JPQL query is clear and efficient
For large datasets, consider adding a batch size hint if bulk deletions are common:
@QueryHints(value = @QueryHint(name = "org.hibernate.jdbc.batch_size", value = "30"))core/src/main/resources/templates/core/management_places.html (1)
7-9: Consider consolidating CSS importsThe CSS structure has been modularized into separate files, which is good for maintenance. However, consider bundling these CSS files during production to reduce HTTP requests.
Also applies to: 23-25
core/src/test/java/greencity/webcontroller/ManagementPlacesControllerTest.java (3)
147-176: Enhance test coverage for file upload scenariosWhile the test properly handles the happy path, consider adding test cases for:
- Invalid image formats
- Empty files
- Large file sizes
- Multiple file uploads
Example test case for invalid format:
@Test void updatePlaceWithInvalidImageFormat() throws Exception { MockMultipartFile invalidImage = new MockMultipartFile( "images", "test.txt", MediaType.TEXT_PLAIN_VALUE, "invalid-image".getBytes(StandardCharsets.UTF_8)); // Test implementation }
178-194: Consider making the test data helper more flexibleThe
getPlaceUpdateDtomethod provides good test data, but consider:
- Adding builder patterns for nested objects
- Creating separate helper methods for different test scenarios
- Using test data factories or object mothers pattern
Example approach:
private static class PlaceUpdateDtoBuilder { private static PlaceUpdateDto defaultPlace() { return PlaceUpdateDto.builder() .id(1L) .name("Test Place") .build(); } private static PlaceUpdateDto placeWithDiscount() { return defaultPlace().toBuilder() .discountValues(createTestDiscounts()) .build(); } }
205-225: Enhance validation error testingThe test verifies the bad request scenario, but consider:
- Adding assertions for specific validation error messages
- Testing each required field individually
- Verifying the response body for validation details
Example enhancement:
mockMvc.perform(multipart("/management/places/") .file(new MockMultipartFile(...)) .contentType(MediaType.MULTIPART_FORM_DATA_VALUE)) .andExpect(status().isBadRequest()) .andExpect(jsonPath("$.errors").isArray()) .andExpect(jsonPath("$.errors[0].field").value("id")) .andExpect(jsonPath("$.errors[0].message").value("must not be null"));service/src/main/java/greencity/service/PlaceServiceImpl.java (1)
185-185: Consider restoring access modifiers to private for helper methodsThe methods
updateDiscountandupdateOpeninghave their access modifiers changed fromprivateto package-private. If these methods are not intended to be accessed outside this class, it’s best to keep themprivateto encapsulate functionality.service/src/main/java/greencity/service/LocationServiceImpl.java (1)
79-87: LGTM! Consider adding validation for location coordinates.The refactoring to work directly with the Location entity is a good improvement. The addition of addressUa supports multilingual functionality well.
Consider adding coordinate range validation to ensure lat/lng values are within valid bounds:
Location updatable = locationRepo.findById(id) .orElseThrow(() -> new NotFoundException(ErrorMessage.LOCATION_NOT_FOUND_BY_ID + id)); +if (location.getLat() < -90 || location.getLat() > 90 || + location.getLng() < -180 || location.getLng() > 180) { + throw new IllegalArgumentException("Invalid coordinates"); +} updatable.setLat(location.getLat()); updatable.setLng(location.getLng()); updatable.setAddress(location.getAddress()); updatable.setAddressUa(location.getAddressUa());service/src/test/java/greencity/service/PlaceServiceImplTest.java (3)
982-1037: Add more assertions to verify the complete state after updateOpening.The test coverage for updateOpening is good, but could be enhanced:
@Test void updateOpeningExecutesCorrectlyForValidInputTest() { Place updatedPlace = new Place(); updatedPlace.setId(1L); Set<OpeningHoursDto> hoursUpdateDtoSet = new HashSet<>(); OpeningHoursDto openingHoursDto = new OpeningHoursDto(); hoursUpdateDtoSet.add(openingHoursDto); Set<OpeningHoursVO> existingOpeningHoursVO = new HashSet<>(); OpeningHoursVO openingHoursVO = new OpeningHoursVO(); existingOpeningHoursVO.add(openingHoursVO); Mockito.when(openingHoursService.findAllByPlaceId(updatedPlace.getId())) .thenReturn(existingOpeningHoursVO); placeServiceImpl.updateOpening(hoursUpdateDtoSet, updatedPlace); + + // Verify the complete flow + ArgumentCaptor<OpeningHoursVO> openingHoursCaptor = ArgumentCaptor.forClass(OpeningHoursVO.class); + Mockito.verify(openingHoursService).save(openingHoursCaptor.capture()); + OpeningHoursVO savedHours = openingHoursCaptor.getValue(); + assertNotNull(savedHours, "Saved hours should not be null"); Mockito.verify(openingHoursService).deleteAllByPlaceId(updatedPlace.getId()); Mockito.verify(openingHoursService, Mockito.times(hoursUpdateDtoSet.size())) .save(Mockito.any(OpeningHoursVO.class)); }
1001-1053: Enhance updateDiscount tests with more specific scenarios.While the basic cases are covered, consider adding tests for:
+@Test +void updateDiscountHandlesEmptyDiscountSetTest() { + Place updatedPlace = new Place(); + updatedPlace.setId(1L); + Set<DiscountValueDto> emptyDiscounts = new HashSet<>(); + + Mockito.when(discountService.findAllByPlaceId(Mockito.anyLong())) + .thenReturn(new HashSet<>()); + + placeServiceImpl.updateDiscount(emptyDiscounts, updatedPlace); + + Mockito.verify(discountService).deleteAllByPlaceId(updatedPlace.getId()); + Mockito.verify(discountService, Mockito.never()).save(Mockito.any()); +} + +@Test +void updateDiscountPreservesDiscountValuesTest() { + Place updatedPlace = new Place(); + updatedPlace.setId(1L); + Set<DiscountValueDto> discounts = new HashSet<>(); + DiscountValueDto discount = new DiscountValueDto(); + discount.setValue(10); + discounts.add(discount); + + placeServiceImpl.updateDiscount(discounts, updatedPlace); + + ArgumentCaptor<DiscountValueVO> discountCaptor = ArgumentCaptor.forClass(DiscountValueVO.class); + Mockito.verify(discountService).save(discountCaptor.capture()); + assertEquals(10, discountCaptor.getValue().getValue()); +}
1055-1116: Good test coverage for updateFromUI. Consider adding edge cases.The tests cover the main scenarios well, but could be enhanced with:
+@Test +void updateFromUI_ShouldHandleEmptyImagesArray() { + PlaceUpdateDto dto = getPlaceUpdateDto(); + MultipartFile[] emptyImages = new MultipartFile[0]; + + when(categoryService.findByName(anyString())).thenReturn(categoryDtoResponse); + when(placeRepo.findById(anyLong())).thenReturn(Optional.of(genericEntity1)); + when(userRepo.findByEmail(anyString())).thenReturn(Optional.of(user)); + + PlaceVO result = placeServiceImpl.updateFromUI(dto, emptyImages, "[email protected]"); + + assertNotNull(result); + verify(photoRepo, never()).save(any(Photo.class)); +} +@Test +void updateFromUI_ShouldHandleNullLocation() { + PlaceUpdateDto dto = getPlaceUpdateDto(); + dto.setLocation(null); + + when(categoryService.findByName(anyString())).thenReturn(categoryDtoResponse); + when(placeRepo.findById(anyLong())).thenReturn(Optional.of(genericEntity1)); + when(userRepo.findByEmail(anyString())).thenReturn(Optional.of(user)); + + assertThrows(IllegalArgumentException.class, + () -> placeServiceImpl.updateFromUI(dto, null, "[email protected]")); +}service/src/test/java/greencity/ModelUtils.java (1)
3338-3344: Consider adding validation for coordinate ranges in test data.The LocationAddressAndGeoForUpdateDto constructor could validate coordinate ranges to catch potential issues early in tests.
public static LocationAddressAndGeoForUpdateDto getLocationAddressAndGeoForUpdateDto() { + double lat = 50.4501; + double lng = 30.5236; + if (lat < -90 || lat > 90 || lng < -180 || lng > 180) { + throw new IllegalArgumentException("Invalid coordinates in test data"); + } return new LocationAddressAndGeoForUpdateDto( "Test Address", - 50.4501, - 30.5236, + lat, + lng, "Тестова адреса"); }core/src/main/resources/static/management/places/buttonsAJAX.js (1)
190-210: Refactor form submission functions to reduce duplicationThe
submitPostFormDataandsubmitPutFormDatafunctions have similar logic for appending data toformDataand sending AJAX requests. Consider refactoring these into a single function to improve maintainability and reduce redundancy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
core/src/main/java/greencity/webcontroller/ManagementPlacesController.java(2 hunks)core/src/main/resources/messages.properties(1 hunks)core/src/main/resources/messages_ua.properties(1 hunks)core/src/main/resources/static/management/places/buttonsAJAX.js(4 hunks)core/src/main/resources/templates/core/management_places.html(8 hunks)core/src/test/java/greencity/controller/PlaceControllerTest.java(0 hunks)core/src/test/java/greencity/webcontroller/ManagementPlacesControllerTest.java(5 hunks)dao/src/main/java/greencity/repository/DiscountValuesRepo.java(2 hunks)service-api/src/main/java/greencity/dto/location/LocationAddressAndGeoForUpdateDto.java(1 hunks)service-api/src/main/java/greencity/dto/location/LocationVO.java(1 hunks)service-api/src/main/java/greencity/service/PlaceService.java(1 hunks)service/src/main/java/greencity/mapping/PhotoVOMapper.java(1 hunks)service/src/main/java/greencity/service/LocationServiceImpl.java(1 hunks)service/src/main/java/greencity/service/PlaceServiceImpl.java(9 hunks)service/src/test/java/greencity/ModelUtils.java(3 hunks)service/src/test/java/greencity/mapping/PhotoVOMapperTest.java(2 hunks)service/src/test/java/greencity/service/PlaceServiceImplTest.java(10 hunks)
💤 Files with no reviewable changes (1)
- core/src/test/java/greencity/controller/PlaceControllerTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (17)
service/src/main/java/greencity/mapping/PhotoVOMapper.java (1)
26-27: Good addition of null checks! 👍The defensive programming approach with null checks for
placeanduserproperties prevents potentialNullPointerExceptions, making the mapper more robust. This is consistent with the existing null check pattern for thecommentproperty.To ensure comprehensive test coverage, please verify that the following test scenarios are covered:
✅ Verification successful
Excellent test coverage confirmed! ✅
The codebase already includes comprehensive test coverage for null scenarios in
PhotoVOMapperTest.java, with dedicated test cases for both complete null fields (convertWithNullFieldsTest) and partial null fields (convertWithPartialFieldsTest). These tests explicitly verify the null handling behavior for user and place properties.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering null scenarios in PhotoVOMapperTest rg -A 5 "PhotoVOMapperTest" | rg -A 10 "null"Length of output: 43
Script:
#!/bin/bash # Find PhotoVOMapperTest file fd "PhotoVOMapperTest.java" # If the file exists, show its content fd "PhotoVOMapperTest.java" -x cat {}Length of output: 1825
service-api/src/main/java/greencity/service/PlaceService.java (1)
96-104: Well-structured method designGood job on:
- Maintaining separation of concerns between UI and non-UI update operations
- Including admin email parameter for proper authorization
- Following the existing interface patterns and documentation style
core/src/main/java/greencity/webcontroller/ManagementPlacesController.java (3)
Line range hint
105-108: Consider consolidating place creation logicThe method name
addPlaceFromUisuggests there might be separate logic paths for UI vs API place creation. Consider consolidating these to maintain DRY principles and ensure consistent validation across all entry points.Let's check for potential duplicate logic:
101-104: Well-structured parameter validation enhancements!The addition of
@Validand proper placement ofBindingResultfollows Spring MVC best practices for validation. The@Nullableannotation clearly documents that images are optional.Let's verify that all required fields in AddPlaceDto are properly annotated:
✅ Verification successful
Validation setup is properly implemented and comprehensive
The
@Validannotation in the controller correctly triggers the following validations inAddPlaceDto:
- Required
placeNamewith length constraints- Required
categoryNamewith cascading validation- Required
openingHoursListwith minimum size validation- Optional fields appropriately lack constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for AddPlaceDto class definition and its field annotations ast-grep --pattern 'class AddPlaceDto { $$$ }'Length of output: 50
Script:
#!/bin/bash # First, let's find the file containing AddPlaceDto rg -l "class AddPlaceDto" # Then get the content with context rg -A 20 "class AddPlaceDto"Length of output: 4792
117-123: Good alignment with savePlace pattern and proper file upload handling!The method signature changes properly support multipart form data and maintain consistency with the savePlace method. The addition of Principal parameter enables proper user context tracking.
Let's verify proper security constraints:
✅ Verification successful
Security constraints are properly implemented at the configuration level
The controller's endpoints are well-protected through the global security configuration, which restricts all
/management/**paths to users with the ADMIN role. The Principal parameter is appropriately used for admin action tracking.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for security annotations on the controller and its methods ast-grep --pattern '@$_(secured|PreAuthorize|RolesAllowed)($$$)'Length of output: 66
Script:
#!/bin/bash # Check the specific controller file and its security annotations cat core/src/main/java/greencity/webcontroller/ManagementPlacesController.java # Also search for any security-related annotations in case they use different patterns rg -B 2 -A 1 "@(Secured|PreAuthorize|RolesAllowed|Security)" core/src/main/java/greencity/webcontroller/ManagementPlacesController.javaLength of output: 6396
Script:
#!/bin/bash # Check for Spring Security configuration files and WebSecurityConfigurerAdapter implementations rg -t java -B 2 -A 2 "WebSecurityConfigurerAdapter|SecurityFilterChain|antMatchers.*management|requestMatchers.*management" # Also check for any security config related to management endpoints rg -t java -B 2 -A 2 "configure.*Security.*management"Length of output: 2113
dao/src/main/java/greencity/repository/DiscountValuesRepo.java (1)
29-32: Verify transaction handling in service layerThe method is correctly annotated with
@Transactional, but let's ensure proper usage in the service layer.✅ Verification successful
Transaction handling is properly implemented ✅
The repository method's transaction handling is well-integrated within the service layer's transaction boundaries. The class-level
@Transactionalannotation onPlaceServiceImplensures atomic operations when multiple deletes are performed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for service layer calls to verify transaction handling rg -t java "deleteAllByPlaceId.*\(" service/Length of output: 1785
Script:
#!/bin/bash # Check transaction annotations on service implementations rg -B 5 -A 1 "deleteAllByPlaceId.*\(" service/src/main/java/greencity/service/ --type javaLength of output: 3192
Script:
#!/bin/bash # Check transaction annotation on PlaceServiceImpl update method rg -B 10 "discountService.deleteAllByPlaceId" service/src/main/java/greencity/service/PlaceServiceImpl.javaLength of output: 686
Script:
#!/bin/bash # Check class-level annotations on PlaceServiceImpl rg -B 5 "class PlaceServiceImpl" service/src/main/java/greencity/service/PlaceServiceImpl.javaLength of output: 212
core/src/main/resources/templates/core/management_places.html (2)
493-502: Good implementation of localized messages!The messages object provides a clean way to handle internationalization on the client side.
334-336: Review default values for geocoding fieldsSetting default values for lat/lng to "0.0" might cause issues if these values are used before proper geocoding. Consider using null or implementing client-side validation.
✅ Verification successful
Default coordinate values are appropriate
The default values (0.0, 0.0) are valid coordinates within the system's validation bounds (-90 to 90 for latitude, -180 to 180 for longitude). The application has comprehensive coordinate validation that will prevent any issues if these values are used before proper geocoding.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there's any server-side validation for these coordinates rg -A 5 "lat.*0\.0.*lng.*0\.0"Length of output: 32
Script:
#!/bin/bash # Search for coordinate validation patterns and geocoding echo "=== Searching for coordinate validation patterns ===" rg -l "(?i)(latitude|longitude|coord|geocod)" echo -e "\n=== Looking for form submission handlers ===" ast-grep --pattern 'public $_ $_($$$ @RequestParam("lat") $_lat$$$)'Length of output: 4652
Script:
#!/bin/bash # Check validation constants and validator implementation echo "=== Checking validation constants ===" rg -A 3 "(?i)(lat|lng|coord).*=.*;" service-api/src/main/java/greencity/constant/ServiceValidationConstants.java dao/src/main/java/greencity/constant/ValidationConstants.java core/src/main/java/greencity/constant/ValidationConstants.java echo -e "\n=== Checking validator implementation ===" rg -A 5 "(?i)(validate.*lat|lat.*validate)" core/src/main/java/greencity/validator/EventDtoRequestValidator.javaLength of output: 6276
Script:
#!/bin/bash # Check validation messages echo "=== Checking validation messages ===" rg "greenCity\.validation\.(min|max)\.(lat|lng|latitude|longitude)" core/src/main/resources/messages.propertiesLength of output: 770
core/src/test/java/greencity/webcontroller/ManagementPlacesControllerTest.java (2)
5-8: Well-structured import additions!The new imports appropriately support the enhanced functionality for handling discounts, opening hours, and proper date/time serialization.
Also applies to: 15-18, 34-36, 42-42
114-114: Verify case sensitivity handling in location namesThe location name has been updated to use proper case ("Смиків" instead of "смиків"). Consider adding test cases to verify how the system handles different cases in location names.
service/src/main/java/greencity/service/PlaceServiceImpl.java (2)
683-691: Handle potential null values increateLocationVOmethodWhen building the
LocationVOobject, ifdto.getAddressUa()can benull, it might be prudent to handle it to prevent potentialNullPointerException.Please ensure that
addressUais validated before usage or consider adding a null check.
701-712: Verify that location updates persist correctly across servicesIn the
updatemethod, after callingupdateLocation, you may want to ensure that any changes to the location are properly reflected inupdatedPlacebefore mapping it back toPlaceVO.Run the following script to check if the location updates are correctly persisted:
service-api/src/main/java/greencity/dto/location/LocationVO.java (1)
21-21: Ensure consistency with address fieldsThe new field
addressUahas been added toLocationVO. Please verify that this field is properly handled in all places whereLocationVOis used, ensuring that bothaddressandaddressUaare consistently populated and utilized.If needed, update the corresponding mappings and service methods to handle
addressUa.service-api/src/main/java/greencity/dto/location/LocationAddressAndGeoForUpdateDto.java (1)
31-36: Validate the newaddressUafield thoroughlyThe
addressUafield has been added with validation annotations. Ensure that incoming data satisfies these constraints to prevent validation errors.Good job on adding detailed validation to
addressUato maintain data integrity.service/src/test/java/greencity/ModelUtils.java (2)
3329-3336: LGTM! Well-structured test data builder.The PlaceUpdateDto builder provides a clean way to create test data with all necessary fields.
3346-3351: LGTM! Clear and concise CategoryDto test data.The method provides all necessary fields for testing category-related functionality.
core/src/main/resources/static/management/places/buttonsAJAX.js (1)
190-198: Ensure consistent handling of image uploads in POST and PUT requestsIn
submitPostFormData, only a single image is appended toformData, whereas insubmitPutFormData, multiple images are appended. Verify if both POST and PUT requests should support multiple images, and align the logic accordingly.Also applies to: 200-210
| <div class="image-container" th:if="${!place.getImages().isEmpty()}"> | ||
| <th:block th:each="image : ${place.getImages()}"> | ||
| <img th:src="${image}" alt="place image" width="100" height="100" style="margin-right: 10px;"> | ||
| </th:block> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance image accessibility
The image elements lack meaningful alt text. Consider adding descriptive alt text that includes the place name.
-<img th:src="${image}" alt="place image" width="100" height="100" style="margin-right: 10px;">
+<img th:src="${image}" th:alt="${'Image of ' + place.getName()}" width="100" height="100" style="margin-right: 10px;">📝 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.
| <div class="image-container" th:if="${!place.getImages().isEmpty()}"> | |
| <th:block th:each="image : ${place.getImages()}"> | |
| <img th:src="${image}" alt="place image" width="100" height="100" style="margin-right: 10px;"> | |
| </th:block> | |
| </div> | |
| <div class="image-container" th:if="${!place.getImages().isEmpty()}"> | |
| <th:block th:each="image : ${place.getImages()}"> | |
| <img th:src="${image}" th:alt="${'Image of ' + place.getName()}" width="100" height="100" style="margin-right: 10px;"> | |
| </th:block> | |
| </div> |
| <td th:text="${place.getLocation() != null ? place.getLocation().getAddress() : 'Адреса не вказана'}">Адреса</td> | ||
| <td th:text="${place.getLocation() !=null ? place.getLocation().getLat(): 'Адреса не вказана'}"></td> | ||
| <td th:text="${place.getLocation() !=null ? place.getLocation().getLng(): 'Адреса не вказана'}"></td> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility for address display
The fallback text "Адреса не вказана" (Address not specified) is hardcoded in Ukrainian. This should be internationalized using Thymeleaf's message resolution.
-<td th:text="${place.getLocation() != null ? place.getLocation().getAddress() : 'Адреса не вказана'}">Адреса</td>
-<td th:text="${place.getLocation() !=null ? place.getLocation().getLat(): 'Адреса не вказана'}"></td>
-<td th:text="${place.getLocation() !=null ? place.getLocation().getLng(): 'Адреса не вказана'}"></td>
+<td th:text="${place.getLocation() != null ? place.getLocation().getAddress() : #{greenCity.places.address.not.specified}}">Address</td>
+<td th:text="${place.getLocation() !=null ? place.getLocation().getLat(): #{greenCity.places.address.not.specified}}"></td>
+<td th:text="${place.getLocation() !=null ? place.getLocation().getLng(): #{greenCity.places.address.not.specified}}"></td>Committable suggestion skipped: line range outside the PR's diff.
| newPhotos.add(savedPhoto); | ||
| newPhotos.add(newPhoto); |
There was a problem hiding this comment.
Avoid adding the same photo object twice to the list
In the mapMultipartFilesToPhotos method, both savedPhoto and newPhoto are added to newPhotos. Since savedPhoto is the persisted version of newPhoto, adding both may lead to duplicate entries.
Apply this diff to add only the saved photo:
Photo savedPhoto = photoRepo.save(newPhoto);
newPhotos.add(savedPhoto);
-newPhotos.add(newPhoto);📝 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.
| newPhotos.add(savedPhoto); | |
| newPhotos.add(newPhoto); | |
| newPhotos.add(savedPhoto); |
| mapMultipartFilesToPhotos(images, place, user.orElse(null)); | ||
| placeRepo.save(updatedPlace); | ||
| return modelMapper.map(updatedPlace, PlaceVO.class); |
There was a problem hiding this comment.
Ensure updated place entity is correctly saved after adding photos
In the updateFromUI method, after mapping photos to the place object, you are saving updatedPlace. This may result in the new photos not being persisted with the place, as updatedPlace may not have the updated photo list.
Consider saving the place object instead:
mapMultipartFilesToPhotos(images, place, user.orElse(null));
-placeRepo.save(updatedPlace);
+placeRepo.save(place);📝 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.
| mapMultipartFilesToPhotos(images, place, user.orElse(null)); | |
| placeRepo.save(updatedPlace); | |
| return modelMapper.map(updatedPlace, PlaceVO.class); | |
| mapMultipartFilesToPhotos(images, place, user.orElse(null)); | |
| placeRepo.save(place); | |
| return modelMapper.map(updatedPlace, PlaceVO.class); |
| greenCity.places.page.add.working.hovers=Opening and closing hours are required for selected days | ||
| greenCity.places.page.add.day.hovers=At least one day with opening hours must be selected | ||
| greenCity.places.page.hovers.is.incorrect=The difference between opening and closing time must be at least 30 minutes |
There was a problem hiding this comment.
Correct typos in message keys and values: "hovers" should be "hours"
The keys and messages use "hovers" instead of "hours". Please correct "hovers" to "hours" for clarity and consistency.
Apply this diff to fix the typos:
-greenCity.places.page.add.working.hovers=Opening and closing hours are required for selected days
-greenCity.places.page.add.day.hovers=At least one day with opening hours must be selected
-greenCity.places.page.hovers.is.incorrect=The difference between opening and closing time must be at least 30 minutes
+greenCity.places.page.add.working.hours=Opening and closing hours are required for selected days
+greenCity.places.page.add.day.hours=At least one day with opening hours must be selected
+greenCity.places.page.hours.is.incorrect=The difference between opening and closing time must be at least 30 minutes📝 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.
| greenCity.places.page.add.working.hovers=Opening and closing hours are required for selected days | |
| greenCity.places.page.add.day.hovers=At least one day with opening hours must be selected | |
| greenCity.places.page.hovers.is.incorrect=The difference between opening and closing time must be at least 30 minutes | |
| greenCity.places.page.add.working.hours=Opening and closing hours are required for selected days | |
| greenCity.places.page.add.day.hours=At least one day with opening hours must be selected | |
| greenCity.places.page.hours.is.incorrect=The difference between opening and closing time must be at least 30 minutes |
There was a problem hiding this comment.
@VasylyshynDmytro is the CodeRabbit right?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Yes, the CodeRabbit is right? I will change it
| greenCity.places.page.add.working.hovers=\u041f\u043e\u0442\u0440\u0456\u0431\u043d\u043e\u0020\u0432\u043a\u0430\u0437\u0430\u0442\u0438\u0020\u0447\u0430\u0441\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0434\u043b\u044f\u0020\u0432\u0438\u0431\u0440\u0430\u043d\u0438\u0445\u0020\u0434\u043d\u0456\u0432 | ||
| greenCity.places.page.add.day.hovers=\u041d\u0435\u043e\u0431\u0445\u0456\u0434\u043d\u043e\u0020\u0432\u0438\u0431\u0440\u0430\u0442\u0438\u0020\u0445\u043e\u0447\u0430\u0020\u0431\u0020\u043e\u0434\u0438\u043d\u0020\u0434\u0435\u043d\u044c\u0020\u0456\u0437\u0020\u0433\u043e\u0434\u0438\u043d\u0430\u043c\u0438\u0020\u0440\u043e\u0431\u043e\u0442\u0438 | ||
| greenCity.places.page.hovers.is.incorrect=\u0420\u0456\u0437\u043d\u0438\u0446\u044f\u0020\u043c\u0456\u0436\u0020\u0447\u0430\u0441\u043e\u043c\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u043f\u043e\u0432\u0438\u043d\u043d\u0430\u0020\u0431\u0443\u0442\u0438\u0020\u043d\u0435\u0020\u043c\u0435\u043d\u0448\u0435\u0020\u0033\u0030\u0020\u0445\u0432\u0438\u043b\u0438\u043d |
There was a problem hiding this comment.
Виправте опечатки у ключах повідомлень: "hovers" має бути "hours"
У ключах повідомлень використовується "hovers" замість "hours". Будь ласка, замініть "hovers" на "hours" для підтримки узгодженості.
Застосуйте цей диф для виправлення опечаток:
-greenCity.places.page.add.working.hovers=потрібно вказати час відкриття та закриття для вибраних днів
-greenCity.places.page.add.day.hovers=необхідно вибрати хоча б один день із годинами роботи
-greenCity.places.page.hovers.is.incorrect=Різниця між часом відкриття та закриття повинна бути не менше 30 хвилин
+greenCity.places.page.add.working.hours=потрібно вказати час відкриття та закриття для вибраних днів
+greenCity.places.page.add.day.hours=необхідно вибрати хоча б один день із годинами роботи
+greenCity.places.page.hours.is.incorrect=Різниця між часом відкриття та закриття повинна бути не менше 30 хвилин📝 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.
| greenCity.places.page.add.working.hovers=\u041f\u043e\u0442\u0440\u0456\u0431\u043d\u043e\u0020\u0432\u043a\u0430\u0437\u0430\u0442\u0438\u0020\u0447\u0430\u0441\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0434\u043b\u044f\u0020\u0432\u0438\u0431\u0440\u0430\u043d\u0438\u0445\u0020\u0434\u043d\u0456\u0432 | |
| greenCity.places.page.add.day.hovers=\u041d\u0435\u043e\u0431\u0445\u0456\u0434\u043d\u043e\u0020\u0432\u0438\u0431\u0440\u0430\u0442\u0438\u0020\u0445\u043e\u0447\u0430\u0020\u0431\u0020\u043e\u0434\u0438\u043d\u0020\u0434\u0435\u043d\u044c\u0020\u0456\u0437\u0020\u0433\u043e\u0434\u0438\u043d\u0430\u043c\u0438\u0020\u0440\u043e\u0431\u043e\u0442\u0438 | |
| greenCity.places.page.hovers.is.incorrect=\u0420\u0456\u0437\u043d\u0438\u0446\u044f\u0020\u043c\u0456\u0436\u0020\u0447\u0430\u0441\u043e\u043c\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u043f\u043e\u0432\u0438\u043d\u043d\u0430\u0020\u0431\u0443\u0442\u0438\u0020\u043d\u0435\u0020\u043c\u0435\u043d\u0448\u0435\u0020\u0033\u0030\u0020\u0445\u0432\u0438\u043b\u0438\u043d | |
| greenCity.places.page.add.working.hours=\u041f\u043e\u0442\u0440\u0456\u0431\u043d\u043e\u0020\u0432\u043a\u0430\u0437\u0430\u0442\u0438\u0020\u0447\u0430\u0441\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0434\u043b\u044f\u0020\u0432\u0438\u0431\u0440\u0430\u043d\u0438\u0445\u0020\u0434\u043d\u0456\u0432 | |
| greenCity.places.page.add.day.hours=\u041d\u0435\u043e\u0431\u0445\u0456\u0434\u043d\u043e\u0020\u0432\u0438\u0431\u0440\u0430\u0442\u0438\u0020\u0445\u043e\u0447\u0430\u0020\u0431\u0020\u043e\u0434\u0438\u043d\u0020\u0434\u0435\u043d\u044c\u0020\u0456\u0437\u0020\u0433\u043e\u0434\u0438\u043d\u0430\u043c\u0438\u0020\u0440\u043e\u0431\u043e\u0442\u0438 | |
| greenCity.places.page.hours.is.incorrect=\u0420\u0456\u0437\u043d\u0438\u0446\u044f\u0020\u043c\u0456\u0436\u0020\u0447\u0430\u0441\u043e\u043c\u0020\u0432\u0456\u0434\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u0442\u0430\u0020\u0437\u0430\u043a\u0440\u0438\u0442\u0442\u044f\u0020\u043f\u043e\u0432\u0438\u043d\u043d\u0430\u0020\u0431\u0443\u0442\u0438\u0020\u043d\u0435\u0020\u043c\u0435\u043d\u0448\u0435\u0020\u0033\u0030\u0020\u0445\u0432\u0438\u043b\u0438\u043d |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
service/src/test/java/greencity/service/PlaceServiceImplTest.java (6)
3-5: Consider organizing imports by package groups.The new imports are scattered throughout the import section. Consider organizing them into logical groups:
- Google Maps related (
com.google.maps.model.*)- DTO classes
- Entity classes
- Service classes
Also applies to: 11-12, 18-19, 51-51, 221-222
982-999: Add more specific assertions for exception testing.While the test correctly verifies that an exception is thrown, consider adding assertions about the exception message to ensure the correct error scenario is being tested.
- Assertions.assertThrows(RuntimeException.class, - () -> placeServiceImpl.updateOpening(hoursUpdateDtoSet, updatedPlace), - "Expected updateOpening to throw an exception"); + RuntimeException exception = Assertions.assertThrows(RuntimeException.class, + () -> placeServiceImpl.updateOpening(hoursUpdateDtoSet, updatedPlace), + "Expected updateOpening to throw an exception"); + assertEquals("Test exception", exception.getMessage());
1019-1037: Enhance test coverage with edge cases.The test verifies the happy path but could benefit from testing edge cases. Consider adding tests for:
- Empty opening hours set
- Null opening hours
- Invalid time formats
1001-1017: Add more specific assertions for discount exception testing.Similar to the opening hours tests, enhance the exception testing with message verification.
- Assertions.assertThrows(RuntimeException.class, - () -> placeServiceImpl.updateDiscount(discounts, updatedPlace), - "Expected updateDiscount to throw an exception"); + RuntimeException exception = Assertions.assertThrows(RuntimeException.class, + () -> placeServiceImpl.updateDiscount(discounts, updatedPlace), + "Expected updateDiscount to throw an exception"); + assertEquals("Test exception", exception.getMessage());
1055-1096: Excellent test coverage for place updates.The test is well-structured with good setup and verification. A few suggestions for enhancement:
- Consider extracting the GeocodingResult setup to a helper method
- Add verification for the order of operations
- Add assertions for the location update details
+ // Verify order of operations + InOrder inOrder = Mockito.inOrder(locationService, placeRepo, photoRepo); + inOrder.verify(locationService).update(anyLong(), any(LocationVO.class)); + inOrder.verify(placeRepo).save(any(Place.class)); + inOrder.verify(photoRepo).save(any(Photo.class));
1098-1129: Good coverage of error scenarios.The tests effectively cover various error cases. Consider adding:
- Test for blocked user status
- Test for invalid location data
- Test for file upload failures
| @Test | ||
| void updateDiscountDoesNotThrowExceptionTest() { | ||
| Place updatedPlace = new Place(); | ||
| updatedPlace.setId(1L); | ||
| Set<DiscountValueDto> discounts = new HashSet<>(); | ||
|
|
||
| Mockito.when(discountService.findAllByPlaceId(Mockito.anyLong())) | ||
| .thenReturn(new HashSet<>()); | ||
| Mockito.doAnswer(invocation -> null) | ||
| .when(discountService).deleteAllByPlaceId(Mockito.anyLong()); | ||
| Mockito.doAnswer(invocation -> null) | ||
| .when(discountService).save(Mockito.any(DiscountValueVO.class)); | ||
|
|
||
| Assertions.assertDoesNotThrow(() -> placeServiceImpl.updateDiscount(discounts, updatedPlace)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add positive assertions to discount update test.
The test verifies that no exception is thrown but doesn't assert the state after the update. Consider adding assertions to verify:
- The discounts were actually saved
- The correct number of discounts were processed
- The discount values are correct
Assertions.assertDoesNotThrow(() -> placeServiceImpl.updateDiscount(discounts, updatedPlace));
+ verify(discountService, times(1)).deleteAllByPlaceId(updatedPlace.getId());
+ verify(discountService, times(discounts.size())).save(any(DiscountValueVO.class));📝 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 updateDiscountDoesNotThrowExceptionTest() { | |
| Place updatedPlace = new Place(); | |
| updatedPlace.setId(1L); | |
| Set<DiscountValueDto> discounts = new HashSet<>(); | |
| Mockito.when(discountService.findAllByPlaceId(Mockito.anyLong())) | |
| .thenReturn(new HashSet<>()); | |
| Mockito.doAnswer(invocation -> null) | |
| .when(discountService).deleteAllByPlaceId(Mockito.anyLong()); | |
| Mockito.doAnswer(invocation -> null) | |
| .when(discountService).save(Mockito.any(DiscountValueVO.class)); | |
| Assertions.assertDoesNotThrow(() -> placeServiceImpl.updateDiscount(discounts, updatedPlace)); | |
| } | |
| @Test | |
| void updateDiscountDoesNotThrowExceptionTest() { | |
| Place updatedPlace = new Place(); | |
| updatedPlace.setId(1L); | |
| Set<DiscountValueDto> discounts = new HashSet<>(); | |
| Mockito.when(discountService.findAllByPlaceId(Mockito.anyLong())) | |
| .thenReturn(new HashSet<>()); | |
| Mockito.doAnswer(invocation -> null) | |
| .when(discountService).deleteAllByPlaceId(Mockito.anyLong()); | |
| Mockito.doAnswer(invocation -> null) | |
| .when(discountService).save(Mockito.any(DiscountValueVO.class)); | |
| Assertions.assertDoesNotThrow(() -> placeServiceImpl.updateDiscount(discounts, updatedPlace)); | |
| verify(discountService, times(1)).deleteAllByPlaceId(updatedPlace.getId()); | |
| verify(discountService, times(discounts.size())).save(any(DiscountValueVO.class)); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
service/src/main/java/greencity/service/PlaceServiceImpl.java (2)
536-540: Consider security implications of exposing email in error messagesWhile the error handling is robust, including the full email address in error messages could expose sensitive information in logs or stack traces.
Consider masking the email address in the error message:
- .orElseThrow(() -> new NotFoundException(ErrorMessage.USER_NOT_FOUND_BY_EMAIL + email)); + .orElseThrow(() -> new NotFoundException(ErrorMessage.USER_NOT_FOUND_BY_EMAIL + + email.replaceAll("(?<=.{3}).(?=.*@)", "*")));
575-591: Consider batch saving photos for better performanceThe current implementation saves photos one at a time. For better performance with multiple photos, consider using batch operations.
private void mapMultipartFilesToPhotos(MultipartFile[] images, Place place, User user) { if (images != null && images.length > 0 && images[0] != null) { List<Photo> newPhotos = new ArrayList<>(); for (MultipartFile image : images) { if (image != null) { Photo newPhoto = Photo.builder() .place(place) .name(fileService.upload(image)) .user(user) .build(); - Photo savedPhoto = photoRepo.save(newPhoto); - newPhotos.add(savedPhoto); + newPhotos.add(newPhoto); } } + List<Photo> savedPhotos = photoRepo.saveAll(newPhotos); - place.getPhotos().addAll(newPhotos); + place.getPhotos().addAll(savedPhotos); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/main/java/greencity/service/PlaceServiceImpl.java(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/main/java/greencity/service/PlaceServiceImpl.java (1)
554-560: Add validation for geocoding results sizeThe code assumes at least two results (for Ukrainian and English addresses) but doesn't validate this assumption.
Add size validation before accessing the results:
AddPlaceLocation getLocationDetailsFromGeocode(String locationName) { List<GeocodingResult> geocodingResults = Optional .ofNullable(googleApiService.getResultFromGeoCode(locationName)) .filter(results -> !results.isEmpty()) + .filter(results -> results.size() >= 2) .orElseThrow(() -> new NotFoundException(ErrorMessage.ADDRESS_NOT_FOUND_EXCEPTION + locationName)); return initializeGeoCodingResults(geocodingResults); }✅ Verification successful
Validation requirement confirmed - Add size check to prevent runtime errors
The implementation of
initializeGeoCodingResultsrequires exactly two geocoding results (Ukrainian and English addresses). The current code will throw anIndexOutOfBoundsExceptionif fewer results are returned. The suggested size validation is necessary to provide a better error message.AddPlaceLocation getLocationDetailsFromGeocode(String locationName) { List<GeocodingResult> geocodingResults = Optional .ofNullable(googleApiService.getResultFromGeoCode(locationName)) .filter(results -> !results.isEmpty()) + .filter(results -> results.size() >= 2) .orElseThrow(() -> new NotFoundException(ErrorMessage.ADDRESS_NOT_FOUND_EXCEPTION + locationName)); return initializeGeoCodingResults(geocodingResults); }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find implementation of initializeGeoCodingResults ast-grep --pattern 'initializeGeoCodingResults($$$) { $$$ }' # Search for related test files fd -e java -e kt test | grep -i "place.*test" # Look for documentation fd README.mdLength of output: 1575
Script:
#!/bin/bash # Find the implementation in the service file cat service/src/main/java/greencity/service/PlaceServiceImpl.java | grep -A 20 "initializeGeoCodingResults" # Look for test cases cat service/src/test/java/greencity/service/PlaceServiceImplTest.java | grep -A 20 "getLocationDetailsFromGeocode\|initializeGeoCodingResults"Length of output: 1303
| @Transactional | ||
| @Override | ||
| public PlaceVO update(PlaceUpdateDto dto) { | ||
| log.info(LogMessage.IN_UPDATE, dto.getName()); | ||
| Category updatedCategory = modelMapper.map( | ||
| categoryService.findByName(dto.getCategory().getName()), Category.class); | ||
| Place updatedPlace = findPlaceById(dto.getId()); | ||
| LocationVO updatable = locationService.findById(updatedPlace.getLocation().getId()); | ||
| updateLocation(dto, updatedPlace, updatable); | ||
| updatePlaceProperties(dto, updatedPlace, updatedCategory); | ||
| return modelMapper.map(updatedPlace, PlaceVO.class); | ||
| } | ||
|
|
||
| @Transactional | ||
| @Override | ||
| public PlaceVO updateFromUI(PlaceUpdateDto dto, MultipartFile[] images, String email) { | ||
| log.info(LogMessage.IN_UPDATE, dto.getName()); | ||
| Category updatedCategory = modelMapper.map( | ||
| categoryService.findByName(dto.getCategory().getName()), Category.class); | ||
| Place updatedPlace = findPlaceById(dto.getId()); | ||
| LocationVO updatable = locationService.findById(updatedPlace.getLocation().getId()); | ||
| updateLocation(dto, updatedPlace, updatable); | ||
| updatePlaceProperties(dto, updatedPlace, updatedCategory); | ||
| Place place = modelMapper.map(updatedPlace, Place.class); | ||
| Optional<User> user = userRepo.findByEmail(email); | ||
| mapMultipartFilesToPhotos(images, place, user.orElse(null)); | ||
| placeRepo.save(updatedPlace); | ||
| return modelMapper.map(updatedPlace, PlaceVO.class); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in update methods
The update() and updateFromUI() methods share significant code. Consider extracting the common logic to reduce duplication.
Create a common method for the shared update logic:
+ private PlaceVO updatePlaceCommon(PlaceUpdateDto dto, Place updatedPlace) {
+ Category updatedCategory = modelMapper.map(
+ categoryService.findByName(dto.getCategory().getName()), Category.class);
+ LocationVO updatable = locationService.findById(updatedPlace.getLocation().getId());
+ updateLocation(dto, updatedPlace, updatable);
+ updatePlaceProperties(dto, updatedPlace, updatedCategory);
+ return modelMapper.map(updatedPlace, PlaceVO.class);
+ }
@Transactional
@Override
public PlaceVO update(PlaceUpdateDto dto) {
log.info(LogMessage.IN_UPDATE, dto.getName());
- Category updatedCategory = modelMapper.map(
- categoryService.findByName(dto.getCategory().getName()), Category.class);
Place updatedPlace = findPlaceById(dto.getId());
- LocationVO updatable = locationService.findById(updatedPlace.getLocation().getId());
- updateLocation(dto, updatedPlace, updatable);
- updatePlaceProperties(dto, updatedPlace, updatedCategory);
- return modelMapper.map(updatedPlace, PlaceVO.class);
+ return updatePlaceCommon(dto, updatedPlace);
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
service/src/test/java/greencity/service/PlaceServiceImplTest.java (3)
982-999: Enhance exception testing.Consider improving the test by:
- Verifying the exception message.
- Using a more descriptive test message that explains the scenario being tested.
@Test -void updateOpeningThrowsExceptionForInvalidOperationTest() { +void updateOpeningShouldThrowExceptionWhenDeleteOperationFails() { Place updatedPlace = new Place(); updatedPlace.setId(1L); updatedPlace.setOpeningHoursList(null); Set<OpeningHoursDto> hoursUpdateDtoSet = new HashSet<>(); OpeningHoursDto openingHoursDto = new OpeningHoursDto(); hoursUpdateDtoSet.add(openingHoursDto); + String expectedMessage = "Test exception"; Mockito.doThrow(new RuntimeException("Test exception")) .when(openingHoursService) .deleteAllByPlaceId(Mockito.anyLong()); - Assertions.assertThrows(RuntimeException.class, - () -> placeServiceImpl.updateOpening(hoursUpdateDtoSet, updatedPlace), - "Expected updateOpening to throw an exception"); + RuntimeException exception = Assertions.assertThrows(RuntimeException.class, + () -> placeServiceImpl.updateOpening(hoursUpdateDtoSet, updatedPlace), + "Should throw RuntimeException when delete operation fails"); + Assertions.assertEquals(expectedMessage, exception.getMessage());
1055-1096: Enhance place update test coverage.The test is well-structured but could be improved by adding assertions for:
- Location details (lat, lng, address)
- Category information
- Image processing verification
assertNotNull(result); assertEquals("Updated Place", result.getName()); + assertEquals(50.45, result.getLocation().getLat()); + assertEquals(30.52, result.getLocation().getLng()); + assertEquals("New Address", result.getLocation().getAddress()); + assertEquals("Test Category", result.getCategory().getName()); verify(placeRepo, atLeastOnce()).save(any(Place.class)); verify(photoRepo, times(1)).save(any(Photo.class)); verify(locationService, times(1)).update(anyLong(), any(LocationVO.class)); + verify(fileService, times(1)).upload(any(MultipartFile.class));
1098-1130: Improve exception testing specificity.The exception tests could be enhanced by:
- Using more specific exception types instead of IllegalArgumentException
- Adding verification that no updates occurred
- assertThrows(IllegalArgumentException.class, + assertThrows(NotFoundException.class, () -> placeService.updateFromUI(dto, null, "[email protected]")); + verify(placeRepo, never()).save(any(Place.class)); + verify(locationService, never()).update(anyLong(), any(LocationVO.class));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/service/PlaceServiceImplTest.java(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/service/PlaceServiceImplTest.java (1)
1039-1053: Add positive assertions to discount update test.The test verifies that no exception is thrown but doesn't assert the state after the update. Consider adding assertions to verify:
- The discounts were actually saved
- The correct number of discounts were processed
Assertions.assertDoesNotThrow(() -> placeServiceImpl.updateDiscount(discounts, updatedPlace)); + verify(discountService, times(1)).deleteAllByPlaceId(updatedPlace.getId()); + verify(discountService, times(discounts.size())).save(any(DiscountValueVO.class));
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/main/resources/messages.properties(1 hunks)core/src/main/resources/messages_ua.properties(1 hunks)core/src/main/resources/templates/core/management_places.html(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/resources/messages.properties
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
core/src/main/resources/messages_ua.properties (1)
249-254: Well-structured validation messages!The new validation messages are well-organized and follow good practices:
- Consistent key naming convention using the
greenCity.places.pageprefix- Clear and descriptive Ukrainian messages
- Proper Unicode escaping for Ukrainian characters
core/src/main/resources/templates/core/management_places.html (4)
7-9: Well-structured CSS organization!The modular CSS approach with separate files for header, footer, position, table, slider, and sidebar improves maintainability and follows separation of concerns.
Also applies to: 23-25
273-275: Internationalize address placeholder textThe hardcoded Ukrainian text "Адреса не вказана" should be internationalized.
493-502: Good localization practice!Centralizing client-side translations in a messages object is a clean approach. The use of Thymeleaf's inline JavaScript with proper escaping helps prevent XSS vulnerabilities.
277-281: Enhance image accessibilityThe image elements still lack meaningful alt text.
| <div class="filter-container__search"> | ||
| <input id="search-places-by-id" type="text" class="form-search" th:value="${fields.id}" th:placeholder="#{greenCity.pages.search}"> | ||
| <label for="search-places-by-id" class="search-icon"><i class="material-icons"></i></label> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance filter inputs accessibility
The filter inputs lack proper ARIA labels. Add aria-label attributes to improve accessibility for screen readers.
Example for the ID filter:
-<input id="search-places-by-id" type="text" class="form-search" th:value="${fields.id}" th:placeholder="#{greenCity.pages.search}">
+<input id="search-places-by-id" type="text" class="form-search" th:value="${fields.id}" th:placeholder="#{greenCity.pages.search}"
+ th:aria-label="#{greenCity.pages.table.id} + ' ' + #{greenCity.pages.search}">Also applies to: 139-141, 163-165, 192-194, 208-210
| <input type="hidden" id="lat" name="lat" value="0.0"> | ||
| <input type="hidden" id="lng" name="lng" value="0.0"> | ||
| <input type="hidden" id="addressUa" name="addressUa" value="sometext"> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve default values for geocoding fields
The current default values are problematic:
lat/lngof 0.0 represents an invalid location (null island)addressUacontains a hardcoded placeholder "sometext"
Consider leaving these fields empty and handling the absence of values in your backend validation.
-<input type="hidden" id="lat" name="lat" value="0.0">
-<input type="hidden" id="lng" name="lng" value="0.0">
-<input type="hidden" id="addressUa" name="addressUa" value="sometext">
+<input type="hidden" id="lat" name="lat" value="">
+<input type="hidden" id="lng" name="lng" value="">
+<input type="hidden" id="addressUa" name="addressUa" value="">📝 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.
| <input type="hidden" id="lat" name="lat" value="0.0"> | |
| <input type="hidden" id="lng" name="lng" value="0.0"> | |
| <input type="hidden" id="addressUa" name="addressUa" value="sometext"> | |
| <input type="hidden" id="lat" name="lat" value=""> | |
| <input type="hidden" id="lng" name="lng" value=""> | |
| <input type="hidden" id="addressUa" name="addressUa" value=""> |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
core/src/main/resources/templates/core/management_places.html (2)
7-9: LGTM! Good modular CSS approach.Breaking down the CSS into specific stylesheets improves maintainability. Consider adding a media query to handle text overflow on small screens when using
.no-wrap..no-wrap { white-space: nowrap; + @media (max-width: 768px) { + white-space: normal; + overflow-wrap: break-word; + } }Also applies to: 42-44
494-501: Consider adding more validation messages.The localization object covers basic validation messages. Consider adding messages for:
- Invalid time format
- Break time validation errors
- File upload errors
const messages = { "greenCity.places.page.add.address": [[#{greenCity.places.page.add.address}]], "greenCity.places.page.add.place.name": [[#{greenCity.places.page.add.place.name}]], "greenCity.places.page.add.category": [[#{greenCity.places.page.add.category}]], "greenCity.places.page.add.working.hours": [[#{greenCity.places.page.add.working.hours}]], "greenCity.places.page.add.day.hours": [[#{greenCity.places.page.add.day.hours}]], "greenCity.places.page.hours.is.incorrect": [[#{greenCity.places.page.hours.is.incorrect}]], + "greenCity.places.page.invalid.time.format": [[#{greenCity.places.page.invalid.time.format}]], + "greenCity.places.page.break.time.error": [[#{greenCity.places.page.break.time.error}]], + "greenCity.places.page.file.upload.error": [[#{greenCity.places.page.file.upload.error}]] };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/resources/templates/core/management_places.html(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
core/src/main/resources/templates/core/management_places.html (2)
115-117: Enhance filter inputs accessibilityThe filter inputs lack proper ARIA labels. Add
aria-labelattributes to improve accessibility for screen readers.Also applies to: 139-141, 163-165, 192-194, 208-210
334-336: LGTM! Proper initialization of geocoding fields.The hidden geocoding fields are correctly initialized with empty values, allowing proper validation in the backend.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/resources/templates/core/management_places.html (2)
Line range hint
7-25: Consider bundling CSS files for better performance.The addition of multiple CSS files increases HTTP requests. Consider bundling these files for production.
#!/bin/bash # Check if any bundling tools are configured rg -l "webpack|gulp|grunt|rollup|parcel" .
494-503: Consider using a TypeScript interface for messages.The messages object would benefit from type safety using TypeScript.
interface LocalizedMessages { 'greenCity.places.page.add.address': string; 'greenCity.places.page.add.place.name': string; 'greenCity.places.page.add.category': string; 'greenCity.places.page.add.working.hours': string; 'greenCity.places.page.add.day.hours': string; 'greenCity.places.page.hours.is.incorrect': string; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/resources/templates/core/management_places.html(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
core/src/main/resources/templates/core/management_places.html (5)
31-44: LGTM! Good use of flexbox for table container.The table container's flex layout ensures proper vertical alignment and spacing.
274-276: Previous comment about internationalization still applies.
278-282: Previous comment about image accessibility still applies.
335-337: Previous comment about geocoding fields default values still applies.
419-421: LGTM! Good use of semantic button styling.The discount button correctly uses the
btn-infoclass to indicate its informative nature.
| <input class="form-control" id="address" name="address" type="text"> | ||
| <span class="errorSpan" th:id="errorModalSavelocation"></span> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add client-side validation for address input.
The address input should be validated before form submission to ensure proper geocoding.
-<input class="form-control" id="address" name="address" type="text">
+<input class="form-control" id="address" name="address" type="text"
+ pattern="^[A-Za-z0-9\s,.-]+$"
+ title="Please enter a valid address"
+ required>📝 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.
| <input class="form-control" id="address" name="address" type="text"> | |
| <span class="errorSpan" th:id="errorModalSavelocation"></span> | |
| <input class="form-control" id="address" name="address" type="text" | |
| pattern="^[A-Za-z0-9\s,.-]+$" | |
| title="Please enter a valid address" | |
| required> | |
| <span class="errorSpan" th:id="errorModalSavelocation"></span> |
| <label class="no-wrap">[[#{greenCity.pages.table.end.time}]]</label> | ||
| <input class="form-control-sm" name="closeTime" type="time"> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="form-group col-md-5 break-time"> | ||
| <button class="btn btn-success btn-sm add-break"> | ||
| [[#{greenCity.pages.table.add.break.hourse}]] | ||
| [[#{greenCity.pages.table.add.break.hours}]] | ||
| </button> | ||
| <div class="form-row break-hours displayNone"> | ||
| <div class="form-group"> | ||
| <div class="form-row"> | ||
| <div class="form-group col-5"> | ||
| <div class="form-group col-6"> | ||
| <label>[[#{greenCity.pages.table.break.start.p}]]</label> | ||
| <input class="form-control-sm" name="startTime" type="time"> | ||
| </div> | ||
| <div class="form-group col-5"> | ||
| <div class="form-group col-6"> | ||
| <label>[[#{greenCity.pages.table.break.end.p}]]</label> | ||
| <input class="form-control-sm" name="endTime" type="time"> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance time input validation.
The break time inputs should be validated to ensure they fall within the working hours.
-<input class="form-control-sm" name="startTime" type="time">
+<input class="form-control-sm" name="startTime" type="time"
+ onchange="validateBreakTime(this)">
-<input class="form-control-sm" name="endTime" type="time">
+<input class="form-control-sm" name="endTime" type="time"
+ onchange="validateBreakTime(this)">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/main/resources/templates/core/management_places.html (1)
497-506: Well-structured localization implementation.The addition of a centralized messages object for localized strings is a good practice. Consider adding error messages to this object as well.
Add error messages to the messages object:
const messages = { "greenCity.places.page.add.address": [[#{greenCity.places.page.add.address}]], "greenCity.places.page.add.place.name": [[#{greenCity.places.page.add.place.name}]], "greenCity.places.page.add.category": [[#{greenCity.places.page.add.category}]], "greenCity.places.page.add.working.hours": [[#{greenCity.places.page.add.working.hours}]], "greenCity.places.page.add.day.hours": [[#{greenCity.places.page.add.day.hours}]], - "greenCity.places.page.hours.is.incorrect": [[#{greenCity.places.page.hours.is.incorrect}]] + "greenCity.places.page.hours.is.incorrect": [[#{greenCity.places.page.hours.is.incorrect}]], + "greenCity.places.page.error.invalid.address": [[#{greenCity.places.page.error.invalid.address}]], + "greenCity.places.page.error.invalid.coordinates": [[#{greenCity.places.page.error.invalid.coordinates}]] };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/src/main/resources/templates/core/management_places.html(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (6)
core/src/main/resources/templates/core/management_places.html (6)
7-9: Improved CSS organization through modular stylesheets.The separation of styles into specific modules (
header.css,footer.css,position.css, etc.) enhances maintainability and follows the single responsibility principle.Also applies to: 23-25
274-276: Hardcoded Ukrainian text needs internationalization.The fallback text "Адреса не вказана" (Address not specified) should use Thymeleaf's message resolution for proper internationalization.
388-404: Improved break time UI with validation needed.The break time inputs should be validated to ensure they fall within the working hours.
278-282: Enhance image accessibility.The image elements lack meaningful alt text. Consider adding descriptive alt text that includes the place name.
91-298: Well-structured table with comprehensive filtering and sorting.The table implementation includes:
- Proper column headers with sorting
- Filtered search for each column
- Responsive design with scrollable content
- Proper pagination controls
Consider adding ARIA labels to the filter inputs to improve accessibility.
335-337: Enhanced address handling with geocoding support.Good addition of geocoding fields and address validation. However, consider adding client-side validation for the geocoding fields to ensure they contain valid coordinates when populated.
Run this script to check for proper geocoding validation in the JavaScript files:
Also applies to: 341-344
|
Eco places (management page)
#7682
Fix problems with editing a place, adding a place, updating button labels, geocoding location during place updates, and adding photos and discounts.
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
New Features
Bug Fixes
Documentation
User Experience