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

[bug] 회원 탈퇴가 실패하는 버그를 해결한다. #771

Merged
merged 2 commits into from
Oct 13, 2022

Conversation

gudonghee2000
Copy link
Collaborator

@gudonghee2000 gudonghee2000 commented Oct 12, 2022

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

작업 내용

현재 회원 탈퇴를 할 떄, 예상되는 상황을 테스트 코드로 구현해보았습니다.

회원 탈퇴 예상 상황

1. 회원 가입만 한 경우 => 내 일정(PERSONAL) 카테고리만 존재

2. 회원 가입 후, 다른사람의 카테고리만 구독한 경우 => 내 일정(PERSONAL) 및 다른 사람의 카테고리에 대한 구독과 NONE 권한을 가지고 있음

3. 회원 가입 후, 내 카테고리를 생성한 경우 => 내 일정 및 내 카테고리에 대한 ADMIN 권한을 가지고 있습니다.

4. 회원 가입 후, 내 카테고리를 생성 했을 때 다른 사람이 내 카테고리의 ADMIN이 된 경우 => 내 일정 및 내 카테고리에 대한 ADMIN 권한을 가지고 있습니다.

5. 회원 가입 후, 내 카테고리를 생성 한 다음 카테고리 권한을 다른 사용자에게 양도한 경우 => 내 일정 및 내 카테고리에 대한 NONE 권한을 가지고 있습니다.

6. 회원 가입 후, 내 카테고리를 생성 한 다음 카테고리 권한을 다른 사용자에게 양도하고 구독 취소한 경우 => 내 일정을 가지고 있고 내가만든 카테고리에는 어떠한 권한도 없습니다.

7. 회원 가입 후, 외부 연동 카테고리를 생성한 경우 => 내 일정외부 카테고리(GOOGLE)을 가지고 있습니다.


회원 탈퇴 시 발생 할 수 있는 예외 상황

카테고리 type외부 연동 카테고리(GOOGLE) 또는 개인 카테고리(PERSONAL)이 아닐 때,
내가 카테고리의 유일한 ADMIN인 경우 예외 발생

회원 탈퇴 로직에서 발견한 문제

논의를 통해 CategoryRole을 기반으로 Category를 제거하도록 로직을 수정하던 중, 문제점이 있어 공유해봅니다.

  1. CategoryRole을 기반으로 Category를 삭제한 뒤, MemberRepository에서 Member를 삭제하려고 할 때, 현재 Category의 크리에이터인 member_id외래키 제약조건에 위반되어 정상적인 회원 탈퇴가 불가능한 경우가 있었습니다. => CategoryRole을 통해 회원의 카테고리를 삭제해도 회원이 CategoryRole을 포기한 경우, CategoryRole`을 포기한 카테고리는 삭제되지 않음.

예상 해결방법: 논의를 통해 나온 내용과 같이, Category에서 Member 에 대한 데이터를 제거해야 1번 상황의 불필요한 로직을 제거 할 수 있을것 같습니다.
=> 현재는 Category의 권한을 회원 탈퇴시 다른 ADMIN을 가진 회원에게 양도하도록 구현하였습니다.

  1. 회원 탈퇴 로직이 대략 100줄이 나오고 depth 3~4까지 로직이 구현되어 있습니다. QA를 통해 회원 탈퇴 로직이 정상 작동하는게 확실해지면 로직에 리팩터링이 필요할것 같습니다.

예상 해결방법:

  • 회원 탈퇴시 외래키 제약조건을 제거한다.
  • 회원 탈퇴시 데이터 제거의 일부분을 soft delete로 변경한다.

Closes #751

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

github-actions bot commented Oct 12, 2022

Test Results

  46 files    46 suites   15s ⏱️
338 tests 338 ✔️ 0 💤 0
365 runs  365 ✔️ 0 💤 0

Results for commit 47fac46.

♻️ This comment has been updated with latest results.

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.

후 탈퇴 증맬 쉽지 않네요???? 리버 너무 고생하고 있군여. 이전 PR들 리뷰를 못해서 늦었지만 전부 보고 왔습니다. 대신 이번에 꼼꼼히 리뷰 남겼으니 한 번 읽어봐주세여 ㅎㅎ 메서드 분리도 부탁드립니덩.

그리고 개인적으로는 멤버는 soft delete 해야할 것 같아요. 그냥 지우기엔 중요한 정보이기도 하고 google 관련 예외도 없애려면 soft delete하는 게 좋지 않을까요 🥲 다른 분들 의견도 궁금해요

.stream()
.filter(CategoryRole::isAdmin)
.collect(Collectors.toList());

for (CategoryRole categoryRole : categoryRoles) {
for (CategoryRole categoryRole : adminCategoryRoles) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

        adminCategoryRoles.stream()
                .map(CategoryRole::getCategory)
                .filter(Category::isNormal)
                .forEach(category -> validateSoleAdmin(id, category));

        private void validateSoleAdmin(final Long id, final Category category) {
            if (categoryRoleRepository.isMemberSoleAdminInCategory(id, category.getId())) {
                throw new InvalidMemberException("구독한 카테고리 중 유일한 편집자로 남아 있는 카테고리가 있습니다."
                    + "카테고리를 삭제하거나 편집자 권한을 위임해주세요.");
            }
        }

c: 어떤가요?

Comment on lines -102 to 105
.filter(CategoryRole::isAdmin)
List<Long> adminCategoryIds = adminCategoryRoles.stream()
.map(CategoryRole::getCategory)
.map(Category::getId)
.collect(Collectors.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<Category> adminCategories = categoryRoles.stream()
                .map(CategoryRole::getCategory)
                .collect(Collectors.toList());

r: 여기서 category를 바로 받아 사용하면 바로 아래 for 문에서 다시 categoryRepository.getById(adminCategoryId);를 호출하지 않아도 되겠네요.

categoryRoleRepository.deleteByMemberIdAndCategoryId(id, category.getId());
}
}

List<Long> noneCategoryIds = categoryRoles.stream()
List<CategoryRole> noneCategoryRoles = categoryRoleRepository.findByMemberId(id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 이미 위에서 findByMemberId를 했는데 또 해야 할까요?

.filter(CategoryRole::isNone)
.collect(Collectors.toList());

List<Long> noneCategoryIds = noneCategoryRoles.stream()
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 여기도 getId 하지 않고 Category로 받으면 아래에서 다시 Category를 조회하지 않아도 되겠네요.

Comment on lines +167 to +177
List<Category> creatorCategories = categoryRepository.findByMemberId(id)
.stream()
.filter(category -> category.isCreatorId(id))
.collect(Collectors.toList());

for (Category creatorCategory : creatorCategories) {
List<CategoryRole> categoryRoles = categoryRoleRepository.findByCategoryIdAndCategoryRoleType(
creatorCategory.getId(), ADMIN);
Member member = memberRepository.getById(categoryRoles.get(0).getMember().getId());
creatorCategory.setMember(member);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 이 로직을 바로 위의 role type이 none인 category들을 다루는 로직 위로 가면, 중복되는 156-159 라인을 지울 수 있을 것 같아요. 어차피 삭제될 로직이긴 하지만 ^^;;

.isInstanceOf(NoSuchMemberException.class);
}

@DisplayName("회원 탈퇴를 할 뗴, 구독한 카테고리의 유일한 ADMIN 계쩡이면 예외를 던진다.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

r: 라고 적힌 부분이 세 군데나 있네요! 계쩡은 무엇이죠 ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ

.isInstanceOf(NoSuchMemberException.class);
}

@DisplayName("회원을 삭제할때, 관련된 데이터도 모두 삭제한다.")
@DisplayName("회원 탈퇴를 할 때, 다른 회원이 만든 카테고리를 구독만 하면 정상적으로 탈퇴한다.")
Copy link
Collaborator

@summerlunaa summerlunaa Oct 13, 2022

Choose a reason for hiding this comment

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

r: MemberServiceTest의 전체적인 테스트 메서드 이름을 이런 형식으로 바꿔보면 어떨까요?

Suggested change
@DisplayName("회원 탈퇴를 할 때, 다른 회원이 만든 카테고리를 구독만 하면 정상적으로 탈퇴한다.")
@DisplayName("다른 회원이 만든 카테고리를 구독만 한 경우 탈퇴할 수 있다.")
    @DisplayName("구독한 카테고리의 유일한 최고 관리자가 아닌 경우 탈퇴할 수 있다.")
    @DisplayName("구독한 카테고리의 최고 관리자가 더 있고 권한이 없는 구독 카테고리가 있는 경우 탈퇴할 수 있다.")

이런 식으로요! ~할 때를 빼도 좋을 것 같아요. 그리고 enum 이름을 그대로 쓰는 경우엔 enum 이름이 바뀌면 테스트 네임까지 다 바뀌어야 할 것 같아서 한글로 풀어 써주는 게 좋을 것 같아요 ;_; ADMIN != 관리자라고 해서 최고 관리자라는 이름을 붙여 보았습니다..

@백엔드 전부 추가적으로 member를 '유저', '회원', '멤버' 등으로 부르고 있네요 ㅠ_ㅠ 용어 통일이 필요해보입니다!!!

@summerlunaa
Copy link
Collaborator

r: 아 추가적으로 ScheduleService에 사용하지 않는 import가 있네요! 제거 부탁드립니다.

@gudonghee2000 gudonghee2000 merged commit 0a77fae into develop Oct 13, 2022
@hyeonic hyeonic mentioned this pull request Oct 19, 2022
8 tasks
@dayelop dayelop deleted the feature/751-member-withdrawal branch October 19, 2022 09:18
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.

2 participants