-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: 신고 기능을 구현한다 #29
Conversation
- InvalidReporterException: 신고자 정보(reporterId)가 유효하지 않은 경우 - InvalidReportResultException: 신고 결과가 유효하지 않은 경우 - InvalidReportTypeException: 신고 사유가 유효하지 않은 경우 - ReportNotFoundException: 신고 정보를 찾을 수 없는 경우
- ReportCreateRequest
- ReportFixture - ReportCreateRequestFixture
30일 이상 지난 처리된 신고를 삭제하기 위해 AuditingHandler를 31일 지난 값으로 설정하여 엔티티 객체 생성시 생성 시점을 31일이 지난 날짜로 생성되도록 생성일자 및 수정일자 조작
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다! MySQL의 특정 함수도 새롭게 알게 되고 스케줄에 대해서도 알아간 게 있네요.
궁금한 점은 삭제 진행 시 왜 JdbcTemplate을 선택하셨는지가 궁금하고, 그 외적으론 리뷰 참고해주시면 될 것 같습니다
저도 이제 다시 개발 열심히 진행하겠습니다!
.build(); | ||
} | ||
|
||
private static void validateSameUser(final Long reportedUserId, final Long reporterId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제출자의 id와 보고된 id가 같을 경우 예외를 반환하는 것이므로 validateNotSameUser
가 적합할 것 같습니다
@NotBlank(message = "신고 유형을 입력해주세요") | ||
String reportTypeCode, | ||
|
||
String content |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
신고 내용에 대해서도 NotBlank
처리가 되어야 할 것 같습니다..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 필드는 신고의 상세 내용을 적는 필드인데 이 값이 없어도 되기 때문에 @notblank를 사용하지 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 신고 상세 내용이 없을 수도 있군요! 알겠습니다 :)
.orElseThrow(ReportNotFoundException::new); | ||
} | ||
|
||
@Scheduled(cron = "0 0 0 * * ?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
신고 처리가 완료된 신고 삭제 쿼리가 매일 자정에 실행되는데, 코드에서 더 직관적으로 알기 위해 0 0 0 * * ?
도 상수화하면 어떨지 싶습니다! 임시로 테스트 해 보니 상수 (private static final
)로 해도 cron의 값으로 잘 인식하는 것 같습니다. 이러면 매일 자정 말고도 다른 시각으로 변경할 때 쉽게 인지할 수 있을 것 같은데 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀해주신대로 크론 표현식을 상수처리 해서 사용하는 것이 더 좋아보이네요! 수정하겠습니다 :)
@Repository | ||
public class ReportJdbcRepository { | ||
|
||
private final JdbcTemplate jdbcTemplate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JdbcTemplate을 쓰시게 된 이유가 있으실까요? 단순히 궁금합니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NamedParameterJdbcTemplate나 SimpleJdbcInsert 같은 다른 템플릿을 사용하지 않은 이유를 물어보신 건가요?
NamedParameterJdbcTemplate를 사용하지 않은 이유는 넘겨받은 매개변수가 없기 때문이고, SimpleJdbcInsert는 insert 구문이 아니여서 사용하지 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 아니요 삭제 부분만 jpa가 아니라 JdbcTemplate을 쓰신 이유가 궁금했습니다!
그런데 더 생각해보니 일단 필요한 파라미터가 없기도 하고, JdbcTemplate을 쓰면 작성하신 MySQL 함수로 손쉽게 처리할 수 있어서 사용하신 것 같다는 생각이 드네요 (JPA의 특성을 활용하지 않는 상황)
혹시 제가 생각한 것이 맞을지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 현준님 생각이 일부 맞습니다!
해당 쿼리는 30일 이상 지난 신고를 삭제하기 위해 작성한 쿼리인데 JPQL에서는 현재 날짜 정보만 얻을 수 있고 현재 날짜 - 30일과 같이 날짜 연산을 하는 것이 불가능한 것으로 알고 있습니다.(해당 연산이 데이터베이스마다 문법이 다르다고 알고 있습니다)
그래서 우선 JPQL로는 작성이 불가능하여 네이티브 sql을 사용해서 작성을 해야하는 상황이었습니다. (사실 매개변수로 30일 이전의 날짜를 받으면 가능하긴한데 그러면 서비스에서 넘겨줘야하는데 굳이 서비스에서 주지 않아도 될 인자를 넘겨주는 것이 좋지 못한 구현 방식이라고 생각했습니다.)
네이티브 쿼리를 사용한다면 springDataJpa @query(nativeQuery = true)나 JdbcTemplate을 이용해 sql을 작성하는 방법을 택할 수 있는데 저는 jdbcTemplate을 사용하는 방법을 선택했습니다.
이 부분은 테스트를 편하게 하기 위해서 jdbcTemplate을 사용하는 방법을 택했습니다. springDataJpa에서 네이티브 sql을 사용하면 해당 쿼리는 JPA의 영속성 컨텍스트와 무관하게 동작하기 때문에 영속성 컨텍스트에서 관리되고 있는 엔티티와 동기화 문제가 발생할 수 있습니다. 이를 해결하기 위해서는 EntityManager를 사용하여 flush를 하는 로직을 추가해야하는데 jdbcTemplate을 사용하면 테스트도 따로 진행할 수도 있고 불필요하게 늘어나는 추가 로직을 없앨 수 있습니다.
추가적으로, 지금 삭제 기능을 테스트하기 위해서는 엔티티 생성시점을 조작해야하는데 JdbcRepository에서만 엔티티 생성 시점을 조작하여 테스트하는 것이 좋은 방식이라고 생각해서 JdbcTemplate를 사용했습니다 :)
public void deleteProcessedOldReports() { | ||
String sql = "DELETE FROM Report" | ||
+ " WHERE updated_at < TIMESTAMPADD(DAY, -30, CURRENT_DATE)" + | ||
" AND report_result <> 'WAITING'"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
몰랐던 MySQL 함수들을 알고 갑니다 👍
String validReportType = FAKE_PROFILE.getCode(); | ||
|
||
// When | ||
ReportType foundReportType = findByCode(validReportType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 static 메서드 임포트 관련 부분 내용 같을 것 같습니다 (+ When)
reportService.updateReportResult(reportResult, savedReport.getId()); | ||
|
||
// then | ||
assertThat(savedReport.getReportResult().getName()).isEqualTo(reportResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
savedReport.getReportResult()를 미리 빼 두어 연속 메서드 체이닝 (savedReport.getReportResult().getName())을 방지하는 것은 어떨까요? (50번째 줄과 유사하게)
public class ReportFixture { | ||
|
||
public static Report 신고_생성() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공백 제거하면 좋을 것 같습니다 :)
@TestConfiguration | ||
public class TestAuditingTestConfig { | ||
|
||
private static final int THIRTY_ONE = 31; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수 좋습니다! 그런데 THIRTY_ONE처럼 특정 숫자를 의미하는 것 보다는 다른 이름이 더 나을 것 같습니다. (삭제 기준 일자를 수정할 경우 상수명과 값 모두를 바꿔야 하는데, 삭제 기준 일자를 의미하도록 하면 값만 바꾸면 되어 더 좋을 것 같습니다.)
|
||
private boolean isProcessedOldReport(final Report report) { | ||
LocalDateTime thirtyDaysAgo = LocalDateTime.now() | ||
.minusDays(30); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestAuditingTestConfig
에서 작성하셨던 것 처럼 여기도 상수화하면 어떨까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 !! 구현해주신 것에 크게 문제가 없는 것 같아서 Approve하겠습니다!
다음에 pr 메시지에 어떻게 했는지 설명 짧게라도 해주시면 더 좋을 것 같아요~
고생하셨습니다 👍
@Enumerated(EnumType.STRING) | ||
private ReportResult reportResult; | ||
|
||
@Length(max = 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lob은 어떨까요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import org.springframework.data.auditing.AuditingHandler; | ||
|
||
@TestConfiguration | ||
public class TestAuditingTestConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 클래스 좋네요 👍
fieldWithPath("reportTypeCode").description("신고 사유"), | ||
fieldWithPath("content").description("상세 내용")) | ||
)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- content 필드에 @lob 적용 - 신고자와 신고당한 유저의 정보가 다른지 검증하는 메서드 네이밍 수정
- cron 표현식 상수 처리
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
📄 Summary
신고 기능 구현을 완료했습니다.
🙋🏻 More
close #26