rymghosn commented on PR #6288: URL: https://github.com/apache/fineract/pull/6288#issuecomment-6335372376
@adamsaghy @galovics Thanks for both reviews. I rebuilt the change on the latest `develop` as one commit (d27b4ff357), so the conflict and the out-of-order imports from the merge commit are gone. Point by point: - **"Strictly after today" isn't enforced:** I've changed the description, not the code. The rule is "on or after the business date". That's what a percentage change without `startDate` already does (the new rate starts on the business date), and it's the same rule as for tax group components (FINERACT-2769). Requiring "strictly after" for an explicit date only would contradict that. - **Full-form PUT refused / `parameterExists`:** only a real change is refused now, in one loop over the locked fields, each with its own error. A whole-form PUT with the current values and a new name passes (unit test and IT). An unchanged past `startDate` in the payload is also ignored now; before, it failed the business-date check. - **Start-date rules fighting each other:** an in-use component only gets the lock check. Its start date is locked even before it takes effect, as in the ticket. The date rules apply only to an unused component and only when the date actually changes. The PR description had this wrong before; it's corrected. - **GL accounts on update not validated / unknown type stored as null:** update uses the create validation (type range, id > 0, type and id together), so `debitAccountType: 9` is a 400. `TaxAssembler` now resolves the accounts for both create and update, so an update also checks that the account matches its type and is not a header account. The hand-written account loading in the service is gone. - **"In use" only checked charges:** it now also covers savings products and savings accounts (the same links as `TaxGroupRepository.isLinked`, so withholding tax counts). It also covers loan-charge and savings-transaction tax detail rows for the component, so moving a charge to another tax group doesn't unlock a component that already has transactions. - **Moving a future start date leaves a gap:** a start-date-only move now also moves the previous rate's end date, and the new date must be after that rate's start. `TaxComponentTest` checks `getApplicablePercentage` on both sides of the moved date. - **`ChargeRepository` native SQL / layering:** the query lives in `TaxComponentRepository` now, next to `TaxGroupRepository.isLinked`, and nothing in the charge module changed. It stays native because `fineract-tax` can't see the charge, savings or loan entities; it's `EXISTS`-based, not a count. - **Entity design:** `isInUse(Supplier)` is gone; the service asks the repository. `update(command)` keeps its signature, and the accounts go through `updateDebitAccount` / `updateCreditAccount`, which take resolved values, not the command. `updateStartDate` is no longer called twice. - **Read side:** the flag is `editable`, a primitive. It's on the list, the single GET and the template, and is set through `TaxComponentData.withEditable(...)` rather than copying fields by hand. Swagger has it, plus the new PUT fields. - **Tests:** `TaxValidatorTest` (14), `TaxComponentTest` (4) and the `TaxComponentEditLockTest` integration test (locked vs editable, whole-form rename, the template flag). I ran them with the existing tax ITs against PostgreSQL, all green. On #6427: this branch now covers the same ground with a different "in use" definition and the start-date move. I'm happy to fold anything from it that you'd rather keep, or to align with whichever one you want to take forward. -- 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]
