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

Feature/#12 - 알림 구현 #313

Merged
merged 36 commits into from
Dec 1, 2024
Merged

Feature/#12 - 알림 구현 #313

merged 36 commits into from
Dec 1, 2024

Conversation

swkim12345
Copy link
Collaborator

@swkim12345 swkim12345 commented Dec 1, 2024

close #12

✅ 작업 내용

  • 알림 엔드포인트 CRUD
  • afterInsert - 분봉 구독
  • web-push 이용한 web push 구현

이슈사항

✍ 궁금한 점

  • 현재 이메일 구현이 되어 있지 않은 상태입니다. 추가로 구현할까 생각중인데, 이것은 게스트 로그인에 한정에서는 안 되게 할 까 생각중입니다.

😎 체크 사항

  • label 설정 확인
  • 브랜치 방향 확인

demian-m00n and others added 30 commits November 19, 2024 23:18
@xjfcnfw3
Copy link
Collaborator

xjfcnfw3 commented Dec 1, 2024

남은 시간이 얼마 남지 않아 이메일은 굳이 안해도 된다고 생각합니다

Copy link
Collaborator

@baegyeong baegyeong left a comment

Choose a reason for hiding this comment

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

와우..👍 수고하셨습니다! 저도 남은 기능 끝내고 어서 알림으로 넘어가야겠네욥..

Comment on lines +33 to +35
} else {
if (alarm.targetPrice && alarm.targetPrice >= entity.open) {
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

else가 없어도 될 것 같아요!

endpoint: string;

@Column({ type: 'text' })
p256dh: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 이건 뭔가요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아... 이거 명세 작성해야해요. 이건 주춤주춤 노션 페이지에 작성해놓을게요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

web-push를 구현할 때 필요한 키값입니다! 이거 env도 같이 작성해놓을게요

@baegyeong
Copy link
Collaborator

남은 시간이 얼마 남지 않아 이메일은 굳이 안해도 된다고 생각합니다

저도 동의합니다!

Copy link
Collaborator

@xjfcnfw3 xjfcnfw3 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 👍 👍

@@ -55,14 +57,17 @@ export class StockLiveDataSubscriber
changeRate: change,
volume: volume,
} = updatedStockLiveData;

this.stockGateway.onUpdateStock(stockId, price, change, volume);
Copy link
Collaborator

Choose a reason for hiding this comment

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

에러가 발생했을 때 몇 번 더 기회를 주는건 어떤가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오... 좋은 거 같습니다! 이거 3번정도 기회 주는 방향으로 리팩토링하고, 차후 pr에 올릴게요!

else throw new NotFoundException('등록된 알림을 찾을 수 없습니다.');
}

async update(id: number, updateData: AlarmRequest) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

유저가 주인인지 확인하는 과정이 필요하다고 생각합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오 감사합니다! 이거 추가해서 올릴게요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment