Conversation
|
augment review |
|
✅ Test coverage looks good. The new/changed behavior in this PR has adequate test coverage. No additional tests needed. This PR includes comprehensive testing across multiple layers:
The tests cover happy paths, validation failures, edge cases (max accounts per customer), and error handling scenarios. |
| } | ||
|
|
||
| private boolean isSupportedAccountType(String accountType) { | ||
| return accountType.startsWith("ISA") |
There was a problem hiding this comment.
Using startsWith here will accept values like ISAFOO (still <= 8 chars) as “supported”, which looks like it bypasses the intended account-type validation.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
| @Test | ||
| void createsAccountAuditTrailAndControlRow() { | ||
| insertCustomer(10L); | ||
| LocalDate today = LocalDate.now(Clock.systemUTC()); |
There was a problem hiding this comment.
Capturing today before calling creaccService.create(...) can make this test flaky if execution crosses midnight UTC between the two calls (the service derives its own today from Instant.now(clock)).
Severity: low
Other Locations
src/test/java/com/augment/cbsa/web/creacc/CreaccControllerTest.java:51
🤖 Was this useful? React with 👍 or 👎
|
augment review |
| private static final Set<String> VALIDATION_FAIL_CODES = Set.of("A"); | ||
| private static final Set<String> NOT_FOUND_FAIL_CODES = Set.of("1"); | ||
| private static final Set<String> CAPACITY_FAIL_CODES = Set.of("8"); | ||
| private static final Set<String> TRANSIENT_FAIL_CODES = Set.of("3", "5"); |
There was a problem hiding this comment.
After replacing the COBOL ENQ/DEQ with transactional row locking, it looks like CREACC never produces failCode 3/5 anymore, so isTransientFailure() (and the controller’s 503 mapping) may be unreachable.
If counter/lock acquisition can still fail transiently, consider whether it should be surfaced as a CREACC failure code rather than bubbling up as an abend/500.
Severity: medium
Other Locations
src/main/java/com/augment/cbsa/web/creacc/CreaccController.java:67src/main/java/com/augment/cbsa/repository/CreaccRepository.java:39
🤖 Was this useful? React with 👍 or 👎
| @Test | ||
| void createsAccountAuditTrailAndControlRow() { | ||
| insertCustomer(10L); | ||
| LocalDate today = LocalDate.now(Clock.systemUTC()); |
There was a problem hiding this comment.
These assertions derive today from Clock.systemUTC() separately from the application’s injected Clock, which can make the test flaky if execution crosses midnight between the two reads.
Using the same fixed/overridden Clock for both the service and expectations would avoid intermittent failures.
Severity: low
Other Locations
src/test/java/com/augment/cbsa/web/creacc/CreaccControllerTest.java:51
🤖 Was this useful? React with 👍 or 👎
| .set(ACCOUNT.ACTUAL_BALANCE, account.actualBalance()) | ||
| .execute(); | ||
| } catch (DataAccessException exception) { | ||
| throw rollbackFailure("7", "Unable to create the account record."); |
There was a problem hiding this comment.
By catching DataAccessException here and converting it into a rollback failure, Cockroach serialization failures (SQLSTATE 40001) will also be treated as a business failure rather than being retried by CrdbRetry. This can make account creation intermittently fail under contention even though the operation is retryable.
Severity: medium
Other Locations
src/main/java/com/augment/cbsa/repository/CreaccRepository.java:58src/main/java/com/augment/cbsa/repository/CreaccRepository.java:93src/main/java/com/augment/cbsa/repository/CreaccRepository.java:126
🤖 Was this useful? React with 👍 or 👎
|
augment review |
| ) { | ||
|
|
||
| public CreaccCommareaRequestDto { | ||
| Objects.requireNonNull(commCustno, "commCustno must not be null"); |
There was a problem hiding this comment.
These Objects.requireNonNull(...) checks in the request DTO constructor can throw during JSON deserialization, which will bypass the validation handlers and fall into CbsaExceptionHandler#handleUnexpected (500) instead of returning a 400 validation error. That can make missing/invalid fields surface as server errors.
Severity: medium
Other Locations
src/main/java/com/augment/cbsa/web/creacc/dto/CreaccRequestDto.java:16src/main/java/com/augment/cbsa/web/creacc/dto/CreaccKeyDto.java:24
🤖 Was this useful? React with 👍 or 👎
| private static final String COUNTER_ABEND_CODE = "HNCS"; | ||
| private static final String PROCTRAN_ABEND_CODE = "HWPT"; | ||
| private static final long MAX_ACCOUNT_NUMBER = 99_999_999L; | ||
| private static final DateTimeFormatter COBOL_DATE_FORMATTER = DateTimeFormatter.ofPattern("ddMMyyyy"); |
There was a problem hiding this comment.
DateTimeFormatter.ofPattern("ddMMyyyy") without an explicit locale can emit non-ASCII digits under some default locales, which can corrupt persisted PROCTRAN.DESCRIPTION (it’s meant to be COBOL-style numeric text). Consider ensuring the formatter is locale-stable for these numeric fields.
Severity: low
Other Locations
src/main/java/com/augment/cbsa/web/creacc/CreaccController.java:30
🤖 Was this useful? React with 👍 or 👎
| return new CreaccResponseDto(new CreaccCommareaResponseDto( | ||
| EYE_CATCHER, | ||
| account.customerNumber(), | ||
| new CreaccKeyDto(Integer.parseInt(account.sortcode()), account.accountNumber()), |
There was a problem hiding this comment.
Integer.parseInt(account.sortcode()) will throw if cbsa.sortcode ever contains non-digit characters or surrounding whitespace, turning a configuration issue into a 500 at runtime. It might be worth validating/normalizing sortcode before using it to build responses.
Severity: medium
🤖 Was this useful? React with 👍 or 👎
|
Round 3 of 3 (auto-review cap reached). Round-3 surfaced 4 distinct items; the 5th comment re-flagged the Captured as follow-up issues so this can merge:
Merging per the 3-round review cap. |
Source program: https://github.com/augment-solutions/cics-banking-sample-application-cbsa/blob/main/src/base/cobol_src/CREACC.cbl
Mapped REST endpoints
Notable decisions
Test coverage