-
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
[REFACTOR #115] STOMP를 이용한 성지순례 장소 인증 기능 리팩토링 #122
Conversation
[FIX] 성지순례 필드 수정
fix/#75 성지순례 상세 조회 접근 권한 해제
[배포] 작업사항 반영
[배포] 수정 사항 반영
방문 인증 api 배포
방문 인증 api 수정
서버 시간 UTC 변경
[FIX] 어플리케이션/서버 시간 통일
어플리케이션 시간 한국으로 설정
[FIX] 홈 api 수정, 서버 시간
[FIX] complete rally JPA 에러 수정
[배포] 수정 사항 반영
너무 수고하셨습니다! 우선 바로 머지하고 코드리뷰 달아볼게요~! |
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.
몇가지 궁금한점이나 코멘트 남겨봤습니다!
"인증하기" 버튼 여부를 위해서 redis를 사용했습니다.
여기서 사용자가 성지순례 근처 장소에 왔고 인증버튼을 눌렀다면 24간동안 redis에 저장되는 게 맞을까요?!
Pilgrimage pilgrimage = pilgrimageRepository.findById(pilgrimageId) | ||
.orElseThrow(()->new RestApiException(ErrorCode.PILGRIMAGE_NOT_FOUND)); |
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.
STOMP 구독/발행 특성 때문에 해당 파일명을 Controller로 명시했으나, http 연결 요청을 보내는 controller는 아닙니다. MessageMapping/SendTo 둘 다 이미 http로 연결중인 구독/발행 이벤트를 발생시키는 어노테이션이라 서비스단이라고 생각하고 처리했습니다.
return pilgrimage.getLatitude() + 0.00135 >= form.getLatitude() && pilgrimage.getLatitude() - 0.00135 <= form.getLatitude() | ||
&& pilgrimage.getLongitude() + 0.00135 >= form.getLongitude() && pilgrimage.getLongitude() - 0.00135 <= form.getLongitude(); |
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.
단순 궁금해서 그런데 0.00135는 어떤 용도로 사용되는 값인가요?!
그리고 해당 값은 상수로 선언해서 사용해도 좋을 것 같아요! (어떤 걸 의미하는 지도 전달하기 좋을 것 같습니당)
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.
0.00135는 위도/경도 상에서 100M 거리를 정확하게 표현할 수 없어서 근사치를 구해서 사용했습니다 상수로 빼두겠습니다
public PilgrimageSocketDto.ButtonState determineButtonState(Long memberId, | ||
Long pilgrimageId, |
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.
memberId
, pilgirmageId
가 null일 경우가 없다면 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.
다른 부분도 해당 내용 반영해서 수정하겠습니다 Long과 long 두 차이점을 신경쓰지 않았는데 덕분에 더 좋은 방향으로 수정할 수 있을 것 같습니다!
// 사용자가 인증 장소에 있는지 확인 | ||
boolean nearPilgrimage = isUserAtPilgrimage(pilgrimage, latitude, longitude); | ||
// redis에 사용자의 인증 기록이 남아있는지 확인 (24시간 이후 만료됨) | ||
boolean isCertificationExpired = redisService.isCertificationExpired(member, pilgrimage); |
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.
요기서 선언한 isCertificationExpired
는 혹시 어디서 사용되고 있는 걸까요??
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.
프론트에서 일단 작업한 부분까지 바로 머지해달라고 요청하셔서 미완성한 상태로 올렸습니다 pr올라간 코드에서 사용하지 않고 있습니다
if (nearPilgrimage && !certifiedInLast24Hours) { | ||
newState.setCertifyButtonEnabled(true); // 인증하기 버튼 활성화 | ||
newState.setGuestbookButtonEnabled(false); // 방명록 쓰기 버튼 비활성화 | ||
} | ||
else if (certifiedInLast24Hours && !hasWrittenGuestbook) { | ||
newState.setCertifyButtonEnabled(false); // 인증하기 버튼 비활성화 | ||
newState.setGuestbookButtonEnabled(true); // 방명록 쓰기 버튼 활성화 | ||
} | ||
else { | ||
newState.setCertifyButtonEnabled(false); // 인증하기 버튼 비활성화 | ||
newState.setGuestbookButtonEnabled(false); // 방명록 쓰기 버튼 비활성화 | ||
} |
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.
요기에서 isCertificationExpired
가 사용되고 있지 않는 것 같아서 궁금해서요..!!
(제가 못 찾은 걸 수도 ..🥺)
이 부분에 대해서 사용자가 성지순례 근처 장소에 왔고, 인증버튼을 눌렀다면 RDB에 저장됩니다. redis에 저장되는건 "사용자가 성지순례 근처 장소에 왔다" 라는 정보입니다. 해당 정보는 인터페이스를 나타내는 용도 외에는 저장할 필요가 없기 때문에 RDB에 넣는건 적합하지 않다고 판단했습니다. 버튼 분기를 최대한 단순하게 표현하기 위해서 누적돼서 쌓이고, 24시간이 지나면 자동으로 사라지게 작업하고 있습니다 |
📄 Work Description
⚙️ ISSUE
📷 Screenshot
💬 To Reviewers
리뷰어들에게 하고 싶은 말