-
Notifications
You must be signed in to change notification settings - Fork 4
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
신고조치알림 관련 기능 구현 #756
신고조치알림 관련 기능 구현 #756
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.
안녕하세요 루쿠 ㅎㅎ 다즐입니다
빠르고 깔끔하게 구현 잘 해주셨네요 👍
몇 가지 소소한 커멘트 남겨두었으니 확인해주시면 감사합니다 :)
@GeneratedValue(strategy = GenerationType.IDENTITY) | ||
private Long id; | ||
|
||
@ManyToOne |
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.
fetch
타입 설정이 필요해 보여요 !
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 boolean isChecked; | ||
|
||
@Builder | ||
public ReportActionAlarm( |
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
로 설정하는게 어떨까요?
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.
수정했습니다!
@Getter | ||
@EqualsAndHashCode(of = {"id"}, callSuper = false) | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class ReportToAction extends BaseEntity { |
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.
ERD
에서 해당 도메인이 삭제된 것으로 이해했는데 맞을까요?
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.
네 맞아요 삭제해두었씁니다
@Validated | ||
@RestController | ||
@RequiredArgsConstructor | ||
@RequestMapping("/alarms") |
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.
@Validated
@RequiredArgsConstructor
@RequestMapping("/alarms")
@RestController
다른 컨트롤러와 일관성있게 순서를 맞춰주면 감사합니다 :)
public record ReportActionAlarmResponse( | ||
|
||
@Schema(description = "알림 ID", example = "1") |
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.
불필요한 공백 발견 👀
@Auth final Member member | ||
) { | ||
final ReportActionResponse response = alarmService.getReportActionAlarm(reportActionAlarmId, member); | ||
return ResponseEntity.ok().body(response); |
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.
ditto
|
||
public ReportActionResponse getReportActionAlarm(final Long reportActionAlarmId, final Member member) { | ||
final ReportActionAlarm reportActionAlarm = reportActionAlarmRepository | ||
.findByIdAndMember(reportActionAlarmId, 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.
id
로 찾게되면 member
없이도 항상 id
를 통해서만 조회되지 않을까요?
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.
api요청으로 사용자가 다른 사용자의 알림을 조회 하는 경우를 막기위해 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.
와 전혀 생각지도 못한 부분인데 꼼꼼하게 구현해주셨네요 대박 배워갑니다 👍👍
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.
백엔드서버로 직접 api를 알림id만 바꾸어 쏘면 조회가 가능할거같아요
} | ||
} |
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.
개행 빌런이였네요 ㅋㅋ
|
||
if (post.isWriter(loginMember) && post.isHidden()) { |
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.
루쿠 고생하셨어요!
몇 가지 리뷰 남겼습니다 :)
|
||
@RequestMapping("/report") | ||
public ResponseEntity<List<ReportActionAlarmResponse>> getReportActionAlarms( | ||
@RequestParam @PositiveOrZero(message = "페이지는 0이상 정수만 가능합니다.") final int page, |
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.
@RequestParam 는 생략해도 될 것 같아요 :)
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.
다른 컨트롤러에서는 @RequestParam을 생략하지 않고 명시해두어서 여기서도 따로 생략하지는 않았습니다!
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.
생각해보니 다 붙이고 있었네요...ㅋㅋ
@RestController | ||
@RequiredArgsConstructor | ||
@RequestMapping("/alarms") | ||
public class AlarmController implements AlarmControllerDocs { |
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.
조회에 관한 API만 존재하니 AlarmController -> AlarmCommandController 로 바꿔야 할 것 같아요.
private final ReportActionAlarmRepository reportActionAlarmRepository; | ||
|
||
public List<ReportActionAlarmResponse> getReportActionAlarms(final Member member, final int page) { | ||
final PageRequest pageRequest = PageRequest.of(page, 10, Sort.by(Sort.Direction.DESC, "createdAt")); |
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.
일관성과 가독성을 위해서 10을
private static final int BASIC_PAGE_SIZE = 10;
이렇게 상수화 해주는 것은 어떨까요.
|
||
@Schema(description = "신고조치 세부정보") | ||
ReportActionResponse detail | ||
|
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.
위에 다즐도 적어주셨지만, record의 파라미터 적는 곳에선 앞 뒤 공백이 없어야 하는 것으로 알고 있습니다 :)
if (post.isWriter(loginMember) && post.isHidden()) { | ||
return PostResponse.ofUser( | ||
loginMember, | ||
post, | ||
postCategoryRepository.findAllByPost(post), | ||
post.getFirstContentImage(), | ||
post.getPostOptions(), | ||
voteRepository.findByMemberAndPostOptionPost(loginMember, post) | ||
); | ||
} | ||
|
||
validateHiddenPost(post); | ||
return PostResponse.ofUser( | ||
loginMember, | ||
post, |
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.
return PostResponse.ofUser(
loginMember,
post,
postCategoryRepository.findAllByPost(post),
post.getFirstContentImage(),
post.getPostOptions(),
voteRepository.findByMemberAndPostOptionPost(loginMember, post)
);
이 부분이 완전히 바로 밑에 return문과 중복되는 것으로 보여서 따로 메서드로 빼는 것은 어떨까요??
보니까 PostQueryService의 가장 밑에 있는 메서드인 convertToResponses 에서도 위의 return문과 완전 똑같은 로직이 존재하는데, 그거까지 같이 처리할 겸, 위 return문을 메서드로 분리해 보는 것이 좋을 것 같아요 :)
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.
생각지도 못한 부분에서 예외가 발생할 수 있다는 점을 배워갈 수 있었습니다 👍
잘 구현해주셔서 approve 하도록 할께요 ㅎㅎ 수고하셨습니다 :)
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.
멋지게 구현해주셨네요!
사소한 궁금증 코멘트로 남겨보았습니다 :)
@RequestMapping("/report") | ||
public ResponseEntity<List<ReportActionAlarmResponse>> getReportActionAlarms( | ||
@RequestParam @PositiveOrZero(message = "페이지는 0이상 정수만 가능합니다.") final int page, | ||
@Auth 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.
P2
일관성 있게 loginMember로 변경하는 것이 좋아보입니다.
final PageRequest pageRequest = PageRequest.of(page, BASIC_PAGE_SIZE, | ||
Sort.by(Sort.Direction.DESC, "createdAt")); | ||
final List<ReportActionAlarm> reportActionAlarms = reportActionAlarmRepository | ||
.findByMember(member, pageRequest); |
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.
Q
Repository에 메서드명에 orderBy로 날짜 정렬을 하는 것과 Pageable을 통해서 정렬하는 방식의 차이는 무엇인가요?
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 학습 부족입니다 ㅎㅎ
|
||
public ReportActionResponse getReportActionAlarm(final Long reportActionAlarmId, final Member member) { | ||
final ReportActionAlarm reportActionAlarm = reportActionAlarmRepository | ||
.findByIdAndMember(reportActionAlarmId, 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.
이 코멘트를 보고 궁금한 점이 생겼는데 다른 사용자의 알림이 의도적으로 조회가 가능한가요?
|
||
// when | ||
List<ReportActionAlarmResponse> results = RestAssuredMockMvc | ||
.when().get("/alarms/report?page=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.
P2
queryParam()
을 사용해볼 수 있을 것 같아요.
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.
감사합니다~
.then().log().all() | ||
.status(HttpStatus.OK) | ||
.extract() | ||
.as(new ParameterizedTypeReference<List<ReportActionAlarmResponse>>() { |
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.
Q
TypeRef
를 사용할 수도 있지 않나요? 어떤 차이가 있나요?
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.
간단한 방식이 있었네요!
저도 궁금해서 지피티에 물어보니 TypeRef는 RestAssured 라이브러리에서 제공되고
ParameterizedTypeReference는 Spring Framework에서 제공된다고 하네요
🔥 연관 이슈
close: #744
📝 작업 요약
간지나는 ReportActionAlarm 기능을 구현했습니다.
⏰ 소요 시간
10시간? - api명세서 및 테이블 변경으로 인해 시간 소요
🔎 작업 상세 설명
신고조치알림 목록 조회 기능 구현
신고조치알림 상세 조회 기능 구현
게시글 상세조회 추가 요구사항 적용
🌟 논의 사항
중간에 변동사항들이 있어서 커밋이 지저분 해졌는데 미리 죄송합니다 😭
하지만 작업 내용은 간단합니다 😮