Skip to content
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

Api: ✏️ 사용자 아이디, 전화번호 수정 API 분리 #127

Merged
merged 25 commits into from
Jul 13, 2024

Conversation

psychology50
Copy link
Member

작업 이유

  • 기획 회의에서 나온 안건으로 인해 API 요구 스펙 변동
  • 기존의 사용자 아이디와 전화번호 수정을 한 번에 처리하는 API를 서로 다른 API로 분리

작업 사항

  • PATCH /v2/users/me/profilePATCH /v2/users/me/username, PATCH /v2/users/me/phone으로 수정
    • 데이터가 변경되지 않은 경우(기존의 아이디 혹은 전화번호를 사용)에 성공 응답이 아닌, 예외 응답을 발생하도록 처리 (409 에러)

리뷰어가 중점적으로 확인해야 하는 부분

  • 만약 사용자가 기존에 사용 중인 전화번호, 혹은 아이디를 수정 데이터로 보낸 경우 200 OK를 보내는 게 나을까요?
    • 비록 이 부분은 클라이언트가 자신의 전화번호 혹은 아이디를 입력한 경우 API 호출을 막긴 하겠지만, 서버에서 409 예외를 반환하는 게 맞을지 모호하네요.

발견한 이슈

  • 기존의 nameusername 유효성 검사가 잘못되어 수정했습니다. 따로 PR을 분리하려고 했는데, username 수정이 이번 PR에 포함되어야 해서 함께 수정했습니다. (미리 이야기하고 진행했어야 했는데, 뒤늦게 깨달았습니다. 죄송합니다 ㅜㅜ)

@psychology50 psychology50 added the fix 기능 수정 label Jul 11, 2024
@psychology50 psychology50 self-assigned this Jul 11, 2024
Copy link
Member

@asn6878 asn6878 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API분리작업 확인했습니다. 고생하셨습니다 👍

만약 사용자가 기존에 사용 중인 전화번호, 혹은 아이디를 수정 데이터로 보낸 경우 200 OK를 보내는 게 나을까요?

저는 클라이언트단에서 검증해 API호출을 막는다면, 저는 서버단에서 200과 409를 분리해줘야 할 이유는 없다는 생각입니다!

@psychology50
Copy link
Member Author

저는 클라이언트단에서 검증해 API호출을 막는다면, 저는 서버단에서 200과 409를 분리해줘야 할 이유는 없다는 생각입니다!

그럼 일단 막는 것으로 진행하도록 하겠습니다~

@psychology50 psychology50 merged commit 634f50f into dev Jul 13, 2024
1 check passed
@psychology50 psychology50 deleted the fix/PW-413-split-update-profile branch July 13, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix 기능 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants