rymghosn commented on PR #6209:
URL: https://github.com/apache/fineract/pull/6209#issuecomment-6338454151
@adamsaghy Thanks for the review. I rebased on the latest `develop` and went
through every point:
**Bugs 1 and 2 / v2 API**
- `/v2/loanproducts` is gone. The existing `GET /loanproducts` now applies
the new rule directly, and that is the endpoint the mapping dropdown reads, so
the web-app needs no change.
**Description**
- Rewritten below; @aya-abdallah-FOO, please paste it into the PR
description, since I can't edit it. The self-service endpoint and the old
method name are gone. It now says that the root cause is develop's strict
allow-list, not an ignored flag, and that FINERACT-2728's charge filtering is
folded into this PR.
**Issues**
1. **`is_active` regression:** fixed.
`retrieveSavingsProductApplicableCharges(false, …)` filters on
`c.is_active=true` again, so the savings, FD and RD templates no longer offer
deactivated charges. While checking this I found a second silent change, the
charge list by currency had lost its office filter, and it has it again.
2. **Two rules side by side:** removed. Everything that used the old
allow-list now goes through the same id-based filter:
- loan product listing, lookup (search, provisioning, datatable checks),
currency lookup and product mix;
- savings product listings;
- charge listings.
`getSQLWhereClauseForProductIDsForUserOffice_ifGlobalConfigEnabled` and
the `getSQLQueryInClause*` / `retrieveEntityAccessFor` methods are deleted.
`GET /loans/{id}?template=true` now scopes products to the loan's office, the
same as `/loans/template` and the validator.
3. **`GET /charges/{id}`:** the office filter is back. A charge the user
can't see returns 404, as it did before.
4. **Sub-offices:** I've kept this on purpose and the description now states
it. Client, group and account paths (templates, validation) use that client's
office plus its parents. User-scoped listings use the user's office plus its
parents and sub-offices, so the list shows what any office the user manages can
use. With parents only, a head-office admin stops seeing a product as soon as
it is mapped to a single branch, so the mapping screen couldn't add that
product to a second branch, which is bug 1 again. If you'd rather keep it
strict, the change is small: `FineractEntityAccessUtil` would pass
`includeSubOffices=false` for user-scoped listings.
**Tests**
- `FeignOfficeSpecificLoanProductVisibilityTest` now also covers:
- `GET /loanproducts` as a branch user and as a head-office user (bugs 2
and 1);
- the client charge template filtering by the client's office;
- `GET /charges/{id}` returning 404 for another office's charge;
- the savings template not offering inactive charges.
<details><summary>Updated PR description</summary>
## Bug
With the global configuration `office-specific-products-enabled` on, develop
treats the entity-to-entity mappings as a strict **allow-list**: an office only
sees the loan products, savings products and charges that are explicitly mapped
to that exact office. Anything unmapped, mapped to a parent office, or mapped
to another office disappears. As a result:
1. **Entity to Entity Mapping screen**: the Loan Product dropdown (fed by
`GET /loanproducts`) shows only "All". There are no individual products to map,
so the mappings the feature depends on can't be created.
2. **Loan product listing** (`GET /loanproducts`): branch users see no
products, including products that have no mapping or that are mapped to a
parent office.
3. **Loan application template** (`GET /loans/template`): the product list
is empty, so no new loan application can be started.
The same allow-list is applied to charges (FINERACT-2728) and savings
products.
## Fix
There is now one visibility rule, and every read path uses it. It is
computed once per office and applied by id. No SQL `IN` clauses are built any
more.
**Rule (flag on).** An entity is visible to an office when any of these
holds:
- it has no mapping for that relation;
- the office, or one of its ancestors, has an "All" (`toId = 0`) mapping;
- it is mapped to the office or to one of its ancestors.
**Which office is used:**
- **Account and client paths use that client's, group's or account's
office:** the loans template, `GET /loans/{id}?template=true`, the loan
application validator, client/savings/deposit/share templates, and loan or
savings account charges.
- **User-scoped listings** (`GET /loanproducts`, which feeds the mapping
dropdown; product lookups for search, provisioning, datatable checks and
product mix; savings product listings; charge listings; `GET /charges/{id}`)
use the user's office **plus its sub-offices**. A listing shows what any office
the user manages can use. Without this, a head-office admin could not see a
product once it was mapped to a single branch, so the mapping screen could not
add a second branch to it.
With the flag off nothing changes.
**Implementation**
- `FineractEntityAccessReadService.retrieveOfficeEntityAccess` loads the
relation's mappings through `FineractEntityToEntityMappingRepository` with an
interface projection, and the office line through JPQL on `Office.hierarchy`.
It returns the restricted and granted id sets
(`FineractOfficeEntityAccessData`).
- The result is cached per tenant/office (`officeEntityAccess`). The cache
is evicted on entity-mapping create, update and delete, on office create and
update, and when a product or charge is automatically restricted to its
creator's office.
- `FineractEntityAccessUtil.filterVisibleToOffice_ifGlobalConfigEnabled`
filters rows by id. All callers of the old
`getSQLWhereClauseForProductIDsForUserOffice_ifGlobalConfigEnabled` have been
moved to it: `LoanProductReadPlatformServiceImpl` (listing, lookup, currency,
product mix), `SavingsProductReadPlatformServiceImpl` and
`ChargeReadPlatformServiceImpl`. The old method has been removed, along with
the `getSQLQueryInClauseIDList_For*` /
`getSQLQueryInClause_WithListOfIDsForEntityAccess` / `retrieveEntityAccessFor`
methods.
-
`LoanProductReadPlatformService.retrieveAllLoanProductsForLookup(activeOnly,
officeId)` scopes product options to a client's or group's office. It is used
by the loans template and by `GET /loans/{id}?template=true`.
- Charges (FINERACT-2728, squashed into this PR as agreed): client, loan,
savings, deposit and share templates filter charge options and pre-selected
product charges by the client's or account's office. `GET /charges/{id}` keeps
its office filter: a charge the user can't see returns 404.
- No new endpoint. The existing v1 `GET /loanproducts` is fixed directly.
## Tests
- Unit: `FineractOfficeEntityAccessDataTest` covers the visibility rule.
`FineractEntityAccessReadServiceImplTest` covers the ancestor and sub-office
resolution.
- Integration (`FeignOfficeSpecificLoanProductVisibilityTest`):
- the loans template for a branch client lists unmapped, ancestor-mapped
and own-office products, but not a sibling office's product (bug 3);
- `GET /loanproducts` as a branch user follows the same rule (bug 2), and
as a head-office user lists every product, including branch-mapped ones (bug 1,
the mapping dropdown);
- the client charge template follows the client's office, and `GET
/charges/{id}` returns 404 for a charge mapped to another office;
- the savings template does not offer inactive charges;
- with the flag off, every product is listed.
</details>
--
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]
