[SOU-456] 우선순위가 있는 도시를 변경시 중복되는 버그 수정#179
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Walkthrough우선순위 상향 이동 시 oldPriority 값을 oldPriority + 1에서 oldPriority로 변경하여 우선순위 조정 범위를 축소했습니다. 이에 따라 테스트 케이스를 확대하여 다양한 시나리오의 우선순위 조정 및 저장소 조회 동작을 검증합니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4790b8bf99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isMovingUp(oldPriority, newPriority)) { | ||
| pushNewPriorityIfExists(excludeCityId, newPriority, countryId); | ||
| pullOldPriorityIfExists(excludeCityId, oldPriority + 1, countryId); | ||
| pullOldPriorityIfExists(excludeCityId, oldPriority, countryId); |
There was a problem hiding this comment.
isMovingUp 분기에서 pullOldPriorityIfExists에 oldPriority를 넘기면, 내부 구현이 다시 oldPriority + 1부터 조회/당김을 수행하므로 실제로는 기존 oldPriority + 1 도시를 oldPriority로 내리게 됩니다. 같은 분기의 pushNewPriorityIfExists(newPriority)가 이미 oldPriority-1 도시를 oldPriority로 올린 상태(예: 5→2 이동, 2/3/4/6 존재)에서는 두 도시가 동일 우선순위(5)를 갖는 중복이 발생해 정렬/노출 결과가 깨집니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the city priority adjustment logic where changing a city's priority to a lower number (higher priority) was incorrectly querying for cities to pull. The bug caused the system to skip one priority level when filling the gap left by the moved city.
Changes:
- Fixed the
isMovingUpcase inCityPriorityDomainServiceto query from the correct priority level when pulling cities - Updated existing tests to verify the actual priority update operations
- Added comprehensive test coverage for edge cases including adjacent priority changes, target city filtering, and null-to-null changes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/java/com/souzip/api/domain/city/service/CityPriorityDomainService.java | Fixed bug by changing oldPriority + 1 to oldPriority in the pull operation when moving a city to a higher priority |
| src/test/java/com/souzip/api/domain/city/service/CityPriorityDomainServiceTest.java | Updated existing tests with proper verifications, added new test cases for edge scenarios, and corrected test expectations to match the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // then | ||
| verify(city3).updatePriority(2); // 3->2로 당겨짐 | ||
| } |
There was a problem hiding this comment.
This test is missing verification of the repository method calls. Consistent with other tests in this file, you should verify which repository queries were made. For this scenario, you should verify that the repository was called with the correct priority values to ensure the test properly validates the implementation behavior.
| cityPriorityDomainService.adjustPriorities(excludeCityId, oldPriority, newPriority, countryId); | ||
|
|
||
| // then | ||
| verify(city2).updatePriority(3); // 2->3으로 밀림 |
There was a problem hiding this comment.
This test is missing verification of the repository method calls. Consistent with other tests in this file, you should verify which repository queries were made. For this scenario, you should verify that the repository was called with the correct priority values to ensure the test properly validates the implementation behavior.
| verify(city2).updatePriority(3); // 2->3으로 밀림 | |
| verify(city2).updatePriority(3); // 2->3으로 밀림 | |
| verify(cityRepository).findByCountryIdAndPriorityGoeOrderByPriorityAsc(countryId, 2); | |
| verify(cityRepository).findByCountryIdAndPriorityGoeOrderByPriorityAsc(countryId, 4); |
| // when | ||
| cityPriorityDomainService.adjustPriorities(excludeCityId, oldPriority, newPriority, countryId); | ||
|
|
||
| // then |
There was a problem hiding this comment.
This test is missing verification of the repository method calls. Consistent with other tests in this file, you should verify which repository queries were made. For this scenario, you should verify that the repository was called with priority value 3 to ensure the test properly validates that the target city filtering logic is working as expected.
| // then | |
| // then | |
| verify(cityRepository, times(1)).findByCountryIdAndPriorityGoeOrderByPriorityAsc(countryId, 3); |
Summary by CodeRabbit
릴리스 노트
버그 수정
테스트