-
Notifications
You must be signed in to change notification settings - Fork 0
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 및 기능 구현 #88
Conversation
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.
바쁘셨을텐데 밤새 고생많으셨습니다..
api 호출 관련 코드에서 어떤 코드는 hook이랑 같이 작성하고, 어떤 코드는 hook이랑 분리해놓아서, 이 부분 조금 통일해서 수정해야할 것 같습니다.
그리고 저희 router 경로 부분은 상수로 만들어 놓은걸로 알고있어서 경로 부분은 상수 경로로 수정해주시면 감사하겠습니다!
src/pages/sinitto/call-back/detail/api/hooks/useAcceptCallback.ts
Outdated
Show resolved
Hide resolved
src/pages/sinitto/call-back/detail/api/hooks/useCancelCallback.ts
Outdated
Show resolved
Hide resolved
src/pages/sinitto/call-back/detail/api/hooks/useCompleteCallback.ts
Outdated
Show resolved
Hide resolved
src/pages/sinitto/call-back/detail/api/hooks/useAcceptCallback.ts
Outdated
Show resolved
Hide resolved
제 생각에는 나중에 상태 관리 라이브러리로 콜백 요청 시 받은 시니어 id 를 관리해서 가이드라인 페이지에서의 API 재호출을 방지하는 방법도 괜찮을 것 같아요. 고생 많으셨습니다! |
전역 상태 관리 라이브러리도 생각해보긴 했는데, a,b처럼 콜백 요청에 해당하는 시니어가 아닌 다른 시니어의 가이드라인이 조회되는 문제가 있을거라고 생각했습니다. a(URI), b(스토리지) 방법과 전역 상태 관리 방법에서 시니어id를 저장하면 현재 콜백 요청에 해당하는 시니어인지 아닌지 검증하는 과정이 없다고 생각해 c(API 재호출) 방법으로 구현했는데요. 지금 다시 생각해보니 콜백id와 시니어id를 모두 관리해서 현재 URI의 콜백id와 비교하는 방식으로 구현하면 될 것 같습니다! 전역 관리로 수정하는 쪽으로 진행할게요. |
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.
음.. 리뷰에 내용이 있는걸 방금 알았네요..
시니어 아이디가 uri에 노출되는게 큰 문제가 있나요...? 유나님이 생각한 예시를 보니까 스토리지(로컬 or 세션)에 넣으면 뺴기 전까지 상태가 그대로 유지되기 때문에 조금 문제가 생기는 것 같은데, 저는 uri로 넘기는게 맞다고 생각합니다.
사실 제가 전역상태관리 라이브러리로 zustand를 쓰자고 제안했긴 한데, 요즘 또 공부하면서 드는 생각이 '굳이 전역 상태를 외부 라이브러리까지 사용하면서 구현할 이유가 있나? 필요하면 context API 쓰는게..?'
그리고 전역 상태관리를 사용하면 새로고침할때 정보가 다 날라가는데 이때 시니어 id가 날라가면 조금 문제가 생길것 같긴 하네요..
전역 상태관리는 '컴포넌트와 컴포넌트 사이가 멀때 사용하자!' 라는 생각을 가지고 있긴 한데, 누군가가 '이거 전역으로 왜 관리했어요?' 라고 물어봤을 때 타당한 이유와 함께 본인이 생각하는 것을 전달하면 전역 상태관리를 굳이 써도 상관없긴 할 것 같습니다!
아니면 백엔드 분들한테 시니어 id나 콜백 id만 받아올 수 있는 api를 만들어 달라고 요청하고, 그들의 생각을 들어보는 것도 좋은 방법일 것 같네요!
유나님이 적절하게 판단해서 구현하실꺼라 믿고 Approve 해놓겠습니다!
시니어id가 단순 숫자다 보니 노출되어도 큰 문제는 없을 것 같긴하네요. api를 수정하지 않는 한 어떤 방법이든 시니어id가 콜백 요청에 해당하는지 아닌지에 대한 검증 과정은 필요하다고 생각해요. 그렇다면 가이드라인 조회 페이지에서 콜백 요청을 불러오긴 해야하는데, 중복 호출이 너무 많이 발생할 것 같아서 이를 방지하고자 전역 상태 관리로 콜백id와 시니어id를 관리하는 방식을 생각해봤습니다. 페이지 이동이나 새로고침 시 날라가는 문제는 context를 스토리지에 저장하면 유지될텐데 지금 생각해보니 이건 그냥 스토리지에 저장하는 것과 관리말고는 별 차이가 없는 것 같네요. 그리고 스토리지에 저장하면 무결성을 완전히 보장할 수는 없으니 결국 검증이 다시 필요할 것 같네요... 검증을 하기 위해 콜백 조회 api를 호출하면 결국 시니어id를 받아오니 따로 시니어id를 넘기거나 저장할 필요가 없어지는 것 같습니다. 계속 생각해봤는데, api를 수정하거나 추가하는 쪽이 깔끔할 것 같습니다.
이건 백엔드쪽과 상의 후에 수정하겠습니다. 일단 중복호출 되더라도 재호출하는 기존 방식으로 구현한 것으로 merge 하겠습니다. 이쪽 관련 수정사항은 새로 issue 만들어서 진행하겠습니다! |
#️⃣ 연관된 이슈
📝 작업 내용
스크린샷 (선택)
💬 리뷰 요구사항(선택)
기존 시니또의 가이드라인 조회 주소는 '/call-back/{콜백id}/{가이드라인id}'
가이드라인 조회 시 시니어id가 필요해서 전달해야함.
a. URL 파라미터(/call-back/{콜백id}/{시니어id}/{가이드라인id}) > 시니어id 노출됨. 사용자가 임의로 URL을 입력 시 콜백 요청에 해당하는 시니어가 아닌 다른 시니어의 가이드라인이 조회될 수 있음.
b. 로컬 또는 세션 스토리지 > 해당 콜백요청의 시니어 id가 저장되고, 다른 페이지로 이동 후 콜백 가이드라인 조회 페이지에 접속(임의로 주소 직접 입력)하면 다른 콜백요청이더라도 이전에 저장된 시니어Id로 조회되는 문제 발생 (= 콜백 요청에 해당하는 시니어가 아닌 다른 시니어의 가이드라인이 조회될 수 있음)
c. 콜백 요청 api를 호출해 받은 시니어id로 가이드라인 api 호출 > 이전 페이지(콜백 요청 조회 페이지)에서도 이미 콜백 요청 api를 호출하는데, 가이드라인 페이지에서도 또 호출하게 됨.
우선은 c.(콜백 요청 api 호출 이후 가이드라인 api 호출)로 구현 했는데, 괜찮을까요? 혹은 더 좋은 방법이 있을까요? (/pages/sinitto/guide-line/SinittoGuideLinePage.ts 참고)
(2,3번은 api 수정에 대한 내용이라 코드리뷰 시 해당 사항 참고부탁드립니다!)
콜백 요청 조회 api (/api/callbacks/{callbackId})에 전화번호 추가 필요함. (현재는 response에 전화번호가 없어서 임의로 넣어놨습니다)
콜백 요청 상세 페이지에서 해당 콜백 요청이 시니또가 진행중인 콜백 요청인지 확인하는 방법이 시니또에게 할당된 콜백조회 api (/api/callbacks/sinitto/accepted)로 할당된 콜백를 받아와 현재 페이지의 콜백id를 비교하는 방법 밖에 없는 것 같은데, 콜백 단건 조회 api (/api/callbacks/{callbackId})에서 현재 시니또에게 할당된 것인지 아닌지를 추가하면 좋을 것 같습니다. (현재는 할당된 콜백을 받아와 비교하는 방법으로 구현)
⏰ 현재 버그
✏ Git Close
close #58