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

feat : 공지 읽음 표시 로직 추가 및 response 수정 #151

Merged
merged 6 commits into from
Nov 13, 2024

Conversation

rladmstn
Copy link
Contributor

@rladmstn rladmstn commented Nov 5, 2024

📌 Related Issue

close #146

🚀 Description

  • 원래 공지 읽음 표시 API를 추가하려 했습니다.
  • 그런데 다시 생각해보니 공지 단건 조회 할 때 내부에서 처리해도 될 것 같아서 공지 조회 API의 로직을 수정해서 처리했습니다.
  • NoitceRead 엔티티로 공지를 읽었는지 관리합니다.
  • 공지 목록 조회 DTO에서 isRead로 해당 공지를 읽었는지 표시해 전달합니다.

📢 Review Point

📚Etc (선택)

@rladmstn rladmstn added the new-feature 기능 추가 label Nov 5, 2024
@rladmstn rladmstn requested review from sh0723 and hwangjokim November 5, 2024 16:41
@rladmstn rladmstn self-assigned this Nov 5, 2024
@hwangjokim hwangjokim requested a review from s-hwan November 7, 2024 00:53
@Entity
@Getter
@NoArgsConstructor
public class NoticeRead {
Copy link
Contributor

Choose a reason for hiding this comment

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

테이블 이름 챗지랑 합의해서 지은건가요? ㅋㅋㅋ 그냥 느낌이 와서..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

정답-!

@@ -67,13 +70,16 @@ public GetNoticeResponse getNotice(@AuthedUser User user, Long noticeId) {
if (!groupMemberRepository.existsByUserAndStudyGroup(user, notice.getStudyGroup()))
throw new StudyGroupValidationException(HttpStatus.FORBIDDEN.value(), "참여하지 않은 스터디 그룹 입니다.");

readNotice(user, notice);
Copy link
Contributor

Choose a reason for hiding this comment

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

개인적으로 이름이 "공지읽기" 여서 뭔가.. 역할을 잘 표현하는 것 같다는 느낌이 와닿지는 않네요. 그렇다고 추천해주자니 애매하고..ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네이밍 때문에 머리 터질 것 같아요.. 저도 찝찝한 이름이지만 더 생각이 안나서 :(

Copy link
Contributor

Choose a reason for hiding this comment

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

markNoticeAsRead 같은거..?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 굳 👍

log.info("success to get notice");
return GetNoticeResponse.builder()
.author(notice.getAuthor().getNickname())
.noticeId(notice.getId())
.noticeTitle(notice.getTitle())
.noticeContent(notice.getContent())
.createAt(DateFormatUtil.formatDate(notice.getCreatedAt().toLocalDate()))
.isRead(noticeReadRepository.existsByNoticeAndUser(notice, user))
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 무조건 true 아닌가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

공지글 하나 조회니 그러겠네요! 수정해야겠어요 감삼다

Copy link
Contributor

@s-hwan s-hwan left a comment

Choose a reason for hiding this comment

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

질문있습니다(진짜 모름)
NoticeRead 객체는
Notice 객체당 1:1 로 매핑되어 생성되는것인가요..?
아니면 notice를 읽을 수 있는 user당 하나씩 생성되는것인가요?

@rladmstn
Copy link
Contributor Author

rladmstn commented Nov 8, 2024

질문있습니다(진짜 모름) NoticeRead 객체는 Notice 객체당 1:1 로 매핑되어 생성되는것인가요..? 아니면 notice를 읽을 수 있는 user당 하나씩 생성되는것인가요?

정확히는NoticeReadUserNotice의 N:M 관계를 나타내는 중간자 테이블이라고 생각해주심 될 것 같아요!
유저 한 명이 여러 개의 공지를 읽을 수도 있고, 공지 하나는 여러 유저에게 읽혀질(?) 수도 있는 관계를 생각해주면 됩니다

@s-hwan
Copy link
Contributor

s-hwan commented Nov 8, 2024

질문있습니다(진짜 모름) NoticeRead 객체는 Notice 객체당 1:1 로 매핑되어 생성되는것인가요..? 아니면 notice를 읽을 수 있는 user당 하나씩 생성되는것인가요?

정확히는NoticeReadUserNotice의 N:M 관계를 나타내는 중간자 테이블이라고 생각해주심 될 것 같아요! 유저 한 명이 여러 개의 공지를 읽을 수도 있고, 공지 하나는 여러 유저에게 읽혀질(?) 수도 있는 관계를 생각해주면 됩니다

그러면 NoticeRead가 생성되는 시점이 궁금합니다 언제 생성되는지 잘 모르겠어요 ...

  1. Notice가 생성 될 때..?
  2. User가 Notice 를 읽을 때? (아닐 것 같다고 예상중)
    3.위의 두 경우 모두 아님

@rladmstn
Copy link
Contributor Author

rladmstn commented Nov 8, 2024

  1. Notice가 생성 될 때..?
  2. User가 Notice 를 읽을 때? (아닐 것 같다고 예상중)
    3.위의 두 경우 모두 아님

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ아닐 것 같다고 생각하신 2번 경우가 정답입니다

특정 유저가 공지글을 읽으면 해당 유저의 User와 게시글 Notice를 매핑한 NoticeRead 객체가 생성됩니다.
로직 보시면 공지 목록 조회시에도 읽음 여부인 isRead 필드 넘길 때 existsBy- 메소드 사용해서 NoticeRead가 존재하면 읽음(true), 존재하지 않으면 아직 읽지 않음(false)로 리턴하고 있어요!

@s-hwan
Copy link
Contributor

s-hwan commented Nov 8, 2024

  1. Notice가 생성 될 때..?
  2. User가 Notice 를 읽을 때? (아닐 것 같다고 예상중)
    3.위의 두 경우 모두 아님

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ아닐 것 같다고 생각하신 2번 경우가 정답입니다

특정 유저가 공지글을 읽으면 해당 유저의 User와 게시글 Notice를 매핑한 NoticeRead 객체가 생성됩니다. 로직 보시면 공지 목록 조회시에도 읽음 여부인 isRead 필드 넘길 때 existsBy- 메소드 사용해서 NoticeRead가 존재하면 읽음(true), 존재하지 않으면 아직 읽지 않음(false)로 리턴하고 있어요!

아..! 이해가 좀 되었습니다... NoticeRead의 존재 여부로 읽었는지 아닌지 판단하는군요 ... ?
신기하네요 ..친절한 설명 감사합니당 ㅎㅎㅎ

@@ -121,4 +129,12 @@ private void validateStudyGroupExists(Notice notice) {
.orElseThrow(() -> new StudyGroupValidationException(HttpStatus.BAD_REQUEST.value(), "존재하지 않는 스터디 그룹입니다"));
}

private void markNoticeAsRead(User user, Notice notice) {
if (!noticeReadRepository.existsByNoticeAndUser(notice, user)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

참고차 코멘트입니다

사실 이렇게 ifNonExist -> Insert 구조일때 타이밍 이슈가 생길수 있습니다.
서버가 좀 느린 상황에서 유저가 공지를 광클한다던지.. 하면 행이 두 개씩 생길 수 있겠네요

그래서 엄밀해야하는 상황에서는 유저, 노티스 묶어서 Unique index를 걸고, @SQLInsert로 INSERT IGNORE 쿼리를 설정한다던가.. save 쿼리에 try-catch를 건다거나 해야합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

이 코멘트에 대한 질문인데 ..! 서버가 좀 느린 상황에서 광클을 하면 똑같은 요청이 두세번 가게 되고 그에 대해 똑같은 NoticeRead객체가 저장될 수 있으니 까지는 이해했는데
그 아래에 대한 내용 설명해주실 수 있나요 ㅜ.ㅜ

Copy link
Contributor

Choose a reason for hiding this comment

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

이 코멘트에 대한 은수 답이 달리면 머지할게용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어우 이제봤네요 늦어져서 죄송합니담

만약에 그럼 Notice 생성 됐을 때 모든 멤버들에 대해 일괄적으로 NoticeRead 객체를 생성한다고 가정한다면,
읽음 여부가 boolean 필드로 관리가 될텐데 이럴 때는 광클 했을 때 동일한 이슈 안생기나요??
insert일 때만 타이밍 이슈가 치명적으로 발생하는건가요?

@@ -121,4 +129,12 @@ private void validateStudyGroupExists(Notice notice) {
.orElseThrow(() -> new StudyGroupValidationException(HttpStatus.BAD_REQUEST.value(), "존재하지 않는 스터디 그룹입니다"));
}

private void markNoticeAsRead(User user, Notice notice) {
if (!noticeReadRepository.existsByNoticeAndUser(notice, user)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 코멘트에 대한 질문인데 ..! 서버가 좀 느린 상황에서 광클을 하면 똑같은 요청이 두세번 가게 되고 그에 대해 똑같은 NoticeRead객체가 저장될 수 있으니 까지는 이해했는데
그 아래에 대한 내용 설명해주실 수 있나요 ㅜ.ㅜ

@@ -121,4 +129,12 @@ private void validateStudyGroupExists(Notice notice) {
.orElseThrow(() -> new StudyGroupValidationException(HttpStatus.BAD_REQUEST.value(), "존재하지 않는 스터디 그룹입니다"));
}

private void markNoticeAsRead(User user, Notice notice) {
if (!noticeReadRepository.existsByNoticeAndUser(notice, user)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 코멘트에 대한 은수 답이 달리면 머지할게용

@rladmstn
Copy link
Contributor Author

황교수님 코멘트는 일단 참고라 하셨으니 self-merge 하겠습니다-

@rladmstn rladmstn added the self-merge 리뷰어가 아닌, 작성자가 머지하겠습니다. label Nov 13, 2024
@rladmstn rladmstn merged commit c7f8043 into develop Nov 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature 기능 추가 self-merge 리뷰어가 아닌, 작성자가 머지하겠습니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat : 공지 읽음 표시 API 추가
4 participants