[Feat/#52] 팀, 팀 멤버에 로그인 기능 적용 #60

pyy2114 · 2024-10-03T14:43:47Z

📢 기능 설명

팀 생성하는 유저에 팀장(creator)기능 부여 후 팀 멤버로 저장
팀 기능에 로그인된 사용자 정보 적용
멤버 역할 별 권한 부여
삭제되었던 팀원 재 등록 시 is_deleted false로 변경(team_member테이블에 중복 저장 방지 용)
fix: 멤버 성별 카운트 시 팀아이디를 체크하지 않아 모든 팀 멤버의 성별을 카운트하는 기능 쿼리 수정

연결된 issue

close [feat]팀에 로그인 기능 적용 #52

✅ 체크리스트

PR 제목 규칙 잘 지켰는가?
추가/수정사항을 설명하였는가?
이슈넘버를 적었는가?

📣 To Reviewers

기존 구현되어있던 팀 기능에 로그인 된 유저 정보를 전달 받도록 기능 구현하였습니다.
그에 따라 멤버들의 role에 맞게 삭제나 수정 기능을 못하도록 권한 설정 기능도 추가하였습니다.

이 중에 is_deleted의 값을 변경하는 메서드를 BaseEntity에 추가하였는데
변경해도되는지에 대한 의문이 있어 의견 부탁드립니다!

기능이 좀 복잡해져서 코드가 난잡해진 것 같은데..불필요해 보이는 부분이나 잘못된 부분있으면 리뷰 부탁드립니다!
코드 리뷰 후 민우님이 수정하신 부분들 적용하여 push하겠습니다

NaMinhyeok

고생하셨습니다. 몇가지 리뷰랑 궁금한 사항 남겨드렸습니다~ 참고해보시면 좋을 것 같아요
쉬는날에도 고생많으셨습니다 !

NaMinhyeok · 2024-10-03T15:46:51Z

src/main/java/team4/footwithme/global/domain/BaseEntity.java

+    public void restore(){
+        this.isDeleted = IsDeleted.FALSE;
+    }


삭제된 컬럼을 다시 살려내는 메서드가 필요한가요 ?

오늘 의견 나눠보니까 굳이 살릴 필요가 없었네요;; 다시 수정하겠습니다!

NaMinhyeok · 2024-10-03T15:48:53Z

src/main/java/team4/footwithme/team/api/TeamApi.java

+    public ApiResponse<TeamDefaultResponse> createTeam(@RequestBody TeamCreateRequest request, @AuthenticationPrincipal PrincipalDetails principalDetails) {
+        return ApiResponse.created(teamService.createTeam(request.toServiceRequest(), principalDetails.getMember()));


여기는 내일 이야기 해보면 좋을 것 같아요 저는 email만 초반에 재경님이 email을 사용할 수 있게 해주신다 해서 email을 받도록 했는데, Member를 해도 별 문제가 없다면 Member자체를 가져오는 것도 좋은 방법일 것 같네요
굳이 email을 통해 검증을 또 할 필요는 없으니까요 ! 내일 이야기 해보면 좋을 것 같아요

NaMinhyeok · 2024-10-03T15:51:02Z

src/main/java/team4/footwithme/team/repository/CustomTeamMemberRepository.java

+import team4.footwithme.team.domain.TeamMember;
+
+public interface CustomTeamMemberRepository {
+    TeamMember findByTeamIdAndMemberId(Long teamId, Long memberId);


제가 슬랙에도 올렸던 내용이고 말씀드렸던 내용인데 Optional을 받는 것이 좋을 지 아니면 객체 그대로 받을지 고민해보는 것도 좋을 것 같아요 !
대부분은 Optional로 감싸서 하라고 하시더라구요 저도 그렇게 하기로했습니다~

저도 Optional로 받도록 수정하겠습니다!

NaMinhyeok · 2024-10-03T15:56:16Z

src/main/java/team4/footwithme/team/service/TeamMemberServiceImpl.java

+            TeamMember teamMember = teamMemberRepository.findByTeamIdAndMemberId(teamId, member.getMemberId());
+            //해당 멤버가 팀에 이미 존재 할 경우
+            if(teamMember != null){
+                //추가되었지만 삭제되었던 멤버
+                if(teamMember.getIsDeleted().equals(IsDeleted.TRUE)){
+                    teamMember.restoreTeamMember();
+                }else{ //이미 등록되어있는 멤버
+                    continue;
+                }
+            }else{
+                teamMember = TeamMember.create(team, member, TeamMemberRole.MEMBER);
+            }
+            addList.add(TeamResponse.of(teamMemberRepository.save(teamMember)));


분기문이 조금 복잡해 보여요 ! if 와 else를 반복적으로 사용하기 보다는 줄일 수 있다면 else를 사용하지 않는 것도 가독성을 높일 수 있어요 !

얼리리턴

한번 보시고 분기문을 줄일 수 있다거나, else를 줄일 수 있다면 줄여보면 좋을 것 같아요 !

저도 이 부분 구현하면 if문이 좀 많다고 생각했었습니다;; 좀 더 가독성있게 수정해보도록 하겠습니다!

NaMinhyeok · 2024-10-03T15:58:26Z

src/main/java/team4/footwithme/team/service/TeamMemberServiceImpl.java

@@ -32,21 +33,30 @@ public List<TeamResponse> addTeamMembers(Long teamId, TeamMemberServiceRequest r
        Team team = findTeamByIdOrThrowException(teamId);

        //return할 DTO
-        List<TeamResponse> teamMembers = new ArrayList<>();
+        List<TeamResponse> addList = new ArrayList<>();


이렇게 빈 배열을 만들어서 add 해주시는 이유가 있을까요 ?

멤버 추가 후 반환값으로 추가된 멤버들의 정보를 반환해주려고 하다보니 빈 배열을 만들었는데
추가된 멤버 정보를 반환해줘야 되나 싶긴 하네요..딱히 큰 의미가 있는 빈 배열은 아니였습니다!

대부분의 경우 Response로 잘 정제해서 stream사용해서 toList하다 보니까 뭔가 어색해보인건지모르겠는데 빈 배열이 선언되어서 response에 들어가는 경우를 잘 못봤어가지구...

로직이 잘못됐다는게 아니라 혹시 다른 의도가 있었나 했습니다 !

NaMinhyeok · 2024-10-03T16:20:37Z

src/test/java/team4/footwithme/team/service/TeamServiceImplTest.java

        TeamCreateRequest request = new TeamCreateRequest("팀명", "팀 설명", "선호지역");
+        Member leaderMember = memberRepository.findByEmail("teamLeader@gmail.com")
+                .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 입니다."));
        TeamDefaultServiceRequest dto = request.toServiceRequest();
-        teamService.createTeam(dto);
+        TeamDefaultResponse teamDefaultResponse = teamService.createTeam(dto,leaderMember);

        //when
        long count = teamRepository.count();
-
+        TeamMember teamLeader = teamMemberRepository.findByTeamIdAndMemberId(teamDefaultResponse.teamId(), leaderMember.getMemberId());
        //then
        assertThat(count).isEqualTo(1);
+        assertThat(teamLeader.getRole()).isEqualTo(TeamMemberRole.CREATOR);
    }


이 테스트는 뭐가 when인건가요 ?
then은 알겠는데... given에서 부터 throw를 하고 있고 when에서는 서비스테스트인데 레포지토리의 메서드를 when 주석 밑에 넣어주셔서 무슨 메서드를 테스트하려는지 헷갈립니다! createTeam 메서드를 테스트하고싶으신거였으면 when 밑에 createTeam을 넣어주시는게 좋을 것 같아요.

그리고 findByTeamId~ 와 같이 다른 메서드로 테스트를 하는건 좋지 않아요 어디서 문제가 발생했는지 알기 어렵기 때문에요.
차라리 findAll()이나 findById같은 것을 하는걸 추천드립니다

한가지 메서드가 한가지 역할만 하듯이 테스트도 마찬가지로 한가지 테스트는 한가지 메서드에대한 테스트만 진행해야합니다 !

제가 아직 test코드에 익숙하지 않아서 given, when, then 나누는 부분이 헷갈렸습니다
리뷰에 작성해주신 내용 검토하여 다시 테스트 코드 짜보겠습니다!!

NaMinhyeok · 2024-10-03T16:23:20Z

src/test/java/team4/footwithme/team/service/TeamServiceImplTest.java

+        TeamCreateRequest request = new TeamCreateRequest("팀명", "팀 설명", "선호지역");
+        Member leaderMember = memberRepository.findByEmail("teamLeader@gmail.com")
+                .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 입니다."));
+        TeamDefaultServiceRequest dto = request.toServiceRequest();
+        TeamDefaultResponse beforeEntity = teamService.createTeam(dto, leaderMember);
+        Team team = teamRepository.findByTeamId(beforeEntity.teamId());
+
+        Member member01 = memberRepository.findByEmail("member01@gmail.com")
+                .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 입니다."));
+        teamMemberRepository.save(TeamMember.create(team, member01, TeamMemberRole.MEMBER));
+
+        //when
+        //팀 정보 수정
+        TeamDefaultServiceRequest updateDTO = new TeamDefaultServiceRequest(null, "우리애 월드클래스 아닙니다.", "서울 전역");
+
+        //then -- 팀원이 정보 수정 시 예외
+        Throwable exception = assertThrows(IllegalArgumentException.class, () ->{
+            teamService.updateTeamInfo(team.getTeamId(), updateDTO, member01);
+        });
+    }


여기도 마찬가지로 무엇을 테스트하고자 하는지 정확하게 알기 어려운 것 같아요 ㅠㅠ
teamService.updateTeamInfo()를 테스트하고싶으셨다면 createTeam같은것은 사용하지 않는게 좋아요.
레포지토리의 save 또는 saveAll을 사용하는게 더 테스트의 목적에 적합하다고 생각합니다 !

NaMinhyeok · 2024-10-03T16:30:21Z

src/test/java/team4/footwithme/team/service/TeamServiceImplTest.java

    void deleteTeam(){
        //given
        //팀 정보 저장
        TeamCreateRequest request = new TeamCreateRequest("팀명", "팀 설명", "선호지역");
        TeamDefaultServiceRequest dto = request.toServiceRequest();
-        TeamDefaultResponse team = teamService.createTeam(dto);
+        Member leaderMember = memberRepository.findByEmail("teamLeader@gmail.com")
+                .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 사용자 입니다."));
+        TeamDefaultResponse team = teamService.createTeam(dto, leaderMember);

        //when
-        Long deletedTeamId = teamService.deleteTeam(team.teamId());
+        Long deletedTeamId = teamService.deleteTeam(team.teamId(), leaderMember);

        //then
        assertThat(deletedTeamId).isNotNull();


여기도 저희는 실제로 삭제하는게 아닌 softDelete를 하니까 실제 데이터는 살아있고 상태만 바뀌었다는걸 검증해주는게 더 좋을 것 같아요 !
deleteTeam에서 가장 검증해야 할 사항은 실제 데이터가 삭제상태로 변경되었는지 확인하는거니까요. 사실은 deleteTeam에서 Long값으로 반환되는 값을 검증해야 할 필요가 있을까요 ?
deleteTeam 메서드는 사실 void를 반환해도 우리가 deleteTeam이라는 메서드에서 주 목적으로 주어진 행동인 '팀의 상태를 삭제상태로 바꾼다' 라는 사실에는 변함이 없답니다. 테스트에서는 우리가 의도했던 행동이 제대로 동작하는지 확인하는게 좋을 것 같아요 !

delete는 뭘 검증해야될지 모르겠어서 저런식으로 테스트를 만들었었는데 민혁님 의견이 맞는 것 같습니다
'데이터는 살아있지만 상태는 삭제되었다' 이 방향으로 작성해보겠습니다!

NaMinhyeok · 2024-10-03T16:33:49Z

src/test/java/team4/footwithme/team/service/TeamServiceImplTest.java

+    @BeforeEach
+    void setUp() {
+        //팀장용 멤버 생성
+        memberRepository.save(
+                Member.create("teamLeader@gmail.com", "123456", "팀장", "010-1111-1111",
+                        LoginProvider.ORIGINAL,"test", Gender.MALE, MemberRole.USER, TermsAgreed.AGREE)
+        );
+        // 멤버 생성
+        memberRepository.save(
+                Member.create("member01@gmail.com", "123456", "남팀원01", "010-1111-1111",
+                        LoginProvider.ORIGINAL,"test", Gender.MALE, MemberRole.USER, TermsAgreed.AGREE)
+        );
+        memberRepository.save(
+                Member.create("member02@gmail.com", "123456", "남팀원02", "010-1111-1111",
+                        LoginProvider.ORIGINAL,"test", Gender.MALE, MemberRole.USER,TermsAgreed.AGREE)
+        );
+        memberRepository.save(
+                Member.create("member03@gmail.com", "123456", "여팀원01", "010-1111-1111",
+                        LoginProvider.ORIGINAL,"test", Gender.FEMALE, MemberRole.USER,TermsAgreed.AGREE)
+        );
+    }


이건 진짜 개인적인 의견인데 저는 데이터를 setup하는걸 안좋아합니다.... 불필요할 경우도 존재하고, 한 메서드안에서 뭐가 돌아갈지 예측이 잘 안되어서 테스트작성하는데 불편하더라구요 항상 머릿속에 4개의 Member가 들어가있고 여자2명 남자2명 역할은 다 유저고.. 1번이 팀장이고 나머진 팀원.. 이런걸 계속 생각하면서 테스트를 작성해야하다보니 저는 그냥 각 메서드에서 생성해주는걸 선호합니다 !

데이터 셋업해주는게 그래도 반복을 줄일 수 있으니 저는 그냥 이렇게 생각한다 정도로 넘어가주시면 될 것 같습니다 !

의견 감사합니다! 저는 같은 데이터를 테스트마다 추가하는게 너무 길어서 setup으로 뺀 경우이기는 합니다
다른 테스트 작성 시에 고려해보겠습니다!

NaMinhyeok · 2024-10-03T16:34:35Z

src/main/java/team4/footwithme/team/service/TeamServiceImpl.java

+    public boolean checkAuthority(Long teamId, TeamMember teamMember){
+        if(teamMember.getTeam().getTeamId() == teamId && teamMember.getRole() == TeamMemberRole.CREATOR) {
+            return true;
+        }
+        return false;


차라리 void로 여기서 예외를 던지는건 별로인가요 ?? 에러메시지명을 "접근권한이 없습니다" 정도로 통일한다고 생각하면요 !

에러메시지를 따로 나눠서 관리하기 위해서라면 boolean이어도 괜찮다고 생각합니다 !
대신 isAuthority같은 메서드명으로 변경해보는건 어떨까요? check는 void에서 많이 쓰인다고 합니다~ 물론 팀바팀 사바사라고 하구요 ㅎㅎ

void로 예외를 던지는 것도 좋을 것 같습니다! 에러메세지를 분리해야겠다만 생각을 했었어서 boolean으로 리턴을 했었는데 에러메세지를 통일하는 것도 좋은 방법인 것 같습니다!
감사합니다!

Repaion24

제가 궁금한 점이나 제안드릴만한 내용들을 민혁 님께서 다 해주셨네요 ㅎㅎ...
예외처리도 많이 하신 것 같고, 테스트 코드도 열심히 작성하고 계시네요!
스스로 난잡하다고 생각하시는 부분들은 나중에 리펙토링 해보시는 게 좋을 것 같아요!
고생하셨습니다!

HMWG

고생하셨습니다! 아침에 정했던 규칙이나 민혁님 피드백만 정리하셔도 좋을 것 같네요.

나중에 머지하실때 충돌나는 부분만 저랑 같이 수정하면 될 것 같습니다!

MoonJaeGyeong

깔끔하게 잘 짜여진 코드네요 고생하셨습니다!

pyy2114 added 4 commits October 2, 2024 16:13

feat: #52 팀에 로그인 유저정보 포함 및 기능별 권한 설정

189aade

fix: #52 팀 멤버 삭제 시 권한 체크 수정

1b90041

fix: #52 팀 멤버 성별 count 쿼리 수정

6389ae1

test: #52 팀 로그인 정보 및 기능별 권한 테스트

3971c29

pyy2114 linked an issue Oct 3, 2024 that may be closed by this pull request

[feat]팀에 로그인 기능 적용 #52

Closed

2 tasks

pyy2114 requested a review from a team October 3, 2024 14:44

pyy2114 self-assigned this Oct 3, 2024

pyy2114 added 🔥BugFix 버그 수정 ✨Feature 새로운 기능 추가 박윤영 labels Oct 3, 2024

NaMinhyeok approved these changes Oct 3, 2024

View reviewed changes

Repaion24 approved these changes Oct 4, 2024

View reviewed changes

HMWG approved these changes Oct 4, 2024

View reviewed changes

refactor: #52 PR 피드백 적용

636ba6d

MoonJaeGyeong approved these changes Oct 7, 2024

View reviewed changes

merge: #52 충돌 해결 커밋

348bf11

pyy2114 merged commit 68140ed into develop Oct 7, 2024

NaMinhyeok deleted the feat/#52-team_login branch October 10, 2024 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feat/#52] 팀, 팀 멤버에 로그인 기능 적용 #60

[Feat/#52] 팀, 팀 멤버에 로그인 기능 적용 #60

pyy2114 commented Oct 3, 2024 •

edited

Loading

NaMinhyeok left a comment

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 4, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

NaMinhyeok Oct 3, 2024

pyy2114 Oct 4, 2024

Repaion24 left a comment

HMWG left a comment

MoonJaeGyeong left a comment

		public ApiResponse<TeamDefaultResponse> createTeam(@RequestBody TeamCreateRequest request, @AuthenticationPrincipal PrincipalDetails principalDetails) {
		return ApiResponse.created(teamService.createTeam(request.toServiceRequest(), principalDetails.getMember()));

[Feat/#52] 팀, 팀 멤버에 로그인 기능 적용 #60

[Feat/#52] 팀, 팀 멤버에 로그인 기능 적용 #60

Conversation

pyy2114 commented Oct 3, 2024 • edited Loading

📢 기능 설명

연결된 issue

✅ 체크리스트

📣 To Reviewers

NaMinhyeok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Repaion24 left a comment

Choose a reason for hiding this comment

HMWG left a comment

Choose a reason for hiding this comment

MoonJaeGyeong left a comment

Choose a reason for hiding this comment

pyy2114 commented Oct 3, 2024 •

edited

Loading