Skip to content
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

Closed
wants to merge 2 commits into from
Closed

[Feat] 관리자 계정 조회 #80

wants to merge 2 commits into from

Conversation

kwonssshyeon
Copy link
Member

@kwonssshyeon kwonssshyeon commented Sep 4, 2024

🔎 작업 내용

  • 어드민, 매니저 계정 조회

확인 차 pr 올렸고 고칠꺼 확인되면 이번 pr은 닫고 다른기능들이랑 한번에 올리겠습니다.

To Reviewers 📢

  1. 매니저 계정이 많을 것 같지는 않아서 paging 처리 하지 않았습니다. 필요하다면 바꾸겠습니다
  2. AminRes 에서 List -> List 로 매핑했는데 이렇게 하는 거 괜찮은가요 ??
  3. response에 추가하거나 뺄께 있나요 ??
  4. AdminRes를 새로 만들었는데 UserRes에 응답을 정의하는게 맞을까요 ??
  5. 그 외 등등 ...

체크 리스트

  • 테스트를 작성했습니다.
  • 테스트를 통과했습니다.
  • API 변경사항이 존재합니다.
  • API 호출을 직접 실시하였고, 해당 데이터가 정상적으로 표시됩니다.
  • 기존 코드에 영향을 주는 작업 내용이 존재합니다.
  • 향후 추가적인 작업이 필요한 부분이 있습니다.

➕ 관련 이슈

Copy link

github-actions bot commented Sep 4, 2024

Unit Test Results

31 tests   27 ✔️  1s ⏱️
  8 suites    0 💤
  8 files      4

For more details on these failures, see this check.

Results for commit 940c563.

return admins.stream()
.map(UserModel.Admin::from)
.toList();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일부로 getManagerAndAdmin으로 role로 찾지않고 특정해서 만든거 좋아보이네염! (유저의 경우 페이징이 필요하고 매니저와 admin은 필요없다고 판단하셔서 따로 만드신 의도 맞나염?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 관리자 계정만 조회하는거라 레코드가 많지는 않을것 같아서 페이징 처리 안했어유

@momnpa333
Copy link
Collaborator

테스트 깨짐 관련해서 #82 에서 고쳐놨어염 해당 브랜치 머지하시면 테스트 통과될거에염!

Copy link
Collaborator

@momnpa333 momnpa333 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment on lines +27 to +31
public static List<ManagerAndAdmin> from(List<UserModel.Admin> managerAndAdmins) {
return managerAndAdmins.stream()
.map(ManagerAndAdmin::from)
.toList();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List로 변환하는 함수라면 네이밍을 from이 아니라
fromModelAndConvertList와 같은 의도가 있는 네이밍이 좋지 않을까요?!

Copy link
Member Author

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(
Copy link
Collaborator

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안보이게요!

백오피스에서 일반유저로 로그인시, 권한확인을 내정보확인에서 하면 깔끔할거같더라고요

Copy link
Member Author

@kwonssshyeon kwonssshyeon Sep 4, 2024

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이 필요하겠죠 ?

@kwonssshyeon
Copy link
Member Author

피드백 확인했습니당 ~
pr 닫고 다른거랑 한꺼번에 다시 올릴께유 1

@kwonssshyeon kwonssshyeon changed the title Issue/#79 [Feat] 관리자 계정 조회 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants