mariiaKraievska commented on PR #6477: URL: https://github.com/apache/fineract/pull/6477#issuecomment-5811962737
> The enforcement is solid. The bucket type is validated on every write path, not just in the dropdowns: loan product create/update (`findDelinquencyBucketIdIfProvided`), and WC product create/update plus WC loan create/modify through the new `WorkingCapitalDelinquencyBucketResolver`, in both directions (REGULAR-only for loan products, WORKING_CAPITAL-only for WC). `bucket_type` is `NOT NULL DEFAULT 'REGULAR'`, so existing rows are reliably discriminated, and every update path only re-validates when the bucket id actually changes, so existing products/loans linked to a mismatched bucket keep working as long as the client resends the same id. Core delinquency behaviour isn't regressed (`retrieveAllDelinquencyBuckets()` still returns everything for `GET /delinquency/buckets`), and creating a WC loan with an unknown bucket id now fails with a 404 instead of silently storing null - a good silent fix. > > Two follow-ups, neither blocking: > > * **The invariant can still be broken from the bucket side.** `updateDelinquencyBucket` lets `bucketType` change while the bucket is in use, and `DelinquencyBucketParseAndValidator.validateAndParseUpdate` defaults a missing `bucketType` to REGULAR - so a PUT on a WC bucket that just omits the field silently turns it into a REGULAR one while WC products still point at it. I'd reject a type change while the bucket is in use (the delete path already has usage checkers) or keep the current type when omitted. Worth a follow-up ticket. > * A legacy WC product created before this with a REGULAR bucket still hands it to new loans that don't send their own `delinquencyBucketId` (the assembler copies `product.getDelinquencyBucket()` without a type check). Worth a line in the JIRA so operators check existing products. > > Nits: the wrong-type error code for the WC loan uses `validation.msg.WORKINGCAPITALLOANPRODUCT...` hardcoded instead of the constant, the message text is duplicated between the two modules, and the WC loan modify path has no test (nor the "legacy product still updatable when the same id is resent" case). The PR description is still the empty template and should list the API behaviour changes (filtered template options, `delinquencyBucketOptions` null when there are no WC buckets). > > Recommendation: APPROVE Addressed the follow-ups in this PR: 1. updateDelinquencyBucket now rejects bucketType changes while the bucket is in use (same usage checks as delete), and omitted bucketType keeps the existing type instead of defaulting to REGULAR 2. WC wrong-type validation code goes through a constant. The message text is shared via DelinquencyApiConstants 3. Added e2e coverage for WC loan modify with a non-WC bucket (rejected) and resenting the same bucket id (accepted) -- 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]
