-
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
[Feat] 관리자 계정 조회 #80
[Feat] 관리자 계정 조회 #80
Conversation
return admins.stream() | ||
.map(UserModel.Admin::from) | ||
.toList(); | ||
} |
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.
일부로 getManagerAndAdmin으로 role로 찾지않고 특정해서 만든거 좋아보이네염! (유저의 경우 페이징이 필요하고 매니저와 admin은 필요없다고 판단하셔서 따로 만드신 의도 맞나염?)
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.
넵 관리자 계정만 조회하는거라 레코드가 많지는 않을것 같아서 페이징 처리 안했어유
테스트 깨짐 관련해서 #82 에서 고쳐놨어염 해당 브랜치 머지하시면 테스트 통과될거에염! |
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.
lgtm
public static List<ManagerAndAdmin> from(List<UserModel.Admin> managerAndAdmins) { | ||
return managerAndAdmins.stream() | ||
.map(ManagerAndAdmin::from) | ||
.toList(); | ||
} |
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.
List로 변환하는 함수라면 네이밍을 from이 아니라
fromModelAndConvertList와 같은 의도가 있는 네이밍이 좋지 않을까요?!
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.
lgtm
@@ -71,5 +73,23 @@ public String toString() { | |||
} | |||
} | |||
|
|||
@Builder | |||
public record Admin( |
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.
유저모델에 Role 추가하고, API response에서 바꾸는거는 어떻게생각하세요??
내정보조회에서만 role보이게하고, 다른 일반 api(랭킹조회 등등)에서는 role안보이게요!
백오피스에서 일반유저로 로그인시, 권한확인을 내정보확인에서 하면 깔끔할거같더라고요
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.
아 UserModel.Main -> response로 매핑할때 불필요한 정보를 빼거나 하자는 거죠 ?
좋습니다. 그러고보니까 지금 UserModel.Main이랑 UserModelAdmin이랑 필드가 완전히 겹치네요
이건 백오피스에서 관리자 계정 조회하기 위한 response인데 이때는 role이 필요하겠죠 ?
피드백 확인했습니당 ~ |
🔎 작업 내용
확인 차 pr 올렸고 고칠꺼 확인되면 이번 pr은 닫고 다른기능들이랑 한번에 올리겠습니다.
To Reviewers 📢
체크 리스트
➕ 관련 이슈