-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#245 유저 게시글 신고하기 및 신고함 조회 기능 #272
The head ref may contain hidden characters: "Feature/#245-\uC720\uC800_\uAC8C\uC2DC\uAE00_\uC2E0\uACE0\uD558\uAE30_\uBC0F_\uC2E0\uACE0\uD568_\uC870\uD68C_\uAE30\uB2A5"
Feature/#245 유저 게시글 신고하기 및 신고함 조회 기능 #272
Conversation
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.
크게 수정할 점은 보이지 않습니다.
나중에 신고하면 자동으로 차단도 할 것인지에 대해서도 논의해 보아야겠네요.
수고하셨습니다.
private final String content; | ||
private final ReportReasonType reasonType; | ||
private final ReportType type; | ||
@JsonFormat(pattern = "yyyy:MM:dd:HH:mm:ss") |
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.
개인적인 의견입니다.
년/월/일과 시간이 모두 :로 구분되어 있으면 혹시 안드 입장에서 날짜와 시간을 따로 파싱하기 힘들지 않을까요?
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.
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.
이 부분에서 디자이너 분 포맷을 보니까 화면마다 필요한 포맷이 다르더라고요.
그 행사 일정 나타낼때는 yyyy:MM:dd HH:mm이 쓰이고.
다른 게시글에서 날짜 보여줄 때는 yyyy:MM;dd가 쓰이는 식으로
화면에 보여주는 형식 그대로 나타내면 될 것 같아요.
@Column(nullable = false) | ||
private Long reportedId; | ||
@Column(nullable = false) | ||
private 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.
혹시 content를 받는게 아니라 ReportType이 있으니 id를 받는건 어떻게 생각하시나요?
예를 들어 id가 1이고 ReportType이 Comment면 1번 id의 comment를 찾을 수 있도록이요
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.
음음 저도 고민했던 방식인데 어느 방식이 더 나을지 확신이 잘 안서서, 추후 신고 정책 결정할 때 얘기해보겠습니다!
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.
신고관련된 부분은 논의를 해봐야하니까 나중에 이야기해보죠
@@ -22,11 +23,11 @@ create table event | |||
id bigint auto_increment primary key, | |||
created_at datetime(6), | |||
updated_at datetime(6), | |||
end_date datetime(6) not null, | |||
end_date datetime(6) not null, |
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.
이거 왜 자꾸 정렬이 아마란스를 거치면 바뀌는 거죠
private final String content; | ||
private final ReportReasonType reasonType; | ||
private final ReportType type; | ||
@JsonFormat(pattern = "yyyy:MM:dd:HH:mm:ss") |
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.
이 부분에서 디자이너 분 포맷을 보니까 화면마다 필요한 포맷이 다르더라고요.
그 행사 일정 나타낼때는 yyyy:MM:dd HH:mm이 쓰이고.
다른 게시글에서 날짜 보여줄 때는 yyyy:MM;dd가 쓰이는 식으로
화면에 보여주는 형식 그대로 나타내면 될 것 같아요.
if (reportRequest.getType() == ReportType.REQUEST_NOTIFICATION) { | ||
validateRequestNotification(reportRequest); | ||
} | ||
} |
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.
이 메서드부터 코드 중복이 많이 발생하는데, 이를 줄일 수 있는 방법이 있을까요?
혼자 고민해봤는데 마땅한 해결책이 떠오르지 않더라구요...
혹시 아이디어 있으면 알려주시면 감사하겠습니다!
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.
흠,, 이건 type마다 다 다른 repository가 필요해서,, 어쩔 수 없을 것 같다는 생각이 듭니다,,
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.
저도 일단은 생각나는 방법이 없네요ㅠ
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.
고생하셨습니다
커멘트에 남긴 글들 확인해주시고 재요청주세요~
|
||
@PostMapping("/reports") | ||
@ResponseStatus(HttpStatus.CREATED) | ||
public ReportResponse create(@RequestBody final ReportRequest reportRequest, Member member) { |
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.
final 이 없습니다 ! 우리 컨벤션이니까 생각해주시면 좋을 것 같아요
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.
아! 마지막에 한 번 더 확인했어야했는데 깜빡했네요;;;
찾아주셔서 감사합니다...!
|
||
@RequiredArgsConstructor | ||
@Getter | ||
public class ReportRequest { |
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.
이건 그냥 개인적인건데 저는 나중에 Report가 어떻게 바뀔지 모르니까
DTO 네이밍을 최대한 HTTP Method 와 비슷하게 지어줍니다.
저번 리뷰에서도 말씀드린 것 같은데 ReportRequest는 너무 포괄적인 의미가 있어서요
ReportResponse도 마찬가지구요. 네이밍을 엄청 고민하시고 결정하셨다면 상관없지만 그게 아니라면 생각해보시면 좋을 것 같습니다
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.
오 알겠습니다! 전에도 리뷰해주셨던 내용인데 미처 생각을 못했네요,,,반영해서 ReportCreateRequest, ReportCreateResponse, ReportFindResponse로 변경했습니다
@JsonFormat(pattern = "yyyy:MM:dd:HH:mm:ss") | ||
private final LocalDateTime createdAt; | ||
|
||
public static ReportResponse of(Report 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.
정팩메에서 파라미터 하나면 from 쓰지 않나여??
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.
작성할 때 잠깐 헷갈렸던 것 같습니다.
수정했습니다, 감사합니다!
report.getCreatedAt()); | ||
} | ||
|
||
public static List<ReportResponse> of(List<Report> reports) { |
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.
여기두요
private void validateComment(final ReportRequest reportRequest) { | ||
Comment comment = commentRepository.findById(reportRequest.getContentId()) | ||
.orElseThrow(() -> new ReportException(ReportExceptionType.NOT_FOUND_CONTENT)); | ||
if (!comment.getMember().getId().equals(reportRequest.getReportedId())) { |
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.
tell don't ask 를 한번 검색해보시는 것도 좋아보입니다
validateRecruitmentPost, validateRequestNotification 이거 포함해서요~
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.
오 이렇게 처리하는 걸 미처 생각하지 못했네요...!
덕분에 코드가 훨씬 깔끔해졌어요
감사합니다~
- 누락된 final 추가 - Dto 네이밍을 HTTP Method와 비슷하게 변경 - TDA 적용 #245
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.
전에 리뷰를 한 줄 알았는데 안했었네요ㅠ
수고하셨습니다.
validateContent(reportRequest); | ||
} | ||
|
||
private void validateReporterMismatch(final Long reporterId, final Member member) { |
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.
개인적으로 이러한 검증의 경우 무조건 reporterId가 memberId여야 하는 경우이니 dto로 reporterId를 받지 않는건 어떻게 생각하시는지 궁금합니다.
저는 이러한 경우에 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.
오 충분히 납득이 되는 이유인 것 같네요! 코드에 반영했습니다.
감사합니다~!
if (reportRequest.getType() == ReportType.REQUEST_NOTIFICATION) { | ||
validateRequestNotification(reportRequest); | ||
} | ||
} |
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.
저도 일단은 생각나는 방법이 없네요ㅠ
} | ||
} | ||
|
||
private void validateExistReportedMember(final Long reportedId) { |
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.
이건 제 영어실력 문제일 수 있는데 처음에 메서드 명만 보고 이미 신고된 사용자를 검증하는 메서드인 줄 알았습니다ㅎㅎ
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.
아 validateAlreadyExistReport()
메서드가 따로 있는 상황이라 의도가 혼동이 될 수 있겠네요...!
validateNotFoundReportedMember()
로 변경해주었습니다! 감사합니다~
HttpStatus.BAD_REQUEST, | ||
"신고한 게시물이 신고 대상자의 게시물이 아닙니다." | ||
), | ||
REPORT_MYSELF( |
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.
개인적으로 예외이다 보니 CAN_NOT_REPORT_MYSELF
와 같이 금지의 의미가 있으면 좋을 것 같아요.
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.
오 그렇게 하면 가독성이 더 좋아지겠네요! CANNOT이라는 표현은 지양하기로 했던 것 같아서, FORBIDDEN_REPORT_MYSELF로 수정해주었습니다!
- 신고자와 비교해야 할 것을 신고 대상자와 비교하는 로직으로 인해 발생한 오류 수정 #245
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.
수고하셨습니다~!
#️⃣연관된 이슈
close #245
📝작업 내용
사용자 게시물 신고하기 API 및 신고 목록 조회 API 구현
예상 소요 시간 및 실제 소요 시간
3시간/3시간 30분+수정 보완하는 데 4시간
💬리뷰 요구사항(선택)