feat: 관리자 페이지 학점 인증 관리 api 추가

[Gyuhyeok99]

관련 이슈

• resolves: feat: 관리자 페이지 학점 인증 관리 api 추가 #194

작업 내용

관리자 페이지 학점 인증 관리 api 추가하였습니다.

한 번에 학점과 어학 인증 관리 모두 추가하려했는데 pr이 너무 커지는 거 같아서 나눠서 작업하겠습니다..
(추가로 정확한 명세서가 없어서 잘못된 부분 피드백 받고 어학 인증 관련해서 구현하는 게 더 나을 거 같았습니다🥲)

• 관리자 전용 gpaScore 페이징 조회 api 추가
• 관리자 전용 gpaScore 검증 api 추가
• 관리자 전용 gpa 수정 api 추가

특이사항

처음엔 학점/어학을 한 api로 추상화해서 구현해보려했는데 너무 로직이 복잡해지는 거 같아 그냥 나눠서 하는 거로 구현했습니다..

리뷰 요구사항 (선택)

1. 정확하게 Request와 Response로 필요한 값을 몰라서 잘못된 api 검토 부탁드립니다.. 😅 @wibaek

2. 최대한 컨벤션 지켜가며 하려고 기존에 구현되어있는 controller 및 service 코드를 따라가며 구현했습니다.
   그런데 엔티티에서 디티오 변환하는 함수이름이 of, from, toEntity 등 다양하게 있어서 통일하면 좋을 거 같다고 생각이 들었습니다!
   정해지면 제가 따로 pr파서 하겠습니다! 영서님이 주로 of로 한 거 같아서 저도 of로 했습니다!

3. Page의 pageNumber를 1번 시작으로 맞추기 위해

Pageable sortedPageable = PageRequest.of(
                pageable.getPageNumber() - 1,
                pageable.getPageSize()
        );

public static <T> PageResponse<T> of(Page<T> page) {
        return new PageResponse<>(
                page.getContent(),
                page.getNumber() + 1,
                page.getSize(),
                page.getTotalElements(),
                page.getTotalPages()
        );
    }

이런식으로 -1 , +1을 했는데 괜찮을까요? 추가로 Page 를 그대로 감싸서 반환하면 불필요한 응답값이 너무 늘어나는 거 같아서 PageResponse를 따로 정의했습니다.

4. BaseIntegrationTest에서 기본으로 생성되는 유저와 gpaScore 때문에 전체 조회 테스트가 어려워졌습니다. 이 경우 BaseIntegrationTest에서 siteUser나 gpaScore 등을 제거해야할지 고민되네요.. 통합테스트의 단점을 처음 느꼈습니다.🥲

❗️api 추가 사항❗️
https://github.com/solid-connection/api-docs/pull/7

[wibaek]

feat: 페이징 응답 DTO 추가
0111dbf

chore: 잘못 추가한 라인 삭제
98ac050

간단한 오타 수정이 필요한 커밋이 있을 때, 오래전의 커밋을을 수정하는 것이 아닌 바로 직전의 커밋을 수정하는 일이라면

git commit --amend나 git reset --mixed를 사용해보는 것도 좋을 것 같습니다.😊

[wibaek]

Page의 pageNumber를 1번 시작으로 맞추기 위해

Pageable sortedPageable = PageRequest.of(
pageable.getPageNumber() - 1,
pageable.getPageSize()
);

public static PageResponse of(Page page) {
return new PageResponse<>(
page.getContent(),
page.getNumber() + 1,
page.getSize(),
page.getTotalElements(),
page.getTotalPages()
);
}

이런식으로 -1 , +1을 했는데 괜찮을까요? 추가로 Page 를 그대로 감싸서 반환하면 불필요한 응답값이 너무 늘어나는 거 같아서 PageResponse를 따로 정의했습니다.

반환전 +1은 깔끔해보이는데 입력값에서 -1 해주는게 깔끔하게 구현하기 어려운 것 같습니다. setOneIndexedParameters(true) 를 사용하는 것도 생각해볼 수 있겠지만 출력값 보정 없이 입력값만 보정해준다는게 혼란을 가져올 수 있어 지양하는게 좋을 것 같다는 생각이 드네요.

제 생각은

page를 +1 하는 로직인 return ResponseEntity.ok(PageResponse.of(page)); 는 컨트롤러 단에 존재하는데, -1 하는 로직은 서비스에서 제공하고 있어 다소 어색함이 느껴지네요. -1 처리도 컨트롤러에서 해주는 것은 어떻게 생각하시나요?

수동으로 -1 처리를 해주되, 이를 처리하는 부분을 컨트롤러로 옮기는게 좋을 것 같습니다!

[wibaek]

스프링 기본 pagination 인터페이스를 잘 이용해서 제작하신 것 같습니다👍

다른 부분은 옳다고 생각되시는 방법대로 적용하시고, 리팩토링 필요하면 나중에 별개 PR로 진행하셔도 좋을 것 같으나,

즉 요약하자면 /gpas/{gpa_score_id} 와 /gpas/{gpa_score_id}/verify 를 하나의 API로 통합하는 것이 어떨까요?

이 부분은 가능하면 긍정적으로 검토해서 반영해주시면 좋을 것 같습니다🙏

[Gyuhyeok99]

간단한 오타 수정이 필요한 커밋이 있을 때, 오래전의 커밋을을 수정하는 것이 아닌 바로 직전의 커밋을 수정하는 일이라면

git commit --amend나 git reset --mixed를 사용해보는 것도 좋을 것 같습니다.😊

넵, 그게 좋은 거 같네요 😅 제가 깃허브 명령어가 능숙한 편은 아니라 가장 최근 커밋 실수했을 때만 git reset --soft HEAD~1를 사용했는데 새로알았네요

[nayonsoso]

전체적으로 구현 잘해주셨습니다😊

---

BaseIntegrationTest에서 기본으로 생성되는 유저와 gpaScore 때문에 전체 조회 테스트가 어려워졌습니다. 이 경우 BaseIntegrationTest에서 siteUser나 gpaScore 등을 제거해야할지 고민되네요.. 통합테스트의 단점을 처음 느꼈습니다.🥲

사실 어려움을 느끼신 이유는 '통합테스트여서'라기보다 '테스트 데이터 세팅 방법이 잘못되어서'에 더 가깝다 생각합니다. 지금의 BaseIntegrationTest 는 전체 테스트에 사용될 테스트 데이터를 정의하고 있어서요. BaseIntegrationTest 를 상속하면 테스트 클래스 안에서 테스트 데이터를 세팅하지 않아도 되는 장점은 있지만, 각각의 경우에 딱 맞게 데이터를 세팅해주진 못하는 단점이 있어요.

지금까지는 이를 유용하게 써왔지만, 이제는 단점에 직면했으니 같이 개선해나가봐요!
제가 개인적으로 감명깊게 읽었던 글을 공유합니다.

https://velog.io/@gudonghee2000/읽기-좋은-테스트-코드는-소설-같지-않을까
https://velog.io/@langoustine/Test-Fixture

PS. "같이 개선해나가봐요" 라고는 했지만 테스트 코드 개선은 우선순위가 낮긴 합니다😅
성능 테스트 / index 적용 / 쿼리 최적화 등.. 의 문제들이 더 우선순위가 높긴 해서요 ㅎㅎ
그래도 언젠간 해봐유🥹

[nayonsoso]

사실 처음 봤을 때 이 구현을 보고 조금 놀랐습니다😅

관리자가 사용자를 "검색"해서 지원 목록을 볼 것이라고 저는 예상하지 못했어요.
그렇지만 규혁님은 조회 시 검색을 하면 더 편할 것이라 생각하셨던거죠?

사람마다 구현에 대한 기본적인 생각이 다 다르니, 이런건 시작 전에 구현에 대해서 이야기하고 시작하는게 더 좋았을 것 같아요~

[Gyuhyeok99]

저는 둘다 되는게 좋다 생각해서 이렇게 구현하긴 했습니다! 조건 없이 조회하면 전체조회가 되고 조건조회를 하고싶으면 조건조회가 되도록 한 것인데 위백님은 어떻게 생각하시나요? @wibaek

[nayonsoso]

코드를 변경하자는 뜻으로 말씀드린건 아니예요.
다만 "앞으로는 이렇게 구체적으로 구현하기 전에 이야기를 했으면 좋겠다"는 뜻이었습니다!

[wibaek]

전 조건이 선택적으로 적용 가능하면 괜찮다고 봅니다~

제가 자바/스프링을 잘 몰라서 궁금한 부분이 있는데, 여기서 방금 해당 조건들이 nullable한지 알기 위해 레포지토리 구현체까지 가서 확인했는데요, 이런 값들이 nullable한지 바로 파악할 수 있는 방법이 있을까요?

[Gyuhyeok99]

@nullable 어노테이션을 ScoreSearchCondition 쪽에 붙여놓을 수 있을 거 같은데 괜찮으신가요? nullable한지 바로 파악하기 힘들어보이긴 하네요 지금 🥲 저는 보통 스웨거를 썼어서 @Schema로 표현을 해놨었습니다

[wibaek]

지금 다른 dto를 확인해보니 기본적으로 nullable에 null이면 안되는 것들만 contraint.NotNull 해주는 느낌이네요! 단순 궁금증이였어서 수정은 안해주셔도 괜찮을 것 같습니다! 일관성을 위해서도요

[nayonsoso]

리뷰 반영 확인했습니다~
사소한 코멘트 남겨요!