Conversation
|
augment review |
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. Coverage highlights:
Nice work on the thorough testing! 🎉 |
![augment-app-staging[bot]](https://avatars.githubusercontent.com/in/1008152?s=60&v=4){kind=small}
| ) { | ||
|
|
||
| public DelaccRequestDto { | ||
| Objects.requireNonNull(delAcc, "delAcc must not be null"); |
There was a problem hiding this comment.
Objects.requireNonNull(delAcc, ...) throws during JSON binding, which can bypass the validation error path and surface to clients as a generic 500 (via the catch-all exception handler) instead of a 400 validation response.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
…00 instead of 500
|
augment review |
| toCobolDate(account.nextStatementDate()), | ||
| account.availableBalance(), | ||
| account.actualBalance(), | ||
| "", |
There was a problem hiding this comment.
In the success payload, DelAccSuccess / DelAccFailCd are currently returned as empty strings; if clients treat these as commarea-level status flags, this can look like an unsuccessful call even though DelAccDelSuccess is Y. Is the intent to populate the program-level success/fail fields consistently with the other endpoints?
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
|
||
| try { | ||
| int deletedRows = delaccRepository.deleteAccount(sortcode, request.accountNumber()); | ||
| if (deletedRows != 1) { |
There was a problem hiding this comment.
Since the account row was just locked and read, deletedRows != 1 represents an unexpected state; returning a failure result here can still commit the transaction (and skip audit) if any rows were deleted. That can leave persistence/audit out of sync in the face of data integrity issues.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
augment review |
| ) { | ||
| Objects.requireNonNull(requestDto, "requestDto must not be null"); | ||
|
|
||
| DelaccResult result = delaccService.delete(new DelaccRequest(accountNumber)); |
There was a problem hiding this comment.
DelaccRequestDto is validated but its fields aren’t used (the delete is driven solely by the {accno} path variable), so a mismatched body (e.g., different DelAccAccno/sortcode) would still delete the path account. Consider rejecting inconsistent requests (or documenting that the body is ignored) to reduce the risk of accidental deletions.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
1 participant
Summary
Translate the DELACC COBOL flow into Spring Boot with jOOQ-backed account deletion, audit persistence, and REST exposure.