Skip to content

fix: remove incorrect cents-to-dollars conversion in notification currency formatting#21

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

fix: remove incorrect cents-to-dollars conversion in notification currency formatting#21
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1776088523-fix-notification-currency-format

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 amounts (e.g. a $149.99 order showed as $1.50).

Root cause: NotificationRenderer.FormatCurrency() divided the amount by 100, assuming TotalAmount was transmitted in cents. In reality, the OrderPlacedEvent contract and the entire pipeline (OrderPlacedEventDtoOrderPlacedEventOrderNotification.OrderTotal) pass the amount in dollars as a decimal. The erroneous / 100m turned 149.99 into 1.4999, which rounded to $1.50.

Fix: Remove the /100m division and format the dollar amount directly.

Also fixes broken ProjectReference paths in Notification.API.csproj — the relative paths to Shared.Contracts and Shared.Infrastructure were one level short (../../ instead of ../../../), preventing the project from building locally.

Before / After

Before (bug) After (fix)
before after

Review & Testing Checklist for Human

  • Verify no other producer sends TotalAmount in cents. The fix assumes all publishers of OrderPlacedEvent use dollars. If any upstream service (e.g. Order service) actually converts to cents before publishing, this fix would be wrong for that flow. Check the Order service's event publishing code.
  • Reproduce locally: POST to /api/notification/events/order-placed with {"orderId":"11111111-1111-1111-1111-111111111111","customerId":"22222222-2222-2222-2222-222222222222","totalAmount":149.99,"placedAt":"2026-03-17T12:00:00Z"} and open the preview URL — total should read $149.99.
  • Consider adding a unit test for FormatCurrency / RenderNotification to prevent regression.

Notes

  • The ToString("C2") formatting is locale-dependent (pre-existing, not introduced here). On non-US locales the currency symbol may differ. If consistent $ formatting is needed, consider using CultureInfo.InvariantCulture or a US culture explicitly.
  • The .csproj path fix is a pre-existing build issue unrelated to the currency bug but was necessary to build and test locally.

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/5c6f1dc44e6b4a1a92276d817d9c5d9c


Open with Devin

…rency formatting

Root cause: NotificationRenderer.FormatCurrency() was dividing the amount by 100,
assuming TotalAmount was in cents. However, the OrderPlacedEvent contract and the
entire pipeline (DTO -> Event -> OrderNotification) transmit TotalAmount in dollars
as a decimal. The erroneous division turned $149.99 into $1.50.

Fix: Remove the / 100m division and format the dollar amount directly.

Also fixes broken project references in Notification.API.csproj where the relative
paths to Shared projects were missing one directory level (../../ instead of ../../../).
@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