alberto-art3ch commented on PR #6515: URL: https://github.com/apache/fineract/pull/6515#issuecomment-5916898554
> ## Review > The strategies line up with the resources (`createBreachAction`, `createDelinquencyAction`, `createNearBreachActionById/ByExternalId`), the path indexes are right and the permission checks still run on the batch path. Follows the existing WC batch strategies nicely. > > The one thing I'd want before merging: there's no integration/e2e test. The earlier WC batch work has one (`BatchWorkingCapitalLoanIntegrationTest` / `WorkingCapitalBatchApiStepDef`), and the unit tests here only prove the strategy calls the resource. They don't prove a real `POST /batches` with these URLs resolves and runs, including inside `enclosingTransaction=true`, or that a sub-request can reference the loan created by an earlier one (`$.resourceId`). Can we add at least one batch test covering the id and external-id forms, e.g. create a loan, then a delinquency or breach action in the same batch? > > Nit: "batcheable" -> "batchable" in the title, and the headline says breach and delinquency actions but the PR covers near-breach as well. > > Heads up, this and #6519 both append at the end of `CommandStrategyProvider.init()` and `CommandStrategyProviderTest`, so whichever merges second needs a trivial rebase. > > Recommendation: COMMENT Thanks for the review! Batch integration test: Added three tests to BatchWorkingCapitalLoanIntegrationTest. Each one sends a single POST /batches with enclosingTransaction=true that submits, approves and disburses a loan, then creates the action: After each batch, the test reads the action back through the regular GET endpoint to confirm it was stored, not just that the batch returned 200. -- 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]
