Issue/#79 #88

kwonssshyeon · 2024-09-09T08:54:37Z

🔎 작업 내용

챌린지 인증 검토(pre_approve인거 approve로 마크변경)or(취소로 바꾸고 경험치 롤백)
챌린지 인증 조회 (챌린지로 그룹화)
- 챌린지로 그룹화 하려니까 페이징 단위도 애매해지고 그룹화하면 어쨋든 그룹으로 정렬이 필요한대 그런 요구사항이 필요없을것 같아서 그냥 최신순으로 반환했습니다. 대신, 챌린지제목으로 검색이 가능하도록 구현
챌린지 삭제, 수정, 생성 ( 수정기능 구현 / 삭제,생성은 고칠꺼 있는지 확인)
- 챌린지 그룹 수정시 챌린지 리스트를 수정할때 request에 없으면 삭제, id=-1로 하는 챌린지는 생성, 존재하는 id로 있는 경우 수정으로 구현했습니다.
- 챌린지 생성 command와 대부분의 필드가 겹치지만, id를 필수값으로 받기 위해 새로운 command를 정의(그리고 챌린지 그룹생성 command가 challenge group이 아니라 admin쪽에 있던데 이거 challenge group으로 옮기는거 어떻게 생각하시나요?)
매니저, 어드민 계정 조회
- 일반 사용자가 자신의 계정 조회할때 사용하는 Model dto 공유하도록 피드백 반영 완!

이미지 첨부

To Reviewers 📢

변경감지 코드가 많은데 헥사고날 아키텍쳐에서 이렇게 코드를 쓰는게 괜찮은지 여전히 의문...입니다
main브랜치가 아니라 admin브랜치로 올렸습니당

체크 리스트

테스트를 작성했습니다.
테스트를 통과했습니다.
API 변경사항이 존재합니다.
API 호출을 직접 실시하였고, 해당 데이터가 정상적으로 표시됩니다.
기존 코드에 영향을 주는 작업 내용이 존재합니다.
향후 추가적인 작업이 필요한 부분이 있습니다.

➕ 관련 이슈

close 어드민 api 구현 #79

bayy1216 · 2024-09-09T10:28:23Z

...c/main/java/org/haedal/zzansuni/challengegroup/domain/application/ChallengeGroupService.java

+    @Transactional
+    public void updateChallengeGroup(ChallengeGroupCommand.Update command) {
+        ChallengeGroup challengeGroup = challengeGroupReader.getById(command.getId());
+
+        List<Challenge> challenges = new ArrayList<>();
+        Set<Challenge> existedChallenges = new HashSet<>();
+
+        for (ChallengeGroupCommand.UpdateChallenge challenge : command.getUpdateChallenges()) {
+            processChallenge(challenge, challengeGroup, challenges, existedChallenges);
+        }
+
+        challengeGroup.update(command);
+        removeChallenges(challengeGroup, existedChallenges);
+        challengeGroup.addChallenges(challenges);
+    }
+
+    private void processChallenge(ChallengeGroupCommand.UpdateChallenge updateChallengeCommand,
+                                  ChallengeGroup challengeGroup,
+                                  List<Challenge> newChallenges,
+                                  Set<Challenge> existingChallenges)
+    {
+        if (updateChallengeCommand.getId().equals(-1L)) {
+            newChallenges.add(Challenge.create(updateChallengeCommand.convertCreate(), challengeGroup));
+        } else {
+            Challenge updateChallenge = challengeReader.getById(updateChallengeCommand.getId());
+            existingChallenges.add(updateChallenge);
+            updateChallenge.update(updateChallengeCommand);
+        }
+    }
+
+    private void removeChallenges(ChallengeGroup challengeGroup, Set<Challenge> existingChallenges) {
+        List<Challenge> removeChallenges = challengeGroup.getChallenges().stream()
+                .filter(challenge -> !existingChallenges.contains(challenge))
+                .toList();
+        challengeGroup.removeChallenges(removeChallenges);
+    }


이부분 서비스 레이어에 들어갈만한 내용이 아니라 생각이 드는데, challengeGroup에 oneToMany에서 처리하는게 어떤ㄱ요?
변경감지 쓰는게 좋을거같슴다..!

bayy1216 · 2024-09-09T10:30:28Z

zzansuni-api-server/app/src/main/java/org/haedal/zzansuni/user/domain/port/UserReader.java

@@ -16,4 +18,6 @@ public interface UserReader {
    Optional<User> findByAuthToken(String authToken);

    Page<User> getUserPagingByRanking(Pageable pageable);
+
+    List<User> getManagerAndAdmin();


infra에 로직을 두는거보다 그냥 인자로 받는건 별로일까요?
role을 결정하는거는 서비스레이어가 하는게 좋다라고 저는 생각이 드네요

다운님이랑 정반대의 리뷰네용
저도 이부분 좀 더 생각해보겠습니다

bayy1216 · 2024-09-09T10:38:29Z

.../java/org/haedal/zzansuni/userchallenge/domain/application/ChallengeVerificationService.java

+    public void confirm(Long verificationId, ChallengeVerificationStatus status) {
+        ChallengeVerification verification = challengeVerificationReader.getById(verificationId); // verification 가져올때 user, challenge, userChallenge도 join해서 가져오는게 좋을듯
+        if (status == ChallengeVerificationStatus.REJECTED && verification.getStatus() != ChallengeVerificationStatus.REJECTED) {
+            rejectVerification(verification);
+        } else if (status == ChallengeVerificationStatus.APPROVED && verification.getStatus() != ChallengeVerificationStatus.APPROVED) {
+            approveVerification(verification);
+        }
+    }


status 인자로 받지말고 메소드2개로 분할하는 방법은 별로일까요?
랭킹 경험치 롤백도 추가해주세용

엔드포인트를 나누자는 말씀인가요 ??
경험치 롤백은 구현되어있습니다

엔드포인트의 문제보다, 현재 Service에서 함수가 인자로 status를 받아서, if(status== REJECT)이면 reject행위, else status==xx이면 xx행위인데
confirm이라는 메소드명보다 rejectVerification, approveVerification으로 메소드를 분할하자는 의미였습니다 🧐

challenge_group_user_exp롤백이 빠져있어요

컨트롤러에서 함수를 나눠서 호출하게 되는건가요 ...?

api를 2개로 하거나 컨트롤러에서 if else해도 되는데 api2개가 좋겠네요!

혹시 이유가 뭘까요??
앤드포인트를 나누는 거랑 status 인자를 받는 것의 장단점을 잘 모르겠네용

클린코드에서 함수는 하나의 일만 해라인데(status에 대해 내부구현의 노출, if else의 조건분기)
억지클린코드라면 안해도 좋습니다

@Transactional public void confirm(Long verificationId, ChallengeVerificationStatus status) { ChallengeVerification verification = challengeVerificationReader.getById(verificationId); // verification 가져올때 user, challenge, userChallenge도 join해서 가져오는게 좋을듯 if (status == ChallengeVerificationStatus.REJECTED) { rejectVerification(verification); } if (status == ChallengeVerificationStatus.APPROVED) { approveVerification(verification); } }

저라면 이런식으로 가독성만 높일것 같아여! 조건문 뒤에 두번째 조건은 필요없을것 같고 처음에 읽을때 가독성이 오히려 떨어지는거 같더라구여!

두번째 조건은 중복 승인을 피하려고 추가한거에요 approve->approve로 변경을 시도하면 경험치가 두번 더해지니까 이거 피하려고 둔겁니다. !

길어서 가독성이 떨어지는 거면 verification.getStatue != status 이렇게 바꿀까요 ?

.../java/org/haedal/zzansuni/userchallenge/domain/application/ChallengeVerificationService.java

...rg/haedal/zzansuni/userchallenge/infrastructure/adapter/ChallengeVerificationReaderImpl.java

bayy1216 · 2024-09-09T10:53:06Z

챌린지 그룹 수정시 챌린지 리스트를 수정할때 request에 없으면 삭제, id=-1로 하는 챌린지는 생성, 존재하는 id로 있는 경우 수정으로 구현했습니다.

이거

    public static class Update extends SelfValidating<Update> {
        @NotNull(message = "id는 필수값입니다.")
        private final Long id;
        @NotBlank(message = "title은 필수값입니다.")
        private final String title;
        @NotBlank(message = "content는 필수값입니다.")
        private final String content;
        @NotBlank(message = "guide는 필수값입니다.")
        private final String guide;
        @NotNull(message = "category는 필수값입니다.")
        private final ChallengeCategory category;
        @NotNull(message = "joinStartDate는 필수값입니다.")
        private final LocalDate joinStartDate;
        @NotNull(message = "joinEndDate는 필수값입니다.")
        private final LocalDate joinEndDate;
        @NotNull(message = "challenges는 필수값입니다.")
        private final List<UpdateChallenge> updateChallenges;

        private final List<CreateChallenge> createChallenges;

이렇게 필드추가하는게 더 좋지 않을까요?>

kwonssshyeon · 2024-09-09T11:34:58Z

챌린지 그룹 수정시 챌린지 리스트를 수정할때 request에 없으면 삭제, id=-1로 하는 챌린지는 생성, 존재하는 id로 있는 경우 수정으로 구현했습니다.

이거

    public static class Update extends SelfValidating<Update> {
        @NotNull(message = "id는 필수값입니다.")
        private final Long id;
        @NotBlank(message = "title은 필수값입니다.")
        private final String title;
        @NotBlank(message = "content는 필수값입니다.")
        private final String content;
        @NotBlank(message = "guide는 필수값입니다.")
        private final String guide;
        @NotNull(message = "category는 필수값입니다.")
        private final ChallengeCategory category;
        @NotNull(message = "joinStartDate는 필수값입니다.")
        private final LocalDate joinStartDate;
        @NotNull(message = "joinEndDate는 필수값입니다.")
        private final LocalDate joinEndDate;
        @NotNull(message = "challenges는 필수값입니다.")
        private final List<UpdateChallenge> updateChallenges;

        private final List<CreateChallenge> createChallenges;

이렇게 필드추가하는게 더 좋지 않을까요?>

아 create는 따로 받자는거네요 확인했습니다!

… issue/#79

active period 필드 NPE

github-actions · 2024-09-10T08:31:43Z

Unit Test Results

31 tests 31 ✔️ 2s ⏱️
  9 suites   0 💤
  9 files   0 ❌

Results for commit e605148.

kwonssshyeon added 17 commits September 5, 2024 16:41

[Feat]: 매니저, 어드민 계정 조회 api 스펙 정의

37e899c

[Feat]: 매니저, 어드민 계정 조회 기능 구현

b1165ba

[Feat]: 에러 로그 출력 코드 추가

8908d3f

[Fix]: 챌린지 생성 request 수정

36f6827

[Feat]: 챌린지 수정 api spec 정의

5bdc67d

[Feat]: 챌린지 수정 command 클래스 추가

ff9b1ad

[Feat]: 챌린지 그룹, 챌린지 엔티티 업데이트 메서드 추가

cf460c8

[Feat]: 챌린지 수정 기능 구현

d3b521b

[Fix]: 챌린지 인증 model inner 클래스로 변경

cf89166

[Fix]: user challenge 생성시 active 날짜 필드 추가

7334b9e

[Feat]: 챌린지 인증 목록 조회 api 스펙 정의

0f83ff5

[Feat]: 관리자의 챌린지 인증 model 작성

e37769c

[Feat]: 챌린지 인증 조회 쿼리 작성 및 기능 구현

5e90a9e

[Fix]: 챌린지 인증 service 클래스명 오타 수정

394d23f

[Feat]: 챌린지 인증 검토 api 스펙 정의

765d4fe

[Feat]: 챌린지 인증 상태 변경, 경험치 업데이트 메소드 작성

a065dfc

[Feat]: 챌린지 인증 검토 기능 구현

df2f92a

kwonssshyeon requested review from bayy1216 and momnpa333 September 9, 2024 08:54

bayy1216 reviewed Sep 9, 2024

View reviewed changes

kwonssshyeon added 2 commits September 9, 2024 20:16

[Fix]: UserChallenge 최근 성공일자 조회 함수 -> getRecentDate로 이름 변경

3a9985e

[Fix]: (챌린지 인증 조회) 제목 검색 함수명 eq -> contains 수정

58f3545

[Fix]: 챌린지 수정 request에서 create챌린지와 update챌린지 분리 + 그에 따른 로직 수정

2d7599a

kwonssshyeon changed the base branch from admin to main September 9, 2024 15:45

kwonssshyeon added 4 commits September 10, 2024 17:08

[Fix]: 챌린지 그룹의 챌린지 가져오는 로직 변경

747d292

Merge branch 'main' of https://github.com/KNU-HAEDAL/ZZANSUNI_BE into…

b5bad9e

… issue/#79

[Fix]: 경험치 롤백(User, ChallengeGroupUserExp) use case 이용

4c571de

[Fix]: 오류나는 테스트 주석 처리

e605148

active period 필드 NPE

momnpa333 approved these changes Sep 10, 2024

View reviewed changes

kwonssshyeon merged commit 6aae7bf into main Sep 12, 2024
3 checks passed

kwonssshyeon deleted the issue/#79 branch September 12, 2024 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/#79 #88

Issue/#79 #88

kwonssshyeon commented Sep 9, 2024 •

edited

Loading

bayy1216 Sep 9, 2024

bayy1216 Sep 9, 2024

kwonssshyeon Sep 9, 2024

bayy1216 Sep 9, 2024

kwonssshyeon Sep 9, 2024

bayy1216 Sep 9, 2024

kwonssshyeon Sep 9, 2024

bayy1216 Sep 9, 2024

kwonssshyeon Sep 10, 2024

bayy1216 Sep 10, 2024

momnpa333 Sep 10, 2024 •

edited

Loading

kwonssshyeon Sep 10, 2024

kwonssshyeon Sep 10, 2024

bayy1216 commented Sep 9, 2024

kwonssshyeon commented Sep 9, 2024

github-actions bot commented Sep 10, 2024

Issue/#79 #88

Issue/#79 #88

Conversation

kwonssshyeon commented Sep 9, 2024 • edited Loading

🔎 작업 내용

이미지 첨부

To Reviewers 📢

체크 리스트

➕ 관련 이슈

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

momnpa333 Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bayy1216 commented Sep 9, 2024

kwonssshyeon commented Sep 9, 2024

github-actions bot commented Sep 10, 2024

Unit Test Results

kwonssshyeon commented Sep 9, 2024 •

edited

Loading

momnpa333 Sep 10, 2024 •

edited

Loading