Skip to content

fix: correct order total display in notification emails ($149.99 showing as $1.50)#20

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1776088243-fix-notification-currency-formatting
Open

fix: correct order total display in notification emails ($149.99 showing as $1.50)#20
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1776088243-fix-notification-currency-formatting

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

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

Summary

Order confirmation notification emails were displaying wrong totals after the microservice decomposition — a $149.99 order showed as $1.50 in the email preview.

Root cause: FormatCurrency() in NotificationRenderer.cs divided the amount by 100, assuming OrderPlacedEvent.TotalAmount was transmitted in cents. However, the OrderPlacedEvent contract defines TotalAmount as a decimal already representing dollars (e.g. 149.99). The erroneous / 100m division turned 149.99 into 1.4999, which then displayed as $1.50.

Fix: Removed the / 100m division and corrected the misleading comment.

Before (total shows ¤1.50):
before

After (total shows ¤149.99):
after

Note: The ¤ currency symbol in screenshots is due to the test server's locale; in production with an appropriate locale this renders as $.

Review & Testing Checklist for Human

  • Verify all producers of OrderPlacedEvent send TotalAmount in dollars, not cents — if any upstream service (e.g., Order service) actually sends cents, this fix would break that path. Check the Order service's event publishing code.
  • Check for the same bug in other services — other microservices that consume monetary amounts may have copied the same incorrect cents assumption during the decomposition.
  • Test end-to-end: Run the notification service locally, POST {"orderId": "11111111-1111-1111-1111-111111111111", "customerId": "22222222-2222-2222-2222-222222222222", "totalAmount": 149.99, "placedAt": "2026-03-17T12:00:00Z"} to http://localhost:5005/api/notification/events/order-placed, and confirm the preview shows $149.99. Also test with edge cases like 0.01, 999999.99, and 0.

Notes

  • FormatCurrency is private static and only called within NotificationRenderer, so the blast radius of this change is limited to notification email rendering.
  • ToString("C2") relies on the server's current culture for the currency symbol — this is a pre-existing behavior, not introduced by this PR.
  • No existing unit tests cover FormatCurrency; consider adding one to prevent regression.

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/4f9482b9dc0f4d579b44ddce90e737c0


Open with Devin

…il rendering

Root cause: FormatCurrency() in NotificationRenderer.cs divided the amount by 100,
assuming TotalAmount was in cents. However, OrderPlacedEvent.TotalAmount is a decimal
representing dollars (e.g. 149.99), not cents. The division caused 49.99 to display
as .50 in order confirmation emails.

Fix: Remove the erroneous '/ 100m' division and use the amount directly.
Also corrected the misleading comment that claimed the amount was in cents.
@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 2 additional findings.

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