-
Notifications
You must be signed in to change notification settings - Fork 27
Change reply-to on emails #8449
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request modifies the email functionality in several files. In Changes
Possibly related PRs
Suggested labels
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
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. Did you have a way to test it?
app/mail/DefaultMails.scala
Outdated
recipients = List("[email protected]", userEmail) | ||
recipients = List("[email protected]"), | ||
ccRecipients = List(userEmail), | ||
replyTo = List(userEmail, "[email protected]") |
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.
@normanrz Shouldn't this be webknossos-support
A T scalableminds.com
?
… into change-reply-to
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.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/javascripts/admin/organization/pricing_plan_utils.ts (1)
22-22
: Consider rephrasing "Up to Unlimited Users"The phrase "Up to Unlimited Users" is logically confusing as "unlimited" already implies no upper bound. Consider alternative phrasing such as "Unlimited Users Available" or simply "Unlimited Users" if that's what the plan offers.
- "Up to Unlimited Users", + "Unlimited Users Available",app/views/mail/upgradePricingPlanStorage.scala.html (1)
5-5
: Fix grammatical error in thank you messageThere's a missing word in the thank you message.
-<p>Thank you for interest in adding more storage to your WEBKNOSSOS plan. Our sales team will contact you shortly.</p> +<p>Thank you for your interest in adding more storage to your WEBKNOSSOS plan. Our sales team will contact you shortly.</p>app/views/mail/upgradePricingPlanUsers.scala.html (1)
5-5
: There appears to be a typo in the message.The phrase "Thank you for interest in..." is missing the word "your" which makes the sentence grammatically incorrect.
-<p>Thank you for interest in adding more users to your WEBKNOSSOS plan. Our sales team will contact you shortly.</p> +<p>Thank you for your interest in adding more users to your WEBKNOSSOS plan. Our sales team will contact you shortly.</p>app/views/mail/upgradePricingPlanToPower.scala.html (1)
1-1
: Parameter addition looks good, but there's an inconsistent space.The parameter declaration has an extra space after the opening parenthesis that isn't present in other templates.
-@( name: String, additionalFooter: String, organizationName: String) +@(name: String, additionalFooter: String, organizationName: String)app/mail/DefaultMails.scala (1)
142-144
: Verify the order of email addresses in the reply-to field.I notice that across all the upgrade emails, the order is
replyTo = List(userEmail, supportEmail)
which is different from the help email that usesreplyTo = List(supportEmail, userEmail)
. While this likely doesn't affect functionality, consider whether there should be a consistent ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/controllers/OrganizationController.scala
(4 hunks)app/mail/DefaultMails.scala
(2 hunks)app/utils/WkConf.scala
(1 hunks)app/views/mail/extendPricingPlan.scala.html
(1 hunks)app/views/mail/upgradePricingPlanRequest.scala.html
(0 hunks)app/views/mail/upgradePricingPlanStorage.scala.html
(1 hunks)app/views/mail/upgradePricingPlanToPower.scala.html
(1 hunks)app/views/mail/upgradePricingPlanToTeam.scala.html
(1 hunks)app/views/mail/upgradePricingPlanUsers.scala.html
(1 hunks)frontend/javascripts/admin/organization/pricing_plan_utils.ts
(1 hunks)
💤 Files with no reviewable changes (1)
- app/views/mail/upgradePricingPlanRequest.scala.html
🧰 Additional context used
🧬 Code Definitions (2)
app/controllers/OrganizationController.scala (1)
app/mail/DefaultMails.scala (3) (3)
extendPricingPlanMail
(107-117)upgradePricingPlanUsersMail
(137-147)upgradePricingPlanStorageMail
(147-161)
app/mail/DefaultMails.scala (2)
app/mail/Mail.scala (1) (1)
app/utils/WkConf.scala (2) (2)
additionalFooter
(173-173)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (18)
app/utils/WkConf.scala (1)
171-171
: Adding support email configuration looks good.This addition allows for a configurable support email address which is a good practice instead of hardcoding it, aligning with the overall configuration pattern used in this file.
app/views/mail/extendPricingPlan.scala.html (2)
1-1
: LGTM: Parameter added for organization nameAdding the organization name parameter enhances email personalization.
5-6
: Email text improvements look goodThe updated message is more concise and professional. Including the organization name provides better context for the support team.
app/views/mail/upgradePricingPlanStorage.scala.html (2)
1-1
: LGTM: Parameter added for organization nameAdding the organization name parameter enhances email personalization.
7-7
: LGTM: Organization name display addedAdding the organization name to the email provides better context for the support team.
app/views/mail/upgradePricingPlanToTeam.scala.html (2)
1-1
: Parameter addition looks good.The addition of
organizationName
parameter aligns with the PR objective to enhance email functionality. This allows for better personalization of emails.
5-6
: Improved message clarity and added organization context.Good changes:
- The updated message has a more conversational tone, focusing on the customer's "interest" rather than a "request"
- Adding the organization name provides important context for both the recipient and the support team
These changes will make email communication more effective.
app/views/mail/upgradePricingPlanUsers.scala.html (2)
1-1
: Parameter addition looks good.Adding the
organizationName
parameter matches the changes in other email templates, ensuring consistency across the codebase.
7-7
: Organization information addition looks good.Adding the organization name to the email body is consistent with other templates and provides important context.
app/views/mail/upgradePricingPlanToPower.scala.html (1)
5-6
: Improved message clarity and added organization context.The updated message and addition of the organization name are consistent with other templates and provide better context for both the recipient and the support team.
app/controllers/OrganizationController.scala (4)
194-194
: Email sending now includes organization name.The update correctly passes the organization name to the email template, aligning with changes in the
DefaultMails
class.
211-211
: Simplified mail function call with organization name.The code now effectively passes the organization name to the appropriate mail function based on the requested plan.
222-222
: Email for additional users now includes organization name.The update correctly passes the organization name to the email template for upgrades related to additional users.
233-233
: Email for additional storage now includes organization name.The update correctly passes the organization name to the email template for upgrades related to additional storage.
app/mail/DefaultMails.scala (4)
16-16
: Good addition of the supportEmail configuration.This change enables dynamic configuration of the support email address through the application configuration file, which promotes flexibility and maintainability.
102-104
: Well-structured email routing for help requests.The new email structure ensures support requests are properly routed:
- Primary recipient is the support email
- User is CCed to maintain visibility
- Reply-to includes both addresses to facilitate multi-party conversation
This improves the support workflow by keeping all relevant parties in the communication loop.
107-115
: Improved email configuration for pricing plan extension.The changes enhance the email functionality by:
- Adding organization name personalization to the email template
- Directing the primary email to the user instead of support
- CCing support for visibility
- Setting up replies to reach both parties
Consider whether the support email should be using the suggested format mentioned in the previous review.
What is the best practice for support email address formats for SaaS products?
117-125
: Consistent implementation across all upgrade email methods.The changes maintain a consistent pattern across all upgrade notification emails:
- Adding organization name parameter for personalization
- User as primary recipient with support CCed
- Reply-to configuration that includes both addresses
This consistency is excellent for maintainability and user experience.
Also applies to: 127-135, 137-145, 147-159
@normanrz I think you said this didn’t work for you as expected, right? Is there a smaller subset of this PR that we should merge anyway? Or should we close this? |
Change reply-to in emails to be more useful