Refactor: UNVALIDATED 에서 GUEST 로 권한을 변경하고 가입시 기존 권한을 CREW 에서 GUEST 로 변경한다. #1564
Conversation
refactor: js ts 마이그레이션 main에 반영
| @Given("{string}(이)(가) 로그인을 하고") | ||
| public void 멤버가로그인을하고(String member) { | ||
| @Given("{string}(이)(가) 크루역할로 로그인을 하고") | ||
| public void 멤버가크루역할로로그인을하고(String member) { |
There was a problem hiding this comment.
원래 띄워쓰기에 _(언더스코어) 사용하지 않나용?
There was a problem hiding this comment.
새로 작성된 인수테스트를 제외한 기존 인수테스트의 조건에 맵핑될 메서드명을 다 해당 방식으로 작성했더라구요 컨벤션이였던 것으로 파악되서 통일되게 작성했습니다!! ( 저도 언더 스코어가 좋아요ㅎㅎ )
| return articleRepository.findById(id) | ||
| .orElseThrow(() -> new BadRequestException(ARTICLE_NOT_FOUND_EXCEPTION)); |
There was a problem hiding this comment.
저는 보통 Repository에서 Optional로 뽑았을 때 null에 대한 처리를 Repository로 이관하는 편인데, 스플릿은 어떻게 생각하시나요?(이미 프롤로그에서는 서비스에서 담당하고 있긴 하지만 궁금해서 여쭤봅니다 ㅋㅋㅋ)
default PrimaryIngredient getById(Long id) {
return findById(id)
.orElseThrow(() -> new BadRequestException(ARTICLE_NOT_FOUND_EXCEPTION));
}There was a problem hiding this comment.
저는 비즈니스 상황때문에 생기는 검증 처리는 Service 레어이가 하는 것이 맞다고 생각하는 편입니다!!😊
사실상 Repository 가 id 를 통해 무엇인가를 찾을때 해당 값이 무조건 있어야 한다는 비즈니스 로직상의 상황을 알고 있을 필요는 없다고 생각해요ㅎㅎ!! 😁
| if (member.isGuest()) { | ||
| throw new BadRequestException(MEMBER_NOT_ALLOWED); | ||
| } |
There was a problem hiding this comment.
어떤 검증에서는 도메인이 예외까지 던지고, 어떤 검증에서는 도메인한테 메시지만 전달받아서 서비스 계층에서 예외를 던지는데 이 둘이 다른 상황은 뭐가있을까여?? 아니면 단순 실수일까요??
There was a problem hiding this comment.
제가 코드를 파악할때는 모든 예외는 서비스에서 던지는 것으로 보이는데 혹시 현재 코드에서 언제 도메인 예외를 던지는 상황인지 설명 한번 부탁드립니다!!🙇
추가적으로 도메인이 예외를 던지는 상황과 도메인한테 메시지를 전달받아서 서비스 계층에서 던지는 상황이 공존 하는 것에 대한 의견을 물어보시는 건지도 궁금합니다!!🧐
( 제가 봤을때 파악되는 도메인이 예외를 던지는 상황이 ArticleRequest 의 toArticle 의 경우만 보이는데 혹시 이 부분을 말하시는 거라면 Reques 를 실제 아티클로 변환하는 과정에서 값들의 검증의 책임을 Article 으로 분리하기 위해서 사용했습니다!! )
There was a problem hiding this comment.
이건 아티클 엔티티인데요! 여기서는 분명 객체가 예외를 던지는 것 까지 담당하고 있어요. 그래서 예외 처리를 어떤 방식으로 하는게 맞는건지 헷갈려서 물어봤습니다 😭
There was a problem hiding this comment.
베베의 의견 완전히 파악됐습니다!! Member 에게 Owner 인지만 메시지로 전달받아서 Service 에서 예외를 던지는 방식을 말하시는 거군요!!😊
이 부분에 확실히 일관성이 없었네요😭
베베의 의견대로 일관성을 맞추는게 더 좋아보입니다!! 기존에 스쿼드에서 작성하고 사용하고 있던 코드라 스쿼드 팀원들과 함께 말해보도록 할께요!!👍
| public boolean isGuest() { | ||
| return this.role == GUEST; | ||
| } | ||
|
|
||
| public boolean isCrew() { | ||
| return this.role == CREW; | ||
| } |
There was a problem hiding this comment.
Role 에게 해당 부분의 책임을 주는 것에서는 ( Role 에게 전달하는 Role 과 같은 지 물어본다 )
public boolean isGuest() {
return this.role.equals(GUEST);
}
public boolean isCrew() {
return this.role.equals(CREW);
}equals 메서드도 Role 객체에서 제공하는 메서드니 이런 방식으로 바꿀 수 있을 것 같아요!!
근데 추가로 Role 에 importance 를 부여한 상황에서
public boolean hasLowerImportanceRoleThan(final Role role) {
return this.role.hasLowerImportanceThan(role);
}위에 메서드를 이용하면 유지보수하기 좋을 것 같아서 해당 메서드로 변경했습니다!!
|
Kudos, SonarCloud Quality Gate passed!
|
#️⃣연관된 이슈
📝작업 내용
기존에 존재하던 Role 의
UNVALIDATED를GUEST로 변경한다.GIthubLogin 시 부여되는 권한을
CREW에서GUEST로 변경한다.Role에 권한에 중요도 (importance) 를 부여한다.회원의 권한을 변경하는 기능을 구현한다.
AccessToken,PATCH /member/{member_id}/rolemanager라고 칭하겠습니다. )manager.role을application.yml속성에 추가한다.Role.COACHRole.CREWCREW 권한이 필요한 아티클 기능에 검증로직을 추가한다.
추가적으로 바뀐 로직으로 통해 test 모두 수정
QA 결과