Conversation
- 모임 생성 - 모임 수정 - 모임 상세 조회 시, 위도와 경도 응답 여기에 추가 합니다.
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
|
Caution Review failedThe pull request is closed. Walkthrough이 PR은 그룹 주소 엔티티와 관련 DTO에 위도/경도 필드를 추가하고, 위치 검증을 위한 오류 코드를 확장하며, 서비스 클래스의 포맷팅을 개선합니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds latitude and longitude support to the Group V2 entity to enable map-based features for groups. The changes introduce optional coordinate fields that can be provided during group creation and modification, with validation to ensure both coordinates are provided together and fall within valid geographic ranges.
Changes:
- Added
latitudeandlongitudefields toGroupV2Addressentity with validation for coordinate ranges (-90 to 90 for latitude, -180 to 180 for longitude) - Updated group creation and update DTOs and service methods to handle coordinate data
- Added three new error codes for coordinate validation failures
- Included unrelated code formatting changes and comment updates
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/team/wego/wegobackend/group/v2/domain/entity/GroupV2Address.java | Added latitude/longitude fields with validation logic ensuring both are present or both are null, and coordinates are within valid ranges |
| src/main/java/team/wego/wegobackend/group/v2/application/dto/request/CreateGroupV2Request.java | Added optional latitude and longitude fields to group creation request |
| src/main/java/team/wego/wegobackend/group/v2/application/dto/request/UpdateGroupV2Request.java | Added optional latitude and longitude fields to group update request |
| src/main/java/team/wego/wegobackend/group/v2/application/dto/common/Address.java | Added latitude and longitude to the Address response DTO |
| src/main/java/team/wego/wegobackend/group/v2/application/service/GroupV2Service.java | Updated group creation to pass latitude and longitude to entity |
| src/main/java/team/wego/wegobackend/group/v2/application/service/GroupV2UpdateService.java | Updated group modification logic to handle coordinate updates |
| src/main/java/team/wego/wegobackend/group/domain/exception/GroupErrorCode.java | Added three new error codes for coordinate validation |
| src/test/http/group/v2/v2-group-create.http | Updated test request with sample coordinates |
| src/test/http/group/v2/v2-group-get.http | Updated expected response to include coordinates |
| src/main/java/team/wego/wegobackend/group/v2/application/service/GroupV2AttendanceService.java | Code formatting changes (single-line orElseThrow calls) |
| src/main/java/team/wego/wegobackend/group/v2/infrastructure/redis/PreUploadedGroupImageRedisRepository.java | Code formatting changes (single-line if statement) |
| src/main/java/team/wego/wegobackend/group/v2/application/service/PreUploadedGroupImageOrphanGcWorker.java | Minor comment wording change |
| src/main/java/team/wego/wegobackend/group/application/service/v1/GroupService.java | Added algorithm step comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Double newLat = (request.latitude() != null || request.longitude() != null) | ||
| ? request.latitude() | ||
| : cur.getLatitude(); | ||
|
|
||
| Double newLng = (request.latitude() != null || request.longitude() != null) |
There was a problem hiding this comment.
The update logic for latitude and longitude is incorrect. The condition (request.latitude() != null || request.longitude() != null) on lines 150 and 154 is used to determine whether to update coordinates, but this causes a bug:
If a user wants to update ONLY latitude (sending latitude=37.5 and longitude=null), the condition becomes true and newLat gets the new value (37.5), but newLng gets request.longitude() which is null, not the current value.
The logic should be:
- newLat should use request.latitude() if it's not null, otherwise cur.getLatitude()
- newLng should use request.longitude() if it's not null, otherwise cur.getLongitude()
Lines 150-156 should be:
Double newLat = (request.latitude() != null)
? request.latitude()
: cur.getLatitude();
Double newLng = (request.longitude() != null)
? request.longitude()
: cur.getLongitude();
This matches the pattern used for location and locationDetail on lines 142-148.
| Double newLat = (request.latitude() != null || request.longitude() != null) | |
| ? request.latitude() | |
| : cur.getLatitude(); | |
| Double newLng = (request.latitude() != null || request.longitude() != null) | |
| Double newLat = (request.latitude() != null) | |
| ? request.latitude() | |
| : cur.getLatitude(); | |
| Double newLng = (request.longitude() != null) |
| } | ||
|
|
||
| this.location = location.trim(); | ||
| this.locationDetail = locationDetail; |
There was a problem hiding this comment.
On line 49, the location field is trimmed before assignment, but locationDetail is not trimmed on line 50. For consistency and to prevent potential whitespace-only values, consider also trimming locationDetail if it's not null:
this.locationDetail = (locationDetail != null) ? locationDetail.trim() : null;
This ensures consistent handling of string fields and prevents unintended whitespace issues.
| this.locationDetail = locationDetail; | |
| this.locationDetail = (locationDetail != null) ? locationDetail.trim() : null; |
| "latitude": 33.450701, | ||
| "longitude": 126.570667, |
There was a problem hiding this comment.
The test coordinates (latitude: 33.450701, longitude: 126.570667) appear to be for Jeju Island, South Korea, but the location text says "서울 강남구" (Seoul, Gangnam-gu). These coordinates don't match the location description.
For accurate testing, the coordinates should match the stated location. Seoul Gangnam Station area coordinates would be approximately:
- Latitude: 37.498
- Longitude: 127.028
Consider updating the test data to use coordinates that match the location description for more realistic testing.
| "latitude": 33.450701, | |
| "longitude": 126.570667, | |
| "latitude": 37.498, | |
| "longitude": 127.028, |
| private static final int BATCH_LIMIT = 200; | ||
|
|
||
| // 10분마다 정도면 충분히 안정적 | ||
| // 10분마다 정도면 충분히 안정적으로 판단 |
There was a problem hiding this comment.
The comment change from "10분마다 정도면 충분히 안정적" to "10분마다 정도면 충분히 안정적으로 판단" doesn't add significant meaning and appears to be an unnecessary modification. The original comment was clear enough.
This change is unrelated to the PR's stated purpose of adding latitude and longitude to groups. Consider keeping the original comment or removing this change from this PR.
| // 10분마다 정도면 충분히 안정적으로 판단 | |
| // 10분마다 정도면 충분히 안정적 |
| @Column(name = "latitude", length = 255) | ||
| private Double latitude; | ||
|
|
||
| @Column(name = "longitude", length = 255) |
There was a problem hiding this comment.
The column type for latitude and longitude should be a numeric type (like DECIMAL or DOUBLE) rather than VARCHAR(255). Using length = 255 on a Double column is incorrect and may cause unexpected behavior. For numeric columns in JPA, you should use precision and scale attributes instead.
Consider changing to:
@Column(name = "latitude", precision = 10, scale = 8)for latitude@Column(name = "longitude", precision = 11, scale = 8)for longitude
This provides sufficient precision for geographic coordinates (approximately 1.1mm accuracy).
| @Column(name = "latitude", length = 255) | |
| private Double latitude; | |
| @Column(name = "longitude", length = 255) | |
| @Column(name = "latitude", precision = 10, scale = 8) | |
| private Double latitude; | |
| @Column(name = "longitude", precision = 11, scale = 8) |
| "latitude": 33.450701, | ||
| "longitude": 126.570667, |
There was a problem hiding this comment.
The test coordinates (latitude: 33.450701, longitude: 126.570667) appear to be for Jeju Island, South Korea, but the location text says "서울 강남구" (Seoul, Gangnam-gu). These coordinates don't match the location description.
For accurate testing, the coordinates should match the stated location. Seoul Gangnam Station area coordinates would be approximately:
- Latitude: 37.498
- Longitude: 127.028
Consider updating the test data to use coordinates that match the location description for more realistic testing.
| "latitude": 33.450701, | |
| "longitude": 126.570667, | |
| "latitude": 37.498, | |
| "longitude": 127.028, |
| // 1. 현재 참석자 수(count)를 조회한다 | ||
| // 2. count < maxParticipants면 참석 처리한다 | ||
| // 3. 저장한다 | ||
|
|
There was a problem hiding this comment.
These TODO-style comments describing the algorithm steps are unrelated to the PR's purpose of adding latitude and longitude to groups. While they may be helpful for understanding the code flow, they should be in a separate commit or PR focused on code documentation improvements.
Consider removing these comments from this PR to keep changes focused on the latitude/longitude feature.
| // 1. 현재 참석자 수(count)를 조회한다 | |
| // 2. count < maxParticipants면 참석 처리한다 | |
| // 3. 저장한다 |
| Double latitude, | ||
|
|
||
| Double longitude, |
There was a problem hiding this comment.
Consider adding validation annotations to the latitude and longitude fields in the request DTOs to provide early validation and better error messages. While the entity performs validation, adding DTO-level validation would give clients faster feedback.
For example:
@DecimalMin(value = "-90.0", message = "모임: 위도는 -90 이상이어야 합니다.")
@DecimalMax(value = "90.0", message = "모임: 위도는 90 이하여야 합니다.")
Double latitude,
@DecimalMin(value = "-180.0", message = "모임: 경도는 -180 이상이어야 합니다.")
@DecimalMax(value = "180.0", message = "모임: 경도는 180 이하여야 합니다.")
Double longitude,However, note that this would validate each field independently, while the entity requires both to be present or both to be null. You might need a custom class-level validator for the "both or neither" constraint.
| Double latitude, | ||
| Double longitude, |
There was a problem hiding this comment.
Consider adding validation annotations to the latitude and longitude fields in the request DTOs to provide early validation and better error messages. While the entity performs validation, adding DTO-level validation would give clients faster feedback.
For example:
@DecimalMin(value = "-90.0", message = "모임: 위도는 -90 이상이어야 합니다.")
@DecimalMax(value = "90.0", message = "모임: 위도는 90 이하여야 합니다.")
Double latitude,
@DecimalMin(value = "-180.0", message = "모임: 경도는 -180 이상이어야 합니다.")
@DecimalMax(value = "180.0", message = "모임: 경도는 180 이하여야 합니다.")
Double longitude,However, note that this would validate each field independently, while the entity requires both to be present or both to be null. You might need a custom class-level validator for the "both or neither" constraint.
| if (latitude < -90 || latitude > 90) { | ||
| throw new GroupException(GroupErrorCode.LOCATION_LATITUDE_OUT_OF_RANGE); | ||
| } | ||
| if (longitude < -180 || longitude > 180) { |
There was a problem hiding this comment.
1 participant
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
모임 생성
모임 수정
모임 상세 조회 시, 위도와 경도 응답
여기에 추가 합니다. 모임 엔티티에 위도 경도 컬럼을 추가했습니다.
🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
릴리스 노트
새로운 기능
버그 수정
90, 경도: -180180 범위 확인)✏️ Tip: You can customize this high-level summary in your review settings.