Cocoa-Puffs commented on PR #6659: URL: https://github.com/apache/fineract/pull/6659#issuecomment-6264475821
- Why is "USD" hardcoded as a fallback when the org has no currencies? Silently inventing a currency feels off, shouldn't this be a validation error like the multi-currency case? In practice that case can’t happen. Every tenant is seeded with USD in `m_organisation_currency` (0002_initial_data.xml:3426). The only way to change the list, PUT `/currencies`, takes a `CurrencyUpdateRequest` whose list is `@NotNull` `@NotEmpty`, so it can’t be emptied through the API. So the fallback is code that can’t be reached; the requester’s spec asked for it, but it never runs. If currencyCode is left out and the organisation doesn’t have exactly one currency (none or several), the request gets 400 currencyCode.cannot.be.blank. The USD/2 constants are gone, and the Swagger description now says “defaults to the organisation currency when the organisation has exactly one; required otherwise”. The test that expected USD now expects the error. - The 1200 default lives in the Java fallback, both Liquibase changesets and the stepdef. Which is the truth? A 0 or negative value is silently turned into 1200, I would rather fail loudly. Also, why two global configs and two changelogs (0260/0261) for the same cap? `DEFAULT_MAX_PERIODS` is gone from the service. A null value throws `GeneralPlatformDomainRuleException` with `error.msg.configuration.payoff-schedule-preview-max-term-periods.value.is.not.set` (or the payoff-periods equivalent). That’s the same pattern `JournalEntryWritePlatformServiceImpl` uses for `office-opening-balances-contra-account`. 0 is applied literally, so every request is rejected with “exceeds the maximum of 0”, which is a loud failure. Negative values can’t be stored, because the config API already validates `zeroOrPositiveAmount()`. As for why there are two configs with the same cap, they are not the same cap, just the default is the same. `max-payoff-periods` limits fixed-payment mode, which is cheap: 44 ms at 1,200 installments. `max-term-periods` limits term mode, which goes through the engine and grows with the square of the period count: 9.6 s at 1,200. A tenant will likely want them set differently, which is why I chose separate configs. Originally the ticket did not include a `max-term-periods` property which resulted in being able to send in a request with a 10.000 month long term or more which could be used to destabilise the system. One config per changelog file follows the existing pattern (e.g. 0251_add_disallow_backdated_transactions_configuration.xml). - Do we need the doubling rate-period growth (16 to 32 ...) on a preview endpoint, can't we build maxPayoffPeriods once and walk them? Building the full cap up front would make every request, including a 6-installment one, cost as much as the longest allowed schedule (about 25–40 ms at 1200, judging by the 44 ms full run). The cap is a tenant setting. At 100,000, every preview would build 100,000 periods. The doubling keeps the cost tied to the schedule actually produced, and it’s only four lines. If you would prefer the simpler code, it’s an easy change. But I think it would be a real performance cost. - interestMethod accepts one value and echoes it back, and decimalPlaces pulls in a CurrencyData rebuild. Were both asked for in the ticket? The ticket lists both inputs: “interestMethod (DECLINING_BALANCE only for now), currencyCode/decimalPlaces (default to the org currency)”. The CurrencyData rebuild is how decimalPlaces takes effect: the engine rounds using the currency’s decimal places, so the override has to be put into the currency it receives. - The validator, service and feature file repeat many of the same cases, keep each case in one layer. Nearly every rejection scenario in the feature repeats a unit test: both/neither field, mixed units, non-multiple term, zero period, missing locale, scale, interest coverage. So do the happy paths. The codebase does have E2E validation scenarios next to unit-tested validators (e.g. WorkingCapitalBreachConfiguration.feature), so it isn’t against house style. The feature could be trimmed to: the spec’s two worked examples (one term, one fixed payment) one multi-error scenario, which proves the wiring and the error format the cap scenario, which needs live config and can’t be unit tested I'd wait for QA with this call, maybe some tests could be rewritten instead of outright deleted. - Every scenario has # TODO: add TestRail ID, please get real IDs in before merge. As I do not have access to testrail I would wait for QA with this one as well. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
