[PM-28100] families 2019 email

[kdenney]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28100

📔 Objective

Families 2019 email for milestone 3.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Checkmarx One – Scan Summary & Details – df5c7ba1-3ab9-4fbb-bbf8-6c01d1c19e4d

Great job! No new security vulnerabilities introduced in this pull request

[claude]

Claude finished @kdenney's task —— View job

---

Code Review Complete

Summary of Changes Since Last Review

The PR has been updated to address the critical issue identified in the previous review. The author confirmed they would throw exceptions instead of logging warnings for coupon validation failures. This change has been implemented correctly.

---

Status of Previous Findings

Finding 1: Critical coupon validation timing issue ✅ FIXED

• Previously: Coupon validation occurred after permanent state changes (organization/subscription updates)
• Now: Exceptions are thrown immediately if coupon is invalid (lines 580, 585)
• State changes only occur within the try-catch block if validation passes
• Email is only sent after successful updates

Finding 2: Missing test coverage for coupon validation ⚠️ PARTIALLY ADDRESSED

• Test added at line 1040: Verifies happy path where coupon exists with valid PercentOff
• Still missing: Tests for null coupon and null PercentOff scenarios
• Note: These are now exceptional cases (throw exceptions), so may be acceptable to skip explicit tests if covered by integration testing

---

New Issues Found

Finding 1: Test coverage for exception paths is incomplete

The implementation now throws InvalidOperationException for two scenarios:

1. Coupon is null (line 580)
2. Coupon.PercentOff is null (line 585)

However, the test suite does not verify these exception-throwing behaviors. Consider adding:

Suggested test cases

[Fact]
public async Task HandleAsync_WhenMilestone3Enabled_AndCouponNotFound_ThrowsException()
{
    // Arrange: Setup with null coupon
    _stripeFacade.GetCoupon(CouponIDs.Milestone3SubscriptionDiscount).Returns((Coupon)null);
    
    // Act & Assert
    await Assert.ThrowsAsync<InvalidOperationException>(() => _sut.HandleAsync(parsedEvent));
}

[Fact]
public async Task HandleAsync_WhenMilestone3Enabled_AndCouponPercentOffNull_ThrowsException()
{
    // Arrange: Setup with coupon.PercentOff = null
    var coupon = new Coupon { PercentOff = null, Id = CouponIDs.Milestone3SubscriptionDiscount };
    _stripeFacade.GetCoupon(CouponIDs.Milestone3SubscriptionDiscount).Returns(coupon);
    
    // Act & Assert
    await Assert.ThrowsAsync<InvalidOperationException>(() => _sut.HandleAsync(parsedEvent));
}

Severity: ⚠️ Minor - Test coverage for error paths would improve maintainability

---

Finding 2: ADR 0024 compliance for nullable reference types

StripeFacade.cs and IStripeFacade.cs have #nullable disable at the top (line 2). The new GetCoupon method follows this pattern but doesn't align with ADR 0024's requirement to use .NET nullable reference types.

Severity: ♻️ Technical debt - Pre-existing issue, not introduced by this PR. Consider tracking for future migration.

---

Good Practices Observed

• Exception-first approach for validation ensures fail-fast behavior
• Comprehensive test coverage for happy paths and various subscription configurations
• Proper use of feature flags for gradual rollout
• Email templates follow existing patterns

---

Action Items

1. ⚠️ Optional: Add test coverage for exception-throwing scenarios in coupon validation
2. ℹ️ Note: Consider tracking nullable reference type migration for StripeFacade as technical debt

---

Conclusion

The critical issue from the previous review has been properly resolved. The implementation now validates the coupon before making any state changes and throws exceptions for invalid cases, which is the correct approach for this scenario.

The PR is ready for merge from a code review perspective. Test coverage for the exception paths would be nice to have but is not blocking.

[codecov]

Codecov Report

❌ Patch coverage is 81.57895% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.28%. Comparing base (1334ed8) to head (b6bded9).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...Services/Implementations/UpcomingInvoiceHandler.cs	83.87%	3 Missing and 2 partials ⚠️
...c/Billing/Services/Implementations/StripeFacade.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6645      +/-   ##
==========================================
+ Coverage   53.27%   53.28%   +0.01%     
==========================================
  Files        1906     1907       +1     
  Lines       84973    85010      +37     
  Branches     7639     7642       +3     
==========================================
+ Hits        45268    45298      +30     
- Misses      37952    37957       +5     
- Partials     1753     1755       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.