[PM-26461] Send F2020 renewal email #6638
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Core/MailTemplates/Mjml/emails/Billing/Renewals/families-2020-renewal.mjml
Show resolved
Hide resolved
|
@sukhleenb Design review on ticket: https://bitwarden.atlassian.net/browse/PM-26461?focusedCommentId=119725 |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6638 +/- ##
==========================================
+ Coverage 53.10% 53.12% +0.02%
==========================================
Files 1903 1903
Lines 84877 84895 +18
Branches 7633 7633
==========================================
+ Hits 45071 45099 +28
+ Misses 38055 38045 -10
Partials 1751 1751 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cyprain-okeke
left a comment
cyprain-okeke
left a comment
There was a problem hiding this comment.
Looks Great. I just added a unblocking comments
| Lines = new StripeList<InvoiceLineItem> | ||
| { | ||
| Data = new List<InvoiceLineItem> { new() { Description = "Test Item" } } | ||
| Data = [new() { Description = "Test Item" }] |
There was a problem hiding this comment.
What extra line?
| Id = passwordManagerItemId, | ||
| Price = new Price { Id = families2019Plan.PasswordManager.StripePlanId } | ||
| }, | ||
|
|
| Lines = new StripeList<InvoiceLineItem> | ||
| { | ||
| Data = new List<InvoiceLineItem> { new() { Description = "Test Item" } } | ||
| Data = [new() { Description = "Test Item" }] |
There was a problem hiding this comment.
Same question here.
| Lines = new StripeList<InvoiceLineItem> | ||
| { | ||
| Data = new List<InvoiceLineItem> { new() { Description = "Test Item" } } | ||
| Data = [new() { Description = "Test Item" }] |
There was a problem hiding this comment.
And here.
|
Claude finished @amorask-bitwarden's task —— View job Code Review CompleteSummary of Changes Since Last ReviewThe PR has been updated with one commit since the previous Claude review:
Previous major issues flagged by Claude remain unaddressed:
Critical IssuesFinding 1: Hardcoded renewal timing remains inaccurateLocation: src/Core/MailTemplates/Mjml/emails/Billing/Renewals/families-2020-renewal.mjml:17 The hardcoded "15 days" text does not match when Stripe's Suggested alternativesOption 1 - Pass actual date: Option 2 - Generic language: Finding 2: Premium users receive misleading "Families" email subjectLocation: src/Billing/Services/Implementations/UpcomingInvoiceHandler.cs:567-578 The TODO comment acknowledges this is temporary, but premium users currently receive an email with subject "Your Bitwarden Families renewal is updating" which is confusing and unprofessional. This is a customer-facing bug that should be resolved before merge. Recommended fixCreate a proper premium renewal template OR parameterize the existing template: public class SubscriptionRenewalMailView : BaseMailView
{
public required string PlanName { get; set; }
public required string MonthlyRenewalPrice { get; set; }
}
public class SubscriptionRenewalMail : BaseMail<SubscriptionRenewalMailView>
{
public required string PlanName { get; set; }
public override string Subject => $"Your Bitwarden {PlanName} renewal is updating";
}Finding 3: Missing coverage for premium user email pathLocation: test/Billing.Test/Services/UpcomingInvoiceHandlerTests.cs No tests verify that Required test: Minor Issues & ImprovementsFinding 4: Hardcoded price ID string literalLocation: src/Billing/Services/Implementations/UpcomingInvoiceHandler.cs:271 The string Finding 5: Currency formatting may not match user's localeLocation: src/Billing/Services/Implementations/UpcomingInvoiceHandler.cs:556, 574 Both renewal email methods format prices using MonthlyRenewalPrice = (familiesPlan.PasswordManager.BasePrice / 12).ToString("C", new CultureInfo("en-US"))💭 This always displays USD formatting ( Finding 6: Email template has no unsubscribe/preference center linkLocation: src/Core/MailTemplates/Mjml/emails/Billing/Renewals/families-2020-renewal.mjml The email template lacks an unsubscribe or email preference management link. While billing/transactional emails may have different requirements than marketing emails, best practice (and some jurisdictions' laws) recommend providing preference controls even for transactional emails. Good Practices Observed
Action ItemsMust fix before merge:
Should fix before merge: Consider for follow-up: |
amorask-bitwarden
deleted the
billing/PM-26461/families-2020-renewal-email
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26461
📔 Objective
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes