refactor(apply): refactor controller #570

summerlunaa · 2022-07-16T09:48:18Z

close: #561

create api reponse 수정 (create시에 Location header에 id 를 넣어준다.)
- 추가로 응답 body에 생성된 객체를 담아주도록 수정
id로 조회하는 api 추가
- cheater
- mission
- term
TermController에 @LoginUser 추가

kth990303

역시 코틀린 마스터들이 코드를 작성하니 뛰어난 결과물이 나오네요.
슬랙에서 얘기나왔던 부분들 관련해서 코멘트 남겼습니다!
해당 컨벤션 부분 외에는 모두 괜찮아서 approve 할게요~

kth990303 · 2022-07-19T06:12:42Z

src/main/kotlin/apply/ui/api/AssignmentRestController.kt

+        val assignmentId = assignmentService.create(missionId, user.id, request)
+        return ResponseEntity
+            .created(URI.create("/api/recruitments/$recruitmentId/missions/$missionId/assignments/$assignmentId"))
+            .build()


슬랙에서 스레드로 얘기했을 때 body로 id를 담아서 객체로 넘겨주는 것으로 결론이 난 것 같은데, 이슈는 반대로 돼있긴 하네요. 이 부분은 확정이 안난 거 맞을까요?

kth990303 · 2022-07-19T06:14:52Z

src/main/kotlin/apply/application/AssignmentService.kt

@@ -17,14 +17,15 @@ class AssignmentService(
    private val missionRepository: MissionRepository,
    private val evaluationTargetRepository: EvaluationTargetRepository
 ) {
-    fun create(missionId: Long, userId: Long, request: AssignmentRequest) {
+    fun create(missionId: Long, userId: Long, request: AssignmentRequest): Long {


다른 서비스에선 save라는 메서드명으로 돼있는데, 이 서비스 클래스에서만 생성 쪽 메서드명이 create로 돼있긴 해요. 크게 상관없을 듯하지만, 혹시나 해서 남겨봅니다!

kth990303 · 2022-07-19T06:17:39Z

src/main/kotlin/apply/ui/api/MailHistoryRestController.kt

@@ -24,8 +25,8 @@ class MailHistoryRestController(
        @LoginUser(administrator = true) user: User
    ): ResponseEntity<Unit> {


이 부분도 아직 확정안된 것 같긴 한데 코멘트는 남겨볼게요! 슬랙에서 controller 생성 api에 save와 create 중에 create로 결론이 난 것 같아요. 이슈엔 별도의 언급이 없긴 하지만 혹시나 해서 남겨봅니다!

kth990303 · 2022-07-19T06:17:57Z

src/test/kotlin/apply/ui/api/AssignmentRestControllerTest.kt

@@ -40,7 +39,13 @@ internal class AssignmentRestControllerTest : RestControllerTest() {
            content = objectMapper.writeValueAsString(createAssignmentRequest())
            header(HttpHeaders.AUTHORIZATION, "Bearer valid_token")
        }.andExpect {
-            status { isOk }
+            status { isCreated }


status code를 좀 더 올바르게 바꿔주셨군요 👍

kth990303 · 2022-07-19T06:18:52Z

src/test/kotlin/apply/ui/api/CheaterRestControllerTest.kt

+        val cheater = Cheater(email = cheatedUser.email, id = cheatedUser.id)
+        every { cheaterService.getById(cheater.id) } answers { cheater }
+
+        mockMvc.perform(
+            RestDocumentationRequestBuilders.get(
+                "/api/cheaters/{cheaterId}", cheater.id
+            )


id를 반환하는 로직, 조회 로직을 추가해줌으로써 테스트가 용이해지네요! 😄
덕분에 코드 짜기에도 편할 것 같아요.

Co-authored-by: dongho108 <[email protected]>

asebn1

수고하셨습니다! id와 객체를 같이 전달받으니 controller테스트에서 값을 검증하기 더 용이해진 것 같네요 👍

asebn1 · 2022-07-23T15:08:02Z

src/test/kotlin/apply/application/CheaterServiceTest.kt

@@ -31,6 +33,7 @@ internal class CheaterServiceTest {
        val cheaterData = createCheaterData()
        every { cheaterRepository.existsByEmail(any()) } returns false
        every { cheaterRepository.save(any()) } returns Cheater(cheaterData.email, cheaterData.description)
+        every { userRepository.findByEmail(any()) } returns createUser()


asebn1 · 2022-07-23T15:10:51Z

src/test/kotlin/apply/ui/api/AssignmentRestControllerTest.kt

+            status { isCreated }
+            header {
+                string(
+                    HttpHeaders.LOCATION,
+                    "/api/recruitments/$recruitmentId/missions/$missionId/assignments/${assignmentData.id}"


잘 바꿔주셨네요 😀

asebn1 · 2022-07-23T15:11:36Z

src/test/kotlin/apply/ui/api/CheaterRestControllerTest.kt

    fun `부정행위자를 추가한다`() {
        val cheaterData = createCheaterData()
-        every { cheaterService.save(cheaterData) } just Runs
+        val cheaterResponse = CheaterResponse(Cheater(cheaterData.email, cheaterData.description), createUser())
+
+        every { cheaterService.save(cheaterData) } returns cheaterResponse

        mockMvc.post("/api/cheaters") {
            header(HttpHeaders.AUTHORIZATION, "Bearer valid_token")
            contentType = MediaType.APPLICATION_JSON
-            content = objectMapper.writeValueAsString(cheaterData)
+            content = objectMapper.writeValueAsString(cheaterResponse)
        }.andExpect {
-            status { isOk }
+            status { isCreated }
+            header { string(HttpHeaders.LOCATION, "/api/cheaters/${cheaterResponse.id}") }
+            content { json(objectMapper.writeValueAsString(ApiResponse.success(cheaterResponse))) }
        }


추가적인 검증이 적용돼서 테스트가 더 명확해진 것 같아요!

jojogreen91

고생한게 보이네요 파랑, 애쉬!
간단하게 리뷰 남겨봤습니다 수고하셧습니다! :)

jojogreen91 · 2022-07-22T03:40:01Z

src/main/kotlin/apply/application/EvaluationService.kt

@@ -34,6 +34,8 @@ class EvaluationService(
                EvaluationItem(it.title, it.description, it.maximumScore, it.position, evaluation.id, it.id)
            }
        )
+        request.id = evaluation.id
+        return request


매개변수로 받은 request 에 저장하면서 생긴 evaluation.id 를 set 해주고 반환하는데 혹시 헷갈릴 수도 있지않을까요?

다른 서비스 클래스에서 save 메서드를 고쳤을 때는 새로운 data 객체를 생성했는데 이것만 안그런거 같아서 다시한번 보게 되네요!

jojogreen91 · 2022-07-22T03:43:27Z

src/main/kotlin/apply/domain/cheater/CheaterRepository.kt

+import org.springframework.data.repository.findByIdOrNull
+
+fun CheaterRepository.getById(id: Long): Cheater = findByIdOrNull(id)
+    ?: throw NoSuchElementException("부정행위자가 존재하지 않습니다. id: $id")


리소스가 존재하지 않는 예외를 repository 에서 발생시키도록 구현했네요!
저도 이렇게 하는게 좋은것 같습니다! :)

jojogreen91 · 2022-07-22T03:57:07Z

src/main/kotlin/apply/ui/api/TermRestController.kt

    }

    @GetMapping
-    fun findAll(): ResponseEntity<ApiResponse<List<TermResponse>>> {
+    fun findAll(
+        @LoginUser(administrator = true) user: User


@LoginUser Argument Resolver 에 관리자인지 아닌지를 판단하는 로직이 아직 구현되어 있지 않네요!

그냥 일반 유저가 접근하는걸 방지하는 용도로 사용되고 있는거죠?!

저도 확인해보니 아직은 LoginFailedException() 만 터뜨리는 것 같네요 👀

kbsat

애쉬, 파랑 고생하셨어요! 👍

kbsat · 2022-07-25T03:54:21Z

src/main/kotlin/apply/ui/api/TermRestController.kt

    }

    @GetMapping
-    fun findAll(): ResponseEntity<ApiResponse<List<TermResponse>>> {
+    fun findAll(
+        @LoginUser(administrator = true) user: User


저도 확인해보니 아직은 LoginFailedException() 만 터뜨리는 것 같네요 👀

kbsat · 2022-07-25T04:00:48Z

src/test/kotlin/apply/ui/api/CheaterRestControllerTest.kt

+    @Test
+    fun `부정행위자 id로 부정행위자를 조회한다`() {
+        val cheater = Cheater(email = cheatedUser.email, id = cheatedUser.id)
+        every { cheaterService.getById(cheater.id) } answers { cheater }


Suggested change

every { cheaterService.getById(cheater.id) } answers { cheater }

every { cheaterService.getById(cheater.id) } returns cheater

굳이 answers로 람다를 리턴하지 않아도 될 것 같아요! 👍
https://notwoods.github.io/mockk-guidebook/docs/mocking/answers/

kbsat · 2022-07-25T05:30:35Z

src/test/kotlin/apply/ui/api/TermRestControllerTest.kt

+    @Test
+    fun `기수 id로 기수를 조회한다`() {
+        val term = Term("1기", 1L)
+        every { termService.getById(term.id) } answers { term }


여기도 마찬가지로 answers 보다는 returns가 맞아보여요~!

dongho108 assigned dongho108 and summerlunaa Jul 16, 2022

dongho108 added 기능 New feature or request 리팩터링 Refactor code labels Jul 16, 2022

kth990303 approved these changes Jul 19, 2022

View reviewed changes

summerlunaa and others added 20 commits July 19, 2022 22:16

refactor(Term): change response of create api

79a2238

Co-authored-by: dongho108 <[email protected]>

refactor(Recruitment): change response of create api

7e48dac

Co-authored-by: dongho108 <[email protected]>

refactor(Mission): change response of create api

7ad2dd3

Co-authored-by: dongho108 <[email protected]>

refactor(Evaluation): change response of create api

822824a

Co-authored-by: dongho108 <[email protected]>

refactor(Cheater): change response of create api

c1174ad

Co-authored-by: dongho108 <[email protected]>

refactor(Assignment): change response of create api

244653e

Co-authored-by: dongho108 <[email protected]>

refactor(Mail): change response of create api

4cf81ae

Co-authored-by: dongho108 <[email protected]>

refactor: change variable name

33b545f

Co-authored-by: dongho108 <[email protected]>

refactor: change variable name

7142a04

Co-authored-by: dongho108 <[email protected]>

feat(Mission): getById api

2956f78

Co-authored-by: dongho108 <[email protected]>

feat(Cheater): getById api

c5f8d95

Co-authored-by: dongho108 <[email protected]>

refactor(Term): add administrator authorization

228c989

Co-authored-by: dongho108 <[email protected]>

refactor(Term): add entity to response of create api

4c320b9

Co-authored-by: dongho108 <[email protected]>

refactor(Recruitment): add entity to response of create api

2605e0c

Co-authored-by: dongho108 <[email protected]>

refactor(Term): change response type of create api

e4f4836

Co-authored-by: dongho108 <[email protected]>

refactor(Mission): add entity to response of create api

c509827

Co-authored-by: dongho108 <[email protected]>

refactor(Evaluation): add entity to response of create api

ee60c5c

Co-authored-by: dongho108 <[email protected]>

refactor(Cheater): add entity to response of create api

2ef049d

Co-authored-by: dongho108 <[email protected]>

refactor(MailHistory): add entity to response of create api

b2ddcdf

Co-authored-by: dongho108 <[email protected]>

refactor(Assignment): add entity to response of create api

f171257

Co-authored-by: dongho108 <[email protected]>

summerlunaa force-pushed the refactor/controller branch from 9a863f3 to f171257 Compare July 19, 2022 13:16

asebn1 approved these changes Jul 23, 2022

View reviewed changes

jojogreen91 approved these changes Jul 23, 2022

View reviewed changes

woowahan-pjs removed 기능 New feature or request 리팩터링 Refactor code labels Jul 25, 2022

kbsat approved these changes Jul 25, 2022

View reviewed changes

summerlunaa closed this Jul 28, 2022

summerlunaa deleted the refactor/controller branch July 28, 2022 00:43

summerlunaa mentioned this pull request Jul 28, 2022

refactor(apply): changes http api specifications for creation and single lookup #579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(apply): refactor controller #570

refactor(apply): refactor controller #570

summerlunaa commented Jul 16, 2022 •

edited

Loading

kth990303 left a comment

kth990303 Jul 19, 2022

kth990303 Jul 19, 2022

kth990303 Jul 19, 2022

kth990303 Jul 19, 2022

kth990303 Jul 19, 2022

asebn1 left a comment

asebn1 Jul 23, 2022

asebn1 Jul 23, 2022

asebn1 Jul 23, 2022

jojogreen91 left a comment

jojogreen91 Jul 22, 2022

jojogreen91 Jul 22, 2022

jojogreen91 Jul 22, 2022

kbsat Jul 25, 2022

kbsat left a comment

kbsat Jul 25, 2022

kbsat Jul 25, 2022

kbsat Jul 25, 2022

		@@ -24,8 +25,8 @@ class MailHistoryRestController(
		@LoginUser(administrator = true) user: User
		): ResponseEntity<Unit> {

	every { cheaterService.getById(cheater.id) } answers { cheater }
	every { cheaterService.getById(cheater.id) } returns cheater

refactor(apply): refactor controller #570

refactor(apply): refactor controller #570

Conversation

summerlunaa commented Jul 16, 2022 • edited Loading

kth990303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asebn1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jojogreen91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbsat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

summerlunaa commented Jul 16, 2022 •

edited

Loading