-
Notifications
You must be signed in to change notification settings - Fork 4
Feat/23 implement Bookmark #28
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
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
=============================================
+ Coverage 50.00% 71.73% +21.73%
- Complexity 2 22 +20
=============================================
Files 2 7 +5
Lines 8 46 +38
Branches 0 1 +1
=============================================
+ Hits 4 33 +29
- Misses 4 13 +9
☔ View full report in Codecov by Sentry. |
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.
매우매우 매우 좋습니다 쵝오
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkControllerTest.kt
Outdated
Show resolved
Hide resolved
추가 테스트 시나리오
db mockup으로 가능하다면
만약 위 db mockup이 해당 spring boot test에서만 가능하다면 넘어가주시면 됩니다. 해당 #31 이슈와 함께 진행하겠습니다. (해당 사항에 대해서 댓글로 조사한 부분, 의견 알려주세요) 차후 Git issue화 또는 추가 기능JSON request 지원 |
잘하셨습니다. 다만 |
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkServiceTest.kt
Show resolved
Hide resolved
위 댓글에서 남긴 db mock up 테스트는 추가로 PR test checklist에 새로 추가한 Test도 포함시켜주세요 (Git issue도 마찬가지) |
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.
고생 많으셨습니다 ㅎㅎ 역쉬 에이스 JSP!!
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkControllerTest.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Show resolved
Hide resolved
- change variable names - change response HTTP status - add Content-Location
|
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.
좋습니다. 계속 좋아지고 있네요! (사실 코딩하시는 기간동안 읽은걸 체크하는거에 가깝네요)
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/group4/ticketingservice/controller/BookmarkController.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkControllerTest.kt
Show resolved
Hide resolved
src/test/kotlin/com/group4/ticketingservice/bookmark/BookmarkControllerTest.kt
Outdated
Show resolved
Hide resolved
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.
아주 좋습니다! 다음 수정이 마지막일것 같은 예감!
|
||
// 북마크 삭제 | ||
@DeleteMapping("/{id}") | ||
fun deleteBookmark(@PathVariable id: Long): ResponseEntity<Any> { |
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.
entity/Bookmark.kt
에서는 INT
인데 통일하시는게 좋을것 같습니다.
mysql pk data type에 관련된 정보 함께 학습하셔서 정해주시면 좋을 것 같습니다.
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.
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.
@ParkJeongseop 아래 주제에 대해 생각 남겨주시면 좋을 것 같습니다.
- MySQL에서 PK int vs long
- (1)에 따른 Kotlin Int vs Long 자료형 표현 범위
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.
Gj!!
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.
GJ
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.
수고수고하셨습니다!!
What is this PR?
Key Changes
Test Checklist
controller
service
repository