You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@fineract.apache.org by "DeathGun44 (via GitHub)" <gi...@apache.org> on 2026/10/02 04:14:20 UTC

[PR] FINERACT-2883: migrate the teller, email, credit bureau and scheduler job result tests to feign [fineract]

DeathGun44 opened a new pull request, #6561:
URL: https://github.com/apache/fineract/pull/6561

## Description

FINERACT-2883: the last part of roadmap row 13 of FINERACT-2609. #6559 moved 24 organisation, configuration and jobs test classes to the Feign client. This PR moves the seven that #6559 left out because each one needed a spec fix first. With it, every class row 13 still listed is migrated. The Retrofit tests in `client/` (for example `client/OfficeTest`) are row 17 and are not touched.

**Stacked on #6559.** Until #6559 merges, the diff here also shows its commits. The six commits of this PR start after `FINERACT-2883: remove the legacy helpers left without callers` (#6559's last commit).

| Area | Class | Tests | Was on |
|---|---|---|---|
| Teller | `organization/teller/CashierSummaryAndTransactionsTest` | 1 | REST Assured |
| | `organization/teller/AllocateCashToCashierValidationTest` | 3 | REST Assured |
| Email | `campaigns/EmailTest` | 3 | REST Assured |
| Credit bureau | `CreditBureauConfigurationValidationTest` | 12 | REST Assured + Retrofit |
| | `CreditBureauConfigurationTest` | 1 | Retrofit |
| | `CreditBureauTest` | 2 | Retrofit |
| Jobs | `SchedulerJobsTestResults` | 20 | REST Assured + Retrofit |

Every test name is kept.

### Commits

1. Credit bureau: the schemas, a Feign helper, the three tests, and removal of the two legacy credit bureau helpers. These go together because the schema fix retypes the generated Retrofit API those helpers called.
2. OpenAPI: teller status, email payloads, loan `isNPA`.
3. Feign helpers for the teller and email tests.
4. Tests: teller and email.
5. Tests: `SchedulerJobsTestResults`.
6. Removal of the legacy helper code these tests were the last users of, and of their `restassured-baseline.xml` entries.

Each commit compiles on its own.

### OpenAPI fixes (no runtime change)

Each one was checked against the running server before it was written:

1. **`POST /v1/tellers`**: `status` was documented as the `TellerStatus` enum, so the generated client sent `"ACTIVE"`, and the server answers that with 400 `validation.msg.invalid.decimal.format`, because `Teller.fromJson` reads an integer. It is now an `Integer` (100 pending, 300 active, 400 inactive, 600 closed). This is why a teller could not be created through the typed client, and why the teller tests stayed on REST Assured. The response side (`GET /v1/tellers/{id}`) really does return the enum name and is unchanged.
2. **`/v1/email`**: create, retrieve one, update and delete had no request or response schema, so the client typed them as `String`. They now document `PostEmailRequest`, `PutEmailRequest`, `EmailData` and `CommandProcessingResult`.
3. **`/v1/CreditBureauConfiguration`**: the seven operations the tests use had no request or response schema. They now document the request bodies the server's deserialisers accept, `OrganisationCreditBureauData`, `CreditBureauConfigurationData` and `CommandProcessingResult`.
4. **`POST /v1/creditBureauIntegration/creditReport`**: the request body was documented as `Object`. It is now `PostCreditReportRequest` (`creditBureauID`, `NRC`), with a `CommandProcessingResult` response, which carries the report in `creditBureauReportData`.
5. **`GET /v1/loans/{loanId}`**: added `isNPA`, which the server returns at the top level and `SchedulerJobsTestResults` asserts before and after the NPA job.

### Findings worth a look

- **Creating a cashier never returns the cashier's id.** `POST /v1/tellers/{tellerId}/cashiers` answers `{"resourceId": <tellerId>}`. The service sets `subResourceId` from `cashier.getId()` straight after `save()`, before an id is assigned. The old `AllocateCashToCashierValidationTest` used the teller id as the cashier id, so it only passed when the two happened to match; on develop it fails with a 404 here. `CashierSummaryAndTransactionsTest` hardcoded teller 1 and cashier 1. Both now look the new cashier up among the teller's cashiers by its unique description, and both pass. The server fix belongs in its own ticket.
- **`PUT /v1/tellers` has the same `status` problem** as the create. Nothing here updates a teller, so I left it alone. A `PUT` with an integer status and a description also fails with 403 `The given id must not be null`, which is a separate server defect.
- **Two small typed interfaces**, the same approach as `StaffCommandsApi`:
- `TellerCommandsApi`: the allocate-cash validation tests send a non-numeric `txnAmount` (the model types it as `BigDecimal`) and a body that is not valid JSON. Neither can go through the generated model.
- `CreditBureauIntegrationCommandsApi`: in `integration-tests` the generated `PostCreditReportRequest` resolves to the Retrofit model, which names the field `NRC` only in a Gson annotation, so Jackson sends `nrc`. The server then calls the bureau's search URL without an id. I confirmed this from the server log. The schema fix above is still right for real SDK users.
- **`SchedulerJobsTestResults` keeps its exact payloads.** I checked each request against the legacy JSON builders key by key before running anything:
- approval sends no `approvedLoanAmount`;
- disbursal sends `netDisbursalAmount`;
- loan applications send `charges: []`, a client collateral and `maxOutstandingLoanBalance` 36000;
- the overdue fee keeps its monthly `feeFrequency`/`feeInterval`.

One legacy call read differently from what it sent: `withLockinPeriodFrequency("1", DAYS)` takes its arguments as (type, frequency), so it sent a lock-in of 0 weeks. The migrated test sends the same 0 weeks.
- **`SchedulerJobsTestResults`** now extends `FeignLoanTestBase`, which carries the `LoanTestLifecycleExtension` it declared before, and also `ExternalEventsExtension`. `@Order(1)` is kept.
- `SchedulerJobHelper`, `BusinessDateHelper` and `HolidayHelper` were already Feign-based and are used as they are. `StandingInstructionsHelper` only lost an unused REST Assured constructor parameter, which takes it off the baseline.

### Legacy helper cleanup

- Deleted `CreditBureauConfigurationHelper`, `CreditBureauIntegrationHelper` and `CashierTransactionsHelper`, which have no users left.
- Removed the methods these tests were the last callers of from `LoanTransactionHelper`, `LoanStatusChecker`, `SavingsAccountHelper`, `ChargesHelper`, `AccountHelper`, `JournalEntryHelper`, `FixedDepositAccountHelper` and `FixedDepositAccountStatusChecker`. I checked callers against the parent branch, so nothing that was already unused was touched.
- Removed 8 `restassured-baseline.xml` entries: the five migrated REST Assured tests, `CashierTransactionsHelper`, and `StandingInstructionsHelper` and `FixedDepositAccountStatusChecker`, which no longer import REST Assured.

### API compatibility

`verify-api-backward-compatibility` will report:
- **R010 and R011 on `POST /v1/tellers`:** `status` changes from a string enum to an integer. This is the correction in fix 1.
- **R012 on the twelve email and credit bureau operations above:** each one documented only a `default` response with an untyped string, and the real `200` schema replaces it. Nothing on the wire changes.

The new request schemas and `isNPA` cause no violations. I reproduced this on a sliced spec with the real `checkBreakingChanges` task, after an identical-spec control passed.

### Verification

- **Baseline on the parent branch** (the original seven classes, live server): 42 tests, 41 passed, 1 failed. `AllocateCashToCashierValidationTest.allocateCashWithValidNumericAmountIsNotRejectedAsInvalidNumber` gets a 404 because of the cashier-id defect above.
- **This branch, same server and database, on the final tree:** 42 tests, 42 passed.
- **Static build:** the CI static build (Error Prone), including `compileTestJava` for `fineract-e2e-tests-core`, `fineract-e2e-tests-runner`, `oauth2-tests` and `twofactor-tests`. `checkstyleTest`, `spotbugsTest` and `spotlessCheck` on `integration-tests`; `checkstyleMain` and `spotlessCheck` on `fineract-provider`, `fineract-core` and `fineract-branch`. All green.
- **Each commit** compiles `integration-tests` on its own.

## Checklist

- [x] Write the commit message as per our guidelines
- [x] Acknowledge that we will not review PRs that are not passing the build
- [x] Create/update unit or integration tests for verifying the changes made
- [x] Follow our coding conventions
- [x] Add required Swagger annotation and update API documentation
- [x] This PR must not be a "code dump"
- [ ] If merging this PR resolves a JIRA issue, I will mark that issue as resolved and set "Fix Version/s" appropriately

--
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