edk12564 commented on PR #39:
URL: 
https://github.com/apache/fineract-consumer-facing/pull/39#issuecomment-4986200289

   > ### 🟠 Major (6)
   > 3. Magic numbers —  NON_CLOSED_STATUS_IDS = Set.of(100L, 200L, 300L, 303L,
   >    304L)  with no documentation of what these Fineract status codes mean.
   > 4. initiateAdd / initiateUpdate  lack  @transactional(readOnly=true)  —
   >    the  existsByUserId...  check runs without a transaction context, 
widening
   >    the TOCTOU window.
   > 5. accountType  is accepted as raw  String  — the enum
   >    BeneficiaryAccountType  exists; use it directly in the DTO and let 
Jackson
   >    handle deserialization errors cleanly.
   > 6. UPDATE_ENDPOINT  constant value ( "/api/v1/beneficiaries/update" )
   >    doesn't match the actual PUT route — misleading as an action fingerprint
   >    salt.
   > 7. BeneficiaryQueryRepository  imports the command-side  Beneficiary
   >    domain class — breaks CQRS package isolation.
   > 8. call()  Feign-exception wrapper duplicated across Beneficiaries, Loans,
   >    Savings, Transfers — extract a shared utility.
   > 
   > ### 🟡 Minor (5)
   > 9. @tostring  on  BeneficiaryCommandData / BeneficiaryQueryData  logs
   >    transferLimit  — use opt-in pattern.
   > 10. SessionCommandData  regression — removed the defensive
   >     @tostring(onlyExplicitlyIncluded = true) .
   > 11. maskEmail()  returns unmasked string when input has no  @  symbol.
   > 12. DeviceFingerprintFilter  also returns 401 for fingerprint mismatch —
   >     should be 403.
   > 13. Removed "browse mode / detail mode" comments in template DTOs — move to
   >     Javadoc.
   
   Hi @Aman-Mittal, a new commit solving these problems is out now. Please let 
me know what you think. 
   
   A few more items to cover: 
   - I also had a question on the Valkey migration above that I'd like your 
experience on. 
   - As for the CQRS Aspect change, I added that with the intention to use it 
later with auditing. This is why there is no function at the moment.  
   - Finally, for Spring Modulith, I will be raising a new PR I think to 
address this issue since it covers items not in this 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]

Reply via email to