Skip to content

fix: remove erroneous cents-to-dollars division in notification currency formatting#32

Open
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1777950823-fix-notification-currency-format
Open

fix: remove erroneous cents-to-dollars division in notification currency formatting#32
devin-ai-integration[bot] wants to merge 2 commits into
mainfrom
devin/1777950823-fix-notification-currency-format

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented May 5, 2026

Summary

Fixes incorrect order amounts displayed in notification email previews. A $149.99 order was rendering as $1.50 due to an erroneous division by 100 in the NotificationRenderer.FormatCurrency() method.

Root Cause

During the microservice decomposition, the NotificationRenderer.FormatCurrency() method was written with an incorrect assumption documented in a misleading comment:

// The OrderPlacedEvent.TotalAmount is transmitted in cents (integer
// representation) to avoid floating-point precision issues across
// service boundaries. Convert back to dollars for display.
var dollars = amount / 100m;

However, the OrderPlacedEvent contract in Shared.Contracts defines TotalAmount as a decimal representing dollars, not cents. The Order service sends 149.99 (dollars), but the renderer divides by 100, producing 1.4999 → rounded to $1.50.

Fix

Removed the /100m division — the amount is already in dollars and just needs formatting:

private static string FormatCurrency(decimal amount)
{
    return amount.ToString("C2");
}

Also fixed incorrect ProjectReference paths in Notification.API.csproj (../../Shared/../../../Shared/) which prevented the project from building when referenced directly.

Before / After

Before (Bug) After (Fix)
before after

Review & Testing Checklist for Human

  • Verify the Order service also transmits TotalAmount in dollars (not cents) when publishing OrderPlacedEvent via RabbitMQ in production — this fix assumes the HTTP testing endpoint matches the real contract
  • Confirm no other services (e.g., shipping notifications) rely on the old cents-based interpretation

Notes

  • The ¤ symbol in screenshots is the generic currency symbol used by .NET's ToString("C2") when running in invariant culture. In production with proper locale settings, this would display as $.
  • The .csproj path fix (../../../../../) was necessary to allow the Notification.API project to build standalone (resolves project reference to Shared.Contracts).

Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/23895db2644343bca01a035cc250ced1
Requested by: @bsmitches


Open in Devin Review

…ncy formatting

The NotificationRenderer.FormatCurrency() method was dividing the amount by 100,
incorrectly assuming TotalAmount was transmitted in cents. However, the
OrderPlacedEvent contract (Shared.Contracts) defines TotalAmount as a decimal in
dollars. This caused a $149.99 order to display as $1.50 in email previews.

Also fixes incorrect project reference paths for Shared.Contracts and
Shared.Infrastructure in Notification.API.csproj (was ../../ 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 2 additional findings.

Open in Devin Review

@bsmitches
Copy link
Copy Markdown
Contributor

Can you double check that this is the only place this bug exists? Also, please add inline comments around what you changed

@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

@bsmitches Searched the entire src/ tree for any similar / 100 or /100m patterns on monetary fields (amount, total, price, cost) — no other instances exist. The FormatCurrency method in NotificationRenderer.cs was the only place this bug occurred.

I've also pushed a follow-up commit (fc0f935) adding an inline XML doc comment on FormatCurrency clarifying that the amount is expected in the base currency unit (dollars), matching the OrderPlacedEvent contract.

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.

1 participant