Aana123 commented on issue #113: URL: https://github.com/apache/fineract-consumer-facing/issues/113#issuecomment-6388417086
Hi @edk12564 @Aman-Mittal, I was looking into this and it seems the core issue was addressed in 1028685 (“Fix duplicate action fingerprint overloads”), which replaced the four overloads with a single actionFingerprint(String endpoint, Object... parts), and hardened in 4ee32f0 (null handling + restricted part types). So the original problem looks resolved. While reading through the callers, I noticed a few remaining gaps related to the concern in this issue: 1. Transfers still computes the fingerprint inline twice. TransfersCommandServiceImpl builds the same actionFingerprint(...) call separately in initiate() ([L94-L99](https://github.com/apache/fineract-consumer-facing/blob/656be00/consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java#L94-L99)) and confirm() ([L115-L120](https://github.com/apache/fineract-consumer-facing/blob/656be00/consumer/src/main/java/org/apache/fineract/consumer/transfers/command/service/TransfersCommandServiceImpl.java#L115-L120)). Beneficiaries and Users route both steps through a single helper, so Transfers is still the one place where the two calls could drift apart. 2. Beneficiaries keeps a local fingerprintPart(BigDecimal) ([L270-L272](https://github.com/apache/fineract-consumer-facing/blob/656be00/consumer/src/main/java/org/apache/fineract/consumer/beneficiaries/command/service/BeneficiariesCommandServiceImpl.java#L270-L272)) that duplicates the normalization now inside StepUpTokenService. It also maps null to "", while the service maps it to "null". 3. No test verifies that initiate and confirm produce the same fingerprint. TransfersCommandServiceImplTest mocks StepUpTokenService, so the tests would still pass if the two calls diverged. Proposed change (small, no behaviour change for transfers): 1. Extract a private transferActionFingerprint(...) helper in TransfersCommandServiceImpl and use it in both initiate() and confirm() 2. Remove the local fingerprintPart in Beneficiaries and pass the BigDecimal directly to the service 3. Add a test asserting that the fingerprint passed to issue() during initiate equals the one passed to verify() during confirm One thing to confirm: removing the Beneficiaries helper changes the fingerprint when transferLimit is null ("" → "null"), so a beneficiary step-up started just before a deploy would fail once until its token expires. If that’s not acceptable, I can keep the existing null mapping for compatibility. Would this be useful? If so, could you assign this to me? I’d be happy to open a PR. -- 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]
