Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[BE] feat: 스태프 권한 인증 기능 구현 (#478) #480
base: dev
Are you sure you want to change the base?
[BE] feat: 스태프 권한 인증 기능 구현 (#478) #480
Changes from 18 commits
cbad3bf
fe45912
803bd40
9dc30a8
1f8c53e
4a3d57c
be1d5f5
182dfd9
6dd58bc
d013bd6
5860245
060abff
54fd7c7
b7f7431
dd4f1aa
1bd85fa
3edd22d
bb03de0
3fb94d4
84d7206
01fb56a
6fba6ab
4d83bbc
f4d7a16
b85aa9e
7e8a268
c3c1d25
a79cec0
d850d2c
c6c6e8c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
또한 entryService의 validate 메서드 또한, 파라미터 명이 잘못된 것 같네요!
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.
코드로 인증할 때, jwt 토큰에 StaffCode 정보를 담지 않고 Festival 정보를 담아서 festivalId를 의도한게 맞긴합니다..!!
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.
Staff정보를 담는게 더 좋을까요 ?!
그렇게 하면 Staff -> Festival 한 단계 타고 가야해서 이렇게 설계했었어요!
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.
지금보니 Staff 도메인의 Staff 엔티티가
StaffCode
로 되어 있군요 😂Staff
로 이름을 바꾸는게 더 명확해보이네요.지금 같은 방법을 사용하면 티켓을 검증할 때 바로 festivalId로
MemberTicket
엔티티에서belongsFestival
메서드를 호출하여 추가적인 쿼리문 없이 티켓을 검증할 수 있겠네요!하지만 코드 상에서 흐름을 읽기가 더 어려워졌지 않나 싶습니다.. (컨트롤러에서
@Staff
로 받아온 Long 타입은 staff의 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.
7e8a268 staffId를 넘기는 방식으로 반영완료했습니다!
This file was deleted.
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.
RandomStaffCodeProvider의 패키지 위치
RandomStaffCodeProvider 클래스를 infrastructure가 아닌 application 패키지에 위치시켰습니다.
infrastructure 패키지는 “외부 종속성 분리”를 위한 패키지인데, RandomStaffCodePRovider에는 외부 종속성이 전혀 없고 비즈니스 로직을 담았기 때문에 application 계층이 적합하다 판단했습니다.
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.
저도 애쉬 생각에 동의해요. Random 값을 뿌려주기에 interface를 구현 하는게 좋을 것 같고, 외부 의존성 없기 때문에 같은 패키지로 가도 문제없다고 생각해요.
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.
비즈니스 로직을 담고 있다면
domain
계층이 더 올바르다고 생각하는데 이 의견은 어떠신가요?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.
덤으로
student
패키지의RandomVerificationCodeProvider
의 위치 또한 고민해봤으면 좋겠네요