신고 조치 예정 목록 조회 기능 구현 #750

tjdtls690 · 2023-10-15T16:55:11Z

🔥 연관 이슈

close: #746

📝 작업 요약

신고 조치 예정 목록 조회 기능 구현

⏰ 소요 시간

6시간

🔎 작업 상세 설명

신고가 하나라도 존재하는 요소를 관리자 페이지의 신고 조치 예정 목록에서 조회한다.

🌟 논의 사항

얘기할 부분이 좀 많아서 논의 사항은 캠퍼스에서 직접 같이 얘기해봐야 할 듯 합니다.

github-actions · 2023-10-15T16:59:25Z

Test Results

377 tests 377 ✔️ 20s ⏱️
119 suites     0 💤
119 files     0 ❌

Results for commit 34a7350.

♻️ This comment has been updated with latest results.

woo-chang

안녕하세요 아벨 :) 다즐입니다 ㅎㅎ

전체적인 로직을 깔끔하게 작성해주셔서 읽기 쉬웠던 것 같아요
몇 가지 소소한 커멘트와 질문 남겨두었는데 확인해주시면 감사합니다 🙇🏻‍♂️

woo-chang · 2023-10-16T04:11:27Z

backend/src/main/java/com/votogether/domain/report/controller/ReportQueryController.java

+
+ private final ReportQueryService reportQueryService;
+
+ @GetMapping("/reports/admin")


/reports는 클래스 레벨에 @RequestMapping을 통해 매핑시키는 것은 어떨까요?

다른 컨트롤러 보니까 메서드가 하나만 있는 경우엔 그냥 메서드에 붙여져 있더라구요. 그래서 가독성을 더 챙기기 위해 그렇게 했다고 생각했어서 똑같이 이렇게 하긴 했는데, 다즐의 의견이 궁금합니다 :)

현재는 메서드가 1개밖에 없기에 지금으로도 괜찮다고 생각해요 ㅎㅎ

메서드가 추가된다면 분리하는 것으로 개선해봐도 좋을 것 같아요 👍

woo-chang · 2023-10-16T04:12:16Z

backend/src/main/java/com/votogether/domain/report/dto/response/ReportsResponse.java

+import java.util.stream.IntStream;
+
+@Schema(description = "신고 조치 예정 목록 응답")
+public record ReportsResponse(


ReportPageResponse라는 클래스명은 어떠신가요?! 🤓

woo-chang · 2023-10-16T04:14:03Z

backend/src/main/java/com/votogether/domain/report/dto/response/ReportsResponse.java

+@Schema(description = "신고 조치 예정 목록 응답")
+public record ReportsResponse(
+ @Schema(description = "신고 조치 예정 목록 전체 페이지 수", example = "20")
+ long totalPageCount,


totalPageNumber를 사용하기로 얘기했던 것 같아요 :)

API 명세에서 totalPageCount라고 되어있길래 변수명을 이렇게 했었는데, 방금 제로랑 얘기해서 totalPageNumber로 하기로 결정 되었습니다. 수정할게요!

woo-chang · 2023-10-16T04:16:27Z

backend/src/main/java/com/votogether/domain/common/BaseEntity.java

+ protected BaseEntity() {
+ }
+
+ protected BaseEntity(final LocalDateTime createdAt) {
+ this.createdAt = createdAt;
+ }


BaseEntity는 JPA에 의해 자동으로 결정되는 값이라 조작이 가능하다면 의도치 않은 문제가 발생할 수 있을 것이라 생각해요.

어떤 이유로 값을 조작하도록 수정하셨는지 이유가 궁금합니다 !

현재 ReportCustomRepositoryImpl의 findReportsGroupedByMemberAndReportTypeAndTargetId 메서드에서 Projections.constructor를 통해 Report를 초기화 하는 중인데, 이게 생성자를 직접 사용해서 초기화하는 형식이더라구요. 그리고 여기서 전달한 createdAt을 초기화 하려면 super() 생성자를 사용해서 초기화 해야만 createdAt이 null이 안되더라구요.

조회하는 과정에서 max()나 group_concat()과 같은 함수를 같이 써야하다보니, 자동으로 Report에 매핑해서 가져올 수 없다고 판단을 내려서 이렇게 구현했습니다.

Report 자체는 엔티티이기 때문에 DB에 저장되어 있는 데이터를 그대로 가져와야한다고 생각해요.

findReportsGroupedByMemberAndReportTypeAndTargetId 메서드는 DB 데이터를 조작해서 필요한 데이터를 만들어내는 메서드이기 때문에 Entity를 직접적으로 매핑시키기 보다는 DTO를 통해 필요한 데이터를 만들어내는 방법으로 위의 문제를 해결할 수 있을 것 같은데 이 부분은 어떻게 생각하시나요? 🤓

괜찮은 것 같아요! 개선해보겠습니다 :)

woo-chang · 2023-10-16T04:18:10Z

backend/src/main/java/com/votogether/domain/report/entity/Report.java

+ final String reason,
+ final LocalDateTime createdAt
+ ) {
+ super(createdAt);


JPA에 의해 자동으로 생성되지 않은 값을 사용하지 않고 직접 설정해주시는 이유가 있으신가요? 🤔

위 답변과 동일합니다 :)

woo-chang · 2023-10-16T04:24:47Z

backend/src/main/java/com/votogether/domain/report/repository/ReportCustomRepositoryImpl.java

+ )
+ )
+ .from(report)
+ .orderBy(report.id.max().desc())


최대 ID로 정렬하는 방법 좋네요 👍

woo-chang · 2023-10-16T04:25:05Z

backend/src/main/java/com/votogether/domain/report/repository/ReportCustomRepositoryImpl.java

+ report.id.max(),
+ report.reportType,
+ report.targetId,
+ Expressions.stringTemplate("group_concat({0})", report.reason),


사유 여러 개를 이런 방법으로 묶을 수 있군요 배워갑니다 🙇🏻‍♂️

woo-chang · 2023-10-16T04:27:35Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+ public ReportsResponse getReports(final int page) {
+ final Pageable pageable = PageRequest.of(page, BASIC_PAGE_SIZE);
+ final List<Report> reports = reportRepository.findReportsGroupedByMemberAndReportTypeAndTargetId(pageable);
+ final int totalPages = reportRepository.findAll(pageable).getTotalPages();


Page 객체를 조회하면 추가적인 쿼리들이 몇몇 나가는 것으로 알고 있어요 !

전체 개수만 조회해서 직접 페이지를 구하는 방법은 어떨까요?!

개선해보겠습니다 :)

woo-chang · 2023-10-16T04:32:29Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+ .parseTarget(report.getTargetId())
+ )
+ .toList();
+ return ReportsResponse.of(totalPages, page + 1, reports, targets);


reports와 targets를 각각 넘겨주니 ReportsResponse 입장에서는 순서가 일치하지 않을 수 있겠다는 생각이 들 수도 있을 것 같아요.

getReports 메서드 내에서 reports를 stream을 통해 돌면서 List<ReportResponse>를 생성해서 ReportsResponse로 넘겨주는 방법이 더욱 직관적일 것이라 생각드는데 아벨은 어떻게 생각하시나요 🤓

그게 더 좋을 것 같네요. 개선해보겠습니다.

woo-chang

변경 사항 확인했습니다 ㅎㅎ

같이 얘기하고 싶은 부분 몇 가지가 더 있어 커멘트 남겨두었습니다 :)

woo-chang · 2023-10-16T11:41:23Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+ private final ReportActionProvider reportActionProvider;
+
+ public ReportPageResponse getReports(final int page) {
+ long totalCount = reportRepository.count();


final 키워드가 빠져있어요 :)

woo-chang · 2023-10-16T11:46:08Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+ private List<ReportResponse> parseReportResponses(final List<Report> reports) {
+ final List<String> targets = parseTargets(reports);
+ return IntStream.range(0, reports.size())
+ .mapToObj(index -> ReportResponse.of(reports.get(index), targets.get(index)))
+ .toList();
+ }
+
+ private List<String> parseTargets(final List<Report> reports) {
+ return reports.stream()
+ .map(this::parseTarget)
+ .toList();
+ }
+
+ private String parseTarget(final Report report) {
+ return reportActionProvider.getStrategy(report.getReportType())
+ .parseTarget(report.getTargetId());
+ }


private List<ReportResponse> convertToReportResponses(final List<Report> reports) { return reports.stream() .map(report -> { final var strategy = reportActionProvider.getStrategy(report.getReportType()); return ReportResponse.of(report, strategy.parseType(report.getTargetId()); }) .toList(); }

수도 코드라 동작할지 모르겠는데 요렇게도 가능하지 않나요?!

aiaiaiai1

아벨 깔끔하게 잘 구현해주셨네요!
사소한 커멘트 남겨드려보았습니다!
고생하셨어요~

aiaiaiai1 · 2023-10-17T04:55:55Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+ final List<ReportResponse> reportResponses = parseReportResponses(reports);
+
+ return ReportPageResponse.of(totalPageNumber, page, reportResponses);
+ }
+
+ private List<ReportResponse> parseReportResponses(final List<Report> reports) {
+ final List<String> targets = parseTargets(reports);
+ return IntStream.range(0, reports.size())
+ .mapToObj(index -> ReportResponse.of(reports.get(index), targets.get(index)))
+ .toList();
+ }


P2
ReportPageResponse의 파라미터로 List 가 아닌 List reports를 받아서
parseReportResponses의 메서드를 ReportPageResponse에 두면 어떤것 같나요?
dto를 위한 로직 같아서요!

parseReportResponse 메서드에서 reportActionProvider를 사용하고 있어서 서비스에다 두었습니다. 지금 다즐 피드백 보고 메서드를 2개로 줄였는데, 개인적으로 이 정도면 서비스에 두어도 괜찮을 것 같다는 생각이 들었어요.

만약 public 메서드가 6~7개를 넘어가면 reportActionProvider를 response에 같이 주어서라도 private 로직을 response에 전부 넘겨주는 것이 더 좋을 것 같다는 생각도 드는데, 현재 상황에선 괜찮을 것 같아서요.

루쿠는 어떻게 생각하시나요??

이정도의 메서드 개수이면 괜찮을것같습니다!

aiaiaiai1 · 2023-10-17T05:31:42Z

backend/src/main/java/com/votogether/domain/report/service/ReportCommandService.java

 @Transactional
 @Service
 public class ReportCommandService {

- private final Map<ReportType, ReportStrategy> reportActions;
-
- public ReportCommandService(
- final ReportPostStrategy reportPostStrategy,
- final ReportCommentStrategy reportCommentStrategy,
- final ReportNicknameStrategy reportNicknameStrategy
- ) {
- this.reportActions = new EnumMap<>(ReportType.class);
- this.reportActions.put(ReportType.POST, reportPostStrategy);
- this.reportActions.put(ReportType.COMMENT, reportCommentStrategy);
- this.reportActions.put(ReportType.NICKNAME, reportNicknameStrategy);
- }
+ private final ReportActionProvider reportActionProvider;


provider로 묶으니까 훨씬 깔끔해지군요!

aiaiaiai1 · 2023-10-17T05:34:31Z

backend/src/test/java/com/votogether/domain/report/service/strategy/ReportPostStrategyTest.java

+ @Test
+ @DisplayName("targetId를 통해 해당 멤버의 Nickname을 가져온다")
+ void parseTarget() {
+ // given
+ Post post = postTestPersister.postBuilder().save();
+
+ // when
+ final String postId = reportPostStrategy.parseTarget(post.getId());
+
+ // then
+ assertThat(postId).isEqualTo(post.getId().toString());
+ }
+


p1
DisplayName이 잘못된거 같아요

aiaiaiai1

아벨 확인했습니다~
빠르게 approve 할께요!
고생하셨습니다!

woo-chang

확인했습니다 ㅎㅎ 수고하셨어요 🤓👍

jeomxon

역시나 멋지게 잘 만들어주셨군요!
사소한 의문증들 남겨 두었으니 확인 부탁드릴게요 :)

jeomxon · 2023-10-17T06:52:58Z

backend/src/main/java/com/votogether/domain/report/repository/ReportCustomRepositoryImpl.java

+ report.id.max(),
+ report.reportType,
+ report.targetId,
+ Expressions.stringTemplate("group_concat({0})", report.reason),


Q
이 식이 이해가 잘 안가는데 어떻게 동작하는건가요?
id와 createdAt에 max를 걸어준 이유가 무엇인가요?

동작 방식은reportType, targetId를 group by로 묶어서 reportType, targetId가 같은 레코드끼리는 하나로 합쳐져서 반환됩니다.

여기서 문제는 reportType, targetId는 group by로 묶였기 때문에 1개씩 잘 나오게 되는데, report의 id, reason, createdAt은 여러 행이 나올 수 있는 컬럼들입니다. 그래서 max()는 여러개 묶인 것 중 가장 최신 것을 가져오기 위해 max()를 사용하였습니다.

.orderBy(report.id.max().desc())

위처럼 정렬할 때도 최신순으로 정렬하도록 구현했습니다.

레전드 설명 잘 들었습니다.
이해 되었습니다!

jeomxon · 2023-10-17T06:55:51Z

backend/src/main/java/com/votogether/domain/report/dto/ReportAggregateDto.java

+ long reportMaxId,
+ ReportType reportType,
+ long targetId,
+ String reasons,


Q
String 타입인데 reasons인 이유가 여러 사유들을 concat하기 때문인가요?
그렇다면 api명세 수정이 필요해보입니다.

ReportAggregateDto는 api에 관련된 것이라기 보단, ReportCustomRepositoryImpl 클래스의 findReportsGroupedByReportTypeAndTargetId에서 데이터를 조회할 때 매핑할 용도로 만들었습니다.

Service 계층에서 ReportAggregateDto를 ReportPageResponse로 매핑하는 과정이 있어서 괜찮을 것 같다고 생각하는데 어떻게 생각하시나요

String인데 reasons인 이유가 궁금했습니다!

String이긴 한데 일단 쉼표로 이어져 있는 방식의 String이기 때문에 reasons로 했는데, 좀 더 가독성 좋은 변수명이 있을까요.
제 머리의 한계가...ㅋㅋ

jeomxon · 2023-10-17T07:08:14Z

backend/src/main/java/com/votogether/domain/report/service/ReportQueryService.java

+
+ private ReportResponse parseReportResponse(final ReportAggregateDto reportAggregateDto) {
+ final ReportStrategy strategy = reportActionProvider.getStrategy(reportAggregateDto.reportType());
+ return ReportResponse.of(reportAggregateDto, strategy.parseTarget(reportAggregateDto.targetId()));


Q
parseTarget이라는 메서드 명이 적절한 지 의문이 듭니다!
Target을 파싱해주는 역할이 맞나요?

뭔가 target으로 파싱한다는 의미로 했었었는데, 이게 Post의 id로 변할수도, 댓글 내용이나 멤버의 닉네임으로 변할 수도 있어서 어떤 이름이 좋을지 잘 생각이 안나네요.

targetId를 파싱한다는 의미에서 parseTargetId가 나을까요?

GPT한테 추천 메서드명을 물어보니,

retrieveTargetInfo(): "검색"이라는 뜻을 갖는 retrieve와 "정보"라는 뜻을 갖는 info를 합쳐서 대상의 정보를 검색한다는 의미를 잘 전달하는 이름입니다.

getTargetDescription(): 대상의 설명이나 정보를 얻어오는 것에 중점을 둔 이름입니다.

identifyTarget(): 대상을 식별한다는 의미로, 어떠한 식별자를 사용하여 특정 대상을 구분하거나 식별하는 작업에 적합한 이름입니다.

lookupTargetDetails(): "조회"라는 뜻의 lookup와 "세부 사항"이라는 뜻의 details를 합친 이름으로, 대상의 세부 사항을 조회한다는 의미를 가집니다.

이렇게 나오네요...ㅋㅋ

뭔가 parse라는 단어의 의미랑 맞지 않아서 한번에 이해가 잘 되지 않았어요.
제가 아는 parse의 의미는 "분해하다"라서요!

개인적으로 parse 라는 이름을 많이 쓰는 이유가, 자바에서 제공하는 라이브러리에서도 Integer의 parseInt를 참고했었는데요. parse가 분석하다라는 뜻을 가지고 있어서, 분석해서 변환한다 라는 의미로 받아들였습니다.

jeomxon · 2023-10-17T07:11:25Z

backend/src/main/java/com/votogether/domain/report/service/strategy/ReportActionProvider.java

+import org.springframework.stereotype.Component;
+
+@Component
+public class ReportActionProvider {


Q

클래스 명을 위와 같이 지으신 이유가 궁금합니다.

신고 타입에 대해 전략을 뱉어주는 방법에서 이 방식 외에 어떤 다른 방식도 생각해보셨는지 궁금합니다.

클래스 명은 신고 관련 동작을 제공해준다는 의미에서 이렇게 붙였습니다.

먼저 이걸 분리한 이유는 여러 서비스에서 공통으로 사용하게 되어서 중복 로직 제거, 서비스 크기 단축을 위해서였습니다. 처음 생각은 ReportType과 나머지 파라미터 하나를 전부 전달해서, 이 클래스 안에서 전략 메서드의 실행까지 역할을 맡도록 구현할까도 생각했었는데, 그렇게 되면 이 안에서 분기점이 많이 나올 수밖에 없다고 생각했습니다. 또한 파라미터에 따라 어떤 메서드를 실행시켜줄 지 판단하는 것도 굉장히 애매하다고 느껴져서 그냥 전략을 매핑해주는 용도로 사용하게 되었습니다.
이것 외엔 서비스의 Map 공통 로직을 처리할 방법을 생각해보진 못했습니다.

tjdtls690 added 2 commits October 16, 2023 01:52

feat: (#746) 컨트롤러 및 DTO 구현

065d331

feat: (#746) 신고 조치 예정 목록 조회 기능 구현

79f347b

tjdtls690 added BE 🐾 개발 labels Oct 15, 2023

tjdtls690 requested review from jeomxon, woo-chang and aiaiaiai1 October 15, 2023 16:55

tjdtls690 self-assigned this Oct 15, 2023

feat: (#746) report 엔티티의 toString 제거

7859e43

tjdtls690 added 2 commits October 16, 2023 02:03

feat: (#746) String 파싱 로직을 toString으로 개선

c51db46

feat: (#746) 인터페이스의 메서드 순서 개선

519cc77

woo-chang requested changes Oct 16, 2023

View reviewed changes

tjdtls690 added 5 commits October 16, 2023 14:29

refactor: (#746) ReportsResponse DTO 클래스명 개선

7ed6dce

refactor: (#746) 전체 페이지 수 변수명 개선

4d3b6b0

refactor: (#746) 전체 페이지 개수 구하는 로직을 쿼리 발생 개수를 줄이기 위해 개선

804a816

refactor: (#746) ReportQueryService의 getReports메서드 로직 가독성 개선

f7305ce

refactor: (#746) dto 클래스명 개선

632ecb8

woo-chang reviewed Oct 16, 2023

View reviewed changes

refactor: (#746) 서비스 import문 정리

08b5e61

aiaiaiai1 requested changes Oct 17, 2023

View reviewed changes

tjdtls690 added 4 commits October 17, 2023 15:09

refactor: (#746) 커스텀 데이터를 조회할 시, 엔티티 대신 DTO로 받는 것으로 개선

13fa701

refactor: (#746) final 키워드 붙이기

81ea178

:refactor: (#746) parseTarget 로직 변경

e70a104

refactor: (#746) 테스트 displayName 수정

02717b9

aiaiaiai1 approved these changes Oct 17, 2023

View reviewed changes

refactor: (#746) repository 메서드명 수정

34a7350

woo-chang approved these changes Oct 17, 2023

View reviewed changes

tjdtls690 merged commit 14ed1c6 into dev Oct 17, 2023
3 checks passed

tjdtls690 deleted the feat/#746 branch October 17, 2023 07:11

jeomxon requested changes Oct 17, 2023

View reviewed changes


		private final ReportQueryService reportQueryService;

		@GetMapping("/reports/admin")

신고 조치 예정 목록 조회 기능 구현 #750

신고 조치 예정 목록 조회 기능 구현 #750

Conversation

tjdtls690 commented Oct 15, 2023

🔥 연관 이슈

📝 작업 요약

⏰ 소요 시간

🔎 작업 상세 설명

🌟 논의 사항

github-actions bot commented Oct 15, 2023 • edited Loading

Test Results

woo-chang 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

tjdtls690 Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

woo-chang Oct 16, 2023 • edited Loading

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

woo-chang 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

aiaiaiai1 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

aiaiaiai1 left a comment

Choose a reason for hiding this comment

woo-chang left a comment

Choose a reason for hiding this comment

jeomxon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjdtls690 Oct 17, 2023 • edited Loading

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

github-actions bot commented Oct 15, 2023 •

edited

Loading

tjdtls690 Oct 16, 2023 •

edited

Loading

woo-chang Oct 16, 2023 •

edited

Loading

tjdtls690 Oct 17, 2023 •

edited

Loading