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] 카테고리 ADMIN 역할을 포기하더라도 자신이 생성한 카테고리라면 구독 해제가 안되는 문제를 해결한다. #713

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

devHudi
Copy link
Collaborator

@devHudi devHudi commented Oct 5, 2022

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 💻 git rebase를 사용했나요?
  • 🌈 알록달록한가요?

작업 내용

  • 카테고리 ADMIN 역할을 포기하더라도 자신이 생성한 카테고리라면 구독 해제가 안되는 문제를 해결한다.
  • PERSONAL, GOOGLE 타입의 카테고리는 역할을 포기할 수 없도록 만든다.

스크린샷

주의사항

Closes #711

@devHudi devHudi added backend 백엔드 bug 버그 labels Oct 5, 2022
@devHudi devHudi self-assigned this Oct 5, 2022
@github-actions
Copy link

github-actions bot commented Oct 5, 2022

Test Results

  43 files    43 suites   21s ⏱️
315 tests 315 ✔️ 0 💤 0
342 runs  342 ✔️ 0 💤 0

Results for commit 907a3e5.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@hyeonic hyeonic left a comment

Choose a reason for hiding this comment

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

안녕하세여 후디

간단히 객체 내부로 옮길 수 있는 로직 찾아보았습니다. 찬찬히 살펴보시고 반영해주십쇼

Comment on lines 54 to 57
private void validateCategoryType(final CategoryRole categoryRole) {
CategoryType categoryType = categoryRole.getCategory().getCategoryType();
if (categoryType == PERSONAL || categoryType == GOOGLE) {
throw new NotAbleToChangeRoleException("개인 카테고리 또는 외부 카테고리에 대한 회원의 역할을 변경할 수 없습니다.");
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

category 내부로 로직을 이동하는건 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 그렇네요! 반영하겠습니다.

Comment on lines 59 to 60
if (!member.getId().equals(memberId)) {
throw new NoPermissionException("타인의 구독 정보에 접근할 수 없습니다.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거도 member 쪽으로 위임가능할 것 같네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

헉 멤버쪽에 hasSameId() 가 있었네요! 변경했습니다!

@hyeonic
Copy link
Collaborator

hyeonic commented Oct 5, 2022

추가적으로 현재 category와 category role 패키지가 강하게 의존하고 있어요!

image

관련해서 로직을 옮기던 패키지를 단뱡향으로 의존하는 방향에 대해서도 고려해보면 좋을 것 같네요 😃

Copy link
Collaborator

@gudonghee2000 gudonghee2000 left a comment

Choose a reason for hiding this comment

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

안녕하세요 매트! 작성해주신 기능 잘보았어요!!
로직도 깔끔하고 테스트코드도 꼼곰하게 작성해주셔서 변경사항이 보이지 않네요!

바로 approve 하였습니다!

Comment on lines -98 to -102
deleteSubscription(id, memberId, subscription);
deleteCategoryRole(memberId, subscription);
}

private void deleteSubscription(final Long id, final Long memberId, final Subscription subscription) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

불필요한 로직이 제거되었군요!👍👍

@@ -45,7 +45,7 @@ public class ControllerAdvice {
InvalidSubscriptionException.class,
ExistSubscriptionException.class,
ExistExternalCategoryException.class,
NotAbleToMangeRoleException.class
NotAbleToChangeRoleException.class
Copy link
Collaborator

Choose a reason for hiding this comment

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

더욱 명확한 네이밍이군요!👍👍

Comment on lines +146 to +162

@DisplayName("개인 카테고리에 대한 회원의 역할을 변경할 경우 예외가 발생한다.")
@Test
void 개인_카테고리에_대한_회원의_역할을_변경할_경우_예외가_발생한다() {
// given
Member 후디 = memberRepository.save(후디());
CategoryResponse 개인_카테고리 = categoryService.save(후디.getId(), 내_일정_생성_요청);

// when & then
CategoryRoleUpdateRequest request = new CategoryRoleUpdateRequest(NONE);
assertThatThrownBy(() -> categoryRoleService.updateRole(후디.getId(), 후디.getId(), 개인_카테고리.getId(), request))
.isInstanceOf(NotAbleToChangeRoleException.class);
}

@DisplayName("외부 카테고리에 대한 회원의 역할을 변경할 경우 예외가 발생한다.")
@Test
void 외부_카테고리에_대한_회원의_역할을_변경할_경우_예외가_발생한다() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

꼼꼼한 테스트 코드군요!!

Comment on lines -263 to -275
@DisplayName("자신이 만든 카테고리에 대한 구독을 삭제할 경우 예외를 던진다")
@Test
void 자신이_만든_카테고리에_대한_구독을_삭제할_경우_예외를_던진다() {
// given
Member 관리자 = memberRepository.save(관리자());
Category 공통_일정 = categoryRepository.save(공통_일정(관리자));
Subscription 공통_일정_구독 = subscriptionRepository.save(색상1_구독(관리자, 공통_일정));

// when & then
assertThatThrownBy(() -> subscriptionService.delete(공통_일정_구독.getId(), 관리자.getId()))
.isInstanceOf(NoPermissionException.class);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

불필요한 테스트코드가 삭제되었군요!

Copy link
Collaborator

@summerlunaa summerlunaa left a comment

Choose a reason for hiding this comment

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

확인했습니다! 앞에서 매트가 리뷰 잘 해주신 것 같네요 ㅎㅎ 굳굳

Copy link
Collaborator

@hyeonic hyeonic left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@devHudi devHudi force-pushed the feature/711-category-role-issue branch from c885814 to 907a3e5 Compare October 6, 2022 07:17
@devHudi devHudi merged commit 0d8bcd8 into develop Oct 6, 2022
@hyeonic hyeonic deleted the feature/711-category-role-issue branch October 11, 2022 05:42
@hyeonic hyeonic mentioned this pull request Oct 19, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend 백엔드 bug 버그
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants