-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor: mail module #294
base: develop
Are you sure you want to change the base?
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.
개선이 필요하다고 생각하고있었는데 엄청 적절하게 수정해주셨네요 역시 고수 ㄸㄸㄸ
추후에 메일 서비스가 다른곳에서도 쓰이게 된다면 lib 으로 빼보는것도 재밌을거같습니다.
지금은 굳이 그럴필요는 없을거같아요
홍보용으로 유저들에게 메일 보내는게 법적으로 문제는 없는지 한번 검토해보고
문제없으면 한번 매주 인기글같은거 정기적으로 보내는것도 해보죠
apps/api/src/mail/mail.service.ts
Outdated
@@ -0,0 +1,5 @@ | |||
export const MailServiceToken = Symbol('MailService'); |
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.
이거는 constants.ts 로 빼는게 좋을거같아요
변수 이름은 MAIL_SERVICE_TOKEN
처럼 하는게 좋을거같아요
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.
이거 생각을 좀 해봤는데 constants로 따로 빼는게 맞나 싶네요
mail service를 사용하려면 저 symbol이 필수 인데 분리하면 오히려 헷갈리지 않을까 싶네요
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.
그런가용 apps/api/src/mail/constants.ts 에 있는건 어떤가요?
apps/api/src/mail/mail.module.ts
Outdated
useClass: StibeeService, | ||
}, | ||
], | ||
exports: [MailServiceToken, UnsubscribeStibeeServiceToken], |
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.
확실히 구독 해지는 MailService 의 책임인거같아요
당장은 성능이슈가 있겠지만, 유지보수 생각하면 전송과 동시에 해지까지 같이 하는게 맞는거같습니다.
추후에 messagequeue로 해결해보죠
UnsubscribeStibeeServiceToken 은 export 하지 않는지 좋을거같아요
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.
이거 구독하고 전송하고 바로 해지하는걸로 테스트 해봤는데
스티비 측에서 전송중에 구독 해지하면 메일 전송을 안해버리네요
task queue를 도입하면 일정 시간 이후에 해지 요청하도록 해야할거 같습니다
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.
아 헐 그런문제가 ㄸㄸ;; await 하는 시점이 전송 완료가 아니라 전송 trigger 완료일뿐이군요...
그럼 전송완료를 알수있는 시점이 따로 없나요? 적당히 queue 에 던져놓고 기다려야되나요..?
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.
아니면 일단 settimeout....?
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.
헉 그렇네요 hmmmmmmmmmnmm 그냥 쌓아놓고 최대 인원다 채워지면 가장 오래 구독한사람꺼를 빼기..?
apps/api/src/mail/stibee.service.ts
Outdated
@Injectable() | ||
export default class StibeeService implements MailService, UnsubscribeStibeeService { | ||
constructor(private readonly configService: ConfigService) {} | ||
|
||
private accessToken = this.configService.get<string>('STIBEE_API_KEY'); | ||
|
||
async send(name: string, code: string, githubId: string) { | ||
async send(name: string, email: string, code: string, githubId: string) { |
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 send(email: string, payload: T)
이런 느낌으로 완전 모듈화해서 lib으로 빼면 좋겠네요
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.
LGTM~ 👍
- 상수는 대문자와 언더바로 네이밍을 정했기 때문에 수정
- 실제로 메일을 보내는 부분을 private 함수로 분리
해지 여러번했을때 스티비가 어떻게 응답하는지 알아보고 문제없으면 settimeout 으로 임시조치하고.. 얼른 다른 방법을 모색하는 방향으로 가시죠 |
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.
메일을 별도의 모듈로 분리했으면 테스트도 분리해야겠네요
바뀐점
바꾼이유
설명