-
Notifications
You must be signed in to change notification settings - Fork 1
Feat: 화면 안의 요청들 거리순 정렬 후 반환 #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📊 Code Coverage Report
|
Test Results209 tests 209 ✅ 6s ⏱️ Results for commit 6de7b67. ♻️ This comment has been updated with latest results. |
GitJIHO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
트레이드오프를 고려한 적절한 판단인 것 같습니다 👍 고생하셨습니다!
domain필드 추가에 관한 수정 하나만 부탁드립니다 !
| import java.time.Instant; | ||
| import java.util.UUID; | ||
|
|
||
| public abstract class DdipEventFixture { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixture 만들어두는거 좋네요 👍
| @Column(nullable = false) | ||
| @Setter | ||
| private String cellId; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생각해보니 notion에는 ddipEvent 엔티티 필드에 cellId가 정의되어 있지 않았네요
정규화가 되어있던건지,, 관련해서 한번 어떤식으로 현재 프론트 fakeRepository의 비즈니스 로직이 구현되어있는지 이야기해보면 좋을 것 같습니다
entity의 필드가 변경되면 노션의 필드도 변경하는게 연동에 좋을 것 같아 그렇습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨트롤러단에서 ddipEvent를 생성했을 때 리턴값으로 ddipEvent 도메인을 기반으로 한 dto Json으로 변환해서 제공하기 때문에 cellId에 대한 Mapper에서 toDomain 부분에도 cellId 관련 로직과 DdipEvent 도메인 자체에도 cellId 필드가 있어야 할 것 같습니다.
추후 cellId를 활용한 타 비즈니스 로직이 필요할 때도 현재 domain과 entity가 나뉘어져 있기 때문에 domain단에도 필드가 정의되어 있어야 DDD를 활용가능 할 것 같네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 넵! 수정하겠습니다.
#️⃣ 연관된 이슈
📚 배경
초기 화면에서 사용자의 위치로부터 가까운 순으로 요청을 보여주는 기능이 필요합니다.
📝 작업 내용
📸 스크린샷
x
💬 리뷰 요구사항
✏ Git Close
close #26