Fix email notification reply to #603
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NotificationService
participant EmailService
User->>NotificationService: Trigger email notification
NotificationService->>EmailService: Prepare email using formatSubmissionData(createLinks=false)
EmailService-->>NotificationService: Email prepared with creator's email
NotificationService-->>User: Email sent confirmation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
api/app/Notifications/Forms/FormEmailNotification.php (2)
64-72: Consider adding type declaration for $createLinks parameterThe implementation looks good and provides flexibility for link creation. Consider adding a type declaration for better code clarity.
-private function formatSubmissionData($createLinks = true): array +private function formatSubmissionData(bool $createLinks = true): array
119-119: Document the createLinks parameter usage patternThe usage of
formatSubmissionData(false)for subjects andformatSubmissionData()for content is correct, but it would be helpful to document this pattern.Add a comment to the formatSubmissionData method:
+ /** + * Format the submission data for display + * + * @param bool $createLinks When true, URLs in the data will be converted to clickable links. + * Set to false when formatting data for subjects or reply-to addresses. + * @return array + */ private function formatSubmissionData($createLinks = true): arrayAlso applies to: 151-151, 161-161
api/tests/Feature/Forms/EmailNotificationTest.php (2)
168-197: Consider adding more assertions for comprehensive testing.While the test correctly verifies the reply-to address, consider adding assertions for:
- The mention syntax parsing
- Edge cases where the mentioned field is empty or invalid
Example additions:
// Add these assertions expect($renderedMail->replyTo)->toHaveCount(1); expect($formData[$emailProperty['id']])->toBe('[email protected]');
199-228: Improve test clarity and robustness.A few suggestions to enhance this test:
- The form data includes an email that isn't used in the test, which might be confusing
- The creator's email could be verified more explicitly
Consider this refactor:
- $formData = [ - $emailProperty['id'] => '[email protected]', - ]; + $formData = FormSubmissionDataFactory::generateSubmissionData($form); + + // Explicitly verify the creator's email before using it + expect($form->creator->email)->not->toBeNull();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- api/app/Notifications/Forms/FormEmailNotification.php (5 hunks)
- api/tests/Feature/Forms/EmailNotificationTest.php (2 hunks)
🔇 Additional comments (2)
api/app/Notifications/Forms/FormEmailNotification.php (1)
55-55: LGTM: Improved reply-to email handlingUsing the form creator's email as the fallback ensures there's always a valid reply-to address, which is a robust improvement.
api/tests/Feature/Forms/EmailNotificationTest.php (1)
29-29: LGTM! Good test coverage addition.The new assertion properly verifies that the reply-to address is correctly set in the email notification.
| if ($replyTo) { | ||
| $parsedReplyTo = $this->parseReplyTo($replyTo); | ||
| if ($parsedReplyTo && $this->validateEmail($parsedReplyTo)) { | ||
| return $parsedReplyTo; | ||
| } | ||
| } | ||
|
|
||
| return $this->getRespondentEmail() ?? $default; | ||
| return $default; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding logging for invalid reply-to addresses
While the reply-to handling is improved, it would be helpful to log when an invalid reply-to address is provided to assist with debugging configuration issues.
private function getReplyToEmail($default): string
{
$replyTo = $this->integrationData->reply_to ?? null;
if ($replyTo) {
$parsedReplyTo = $this->parseReplyTo($replyTo);
- if ($parsedReplyTo && $this->validateEmail($parsedReplyTo)) {
+ if ($parsedReplyTo && $this->validateEmail($parsedReplyTo)) {
return $parsedReplyTo;
+ } elseif ($parsedReplyTo) {
+ \Log::warning('Invalid reply-to email configured', [
+ 'form_id' => $this->event->form->id,
+ 'configured_reply_to' => $replyTo,
+ 'parsed_reply_to' => $parsedReplyTo
+ ]);
}
}
return $default;
}Also applies to: 112-113
Summary by CodeRabbit
New Features
Bug Fixes
Tests