Implement currency conversion features and enhance deal handling - #37
Merged
Conversation
- Introduced a new CurrencyModule to manage currency conversion and rates. - Added ConversionService for handling currency conversions and fetching rates. - Updated DealsService to support base amounts and currency conversion logic. - Enhanced Deal and Dashboard functionalities to include reporting currency and unconverted deals. - Implemented new currency-related contracts and routes for setting reporting currency and manual rates. - Added integration tests to ensure correct handling of currency conversions and deal totals.
- Updated the currency rates service to fetch exchange rates from open.er-api.com, replacing the previous provider frankfurter.dev. - Enhanced error handling to check for unsupported base currencies in the response. - Implemented retry logic for fetching rates with a maximum of two attempts and a reduced timeout. - Cleaned up stale exchange rates for unsupported currencies during the refresh process. - Updated documentation to reflect the new exchange rate provider and its implications.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/currency/rates.controller.ts">
<violation number="1" location="apps/api/src/currency/rates.controller.ts:35">
P1: The deployed rate refresh will never run automatically because this adds `/internal/sync/rates` without registering it in the generated Vercel `crons` configuration. Adding this path with a daily schedule would make the refresh and `fillMissing()` workflow execute as documented.</violation>
</file>
<file name="apps/api/src/currency/conversion.service.ts">
<violation number="1" location="apps/api/src/currency/conversion.service.ts:104">
P1: A deal edited concurrently with a reporting-currency change can retain a `baseAmount` in the old currency because rerating is a non-atomic snapshot/update flow and deal conversion is not rechecked before the write. Serializing the setting change with deal writes, or rechecking the reporting currency and rerating after the write, would preserve the invariant that every non-null `baseAmount` uses the current reporting currency.</violation>
</file>
<file name="packages/db/prisma/schema.prisma">
<violation number="1" location="packages/db/prisma/schema.prisma:400">
P2: Large but contract-valid deals can fail during creation or rate filling with a database numeric overflow because the converted product can exceed `Decimal(18,4)` even though both source values fit their columns. Widen `baseAmount` in the schema and migration, or add coordinated upper bounds to amount and rate validation.</violation>
</file>
<file name="packages/db/src/fx.ts">
<violation number="1" location="packages/db/src/fx.ts:73">
P3: `convertToBase` is an unused public helper and duplicates the conversion path that production code actually uses, so the two implementations can drift. Removing it or routing `ConversionService.convert` through this helper would keep one conversion path.</violation>
</file>
<file name="apps/app/app/(app)/[slug]/deals/create-deal-sheet.tsx">
<violation number="1" location="apps/app/app/(app)/[slug]/deals/create-deal-sheet.tsx:226">
P2: Choosing a three-decimal currency such as BHD now allows precision that the create path cannot persist, silently changing `1.234` to `1.23`; the currency choices/input validation should be constrained or the amount contract should honor each currency's `minorUnits`.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| return this.run(authorization); | ||
| } | ||
|
|
||
| @Post("rates") |
Contributor
There was a problem hiding this comment.
P1: The deployed rate refresh will never run automatically because this adds /internal/sync/rates without registering it in the generated Vercel crons configuration. Adding this path with a daily schedule would make the refresh and fillMissing() workflow execute as documented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/currency/rates.controller.ts, line 35:
<comment>The deployed rate refresh will never run automatically because this adds `/internal/sync/rates` without registering it in the generated Vercel `crons` configuration. Adding this path with a daily schedule would make the refresh and `fillMissing()` workflow execute as documented.</comment>
<file context>
@@ -0,0 +1,72 @@
+ return this.run(authorization);
+ }
+
+ @Post("rates")
+ @AllowAnonymous()
+ async ratesViaPost(@Headers("authorization") authorization?: string) {
</file context>
| private async rerate(onlyMissing: boolean): Promise<RerateResult> { | ||
| const base = await this.reportingCurrency(); | ||
|
|
||
| const groups = await this.db.deal.groupBy({ |
Contributor
There was a problem hiding this comment.
P1: A deal edited concurrently with a reporting-currency change can retain a baseAmount in the old currency because rerating is a non-atomic snapshot/update flow and deal conversion is not rechecked before the write. Serializing the setting change with deal writes, or rechecking the reporting currency and rerating after the write, would preserve the invariant that every non-null baseAmount uses the current reporting currency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/currency/conversion.service.ts, line 104:
<comment>A deal edited concurrently with a reporting-currency change can retain a `baseAmount` in the old currency because rerating is a non-atomic snapshot/update flow and deal conversion is not rechecked before the write. Serializing the setting change with deal writes, or rechecking the reporting currency and rerating after the write, would preserve the invariant that every non-null `baseAmount` uses the current reporting currency.</comment>
<file context>
@@ -0,0 +1,179 @@
+ private async rerate(onlyMissing: boolean): Promise<RerateResult> {
+ const base = await this.reportingCurrency();
+
+ const groups = await this.db.deal.groupBy({
+ by: ["currency"],
+ where: {
</file context>
| closedAt DateTime? | ||
| closedReason String? | ||
|
|
||
| baseAmount Decimal? @db.Decimal(18, 4) |
Contributor
There was a problem hiding this comment.
P2: Large but contract-valid deals can fail during creation or rate filling with a database numeric overflow because the converted product can exceed Decimal(18,4) even though both source values fit their columns. Widen baseAmount in the schema and migration, or add coordinated upper bounds to amount and rate validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema.prisma, line 400:
<comment>Large but contract-valid deals can fail during creation or rate filling with a database numeric overflow because the converted product can exceed `Decimal(18,4)` even though both source values fit their columns. Widen `baseAmount` in the schema and migration, or add coordinated upper bounds to amount and rate validation.</comment>
<file context>
@@ -396,6 +397,10 @@ model Deal {
closedAt DateTime?
closedReason String?
+ baseAmount Decimal? @db.Decimal(18, 4)
+ fxRate Decimal? @db.Decimal(20, 10)
+ fxRateAt DateTime?
</file context>
| <SelectValue /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| {CURRENCIES.map((entry) => ( |
Contributor
There was a problem hiding this comment.
P2: Choosing a three-decimal currency such as BHD now allows precision that the create path cannot persist, silently changing 1.234 to 1.23; the currency choices/input validation should be constrained or the amount contract should honor each currency's minorUnits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/app/(app)/[slug]/deals/create-deal-sheet.tsx, line 226:
<comment>Choosing a three-decimal currency such as BHD now allows precision that the create path cannot persist, silently changing `1.234` to `1.23`; the currency choices/input validation should be constrained or the amount contract should honor each currency's `minorUnits`.</comment>
<file context>
@@ -198,15 +205,32 @@ function CreateDealForm({ companyId }: { companyId?: string }) {
+ <SelectValue />
+ </SelectTrigger>
+ <SelectContent>
+ {CURRENCIES.map((entry) => (
+ <SelectItem key={entry.code} value={entry.code}>
+ {entry.code}
</file context>
| }; | ||
| } | ||
|
|
||
| export async function convertToBase( |
Contributor
There was a problem hiding this comment.
P3: convertToBase is an unused public helper and duplicates the conversion path that production code actually uses, so the two implementations can drift. Removing it or routing ConversionService.convert through this helper would keep one conversion path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/src/fx.ts, line 73:
<comment>`convertToBase` is an unused public helper and duplicates the conversion path that production code actually uses, so the two implementations can drift. Removing it or routing `ConversionService.convert` through this helper would keep one conversion path.</comment>
<file context>
@@ -0,0 +1,86 @@
+ };
+}
+
+export async function convertToBase(
+ db: Db,
+ amount: Prisma.Decimal | null,
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/prisma/seed.ts">
<violation number="1" location="packages/db/prisma/seed.ts:538">
P3: When the stored reporting currency is non-USD, `money()` nulls `baseAmount`/`fxRate` for all seeded deals, even a deal whose currency equals the reporting currency — which needs no conversion (identity rate). Such deals will show as unconverted and will only be corrected if/when the rates cron's fillMissing runs. Consider returning `baseAmount = amount` (and an identity fxRate) for the case where the deal currency already matches the reporting currency, so the seed is correct without waiting on the cron.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
- Introduced baseCurrency to the Deal model to track the currency of baseAmount. - Updated ConversionService to streamline currency conversion processes and improve deal field handling. - Enhanced CurrencyService to enforce permissions for managing currency settings based on user roles. - Refactored DealsService to incorporate base currency logic in deal aggregations and reporting. - Improved DashboardService to accurately reflect open deal values based on the current reporting currency. - Updated integration tests to validate new currency handling features and ensure correct behavior across services.
- Updated `pendingWhere` method in `ConversionService` to explicitly match null `baseCurrency`, ensuring no deals are excluded from totals. - Added integration test to verify that deals with missing currency are correctly handled and updated. - Modified seeding logic to ensure `baseCurrency` is set alongside `baseAmount` for newly created deals, preventing issues with unconverted figures. - Updated documentation to clarify changes in currency handling and the implications for deal visibility.
Contributor
There was a problem hiding this comment.
6 issues found across 20 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/currency/conversion.service.ts">
<violation number="1" location="apps/api/src/currency/conversion.service.ts:89">
P1: Deals with a non-null `baseAmount` but NULL `baseCurrency` disappear from both totals and the unconverted queue, so they are never picked up by `fillMissing`. Including an explicit `{ baseCurrency: null }` branch keeps these legacy/seeded rows pending for conversion.</violation>
<violation number="2" location="apps/api/src/currency/conversion.service.ts:145">
P1: A failed `fillMissing()` can erase already converted deals: a missing rate for one pending deal clears every converted deal in that currency. Keeping the clear limited to `!onlyMissing` prevents a transient or unavailable rate from removing valid totals.</violation>
</file>
<file name="apps/app/components/crm/record-sheet/deal-sheet.tsx">
<violation number="1" location="apps/app/components/crm/record-sheet/deal-sheet.tsx:259">
P3: This placeholder will never be displayed: InlineSelectField renders it via <SelectValue placeholder>, which only appears when the select has no value, and deal.currency is always set. So the intended "currency no longer supported" warning never reaches the user. To actually warn, compute whether deal.currency is absent from CURRENCY_OPTIONS and render a conditional message (not the SelectValue placeholder); otherwise drop the misleading prop.</violation>
</file>
<file name="packages/db/prisma/schema.prisma">
<violation number="1" location="packages/db/prisma/schema.prisma:401">
P2: A clean seeded database will exclude every seeded converted deal from totals and report it as unconverted because the seed populates `baseAmount` without the new `baseCurrency`, while `countedWhere()` requires both fields. Populate `baseCurrency` (USD for these seeded conversions) alongside `baseAmount`.</violation>
</file>
<file name="apps/api/test/currency-totals.integration.spec.ts">
<violation number="1" location="apps/api/test/currency-totals.integration.spec.ts:276">
P3: The test name promises the unconverted currency list is deduplicated regardless of case/padding, but the body only checks that rerate normalizes the two variant rows to USD base values; the dedup of unconverted.currencies is never exercised. Either rename the test to reflect what it verifies (normalization on re-rate) or add an assertion on dashboard.summary(...).unconverted to cover the titled behavior.</violation>
<violation number="2" location="apps/api/test/currency-totals.integration.spec.ts:304">
P2: This assertion couples the test to global cumulative DB state: rerated.converted sums rows updated across every deal in the table, while the count is over all deals with a non-null amount. It only passes because every deal left behind by earlier describes is currently convertible, so it silently breaks if any prior test leaves an unconvertible deal or the suite runs in a different order. Scope the comparison to the two variant deals created in this test rather than the whole table.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| pendingWhere(base: string): PrismaTypes.DealWhereInput { | ||
| return { | ||
| amount: { not: null }, | ||
| OR: [{ baseAmount: null }, { baseCurrency: { not: base } }], |
Contributor
There was a problem hiding this comment.
P1: Deals with a non-null baseAmount but NULL baseCurrency disappear from both totals and the unconverted queue, so they are never picked up by fillMissing. Including an explicit { baseCurrency: null } branch keeps these legacy/seeded rows pending for conversion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/currency/conversion.service.ts, line 89:
<comment>Deals with a non-null `baseAmount` but NULL `baseCurrency` disappear from both totals and the unconverted queue, so they are never picked up by `fillMissing`. Including an explicit `{ baseCurrency: null }` branch keeps these legacy/seeded rows pending for conversion.</comment>
<file context>
@@ -62,22 +63,41 @@ export class ConversionService {
+ pendingWhere(base: string): PrismaTypes.DealWhereInput {
+ return {
+ amount: { not: null },
+ OR: [{ baseAmount: null }, { baseCurrency: { not: base } }],
+ };
+ }
</file context>
Suggested change
| OR: [{ baseAmount: null }, { baseCurrency: { not: base } }], | |
| OR: [ | |
| { baseAmount: null }, | |
| { baseCurrency: { not: base } }, | |
| { baseCurrency: null }, | |
| ], |
| if (!rate) { | ||
| missing.push(code); | ||
|
|
||
| cleared += await this.clear(code); |
Contributor
There was a problem hiding this comment.
P1: A failed fillMissing() can erase already converted deals: a missing rate for one pending deal clears every converted deal in that currency. Keeping the clear limited to !onlyMissing prevents a transient or unavailable rate from removing valid totals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/currency/conversion.service.ts, line 145:
<comment>A failed `fillMissing()` can erase already converted deals: a missing rate for one pending deal clears every converted deal in that currency. Keeping the clear limited to `!onlyMissing` prevents a transient or unavailable rate from removing valid totals.</comment>
<file context>
@@ -103,33 +123,31 @@ export class ConversionService {
- if (!onlyMissing) {
- cleared += await this.clear(code);
- }
+ cleared += await this.clear(code);
continue;
</file context>
| closedReason String? | ||
|
|
||
| baseAmount Decimal? @db.Decimal(24, 4) | ||
| baseCurrency String? |
Contributor
There was a problem hiding this comment.
P2: A clean seeded database will exclude every seeded converted deal from totals and report it as unconverted because the seed populates baseAmount without the new baseCurrency, while countedWhere() requires both fields. Populate baseCurrency (USD for these seeded conversions) alongside baseAmount.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/schema.prisma, line 401:
<comment>A clean seeded database will exclude every seeded converted deal from totals and report it as unconverted because the seed populates `baseAmount` without the new `baseCurrency`, while `countedWhere()` requires both fields. Populate `baseCurrency` (USD for these seeded conversions) alongside `baseAmount`.</comment>
<file context>
@@ -397,9 +397,10 @@ model Deal {
- fxRate Decimal? @db.Decimal(20, 10)
- fxRateAt DateTime?
+ baseAmount Decimal? @db.Decimal(24, 4)
+ baseCurrency String?
+ fxRate Decimal? @db.Decimal(20, 10)
+ fxRateAt DateTime?
</file context>
| expect(row.baseAmount?.toNumber()).toBe(1000); | ||
| } | ||
|
|
||
| expect(rerated.converted).toBe( |
Contributor
There was a problem hiding this comment.
P2: This assertion couples the test to global cumulative DB state: rerated.converted sums rows updated across every deal in the table, while the count is over all deals with a non-null amount. It only passes because every deal left behind by earlier describes is currently convertible, so it silently breaks if any prior test leaves an unconvertible deal or the suite runs in a different order. Scope the comparison to the two variant deals created in this test rather than the whole table.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/test/currency-totals.integration.spec.ts, line 304:
<comment>This assertion couples the test to global cumulative DB state: rerated.converted sums rows updated across every deal in the table, while the count is over all deals with a non-null amount. It only passes because every deal left behind by earlier describes is currently convertible, so it silently breaks if any prior test leaves an unconvertible deal or the suite runs in a different order. Scope the comparison to the two variant deals created in this test rather than the whole table.</comment>
<file context>
@@ -233,3 +235,78 @@ describe("the deals list", () => {
+ expect(row.baseAmount?.toNumber()).toBe(1000);
+ }
+
+ expect(rerated.converted).toBe(
+ await db.deal.count({ where: { amount: { not: null } } }),
+ );
</file context>
| save({ currency: currency.toUpperCase() }); | ||
| }} | ||
| options={CURRENCY_OPTIONS} | ||
| placeholder={`${deal.currency} — no longer supported`} |
Contributor
There was a problem hiding this comment.
P3: This placeholder will never be displayed: InlineSelectField renders it via , which only appears when the select has no value, and deal.currency is always set. So the intended "currency no longer supported" warning never reaches the user. To actually warn, compute whether deal.currency is absent from CURRENCY_OPTIONS and render a conditional message (not the SelectValue placeholder); otherwise drop the misleading prop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/crm/record-sheet/deal-sheet.tsx, line 259:
<comment>This placeholder will never be displayed: InlineSelectField renders it via <SelectValue placeholder>, which only appears when the select has no value, and deal.currency is always set. So the intended "currency no longer supported" warning never reaches the user. To actually warn, compute whether deal.currency is absent from CURRENCY_OPTIONS and render a conditional message (not the SelectValue placeholder); otherwise drop the misleading prop.</comment>
<file context>
@@ -256,6 +256,7 @@ function DealOverview({ deal }: { deal: Deal }) {
label="Currency"
value={deal.currency}
options={CURRENCY_OPTIONS}
+ placeholder={`${deal.currency} — no longer supported`}
onSave={(currency) => save({ currency })}
/>
</file context>
| await db.deal.delete({ where: { id: deal.id } }); | ||
| }); | ||
|
|
||
| it("counts a currency once however it was cased or padded", async () => { |
Contributor
There was a problem hiding this comment.
P3: The test name promises the unconverted currency list is deduplicated regardless of case/padding, but the body only checks that rerate normalizes the two variant rows to USD base values; the dedup of unconverted.currencies is never exercised. Either rename the test to reflect what it verifies (normalization on re-rate) or add an assertion on dashboard.summary(...).unconverted to cover the titled behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/test/currency-totals.integration.spec.ts, line 276:
<comment>The test name promises the unconverted currency list is deduplicated regardless of case/padding, but the body only checks that rerate normalizes the two variant rows to USD base values; the dedup of unconverted.currencies is never exercised. Either rename the test to reflect what it verifies (normalization on re-rate) or add an assertion on dashboard.summary(...).unconverted to cover the titled behavior.</comment>
<file context>
@@ -233,3 +235,78 @@ describe("the deals list", () => {
+ await db.deal.delete({ where: { id: deal.id } });
+ });
+
+ it("counts a currency once however it was cased or padded", async () => {
+ const rows = await Promise.all(
+ [" usd ", "Usd"].map((currency, index) =>
</file context>
- Updated `ConversionService` to conditionally clear rates only when `onlyMissing` is false, improving efficiency in handling missing currencies. - Enhanced integration tests to verify correct behavior when dealing with unconverted figures and missing currency rates. - Introduced a new utility function in the deal sheet component to manage currency options, ensuring proper display of unsupported currencies.
Contributor
There was a problem hiding this comment.
1 issue found across 21 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/deals/deals.service.ts">
<violation number="1" location="apps/api/src/deals/deals.service.ts:125">
P2: When all open deals are unconverted or have a stale base currency, the deals page silently omits the “not counted” warning because this filtered aggregate returns `null` and the UI uses that value to hide its metadata. Preserve a renderable zero/summary state for this case or make the UI render the unconverted disclosure independently of `openValueCents`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| this.db.deal.count({ where }), | ||
| this.facetCounts(input), | ||
| this.db.deal.aggregate({ | ||
| where: { AND: [openWhere, this.conversion.countedWhere(base)] }, |
Contributor
There was a problem hiding this comment.
P2: When all open deals are unconverted or have a stale base currency, the deals page silently omits the “not counted” warning because this filtered aggregate returns null and the UI uses that value to hide its metadata. Preserve a renderable zero/summary state for this case or make the UI render the unconverted disclosure independently of openValueCents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/deals/deals.service.ts, line 125:
<comment>When all open deals are unconverted or have a stale base currency, the deals page silently omits the “not counted” warning because this filtered aggregate returns `null` and the UI uses that value to hide its metadata. Preserve a renderable zero/summary state for this case or make the UI render the unconverted disclosure independently of `openValueCents`.</comment>
<file context>
@@ -95,44 +95,38 @@ export class DealsService {
+ this.db.deal.count({ where }),
+ this.facetCounts(input),
+ this.db.deal.aggregate({
+ where: { AND: [openWhere, this.conversion.countedWhere(base)] },
+ _sum: { baseAmount: true },
+ }),
</file context>
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/components/crm/record-sheet/deal-sheet.tsx">
<violation number="1" location="apps/app/components/crm/record-sheet/deal-sheet.tsx:57">
P3: The new `currencyOptions` helper matches the deal's stored currency against the canonical option values with exact string equality, and `deal.currency` comes straight from the row without normalization. The same PR's own test and `normalizeCurrency` in the deals service treat stored currencies as possibly mis-cased or padded, so a row holding e.g. `'Usd'` or `' usd '` would be rendered as "Usd — no longer supported" purely because the comparison didn't normalize. Consider matching through `normalizeCurrency(currency)` (falling back to literal when it returns empty) so a genuinely supported currency is never mislabeled as deprecated.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| label: `${entry.code} · ${entry.name}`, | ||
| })); | ||
|
|
||
| function currencyOptions(currency: string) { |
Contributor
There was a problem hiding this comment.
P3: The new currencyOptions helper matches the deal's stored currency against the canonical option values with exact string equality, and deal.currency comes straight from the row without normalization. The same PR's own test and normalizeCurrency in the deals service treat stored currencies as possibly mis-cased or padded, so a row holding e.g. 'Usd' or ' usd ' would be rendered as "Usd — no longer supported" purely because the comparison didn't normalize. Consider matching through normalizeCurrency(currency) (falling back to literal when it returns empty) so a genuinely supported currency is never mislabeled as deprecated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/components/crm/record-sheet/deal-sheet.tsx, line 57:
<comment>The new `currencyOptions` helper matches the deal's stored currency against the canonical option values with exact string equality, and `deal.currency` comes straight from the row without normalization. The same PR's own test and `normalizeCurrency` in the deals service treat stored currencies as possibly mis-cased or padded, so a row holding e.g. `'Usd'` or `' usd '` would be rendered as "Usd — no longer supported" purely because the comparison didn't normalize. Consider matching through `normalizeCurrency(currency)` (falling back to literal when it returns empty) so a genuinely supported currency is never mislabeled as deprecated.</comment>
<file context>
@@ -54,6 +54,17 @@ const CURRENCY_OPTIONS = CURRENCIES.map((entry) => ({
label: `${entry.code} · ${entry.name}`,
}));
+function currencyOptions(currency: string) {
+ if (CURRENCY_OPTIONS.some((option) => option.value === currency)) {
+ return CURRENCY_OPTIONS;
</file context>
- Updated AGENTS.md to emphasize the importance of reviewing relevant documentation before starting work, including a new index table for quick reference. - Refined API rules in api.md to clarify logging practices and the separation of intelligence from the API. - Consolidated environment setup instructions into a new setup.md file for better organization and ease of access. - Enhanced currency handling in DashboardService and related tests to ensure accurate reporting and conversion logic. - Improved integration tests to validate new currency handling features and ensure correct behavior across services.
- Updated AGENTS.md to include new references for the Agent panel and local setup instructions. - Added a new docs/agent-panel.md file detailing the Agent panel's functionality and usage. - Introduced docs/currency.md to clarify currency handling rules and reporting practices. - Revised environment setup instructions in docs/environment.md for better clarity and organization.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Adds multi-currency support with a reporting currency, live/manual FX rates, deal-level conversions, and a fix so deals missing base currency aren’t dropped from totals. Totals and sorting use the reporting currency, flag unconverted deals, currency settings are admin-only, and dashboard/company/deal views reflect this; currency rules and agent panel docs were added, and setup/env docs were clarified.
New Features
CurrencyModulewithConversionServiceandRatesService;/internal/sync/ratesfetches from open.er-api.com with retries, cleans unsupported currencies, and only clears existing FX on full rerates (not “only missing” runs).baseAmount,baseCurrency,fxRate,fxRateAt; totals and sorting usebaseAmount. Money formatting respects each currency’s fraction digits.descriptionfield and inline editing; shown in the agent preamble.baseCurrencyso pending/uncounted deals are included in totals; seeding setsbaseCurrencyalongsidebaseAmount.docs/currency.md(currency rules) anddocs/agent-panel.md(Agent panel);AGENTS.mdlinks to these; env/setup guidance clarified and moved todocs/setup.mdand streamlined indocs/environment.md.Migration
ExchangeRate,RateSource,deal.baseAmount/baseCurrency/fxRate/fxRateAt, anddeal.description; backfillsbaseCurrencyfor existingbaseAmount).CRON_SECRETand schedule a cron to callPOST /internal/sync/rates(orGET) to refresh rates.baseAmountfor existing deals.Written for commit de682cb. Summary will update on new commits.