-
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
fix(@yourssu/logging-system): 비회원일 때 해시 값이 달라지는 문제 해결 #63
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.
고생하셨습니다! 제가 YLS 리뷰는 처음이라 코드를 잘 이해했나 모르겠네요
아래 내용 확인 후 답변 부탁드립니다~~
hashedID 관리를 sessionStorage로 변경하신 이유에 대해서는 이해했는데요,
로그가 10개 쌓이면 서버로 전송한다
는 부분은 변하지 않았기 때문에
서버에 전송되는 로그 배열 내부를 보면 각 로그의 hashedID가 다를 수 있다고 이해했어요.
(1) 접속 후 사이트 이용. hashedID = A
(2) 새 탭을 열거나, 브라우저 종료 후 재접속
(3) 재접속 후 사이트 이용. hashedID = B
(4) 로그가 10개가 되어 서버로 전송. 이때 각 로그의 hashedID는 A 또는 B
저는 10개의 hasedID가 모두 동일한 게 자연스럽게 느껴져서 그런지
이래도.. 되나..? 싶은데 괜찮을까요? 저보다는 제롬이 더 잘 알 거 같습니다 ㅎㅎ
|
||
if (userId === '') { | ||
if (existLocalHashedId) { | ||
localHashedId = JSON.parse(window.localStorage.getItem('yls-web') as string).hashedId; |
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.
기존 코드는 localStorage의 yls-web에서 hashedId를 가져오는데, hashedId라는 필드가 존재하지 않습니다.
아하 기존 코드에 문제가 2개 있었군요!
-
로그가 있는 경우에는
yls-web
이 배열이므로, 라인 28처럼 접근하면localHashedId
는undefined
일 수 밖에 없음 -
키 값도 다름 (
hashedId
가 아니라hashedID
)
기존에는 userId가 빈 문자열일 때를 기준으로 sha-256 hashing이 진행되었습니다. 그래서 아마... 모든 유저의 해시 값이 똑같이 들어갔을 겁니다.
헉 진짜네요 ㅋㅋㅋㅋㅋ
제가 노션 이슈에 첨부했던 사진이랑, 지금 다시 테스트 해볼 때랑 모두 userId === ''
기준으로 만들어져서 값이 47DEQpj8HBSa~
로 같아요
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.
안드로이드는 종료할 때 개수에 관계 없이 (10개 미만이라고 해도) 서버로 로그 보낸다고 해요!
이게 hashedID
가 섞이지 않게 하기 위한 건지는....... 게일이 잘 모른다고 했지만.........
저희도 그럼 비슷하게 가야하지 않을까요??
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 이슈로... 못하는 상황입니당
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.
고생하셨습니다~
let hashedId = ''; | ||
let localHashedId = ''; | ||
const existLocalHashedId = window.localStorage.getItem('yls-web'); | ||
const existLocalHashedId = window.sessionStorage.getItem('yls-web-hashedId'); | ||
|
||
if (userId === '') { |
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.
- if (userId === '') {
- if (existLocalHashedId) {
+ if (userId === '' && existLocalHashedId)
는 어떤가요?
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.
const existLocalHashedId = window.sessionStorage.getItem('yls-web-hashedId');
도 미리 선언 하지말고 반환 시점에 작성하면 좋을 것 같아요.
@@ -19,21 +19,21 @@ const createRandomId = () => { | |||
}; | |||
|
|||
const createHashedID = (userId: string) => { |
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.
함수명은 대문자 ID
, 변수명은 Id
로 돼있는데 수정하면 좋을 것 같습니다!
1️⃣ 어떤 작업을 했나요? (Summary)
기존 코드에 영향을 미치는 변경사항
yls-web
에서hashedId
를 가져오는데, hashedId라는 필드가 존재하지 않습니다.yls-web-hashed-id
라는 필드를 추가하고, 거기에 hasedId를 별도로 저장하도록 변경했습니다.버그 픽스
2️⃣ 알아두시면 좋아요!
3️⃣ 추후 작업
4️⃣ 체크리스트 (Checklist)
main
브랜치의 최신 코드를pull
받았나요?