Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the functionality to send email notifications for link checking results with a retry mechanism. Key updates include:
- Integration of a new email sending API (send_email_with_retry) across multiple modules.
- Refactoring of the link check summary to include detailed results and generate structured email content.
- Addition of tests to verify both immediate and retried email sending behavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rook/src/main.rs | Updated to use the new send_email_with_retry API and pass along email client and subscriber email parameters. |
| rook/src/link_checker/scheduler.rs | Introduced LinkCheckSummary and supporting types to aggregate link check results and trigger email reports. |
| rook/src/email_client.rs | Added the send_email_with_retry function with retry logic and updated tests for email sending. |
| rook/src/domain/email_verification_service.rs | Updated to utilize send_email_with_retry when sending verification emails. |
Comments suppressed due to low confidence (1)
rook/src/email_client.rs:84
- [nitpick] It might be beneficial to log each retry attempt within send_email_with_retry to improve observability for email sending issues.
attempt += 1;
| } | ||
| email_client | ||
| .send_email( | ||
| .send_email_with_retry( |
There was a problem hiding this comment.
Consider externalizing the retry parameters (e.g., 3 retries and 60 seconds delay) into configuration so they can be adjusted without code changes.
There was a problem hiding this comment.
일단 전부 3번 재시도, 그리고 60초 간격으로 설정을 해두었는데, 이 값을 configuration에 넣자니 "굳이"인가 싶다. 그렇다고 매 send_email_with_retry 함수마다 이 값을 넣고 있자니 그것도 마음에 드는 형태는 아니다.
한마디로 애매함.
| let handle = tokio::spawn(async move { | ||
| let result = check_link(&link.url).await; | ||
| (link, result) | ||
| (link.url, result) |
There was a problem hiding this comment.
[nitpick] Review whether dropping additional link context (such as file path or line number) in favor of just using link.url is intended; if detailed debugging information is useful, consider preserving some of that context in the summary.
♟️ What’s this PR about?
링크 체크를 위한 이메일이 등록되면 아래 이미지와 같이 처음 메일이 전송되고,
매 주기마다 아래와 같이 체크 결과에 대해 메일이 전송된다.
고민이 되는 부분은 현재
link_check/scheduler.rs에 이런 이메일 형태 전송 형태로 변경하는 로직이 포함되어 있는데, 이 로직이 해당 파일에 있는게 적절한 지 고민이 된다.link_check/sse.rs에서도 카운터 증가하는 로직과 sse로 전송하는 로직이sse.rs한 파일안에 있어서, 이번에도 비슷하게 구현하였는데, 적절한 위치(어울리고 유지보수하기 편한)를 찾게 된다면 그 곳으로 옮기도록 하자.🔗 Related Issues / PRs
close: #123