feat(multi-currency): add currency conversion metadata storage and display#367
feat(multi-currency): add currency conversion metadata storage and display#367
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (neogenz/yeosu) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
frontend/projects/webapp/src/app/core/currency/app-currency.pipe.ts
Outdated
Show resolved
Hide resolved
...jects/webapp/src/app/feature/budget-templates/details/components/edit-transactions-dialog.ts
Outdated
Show resolved
Hide resolved
Review PR 367 - Resume: 1 bloquant, 1 important, 1 suggestion - Bloquant: app-currency.pipe.ts:10 pure:true avec signal interne, montants ne se rafraichissent pas au changement de devise (95%) - Important: edit-transactions-dialog.ts:502 metadonnees de conversion perdues a la sauvegarde des lignes template (93%) - Suggestion: error-definitions.ts:603 CURRENCY_RATE_FETCH_FAILED string litterale au lieu API_ERROR_CODES.XXX (90%) - Points positifs: chiffrement original_amount correct, helper decryptAmountFields propre, migration SQL retro-compatible, cache TTL frontend, tests backend complets - Verdict: CHANGES REQUESTED |
frontend/projects/webapp/src/app/feature/complete-profile/complete-profile-page.ts
Outdated
Show resolved
Hide resolved
Review PR 367Resume : 1 bloquant - 1 important - 1 suggestion Bloquants
Le chemin single-insert chiffre correctement Importants
Suggestions
La cle Points positifs
Verdict : CHANGES REQUESTED |
| return `Comptabilisé ${roundedConsumed} sur ${roundedEnvelope} ${currency} (enveloppe)`; | ||
| } | ||
| return messages.withinEnvelope(roundedEnvelope); | ||
| return `Comptabilisé ${roundedEnvelope} ${currency} (enveloppe)`; | ||
| } | ||
|
|
||
| export function computeTransactionSnackbarMessage( | ||
| transactionId: string, | ||
| transactions: Transaction[], | ||
| messages: TransactionSnackbarMessages, | ||
| currency: SupportedCurrency, | ||
| ): string | null { | ||
| const transaction = transactions.find((tx) => tx.id === transactionId); | ||
| if (!transaction || transaction.checkedAt == null) return null; | ||
|
|
||
| return messages.checked(Math.round(Math.abs(transaction.amount))); | ||
| return `Comptabilisé ${Math.round(Math.abs(transaction.amount))} ${currency}`; |
There was a problem hiding this comment.
[IMPORTANT] Régression i18n — messages snackbar hardcodés en français (confiance: 95%)
La règle projet (.claude/rules/03-frameworks-and-libraries/transloco-i18n.md) interdit explicitement les strings français hardcodées dans les fichiers TypeScript : "NEVER hardcode French strings in templates or TS files — always use transloco keys".
L'ancienne architecture était correcte : budget-details-page.ts injectait TranslocoService et passait des factories de messages. Le refactor retire cette intégration et remplace les keys transloco par des strings littérales, rendant les messages non-traduisibles.
Les clés fr.json budget.snackbar.envelopeOver, budget.snackbar.envelopeWithin et budget.snackbar.transactionChecked sont désormais code mort.
Fix suggéré — réintroduire le service transloco ou conserver le pattern factory :
// Dans budget-details-page.ts, au lieu de passer juste `currency`:
computeEnvelopeSnackbarMessage(
budgetLineId,
details.budgetLines,
details.transactions,
{
overEnvelope: (consumed, envelope) =>
this.#transloco.translate('budget.snackbar.envelopeOver', { consumed, envelope }),
withinEnvelope: (envelope) =>
this.#transloco.translate('budget.snackbar.envelopeWithin', { envelope }),
},
);
| import { CURRENCY_CONFIG, DEFAULT_DIGITS_INFO } from './currency-config'; | ||
|
|
||
| @Pipe({ | ||
| name: 'appCurrency', |
There was a problem hiding this comment.
[SUGGESTION] pure: false inutile pour ce pipe déterministe (confiance: 90%)
Le pipe ne dépend que de ses arguments (value, currency, digitsInfo) et de CURRENCY_CONFIG (constante statique). Il n'a pas d'état interne mutable ni de side effects. pure: false force Angular à réévaluer le pipe à chaque cycle de change detection, y compris quand les inputs n'ont pas changé.
Avec l'utilisation intensive dans les cartes du budget grid, cela génère de nombreux appels inutiles de transform().
| name: 'appCurrency', | |
| pure: true, |
| @@ -117,7 +117,7 @@ import { TranslocoPipe } from '@jsverse/transloco'; | |||
| > | |||
There was a problem hiding this comment.
[SUGGESTION] Incohérence : CurrencyPipe natif au lieu de AppCurrencyPipe (confiance: 80%)
Toute la PR migre les composants vers AppCurrencyPipe (budget-grid, budget-table, allocated-transactions-dialog, etc.), mais template-list-item.ts conserve le pipe Angular natif avec des inputs manuels currency() et locale().
AppCurrencyPipe encapsule exactement ce pattern (locale issu de CURRENCY_CONFIG), ce qui évite l'erreur de passer une locale incohérente avec la devise. En l'état, rien n'empêche un futur appelant de passer currency='EUR' et locale='de-CH', produisant un formatage incorrect.
Remplacer par | appCurrency: currency() pour cohérence avec le reste de la codebase.
Review PR 367 - Resume: 0 bloquant, 1 important, 2 suggestionsIMPORTANT - budget-details-snackbar.utils.ts L26-39 - Regression i18n: messages snackbar hardcodes en francais (confiance 95%) Les regles projet interdisent les strings francais hardcodes dans les fichiers TypeScript. L ancienne architecture passait des factories transloco depuis budget-details-page.ts. Le refactor bypass transloco et hardcode les messages. Les cles fr.json budget.snackbar.envelopeOver, budget.snackbar.envelopeWithin, budget.snackbar.transactionChecked sont desormais code mort. SUGGESTION - app-currency.pipe.ts L9 - pure: false inutile (confiance 90%) Pipe deterministe, pure: false force une reevaluation a chaque cycle de change detection. Changer en pure: true. SUGGESTION - template-list-item.ts L117 - Utilise encore CurrencyPipe natif au lieu d AppCurrencyPipe (confiance 80%) Seul composant de la PR a ne pas avoir migre vers AppCurrencyPipe. Incoherence et risque de passer une locale incompatible avec la devise. Points positifs :
Verdict : CHANGES REQUESTED |
…splay - Backend: New currency module with exchange rate service and metadata mapper - DB: Add originalAmount, originalCurrency, targetCurrency, exchangeRate columns to budget_lines and transactions - Frontend: CurrencyConversionBadge component with tooltip showing conversion details - iOS: CurrencyConversionBadge with popover, currency settings view, and conversion service - Integrate badges into detail views and edit sheets across all platforms - i18n: Add currency conversion tooltip translations
Addresses critical type safety, i18n, and code quality issues across frontend and backend. **Type Safety (3)**: - Replace unsafe SupportedCurrency type casts with Zod validation (currency.service.ts) - Fix missing pipe arguments in templates (transactions-table.ts, search-transactions-dialog.ts) - Add type annotation for null literals in tests (currency.service.test.ts) - Remove invalid isChecked tests (web form doesn't have this control; iOS-only feature) **i18n Localization (15)**: - Centralize 40+ hardcoded French strings via transloco keys (fr.json) - Update template, budget, currentMonth sections with 30+ new keys - Add TranslocoPipe imports and provideTranslocoForTest() to specs **Code Quality (3)**: - Add error handling for currency conversion loop - Remove unnecessary JSDoc comments and static computed() wrappers - Extract magic number 1000 to AXIS_ABBREVIATION_THRESHOLD constant All tests pass; pnpm quality passes with 0 type errors.
- Set appCurrency pipe to pure: false (reads external signal) - Propagate conversion metadata in edit-transactions-dialog - Encrypt original_amount in bulk insert path - Use safeParse + BusinessException in CurrencyService - Replace hardcoded "Devise" with Transloco key
…n transaction forms - Add module-level Map caches for Intl.NumberFormat in budget-action-menu.ts and dashboard-next-month.ts to prevent re-allocation in computed() - Restore isChecked form control, mat-slide-toggle, and checkedAt mapping in both allocated transaction creation forms (dialog + bottom sheet) - Add missing i18n keys: budget.forecastCheckedToggle and transactionForm.checkedToggle - Add checkedAt toggle test coverage to both allocated transaction spec files - All 1689 tests passing
| > { | ||
| if (!lines.length || !lines.some((l) => l.amount)) | ||
| return lines.map((l) => ({ ...l, amount: 0 })); | ||
| return lines.map((l) => ({ ...l, amount: 0, original_amount: null })); |
There was a problem hiding this comment.
[IMPORTANT] Perte silencieuse de original_amount dans le chemin de retour anticipé (confidence: 90%)
La condition !lines.some((l) => l.amount) évalue uniquement la présence du champ amount. Si toutes les lignes ont un amount null (ex. erreur de chiffrement en amont, ou ligne créée sans montant), le retour anticipé force original_amount: null pour toutes les lignes, effaçant les données de conversion multi-devises potentiellement stockées.
La correction devrait également tester original_amount pour décider si le déchiffrement est nécessaire :
| return lines.map((l) => ({ ...l, amount: 0, original_amount: null })); | |
| if (!lines.length || (!lines.some((l) => l.amount) && !lines.some((l) => l.original_amount))) | |
| return lines.map((l) => ({ ...l, amount: 0, original_amount: null })); |
|
|
||
| if (!baseResult.success || !targetResult.success) { | ||
| throw new BusinessException( | ||
| ERROR_DEFINITIONS.CURRENCY_RATE_FETCH_FAILED, |
There was a problem hiding this comment.
[IMPORTANT] Code d'erreur HTTP sémantiquement incorrect pour une erreur de validation (confidence: 85%)
Quand originalCurrency ou targetCurrency échoue au safeParse de Zod, l'exception levée utilise CURRENCY_RATE_FETCH_FAILED (HTTP 503 Service Unavailable). Or il s'agit ici d'un problème de validation des données en entrée, pas d'une indisponibilité du service externe — le 503 est donc trompeur pour le client.
En pratique, les DTOs du contrôleur (via createZodDto(budgetLineCreateSchema)) rejettent les devises invalides avant d'atteindre ce code, donc ce chemin n'est jamais déclenché aujourd'hui. Mais si un appel direct au service contournait la validation (test, futur refactoring), le client recevrait un 503 pour ce qui est une erreur 400/422. Il faudrait un error code dédié VALIDATION_FAILED ou CURRENCY_INVALID avec HttpStatus.BAD_REQUEST.
| const encryptedOriginalAmounts = await Promise.all( | ||
| overriddenCreates.map(async (line) => { | ||
| if (line.originalAmount == null) return undefined; | ||
| const prepared = await this.encryptionService.prepareAmountData( |
There was a problem hiding this comment.
[SUGGESTION] Inefficacité : N appels prepareAmountData au lieu de encryptOptionalAmount (confidence: 85%)
Dans le chemin d'insertion bulk, les original_amount sont chiffrés en appelant prepareAmountData individuellement pour chaque ligne (ce qui déclenche potentiellement N récupérations du DEK). La méthode encryptOptionalAmount existe précisément pour ce cas et gère la valeur null/undefined en une seule invocation. Le remplacement simplifie le code et s'aligne avec l'approche utilisée dans createTemplateLine :
| const prepared = await this.encryptionService.prepareAmountData( | |
| return this.encryptionService.encryptOptionalAmount( | |
| line.originalAmount, | |
| user.id, | |
| user.clientKey, | |
| ); |
Review PR #367Résumé : 0 bloquant — 2 important — 1 suggestion BloquantAucun. Important
Suggestions
Points positifs
Verdict : CHANGES REQUESTED |
CHF was formatted with de-CH locale (prefix: "CHF 400") but tests and product intent require fr-CH (suffix: "400 CHF"). Changed locale in both iOS Formatters and frontend currency-config. Updated unit test assertions for narrow non-breaking space grouping separator.
|
|
||
| if (roundedConsumed > roundedEnvelope) { | ||
| return messages.overEnvelope(roundedConsumed, roundedEnvelope); | ||
| return `Comptabilisé ${roundedConsumed} sur ${roundedEnvelope} ${currency} (enveloppe)`; |
There was a problem hiding this comment.
[BLOQUANT] Chaînes françaises hardcodées en violation de la règle transloco (confiance: 95%)
Cette fonction utilitaire retourne maintenant des template literals avec du texte français hardcodé, alors que :
- La règle du projet (
transloco-i18n.md) interdit formellement de hardcoder des chaînes françaises - Les clés transloco correspondantes existent déjà dans
fr.json:budget.snackbar.envelopeOver,budget.snackbar.envelopeWithin,budget.snackbar.transactionChecked - L'ancien design (callbacks
messages) permettait d'utiliser transloco depuis le composant appelant — c'était la bonne approche
Impact : toute modification future des libellés devra chasser des chaînes hardcodées dans le code TS, et l'internationalisation est impossible.
Correction suggérée : réintroduire les callbacks de message ou passer un TranslocoService en paramètre, en s'appuyant sur les clés existantes dans fr.json.
| throw new BusinessException( | ||
| ERROR_DEFINITIONS.CURRENCY_RATE_FETCH_FAILED, | ||
| { base: dto.originalCurrency, target: dto.targetCurrency }, | ||
| { operation: 'overrideExchangeRate' }, |
There was a problem hiding this comment.
[IMPORTANT] Code d'erreur HTTP incorrect pour une currency non supportée (confiance: 88%)
Quand originalCurrency ou targetCurrency n'est pas dans supportedCurrencySchema, le code lève CURRENCY_RATE_FETCH_FAILED qui retourne un 503 SERVICE_UNAVAILABLE. Or il s'agit d'une erreur de validation client (mauvaise devise), pas d'un échec de service externe.
De plus, ces valeurs proviennent des DTOs BudgetLineCreate / TransactionCreate qui valident déjà les currencies via supportedCurrencySchema.optional(). Ce cas ne peut se produire que si quelqu'un contourne la validation — c'est typiquement un 400/422, pas un 503.
Un client recevant 503 va interpréter ça comme une indisponibilité temporaire et potentiellement retenter, alors qu'un retry ne changera rien.
Correction : utiliser ERROR_DEFINITIONS.VALIDATION_FAILED (400) à la place, ou simplement supprimer cette branche puisque les DTOs garantissent déjà la validité des valeurs.
| let response: Response; | ||
| try { | ||
| response = await fetch(url, { signal: AbortSignal.timeout(5000) }); | ||
| } catch (error) { |
There was a problem hiding this comment.
[SUGGESTION] Context de logging incomplet pour le suivi des erreurs (confiance: 85%)
Selon la règle error-handling-backend.md, le paramètre loggingContext doit inclure userId pour permettre le suivi des erreurs par utilisateur. Les 3 instances de BusinessException dans #fetchRate passent { operation: 'getRate' } sans userId — ce qui est cohérent puisque #fetchRate n'a pas accès à l'utilisateur.
Cependant, overrideExchangeRate est appelé avec le contexte du DTO utilisateur — il serait utile de propager un userId optionnel jusqu'à getRate/#fetchRate pour enrichir les logs.
C'est une suggestion mineure pour la traçabilité future, pas bloquant.
| const currency = userSettings.currency; | ||
| const showCurrencySelector = userSettings.showCurrencySelector; | ||
| const inputCurrency = signal<SupportedCurrency>(currency()); | ||
| const conversionError = signal(false); |
There was a problem hiding this comment.
[SUGGESTION] inputCurrency initialisé une seule fois sans réactivité (confiance: 82%)
inputCurrency est initialisé avec currency() au moment de l'injection, mais ne se met pas à jour si currency() change en session (ex: l'utilisateur change sa devise dans les settings pendant que le formulaire est ouvert). L'utilisation de linkedSignal permettrait de synchroniser automatiquement :
| const conversionError = signal(false); | |
| const inputCurrency = linkedSignal(() => currency()); |
C'est un cas de bord peu probable en pratique (le dialog se ferme en changeant de page), mais le pattern linkedSignal est plus correct sémantiquement pour un signal dérivé réinitialisable.
|
Review PR 367 - 1 bloquant, 1 important, 2 suggestions. Voir les commentaires inline pour les details. Verdict: CHANGES REQUESTED |
|
Voir les commentaires inline pour le detail de chaque finding. Resume: 1 bloquant (snackbar utils - chaines FR hardcodees), 1 important (currency service - HTTP 503 vs 400), 2 suggestions (logging userId, linkedSignal). Verdict: CHANGES REQUESTED |
…ting - iOS: update DecimalCurrencyFormattingTests to expect CHF prefix (fr_CH) - E2E: update normalizeSwissNumber to handle narrow no-break space (\u202F) - E2E: update amount assertions from period to comma decimal separator - E2E: update toContainText from apostrophe to narrow no-break space
|
|
||
| if (roundedConsumed > roundedEnvelope) { | ||
| return messages.overEnvelope(roundedConsumed, roundedEnvelope); | ||
| return `Comptabilisé ${roundedConsumed} sur ${roundedEnvelope} ${currency} (enveloppe)`; |
There was a problem hiding this comment.
[Important] Chaînes françaises codées en dur contournant le système i18n (confiance : 92%)
Les templates de traduction budget.snackbar.envelopeOver, envelopeWithin et transactionChecked existent déjà dans fr.json et acceptent des paramètres. Ce refactoring les a remplacés par des interpolations de chaînes directes en TypeScript, violant la règle du projet : NEVER hardcode French strings (règle Transloco dans .claude/rules/).
Impact : si les clés de traduction sont mises à jour ou que d'autres langues sont ajoutées, ce fichier ne sera pas synchronisé.
Correction : passer une fonction de traduction ou le TranslocoService en paramètre plutôt que le currency brut, pour déléguer la formulation aux clés existantes.
|
|
||
| if (!baseResult.success || !targetResult.success) { | ||
| throw new BusinessException( | ||
| ERROR_DEFINITIONS.CURRENCY_RATE_FETCH_FAILED, |
There was a problem hiding this comment.
[Important] Code d'erreur sémantiquement incorrect pour un échec de validation (confiance : 90%)
Lorsque les devises passées ne sont pas dans supportedCurrencySchema, c'est une erreur de validation client (400 Bad Request), mais CURRENCY_RATE_FETCH_FAILED est défini avec httpStatus: HttpStatus.SERVICE_UNAVAILABLE (503). Le client recevra un 503 pour une requête invalide qu'il contrôle entièrement, ce qui est trompeur.
Correction : utiliser ERROR_DEFINITIONS.VALIDATION_FAILED (400) pour ce cas, et réserver CURRENCY_RATE_FETCH_FAILED (503) uniquement aux vraies erreurs réseau/Frankfurter.
| checked_at: createBudgetLineDto.checkedAt ?? null, | ||
| created_at: new Date().toISOString(), | ||
| updated_at: new Date().toISOString(), | ||
| original_currency: createBudgetLineDto.originalCurrency ?? null, |
There was a problem hiding this comment.
[Suggestion] Duplication de la logique de mapping des champs devise entre le service et le mapper (confiance : 85%)
prepareBudgetLineData() écrit original_currency, target_currency, exchange_rate directement — alors que budget-line.mappers.ts expose déjà mapCurrencyMetadataToDb() à cet effet. Le service duplique ce que le mapper fait, à la différence du code toInsert dans budget-line.mappers.ts qui utilise ...mapCurrencyMetadataToDb(createDto).
Ce n'est pas bloquant (les deux chemins donnent le bon résultat), mais la centralisation dans le mapper existant éviterait de diverger si les champs changent.
Review PR #367Résumé : 0 bloquant — 2 importants — 1 suggestion Bloquant(aucun) Important
Suggestion
Points positifs
Verdict : CHANGES REQUESTED |
1 participant
Summary
• Add multi-currency support with automatic exchange rate conversion
• Store original amount, currency, and exchange rate metadata on transactions and budget lines
• Display conversion details via new CurrencyConversionBadge component on detail views
• Add currency settings view on iOS with EUR/CHF selection
• Implement currency conversion service across all platforms
Changes
Backend
• New currency service with exchange rate conversion
• Add originalAmount, originalCurrency, targetCurrency, exchangeRate columns to database
• Update budget line and transaction mappers to handle conversion metadata
• Export conversion data in Excel exports
Frontend (Angular)
• New CurrencyConversionBadge UI component with tooltip showing conversion details
• New CurrencyConverterService for client-side currency conversions
• Integrate badges into budget detail panel, allocated transactions dialog, and bottom sheet
• Add i18n support for conversion tooltips
iOS (SwiftUI)
• New CurrencyConversionBadge with popover showing original amount and exchange rate
• CurrencyConversionService actor for currency conversion
• CurrencyAmountPicker component for selecting input currency
• CurrencySettingView for EUR/CHF selection in account settings
• Integrate badges into edit sheets (transactions, budget lines, templates)
Testing
• Add 6 unit tests for CurrencyConversionBadge component
• Add currency converter service tests
• Add currency pipe tests
Type Safety
• Update shared Zod schemas with currency metadata fields
• Add error codes for currency conversion failures