[PR] FINERACT-2455: WC - Generate account number without empty string placeholder [fineract]

["oleksii-novikov-onix (via GitHub)" <gi...@apache.org>]

oleksii-novikov-onix opened a new pull request, #6704:
URL: https://github.com/apache/fineract/pull/6704

   ## Description
   
   1. A WC loan without accountNo is now inserted with NULL instead of empty string, so concurrent creates without accountNo no longer wait on the unique index.
   2. The generated number is written once, after the insert that assigns the id. The second saveAndFlush is removed.
   3. accountNumberGeneration decides from the loan state, without re-parsing the request.
   4. The auto-named unique constraint is replaced by wc_loan_account_no_UNIQUE. A duplicate account number detected by the database now returns error.msg.wc.loan.duplicate.accountNo instead of a generic data integrity error.
   5. E2E: C70251 now covers a collision between a generated and a manually entered account number.
   
   ## Checklist
   
   Please make sure these boxes are checked before submitting your pull request - thanks!
   
   - [ ] Write the commit message as per [our guidelines](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#pull-requests)
   - [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
   - [ ] Create/update [unit or integration tests](https://fineract.apache.org/docs/current/#_testing) for verifying the changes made.
   - [ ] Follow our [coding conventions](https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions).
   - [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/legacy-docs/apiLive.htm with details of any API changes
   - [ ] [This PR must not be a "code dump"](https://cwiki.apache.org/confluence/display/FINERACT/Pull+Request+Size+Limit). Large changes can be made in a branch, with assistance. Ask for help on the [developer mailing list](https://fineract.apache.org/#contribute).
   - [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately.
   - [ ] I followed the [AI Policy](https://github.com/apache/fineract/blob/develop/CONTRIBUTING.md#ai-policy).
   
   Your assigned reviewer(s) will follow our [guidelines for code reviews](https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide).
   


-- 
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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] FINERACT-2455: WC - Generate account number without empty string placeholder [fineract]

["oleksii-novikov-onix (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org