adamsaghy commented on PR #6298: URL: https://github.com/apache/fineract/pull/6298#issuecomment-5388068920
> Verified this actually builds and the module's tests stay green - component scan, MapStruct-generated mappers, and bean-name-based batch command wiring all still resolve correctly after the move. Nice work. > > One thing worth fixing before merge: > > `WorkingCapitalLoanConstants.WORKING_CAPITAL_LOAN_TRANSACTION_IDENTIFIER = "WC"` is now a second copy of a constant that already exists in `AccountingProcessorHelper` (which still writes the prefix onto journal entry transaction IDs), while the new copy is only used to read it back in `WorkingCapitalLoanJournalEntryDataMapper`. Two magic strings that have to stay in sync - if they ever drift, journal entry event serialization just silently stops matching, with no error. Since `fineract-provider` already depends on `fineract-working-capital-loan`, there's nothing stopping `AccountingProcessorHelper` from referencing this new constant instead of declaring its own. Given the PR is about consolidating WC code into one module, this is exactly the kind of duplication it should be closing, not adding to. > > Smaller thing: `dependencies.gradle` pulls in `org.apache.httpcomponents:httpclient`, but the only `org.apache.http` import in the module (`HttpStatus`) lives in `httpcore`, which is what every other feature module in the repo declares for the same import. Worth swapping to match convention and avoid the extra transitive. > > Also noticed about a dozen other `WorkingCapital*` classes (COB executors, the accounting processor, the COB API filter) are still sitting in `fineract-provider` - fine as incremental work, but worth a note in the description since it's directly why the constant above got duplicated instead of moved. > > Recommendation: APPROVE Thank you. I will address them in the follow up PRs. -- 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]
