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

내 알림 조회 및 푸쉬알림 기능 #61

Merged
merged 14 commits into from
Mar 4, 2024
Merged

Conversation

JaeyoungChoi98
Copy link
Contributor

@JaeyoungChoi98 JaeyoungChoi98 commented Feb 3, 2024

이슈 연결

구현한 API

  • GET /api/v1/notification : 전체 알림 및 내 알림 조회
  • POST /api/v1/notification/open/{notificationId} : 알림 읽음 상태로 변환

작업 사항

클럽 생성, 새로운 모임 회원 가입, 모임의 새로운 일정 등록이 일어나면 알림을 발생시키는 기능 개발
image

리뷰 요청 사항

보시고 궁금하신점은 언제든지 말씀해주세요!

@JaeyoungChoi98 JaeyoungChoi98 added the Feat 새로운 기능 추가 label Feb 3, 2024
@JaeyoungChoi98 JaeyoungChoi98 self-assigned this Feb 3, 2024
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "member_id")
private Member member;

Copy link
Member

Choose a reason for hiding this comment

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

읽음 여부가 컬럼으로 있으면 좋을꺼 같아요!

@GoonManDoo
Copy link
Contributor

고생하셨습니다 !

Menhez Team added 4 commits February 16, 2024 14:50
# Conflicts:
#	src/main/java/com/sideteam/groupsaver/domain/member/domain/Member.java
# Conflicts:
#	src/main/java/com/sideteam/groupsaver/domain/club/service/ClubService.java
Jaewon-pro
Jaewon-pro previously approved these changes Feb 22, 2024
Copy link
Member

@Jaewon-pro Jaewon-pro left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

private List<NotificationResponse> publicNoti;
private List<NotificationResponse> privateNoti;

public static NotificationListResponse of(List<Notification> notificationList) {
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
Member

Choose a reason for hiding this comment

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

notificationList처럼 변수이름에 타입을 적는 것보다는 notifications처럼 복수형으로 적으면 좋을 것 같아요.
그리고 notification 객체에 isType(NotificationType) 함수를 만들고,
스트림을 사용한다면, filter로 간단하게 나타낼 수 있을것 같아요

그리고 NotificationType 클래스 자체가 알림을 나타내는 것이나 PUBLIC 뒤에 NOTI는 없으면 더 간단해질 것 같아요

  List<NotificatinoResponse> publicNotis = notifications.stream()
  .filter(notification -> notification.isType(NotificationType.PUBLIC)
  .toList();
... // 이후 privateNotis

아니면, 이 로직 자체를 NotificationListResponse 클래스에서 static of 정적 생성자 패턴으로 옮겨도 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

List<NotificatinoResponse> publicNotis = notifications.stream() .filter(notification -> notification.isType(NotificationType.PUBLIC) .toList(); ... // 이후 privateNotis
이 로직이 그런 public, private 한번씩 일어나는건가요?

그렇다면 전체조회가 두번일어나게 될텐데 한번 필터링 할때 하나는 public 나머지는 private로 한번에 필터링하면 좋을거 같은데 지금 생각나는건 for문으로 if문을 사용해서 걸러주는거 밖에 생각안나더라구요

Copy link
Member

Choose a reason for hiding this comment

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

네, 전에 작성해주신 것처럼 전체로 1번만 조회하고 was에서 걸러 주는 방식이 좋은거 같아요!
제가 말씀드린 부분은 로직은 같은데, 클린 코드적인 관점에서 스트림으로 리팩토링 해보시면 어떠실까해서 말씀드린 거였습니다.

public void createNewSchedule(long clubId, long scheduleId, String image, Long creatorId) {
List<Member> clubMembers = clubMemberRepository.findAllMembersExceptCreatorByClubId(clubId, creatorId);
notificationRepository.saveAll(clubMembers.stream().map(clubMember ->
Notification.of(NotificationRemoteType.SCHEDULE.getMessage(), null, image, scheduleId, NotificationRemoteType.SCHEDULE, clubMember)
Copy link
Member

Choose a reason for hiding this comment

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

여기도 Nofication.of에서 Type, id, member만 다르고, 이후로 반복되는 부분이 많은데, 보시고 따로 함수를 분리하면 더 깔끔해질 것 같아요!

private final String remoteType;
private final String message;

NotificationRemoteType(NotificationType type, String remoteType) {
Copy link
Member

Choose a reason for hiding this comment

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

혹시 message에 null을 넣는 이 생성자는 없어도 되지 않나요?


@EnableScheduling
Copy link
Member

Choose a reason for hiding this comment

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

요거는 SchedulingConfig에 보시면 있어서 이 파일에 넣은 어노테이션을 지우시면 될것 같아요

Copy link
Member

@Jaewon-pro Jaewon-pro left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

@JaeyoungChoi98 JaeyoungChoi98 merged commit 4ed75c8 into dev Mar 4, 2024
1 check passed
@JaeyoungChoi98 JaeyoungChoi98 deleted the Feat/#53/noti branch March 4, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feat 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants