oleksii-novikov-onix commented on PR #6704: URL: https://github.com/apache/fineract/pull/6704#issuecomment-6364521584
> Hi @oleksii-novikov-onix, thanks for the PR. NULL instead of the empty string placeholder makes a lot of sense, and the named constraint finally makes the existing `wc_loan_account_no_UNIQUE` mapping in the validator work. Two things before I approve. > > **Flush in `submitApplication`:** > > ```java > final WorkingCapitalLoan saved = this.repository.saveAndFlush(loan); > - this.assembler.accountNumberGeneration(command, saved); > - this.repository.saveAndFlush(saved); > + this.assembler.accountNumberGeneration(saved); > ``` > > I'm not convinced about dropping the second flush. The generated number is now only a dirty field on the managed entity, so the UPDATE goes out whenever the persistence context decides to flush. If that ends up being the commit, a collision on the generated number escapes the try block and we're back to the generic data integrity error instead of `error.msg.wc.loan.duplicate.accountNo`. The E2E passes, so something in between probably triggers a flush today, but I'd rather not rely on that. Can we keep an explicit `flush()` right after `accountNumberGeneration(saved)`? In that case `handleDataIntegrityIssues` reads `accountNo` from the command, so the message will say "accountNo `null` already exists". Thoughts? > > **Changelog number:** `parts/0090_wc_loan_account_no_named_unique.xml` has the same number as #6542 and #6662. The changeset ids differ so nothing breaks, but `module-changelog-master.xml` will conflict. Whichever merges second renumbers. > > Small ones, none blocking: > > * the E2E collision step guesses the next number from `getCreatedLoanId() + 2` and a hard-coded length of 9, which breaks if another loan gets created in between > * leftover `""` fallback in `updateFrom`, outdated Javadoc, redundant `unique = true` > * a unit test for the blank-to-null mapping would be nice Thanks for the review @galovics 1. Flush after generation - fixed. Added an explicit flush() right after accountNumberGeneration(saved), so a collision is caught inside the try. 2. “accountNo null” in the message - fixed. The message now shows the account number of the loan being created, so a collision on a generated number names that number. Validation and assembly moved above the try, so the catch can read the loan. 3. Changelog number 0090 - not changed. Agreed, whichever merges second renumbers. 4. E2E guesses the next id and hard-codes length 9 - partly fixed. The length now comes from the first loan’s generated number. The id is still predicted, because the number is built from the id assigned on insert, so the step makes up to 3 attempts if another loan takes the predicted id. 5. “” fallback in updateFrom - fixed. Removed, because accountNo is validated as notBlank on modify, so the fallback was dead code. 6. Outdated Javadoc - fixed. Both Javadocs now describe the check on the loan state. 7. Redundant unique = true - fixed. Removed, the named @UniqueConstraint on the table covers it. 8. Unit test for blank-to-null mapping - fixed. Added WorkingCapitalLoanAssemblerAccountNumberTest. -- 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]
