[#118] 회원가입 2단계 API 구현 #122
Conversation
|
따라서 해당 기능이 다른 클래스로 이전되었으면 좋겠습니다. 네이밍이 잘 안떠오르는데 AdditionalInfoController정도로 해야할거 같네요.
맞는 말씀이라고 생각하지만 프론트 분들이 해당 API를 찾을 때, 회원가입 API란에서 찾으실 거 같아 협업의 편리성을 위해 Tag는 회원가입으로 해놓으면 좋을거 같습니다. (아마 저희가 회원가입이라고 표현해서 멘토님이 아직 User테이블에 저장이 되지 않았다고 오해하시면서 말이 헛돌다가 했던 이야기 였던거 같아요 은비님이 해당 기억때문에 헷갈릴까봐 작성했습니다.)
|
| } | ||
| @Schema(type ="array", description = "나만의 관심사") | ||
| @Size(max = 10) | ||
| private List<AddCustomInterestDto> customInterests = new ArrayList<>(); |
There was a problem hiding this comment.
- 필드에서 초기화를 해주고 계신 이유가 뭘까요?
그냥 선언만 하면 동작안하나요? - AddSocialAccountDto를 확인해보면 그냥 String이던데 클래스를 선언하신 이유가 뭔가요?
- AddCustomInterestDto를 확인해보면 그냥 String이던데 클래스를 선언하신 이유가 뭔가요?
- recommendInterests와 customInterests를 구분해서 받고 계신데 그냥 interests로 받으면 되지 않나요?
- customInterests에서 custom이라는 표현은 부적절하지 않을까요? 어떤 면에서 custom이라 생각하신거죠?
- customInterests의 description에서 나만의 라는 표현은 부적절하지 않을까요?
- customInterests와 recommendInterests를 합쳐서 10개 이하인지를 확인해야하는거 아닌가요?
There was a problem hiding this comment.
추천 관심사와 나만의 관심사를 구분하셨던 이유가 뭘까를 고민하고 있는데 의미가 있을 수는 있겠네요.
화면에 추천 관심사를 넘겨줄 때, name만 넘겨주는 것이 아닌 hashtagId를 함께 넘겨주고 recommendInterests는 hashtagId로 받는다면 DB에서 해야 하는 일을 줄일 수도 있겠네요.
이건 은비님이 고민해보시죠
There was a problem hiding this comment.
- 한 번 선언만 한 후, 실행에 무리가 없는지 확인해볼게요. 잭슨 역직렬화 관련해서 다른 분의 조언을 받아, 초기화까지 진행했던 부분이구요.
- 아래의 3번과 함께 String으로 수정하도록 하겠습니다. 같은 맥락으로 역직렬화 오류를 해결하기 위해서 시도하다가 Dto를 넣었던 부분이에요.
- 하나의 interest로 받는 것으로 수정하도록 하겠습니다. 프론트 분과 이야기를 나눴을 때, customInterest만 최대 10개로 허용이 되는 것으로 이야기를 나눴었어요. 하지만, 회의에서 이야기를 나눴던 것처럼 서로 잘못 기억을 하고 있었던 것 같구요.
| @@ -22,9 +22,9 @@ | |||
| </where> | |||
| </select> | |||
|
|
|||
| <insert id="save" parameterType="hashTag" useGeneratedKeys="true" keyProperty="hashTagId" keyColumn="hashtag_id"> | |||
| <insert id="save" parameterType="flab.project.domain.post.model.HashTag" useGeneratedKeys="true" keyProperty="hashTagId" keyColumn="hashtag_id"> | |||
There was a problem hiding this comment.
parameterType에서 그냥 HashTag로 하면 동작안하나요?
| }) | ||
| @PostMapping(value = "/users/{userId}/social-accounts-and-interests") | ||
| public SuccessResponse addSocialAccountAndInterest(@PathVariable("userId") @Positive long userId, | ||
| @RequestBody @Valid CreateUserAdditionalInfoRequestDto createUserAdditionalInfoRequestDto) { |
There was a problem hiding this comment.
줄 바꿈 컨벤션은 08.14 회의에서 이야기를 좀 해보면 좋을거 같습니다.
There was a problem hiding this comment.
"추가 정보 입력 페이지"에서는 소셜 계정과 관심사를 받고 있지만, 추후에 어떤 것이 추가될 지, 삭제될 지 예상할 수 없습니다.
굳이 social-accounts-and-interests를 API URL에서 노출하는 것 보다는 additional-info정도로 url을 퉁치는 것이 좋지않을까요?
| createUserAdditionalInfoRequestDto.getCustomInterests().forEach(customInterest -> { | ||
| interestFacade.addInterest(new AddInterest(userId, customInterest.getInterestName())); | ||
| }); | ||
|
|
There was a problem hiding this comment.
-
소셜계정과 관심사가 각각 3개,10개면 총 13번의 DB통신이 이루어지는걸까요?
소셜계정를 한번에 묶고 관심사를 한번에 묶어서 쿼리를 보내주는 것이 좋지 않을까요? -
소셜 계정 저장 작업까지 완료되고 관심사 저장 중 에러가 발생하면 소셜 계정 저장은 롤백시켜야하지 않을까요?
해당 작업들이 하나의 트랜잭션으로 묶여야 할 것으로 생각되는데 아닌가요? -
해당 작업들이 Controller에서 이루어 지는 것이 맞을까요?
| interestMapper.save(1L, 1L, "java"); | ||
| interestMapper.save(1L, 2L, "spirng"); | ||
| interestMapper.save(userId, 1L); | ||
| interestMapper.save(1L, 2L); |
There was a problem hiding this comment.
왜
36L의 첫 번째 파라미터는 userId변수를 사용하시고
37L의 첫 번째 파라미터는 1L을 주셨나요?
다른 interestMapper도 통일되면 좋을 거 같습니다.
|
sonarCloud에서 남긴 issue2개도 확인부탁드립니다. |
📢 관련 이슈
📃 작업 사항