-
Notifications
You must be signed in to change notification settings - Fork 1
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] 워크스페이스 설정 기능들 추가 #185
base: develop
Are you sure you want to change the base?
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.
수고하셨습니다! 큰 문제는 없는 거 같아 승인해놓겠습니다! 리뷰 확인해주세요. 리졸브는 제가 하겠습니다.
src/main/java/com/tiki/server/memberteammanager/controller/MemberTeamController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/tiki/server/memberteammanager/controller/MemberTeamController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/tiki/server/memberteammanager/entity/MemberTeamManager.java
Outdated
Show resolved
Hide resolved
...va/com/tiki/server/memberteammanager/controller/dto/request/UpdateTeamMemberNameRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/tiki/server/memberteammanager/controller/MemberTeamController.java
Show resolved
Hide resolved
src/main/java/com/tiki/server/team/controller/dto/request/UpdateTeamNameRequest.java
Outdated
Show resolved
Hide resolved
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.
수고하셨습니다! 리뷰 답변 후 머지해주시면 될 거 같아요.
} | ||
return memberTeamManager; | ||
MemberTeamManager accessMember = memberTeamManagerFinder.findByMemberIdAndTeamId(memberId, teamId); | ||
accessMember.checkMemberAccessible(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.
혹시 코드 수정 후 테스트해보셨나요??
public void checkMemberAccessible(Position accesiblePosition) {
if (this.position.getAuthorization() > accesiblePosition.getAuthorization()) {
throw new MemberTeamManagerException(INVALID_AUTHORIZATION);
}
}
제 생각엔 >라 안 돌아갈 거 같은데 혹시 돌아가나 궁금해 여쭤봅니다.
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.
public boolean isAdmin() {
if (this.position == ADMIN) return true;
return false;
}
이런 식으로 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.
기존에 다른곳에서 사용되던 코드입니다. POSITION의 Authorization부분이 숫자형으로 되어있어서 정상적으로 동작할 것 같습니다.
저도 아래 처럼 구현을 하려다가 나중에 혹시나 권한이 더 추가된다면 추가되는 만큼 메서드도 추가해줘야 해서 단순하게 접근 가능한지 판단하는 메서드를 사용했습니다.
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.
제가 작성한 코드라 기억이 나는데 정상적으로 동작할 것 같다는게 아니라 테스트했는지 여부가 궁금합니다!
제 생각엔 this.position이 ADMIN이더라도 authorization값이 파라미터로 들어오는 accesiblePosition의 authorization값과 동일해서 >로는 돌아가지 않을 거 같아서요.
final Principal principal, | ||
@PathVariable final long teamId, | ||
@RequestBody final UpdateTeamNameRequest request | ||
) { | ||
long memberId = Long.parseLong(principal.getName()); | ||
teamService.updateTeamName(memberId, teamId, request.newTeamName()); | ||
return ResponseEntity.ok().body(success(SUCCESS_UPDATE_TEAM_NAME.getMessage(), null)); | ||
return ResponseEntity.ok(success(SUCCESS_UPDATE_TEAM_NAME.getMessage())); |
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.
앱잼 코드에서는 아래처럼 응답을 보내줬는데요.
return ResponseEntity.ok().body(success(UPDATE_NOTE.getMessage()));
제가 이번 스프린트 때 작성한 코드도 지금 보니 body에 담아주지는 않았더라고요. 일단 이 부분도 클라가 혼란이 올 수 있으니 기존처럼 body에 담는 방식으로 하고 추후에 어떻게 하는게 좋을지 얘기해보면 좋을 거 같아요.
✨ Related Issue
📝 기능 구현 명세
아직 테스트를 안해서 일단 올려놓고 테스트 하고 수정하겠습니다
🐥 추가적인 언급 사항