Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the email verification logic by introducing an email client that sends a verification code to a provided email.
- Added a new email_client module with functions to send emails via an HTTP client.
- Updated the domain module by deriving Clone for SubscriberEmail.
- Modified Cargo.toml to include the secrecy crate required by the email client.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rook/src/lib.rs | Added email_client module to the public API. |
| rook/src/email_client.rs | Introduced the EmailClient for sending emails. |
| rook/src/domain/subscriber_email.rs | Derived Clone for SubscriberEmail to support cloning through the system. |
| rook/Cargo.toml | Added secrecy dependency needed for secure token handling. |
| ) -> Self { | ||
| let http_client = Client::builder().timeout(timeout).build().unwrap(); | ||
| Self { | ||
| http_client, | ||
| base_url, | ||
| sender, | ||
| authorization_token, | ||
| } |
There was a problem hiding this comment.
Using unwrap() here can cause a panic if the HTTP client cannot be built. It is recommended to handle this error gracefully, such as by propagating the error using Result.
| ) -> Self { | |
| let http_client = Client::builder().timeout(timeout).build().unwrap(); | |
| Self { | |
| http_client, | |
| base_url, | |
| sender, | |
| authorization_token, | |
| } | |
| ) -> Result<Self, reqwest::Error> { | |
| let http_client = Client::builder().timeout(timeout).build()?; | |
| Ok(Self { | |
| http_client, | |
| base_url, | |
| sender, | |
| authorization_token, | |
| }) |
|
?? Blunder 😵💫 ❌ Failure DetailsSee the full battle logs here: View Logs Time to rethink your next move! |
|
?? Blunder 😵💫 ❌ Failure DetailsSee the full battle logs here: View Logs Time to rethink your next move! |
1 similar comment
|
?? Blunder 😵💫 ❌ Failure DetailsSee the full battle logs here: View Logs Time to rethink your next move! |
There was a problem hiding this comment.
Pull Request Overview
This PR implements email verification by sending a one-time code and tracking its status in the database until verification completes.
- Adds an
EmailClientmodule to send emails via an HTTP API. - Introduces domain types and services (
EmailVerification,EmailVerificationStore,EmailVerificationService) for generating, storing, and verifying codes. - Updates the
subscriberstable schema with new columns for storing verification codes and adds necessary dependencies.
Reviewed Changes
Copilot reviewed 10 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rook/src/lib.rs | Exported the new email_client module. |
| rook/src/email_client.rs | Implemented EmailClient for sending messages. |
| rook/src/domain/subscriber_email.rs | Added Clone derive to SubscriberEmail. |
| rook/src/domain/mod.rs | Registered new email_verification and email_verification_service modules. |
| rook/src/domain/email_verification_service.rs | Created EmailVerificationService for orchestration. |
| rook/src/domain/email_verification.rs | Defined EmailVerification logic and EmailVerificationStore. |
| rook/migrations/20250527160932_alter_verify_code.sql | Added DB columns and index for verification support. |
| rook/Cargo.toml | Added secrecy, chrono, and rand dependencies. |
Files not reviewed (4)
- rook/.sqlx/query-03a8230734da8b966adda4eadf99bd665be8d2cbba1b71c55677f83ec71969f5.json: Language not supported
- rook/.sqlx/query-37bd35a93ead8ace791a16f9fd49a011438edf41f2967985de1ec9b88518725d.json: Language not supported
- rook/.sqlx/query-5079ceb0a5dadbc9f948da81be241398b81e775b4bbb9c5014d7c6c6ce8ed3e7.json: Language not supported
- rook/.sqlx/query-bd09aa624890b7c599d0a89016d941d3d0b76ecd2d3c603c4814182c5e202be2.json: Language not supported
Comments suppressed due to low confidence (2)
rook/src/domain/email_verification_service.rs:20
- The email is stored before validating its format. Consider validating the email with SubscriberEmail::new before creating the verification record to avoid persisting invalid addresses.
.store.create_verification(email.clone()).await
rook/src/email_client.rs:6
- There are no tests covering the EmailClient integration and error scenarios. Consider adding unit tests or mocking the HTTP client to validate request construction and error handling.
#[derive(Clone)]
| .send() | ||
| .await | ||
| .map_err(|e| format!("Failed to send email: {}", e))? | ||
| .error_for_status(); |
There was a problem hiding this comment.
The Result from .error_for_status() is ignored, so non-success HTTP status codes will not be propagated. Append "?" or handle the error to ensure failures are surfaced.
| .error_for_status(); | |
| .error_for_status() | |
| .map_err(|e| format!("Email request failed with HTTP error: {}", e))?; |
| pub fn new(email: String) -> Self { | ||
| let code = generate_verification_code(); | ||
| let created_at = Utc::now(); | ||
| let expires_at = created_at + Duration::minutes(10); // 10분 후 만료 |
There was a problem hiding this comment.
[nitpick] The expiration duration (10 minutes) is hardcoded in multiple places. Extract this value into a named constant to ensure consistency and ease future changes.
| let expires_at = created_at + Duration::minutes(10); // 10분 후 만료 | |
| let expires_at = created_at + Duration::minutes(EMAIL_VERIFICATION_EXPIRATION_MINUTES); // 10분 후 만료 |
|
흑 코파일럿 리뷰 요청한거 까먹고 merge 해버림. 리팩터링할 기회가 있겠지,, |
♟️ What’s this PR about?
POST /check 에 email 이 포함되어 오면 해당 이메일로 verify code를 보내고 이 verify code로 GET 요청이 오기전까진 해당 이메일로 메일을 보내지 않는다.
🔗 Related Issues / PRs
close: #72