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

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

Merged
merged 16 commits into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,11 @@ public abstract class BaseEntity {
@Column(columnDefinition = "datetime(6)", nullable = false)
private LocalDateTime updatedAt;

protected BaseEntity() {
}

protected BaseEntity(final LocalDateTime createdAt) {
this.createdAt = createdAt;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

@tjdtls690 tjdtls690 Oct 16, 2023

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator

@woo-chang woo-chang Oct 16, 2023

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

@RequiredArgsConstructor
@RestController
public class ReportCommandCommandController implements ReportCommandControllerDocs {
public class ReportCommandController implements ReportCommandControllerDocs {

private final ReportCommandService reportCommandService;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.votogether.domain.report.controller;

import com.votogether.domain.report.dto.response.ReportPageResponse;
import com.votogether.domain.report.service.ReportQueryService;
import jakarta.validation.constraints.PositiveOrZero;
import lombok.RequiredArgsConstructor;
import org.springframework.http.ResponseEntity;
import org.springframework.validation.annotation.Validated;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@Validated
@RequiredArgsConstructor
@RestController
public class ReportQueryController implements ReportQueryControllerDocs {

private final ReportQueryService reportQueryService;

@GetMapping("/reports/admin")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

public ResponseEntity<ReportPageResponse> getReports(
@RequestParam @PositiveOrZero(message = "페이지는 0이상 정수만 가능합니다.") final int page
) {
final ReportPageResponse reportPageResponse = reportQueryService.getReports(page);
return ResponseEntity.ok(reportPageResponse);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package com.votogether.domain.report.controller;

import com.votogether.domain.report.dto.response.ReportPageResponse;
import com.votogether.global.exception.ExceptionResponse;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.media.Content;
import io.swagger.v3.oas.annotations.media.Schema;
import io.swagger.v3.oas.annotations.responses.ApiResponse;
import io.swagger.v3.oas.annotations.responses.ApiResponses;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.validation.constraints.PositiveOrZero;
import org.springframework.http.ResponseEntity;

@Tag(name = "신고 조회", description = "신고 조회 API")
public interface ReportQueryControllerDocs {

@Operation(summary = "신고 조치 예정 목록 조회", description = "신고 조치 예정 목록을 조회한다.")
@ApiResponses({
@ApiResponse(
responseCode = "200",
description = "신고 조치 예정 목록 조회 성공"
),
@ApiResponse(
responseCode = "400",
description = "0이상의 정수가 아닌 페이지",
content = @Content(schema = @Schema(implementation = ExceptionResponse.class))
)
})
ResponseEntity<ReportPageResponse> getReports(
@PositiveOrZero(message = "페이지는 0이상 정수만 가능합니다.") final int page
);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.votogether.domain.report.dto.response;

import io.swagger.v3.oas.annotations.media.Schema;
import java.util.List;

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

@Schema(description = "신고 조치 예정 목록 중 현재 페이지", example = "3")
long currentPageNumber,

@Schema(description = "신고 조치 예정 목록")
List<ReportResponse> reports
) {

public static ReportPageResponse of(
final int totalPageNumber,
final int currentPageNumber,
final List<ReportResponse> reportResponses
) {
return new ReportPageResponse(
totalPageNumber,
currentPageNumber,
reportResponses
);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.votogether.domain.report.dto.response;

import com.fasterxml.jackson.annotation.JsonFormat;
import com.votogether.domain.report.entity.Report;
import com.votogether.domain.report.entity.vo.ReportType;
import io.swagger.v3.oas.annotations.media.Schema;
import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.List;

@Schema(description = "신고 정보 응답")
public record ReportResponse(
@Schema(description = "신고 ID", example = "1")
long id,

@Schema(description = "신고 유형", example = "POST")
ReportType type,

@Schema(description = "신고 이유들")
List<String> reasons,

@Schema(description = "신고 당한 요소의 내용", example = "2")
String target,

@Schema(description = "신고 생성시간", example = "2023-08-01 13:56")
@JsonFormat(pattern = "yyyy-MM-dd HH:mm")
LocalDateTime createdAt
) {

public static ReportResponse of(final Report report, final String target) {
return new ReportResponse(
report.getId(),
report.getReportType(),
Arrays.stream(report.getReason().split(",")).toList(),
target,
report.getCreatedAt()
);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import jakarta.persistence.ManyToOne;
import jakarta.persistence.Table;
import jakarta.persistence.UniqueConstraint;
import java.time.LocalDateTime;
import lombok.AccessLevel;
import lombok.Builder;
import lombok.Getter;
Expand Down Expand Up @@ -48,6 +49,20 @@ public class Report extends BaseEntity {
@Column(nullable = false, length = 50)
private String reason;

public Report(
final Long id,
final ReportType reportType,
final Long targetId,
final String reason,
final LocalDateTime createdAt
) {
super(createdAt);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

위 답변과 동일합니다 :)

this.id = id;
this.reportType = reportType;
this.targetId = targetId;
this.reason = reason;
}

@Builder
private Report(
final Member member,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.votogether.domain.report.repository;

import com.votogether.domain.report.entity.Report;
import java.util.List;
import org.springframework.data.domain.Pageable;

public interface ReportCustomRepository {

List<Report> findReportsGroupedByMemberAndReportTypeAndTargetId(final Pageable pageable);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.votogether.domain.report.repository;

import static com.votogether.domain.report.entity.QReport.report;

import com.querydsl.core.types.Projections;
import com.querydsl.core.types.dsl.Expressions;
import com.querydsl.jpa.impl.JPAQueryFactory;
import com.votogether.domain.report.entity.Report;
import java.util.List;
import lombok.RequiredArgsConstructor;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Repository;

@RequiredArgsConstructor
@Repository
public class ReportCustomRepositoryImpl implements ReportCustomRepository {

private final JPAQueryFactory jpaQueryFactory;

@Override
public List<Report> findReportsGroupedByMemberAndReportTypeAndTargetId(final Pageable pageable) {
return jpaQueryFactory.select(
Projections.constructor(
Report.class,
report.id.max(),
report.reportType,
report.targetId,
Expressions.stringTemplate("group_concat({0})", report.reason),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@tjdtls690 tjdtls690 Oct 17, 2023

Choose a reason for hiding this comment

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

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

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

report.createdAt.max()
)
)
.from(report)
.orderBy(report.id.max().desc())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

.groupBy(report.reportType, report.targetId)
.offset(pageable.getOffset())
.limit(pageable.getPageSize())
.fetch();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import org.springframework.data.jpa.repository.Query;
import org.springframework.data.repository.query.Param;

public interface ReportRepository extends JpaRepository<Report, Long> {
public interface ReportRepository extends JpaRepository<Report, Long>, ReportCustomRepository {

int countByReportTypeAndTargetId(final ReportType reportType, final Long targetId);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,34 +3,26 @@
import com.votogether.domain.member.entity.Member;
import com.votogether.domain.report.dto.request.ReportRequest;
import com.votogether.domain.report.entity.vo.ReportType;
import com.votogether.domain.report.service.strategy.ReportActionProvider;
import com.votogether.domain.report.service.strategy.ReportCommentStrategy;
import com.votogether.domain.report.service.strategy.ReportNicknameStrategy;
import com.votogether.domain.report.service.strategy.ReportPostStrategy;
import com.votogether.domain.report.service.strategy.ReportStrategy;
import java.util.EnumMap;
import java.util.Map;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@RequiredArgsConstructor
@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;
Comment on lines 12 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


public void report(final Member reporter, final ReportRequest request) {
final ReportStrategy reportStrategy = reportActions.get(request.type());
final ReportStrategy reportStrategy = reportActionProvider.getStrategy(request.type());
reportStrategy.report(reporter, request);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package com.votogether.domain.report.service;

import com.votogether.domain.report.dto.response.ReportResponse;
import com.votogether.domain.report.dto.response.ReportPageResponse;
import com.votogether.domain.report.entity.Report;
import com.votogether.domain.report.repository.ReportRepository;
import com.votogether.domain.report.service.strategy.ReportActionProvider;
import java.util.List;
import java.util.stream.IntStream;
import lombok.RequiredArgsConstructor;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

@RequiredArgsConstructor
@Transactional(readOnly = true)
@Service
public class ReportQueryService {

private static final int BASIC_PAGE_SIZE = 20;

private final ReportRepository reportRepository;
private final ReportActionProvider reportActionProvider;

public ReportPageResponse getReports(final int page) {
long totalCount = reportRepository.count();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

final int totalPageNumber = (int) Math.ceil((double) totalCount / BASIC_PAGE_SIZE);

final Pageable pageable = PageRequest.of(page, BASIC_PAGE_SIZE);
final List<Report> reports = reportRepository.findReportsGroupedByMemberAndReportTypeAndTargetId(pageable);
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();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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();
}

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

굿굿


}

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.votogether.domain.report.service.strategy;

import com.votogether.domain.report.entity.vo.ReportType;
import java.util.EnumMap;
import java.util.Map;
import org.springframework.stereotype.Component;

@Component
public class ReportActionProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q

  1. 클래스 명을 위와 같이 지으신 이유가 궁금합니다.
  2. 신고 타입에 대해 전략을 뱉어주는 방법에서 이 방식 외에 어떤 다른 방식도 생각해보셨는지 궁금합니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


private final Map<ReportType, ReportStrategy> reportActions;

public ReportActionProvider(
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);
}

public ReportStrategy getStrategy(ReportType type) {
return reportActions.get(type);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,11 @@ private void validateCommentMine(final Comment comment, final Member member) {
}
}

@Override
public String parseTarget(final Long targetId) {
final Comment reportedComment = commentRepository.findById(targetId)
.orElseThrow(() -> new NotFoundException(CommentExceptionType.NOT_FOUND));
return reportedComment.getContent();
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,11 @@ private void changeNicknameByReport(final Member reportedMember, final ReportReq
}
}

@Override
public String parseTarget(final Long targetId) {
final Member reportedMember = memberRepository.findById(targetId)
.orElseThrow(() -> new NotFoundException(MemberExceptionType.NONEXISTENT_MEMBER));
return reportedMember.getNickname();
}

}
Loading