-
Notifications
You must be signed in to change notification settings - Fork 1
[FEAT] #630 솝탬프 박수 관련 푸시알림 구현 #631
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
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.
알림 트리거 관련 이슈 사항이 있어보여서 빠른 반영 부탁드립니다!
| if (total == 1) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), missionTitle, nickname)); | ||
| } | ||
|
|
||
| if (total == 100) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), 100, missionTitle, ownerName, ownerPart, | ||
| nickname)); | ||
| } else if (total == 500) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), 500, missionTitle, ownerName, ownerPart, | ||
| nickname)); | ||
| } | ||
|
|
||
| if (total >= 1000 && total <= 10000 && total % 1000 == 0) { | ||
| send(ClapRequest.ClapAlarmRequest.of(total, missionTitle, nickname)); | ||
| } | ||
| } |
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.
지금 저희가 bulk로도 clapCount를 받아서 정확히 1, n00을 도달하지 않고 지나칠 수 있습니다! 그래서 예를들어 98에서 +3(=101)이 되면 100을 지나쳤지만 발송되지 않습니다.
이벤트에 oldTotal, newTotal 포함 → 구간 교차로 판정:
// 첫 박수
if (oldTotal < 1 && newTotal >= 1) { ... }
// 100 / 500
if (oldTotal < 100 && newTotal >= 100) { ... }
if (oldTotal < 500 && newTotal >= 500) { ... }
// 1000 단위
for (int k = 1000; k <= 10000; k += 1000) {
if (oldTotal < k && newTotal >= k) { ... }
}
이런식으로 구현해야할 것 같아요!
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.
임계값(1, 100/500, 1000 단위)을 지나쳐도 알림이 전송되도록 개선했습니다
리뷰를 반영하다가 든 생각인데, Clap 엔티티에 낙관적 락(@Version)이 걸려있다 하더라도
혜린님도 코멘트 주셨듯이 임계치 판정이 스탬프 누적 합계 (Stamp.clapCount) 기준입니다
그래서 트랜잭션 초반에 oldClapTotal = stamp.getClapCount()를 읽고
newClapTotal = old + applied 로 계산해서 이벤트로 만들게 되면,
둘 다 99→100으로 판단해서 리스너에서 100 푸시가 2번 나갈 수 있는 엣지케이스를 생각해봤습니다
DB단에 아래처럼 유니크 테이블을 걸어줘서
clap_milestone_hit(stamp_id, milestone
INSERT 성공일 때만 푸시하는 가드를 걸어주는건 어떨까요 ??
@jher235 재헌님도 의견 부탁드립니다 🙇🏻♂️
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.
ClapMilestone 가드 추가해줬습니다
대신에 아래 쿼리문으로 prod 환경도 동일하게 구성해야할거같아요 (dev에는 public에 생성해뒀습니다)
CREATE TABLE IF NOT EXISTS clap_milestone_hit (
stamp_id BIGINT NOT NULL,
milestone INT NOT NULL,
created_at TIMESTAMP NOT NULL DEFAULT now(),
PRIMARY KEY (stamp_id, milestone)
);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 String baseURI; | ||
|
|
||
| @Async | ||
| @Transactional(value = Transactional.TxType.REQUIRES_NEW) |
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.
현재 트랜잭션 애노테이션이 jakarta.transaction.Transactional 입니다.
스프링 표준과 섞이면 행위가 헷갈릴 수 있어서
org.springframework.transaction.annotation.Transactional
@transactional(propagation = Propagation.REQUIRES_NEW)
로 바꾸면 좋을 것 같아요!
# Conflicts: # src/main/java/org/sopt/app/application/stamp/ClapService.java
| ON CONFLICT (stamp_id, milestone) DO NOTHING | ||
| """; | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) |
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.
ClapEventListener.onClap가 @transactional(REQUIRES_NEW) 이고,
ClapMilestoneGuard.tryMark도 @transactional(REQUIRES_NEW)라서 tryMark() 호출 때 또 다른 신규 트랜잭션을 열고, 리스너 트랜잭션을 잠시 중단하게 될텐데 딥링크 발송(send)에서 예외가 나도 마킹은 커밋돼서 재시도 로직 없으면 알림이 영영 재발송되지 않을 것 같아요.
마킹의 목적이 발송인 만큼 발송 성공 시에만 마킹을 커밋해야 발송 실패 시 롤백되어 다음 기회에 재시도 가능할 듯합니다!
그래서 마킹과 알림 발송을 같은 트랜잭션에서 처리할 수 있도록
ClapMilestoneGuard.tryMark는 기본 @transactional(REQUIRED)로 둬서 리스너 트랜잭션에 참여시키면 좋을 것 같아요~
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.
고생하셨습니다..! 새벽에 다들 고생하셨네요 👍 👍
| @Async | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT) | ||
| public void onClap(ClapEvent event) { | ||
| final int oldClapTotal = event.getOldClapTotal(); | ||
| final int newClapTotal = event.getNewClapTotal(); | ||
|
|
||
| Long missionId = stampService.getMissionIdByStampId(event.getStampId()); | ||
| String missionTitle = missionService.getMissionTitleById(missionId); | ||
|
|
||
| val ownerProfile = platformService.getPlatformUserInfoResponse(event.getOwnerUserId()); | ||
| String ownerName = ownerProfile.name(); | ||
| String ownerPart = Optional.ofNullable(ownerProfile.getLatestActivity()) | ||
| .map(PlatformUserInfoResponse.SoptActivities::part) | ||
| .orElseThrow(() -> new NotFoundException(ErrorCode.USER_PART_NOT_FOUND)); | ||
| String nickname = soptampUserFinder.findById(event.getOwnerUserId()).getNickname(); | ||
|
|
||
| if (crossed(oldClapTotal, newClapTotal, 1) && clapMilestoneGuard.tryMark(event.getStampId(), 1)) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), missionTitle, nickname)); | ||
| } | ||
|
|
||
| if (crossed(oldClapTotal, newClapTotal, 100) | ||
| && clapMilestoneGuard.tryMark(event.getStampId(), 100)) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), 100, missionTitle, ownerName, ownerPart, nickname)); | ||
| } else if (crossed(oldClapTotal, newClapTotal, 500) | ||
| && clapMilestoneGuard.tryMark(event.getStampId(), 500)) { | ||
| send(ClapRequest.ClapAlarmRequest.of(event.getOwnerUserId(), 500, missionTitle, ownerName, ownerPart, nickname)); | ||
| } | ||
|
|
||
| // 한 번에 여러 구간(2000, 3000)을 넘어도 낮은 것만 처리 | ||
| for (int k = 1000; k <= 10000; k += 1000) { | ||
| if (crossed(oldClapTotal, newClapTotal, k) | ||
| && clapMilestoneGuard.tryMark(event.getStampId(), k)) { | ||
| send(ClapRequest.ClapAlarmRequest.of(k, missionTitle, nickname)); | ||
| break; | ||
| } | ||
| } | ||
| } |
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.
메세지 전송이 성공한 경우, early return 을 해줘도 좋을 것 같아요~
| private void send(ClapRequest.ClapAlarmRequest body) { | ||
| val entity = new HttpEntity<>(body, headersUtils.createHeadersForSend()); | ||
| restTemplate.exchange( | ||
| baseURI, | ||
| HttpMethod.POST, | ||
| entity, | ||
| PokeResponse.PokeAlarmStatusResponse.class | ||
| ); | ||
| } |
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.
알림 메세지의 응답 형식이 고정이면 공통적으로 사용되는 response를 만들어도 좋을 것 같네요 ~
Related issue 🛠
Work Description ✏️
To Reviewers 📢
먼저 현재 알림 구조가
External API활용해서 외부 알림 서버가 우리 앱으로 알림을 등록 해주는 형태, 즉"/api/v2/notification"엔드포인트에서 싱크만 해주는걸로 이해를 했어요 찌르기를 예시로 들면, 알림 발행이 직접 일어나지 않는걸로 생각했습니다PokeEventListener처럼 이벤트를 알림 서버로 발송을 요청해주는 비동기 리스너 구현하고,PokeAlarmRequest처럼 외부 알림 서버에 전달할 요청 페이로드를 설계하는 식으로 박수 관련 알림도 동일하게 구현해주었습니다알림
content에 들어갈 내용은 피그마에 은비님이 문서화해주신 공백, 줄바꿈 모두 맞춰서 작성했습니다또한 각 알림의 딥링크는 기존에 사용하고 있던
/api/v2/rank/detail?nickname={닉네임}형태로 생성해주도록 해서 바로가기를 눌렀을때, 다른 사람의 미션(또는 내 미션) 으로 리다이렉트 될 수 있도록 설계했는데 요구사항에 맞는지 확인 해주시면 감사하겠습니다 🙇🏻♂️+추가로 제가 현재 다른 사람의 미션 딥링크가
?nickname=%s그대로 들어가는데,만약에 닉네임 기획 요구상 공백이랑 특수문자가 포함된다면 추가로 URL 인코딩 작업이 필요할거같아서 이부분도 확인 부탁드립니다 !