Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The migration and currency validation leave unresolved runtime risks, with additional mapping and test gaps.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds per-program Onafriq currency-code configuration for transaction requests and reconciliation exports.
Changes:
- Adds configuration, migration, seed data, visibility, and portal labeling.
- Uses configured currency in API requests and reconciliation reports.
- Updates mocks and related tests.
File summaries
| File | Summary |
|---|---|
services/mock-service/src/fsp-integration/onafriq/onafriq.mock.service.ts |
Validates currency presence. |
services/121-service/test/onafriq/export-reconciliation-report.test.ts |
Tests configured reconciliation currency. |
services/121-service/src/seed-data/program/program-onafriq.json |
Seeds the currency property. |
services/121-service/src/migration/1789475507841-add-currency-code-onafriq-fsp-config-prop.ts |
Backfills existing configurations. |
services/121-service/src/fsp-integrations/transaction-jobs/services/transaction-jobs-onafriq.service.ts |
Loads currency configuration. |
services/121-service/src/fsp-integrations/transaction-jobs/services/transaction-jobs-onafriq.service.spec.ts |
Updates transaction fixtures. |
services/121-service/src/fsp-integrations/shared/enum/fsp-configuration-properties.enum.ts |
Defines the new property. |
services/121-service/src/fsp-integrations/shared/consts/fsp-configuration-property-visibility.const.ts |
Defines property visibility. |
services/121-service/src/fsp-integrations/shared/consts/fsp-configuration-property-types.const.ts |
Defines property type. |
services/121-service/src/fsp-integrations/settings/onafriq/onafriq-settings.const.ts |
Marks currency as required. |
services/121-service/src/fsp-integrations/reconciliation/onafriq/onafriq-reconciliation.service.ts |
Loads currency for reports. |
services/121-service/src/fsp-integrations/reconciliation/onafriq/onafriq-reconciliation.mapper.ts |
Maps configured currency. |
services/121-service/src/fsp-integrations/integrations/onafriq/services/onafriq.service.spec.ts |
Updates request fixtures. |
services/121-service/src/fsp-integrations/integrations/onafriq/services/onafriq.api.service.spec.ts |
Updates API fixtures. |
services/121-service/src/fsp-integrations/integrations/onafriq/services/onafriq.api.helper.service.ts |
Uses configured currency in payloads. |
services/121-service/src/fsp-integrations/integrations/onafriq/interfaces/onafriq-request-identity.interface.ts |
Adds currency to request identity. |
interfaces/portal/src/app/domains/fsp-configuration/fsp-configuration.helper.ts |
Adds the portal label. |
Review details
Suppressed comments (1)
services/121-service/test/onafriq/export-reconciliation-report.test.ts:25
- This expected value is read from the same seed JSON entry whose value is resolved from
ONAFRIQ_CURRENCY_CODE, so it remains equal to the legacy global setting. The test would pass even if reconciliation still usedenv.ONAFRIQ_CURRENCY_CODE; use a distinct per-program currency value and assert that value to prove the new behavior.
const currencyCodeOnafriq = onafriqFspConfiguration?.properties.find(
(property) => property.name === 'currencyCodeOnafriq',
)?.value;
- Files reviewed: 17/17 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| [FspConfigurationProperties.corporateCodeOnafriq]: 'string', | ||
| [FspConfigurationProperties.passwordOnafriq]: 'string', | ||
| [FspConfigurationProperties.uniqueKeyOnafriq]: 'string', | ||
| [FspConfigurationProperties.currencyCodeOnafriq]: 'string', |
There was a problem hiding this comment.
Will the UI for this FSP-configuration-setting, use the "Currency-dropdown"?
Or will it use "the currency configured on the program" instead, without the need for the UI-part?
There was a problem hiding this comment.
I think these are very good points @elwinschmitz, thanks for raising them.
Will the UI for this FSP-configuration-setting, use the "Currency-dropdown"?
No it's a free text field. We don't support the dropdown type for program fsp configuration properties.
I also don't think it's worth it:
- This interface is not often used
- We don't know that OnAfriq supports a specific ISO code of currencies. Also I don't think they support all available currencies so I think a big dropdown is more confusing
Or will it use "the currency configured on the program" instead, without the need for the UI-part?
I did think about that. However I think it's better to follow the normal process for program fsp configuration properties to make this more an explicit decision.
In practice process for setting up the currency code is: Gabriele emailing OnAfriq what currency letters we should use and than him settings this up. Also I think they have to configure something on their side to set up this currency code. So using the currency code from the program feel more like a hidden decision, also because it differs per fsp if the currency code from a program is used or not
Let me know what you think about this reationale
Signed-off-by: Ruben <vandervalk@geoit.nl>
67c1b50 to
16b5482
Compare
nielsVoogt
left a comment
There was a problem hiding this comment.
I'm not seeing a whole lot that can go wrong here, but I recommend a second review/approval by another team member.
| describe('Export reconciliation report', () => { | ||
| const programId = 1; | ||
| const transferValue = 12327; | ||
| const currencyCodeOnafriq = env.ONAFRIQ_CURRENCY_CODE; |
There was a problem hiding this comment.
Do we still need the ENV-variable, just for this test?
I'd think its safe to hard-code a value in here.
And get rid of the whole env.ts-part for this variable.
There was a problem hiding this comment.
We also use it in services/121-service/src/seed-data/program/program-onafriq.json
services/121-service/src/seed-data/program/program-onafriq.json
In program seed files, property values matching environment variable names are dynamically resolved from process.env during seeding. Which makes it very easy when developing locally to have proper setup when you want to test with against staging/acceptance environment of an FSP, instead of that you have to reconfigure the FSP on every seed
| [FspConfigurationProperties.corporateCodeOnafriq]: 'string', | ||
| [FspConfigurationProperties.passwordOnafriq]: 'string', | ||
| [FspConfigurationProperties.uniqueKeyOnafriq]: 'string', | ||
| [FspConfigurationProperties.currencyCodeOnafriq]: 'string', |
There was a problem hiding this comment.
Will the UI for this FSP-configuration-setting, use the "Currency-dropdown"?
Or will it use "the currency configured on the program" instead, without the need for the UI-part?
| )?.value as string; | ||
| const currencyCode = programFspConfigProperties.find( | ||
| (c) => c.name === FspConfigurationProperties.currencyCodeOnafriq, | ||
| )?.value as string; |
There was a problem hiding this comment.
This would be the correct place (on the edge) to map/parse this value into a CurrencyCode (from: services/121-service/src/exchange-rates/enums/currency-code.enum.ts), right?
So that OnafriqReconciliationMapper.mapTransactionToReportItem could 'just assume' that its value will be a "three uppercase characters"-string-representing-a-currency....
There was a problem hiding this comment.
If we go the the route of having a dropdown in the interface when selecting the currency, I think this should be a as CurrencyCode
If there is no validation when this input enters the system it's better to assume a string here. And we keep everything dumb and just pass along what the user put in there
| readonly corporateCode: string; | ||
| readonly password: string; | ||
| readonly uniqueKey: string; | ||
| readonly currencyCode: string; |
There was a problem hiding this comment.
Would/could/should this be a CurrencyCode? (See comment below.)
AB#44605
Describe your changes
Add onafriq currency code per program config
Checklist before requesting a code review
Portal preview-deployment
This PR does not have any preview deployments yet.