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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
