Skip to content

fix: remove incorrect cents-to-dollars conversion in notification email renderer#25

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1777560275-fix-notification-amount
Open

fix: remove incorrect cents-to-dollars conversion in notification email renderer#25
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1777560275-fix-notification-amount

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Apr 30, 2026

Summary

Root Cause: The FormatCurrency method in NotificationRenderer was dividing the amount by 100 (amount / 100m), based on an incorrect assumption that OrderPlacedEvent.TotalAmount was transmitted in cents. In reality, the shared OrderPlacedEvent contract and the HTTP DTO both define TotalAmount as a decimal in dollars. This caused a $149.99 order to display as $1.50 in the email preview (149.99 / 100 = 1.4999 → rounded to $1.50).

Fix:

  1. NotificationRenderer.cs — Removed the erroneous / 100m division in FormatCurrency. The amount is now formatted directly as received.
  2. Notification.API.csproj — Fixed incorrect relative paths to Shared.Contracts and Shared.Infrastructure project references (../../Shared../../../Shared), which was causing build failures (MSB9008).

Before (bug) — $149.99 order shows as $1.50

Before fix

After (fix) — $149.99 order correctly shows as $149.99

After fix

Review & Testing Checklist for Human

  • Verify the FormatCurrency change doesn't affect any other callers — confirm no other code path sends amounts in cents
  • Run the notification service locally and POST a test order event to confirm the email preview shows the correct amount
  • Verify the csproj path fix doesn't break Docker Compose or CI builds

Test Plan

  1. Run the notification service: cd src/Services/Notification/Notification.API && dotnet run --urls "http://localhost:5005"
  2. POST: curl -X POST http://localhost:5005/api/notification/events/order-placed -H "Content-Type: application/json" -d '{"orderId": "11111111-1111-1111-1111-111111111111", "customerId": "22222222-2222-2222-2222-222222222222", "totalAmount": 149.99, "placedAt": "2026-03-17T12:00:00Z"}'
  3. Open the returned PreviewUrl — the total should show $149.99

Notes

  • The misleading comment about "cents (integer representation)" has been removed along with the division.
  • The csproj path fix is a prerequisite for the Notification.API project to build at all when Shared projects are referenced.

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/70ea65544da44d968663f241a21529c5


Open in Devin Review

…derer

The FormatCurrency method was dividing the amount by 100, assuming the
OrderPlacedEvent.TotalAmount was in cents. However, the shared contract
and HTTP DTO both transmit the amount in dollars (decimal). This caused
a $149.99 order to display as $1.50 in the email preview.

Also fixes the Shared project reference paths in Notification.API.csproj
(../../Shared -> ../../../Shared) to resolve build failures.
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

Open in Devin Review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants