opensource-joe opened a new pull request, #380:
URL: https://github.com/apache/fineract-backoffice-ui/pull/380
## What and why
The 21 routes in `clients.routes.ts` all inherited their tab from the
section above them, so every page under `clients` read `Clients · Fineract`.
Measured in a browser against this branch's own parent, all 21 of them:
```
/clients Clients · Fineract
/clients/create Clients · Fineract
/clients/view/1 Clients · Fineract
/clients/1/identifiers/edit/1 Clients · Fineract
/clients/1/collaterals/create Clients · Fineract
...21 of 21 identical
```
This is the first of the 21 files in #355. It is deliberately one feature
area, so the naming convention can be settled here and reviewed once rather
than 20 more times. `clients` was the right file to settle it on because it
contains every shape the convention has to answer: an empty path, a
record-scoped page, a page whose heading is a record's name, and two-level
nested sub-resources.
Refs #355
## The convention
In the order the rules apply:
1. **The section root takes the same `nav.*` key its section entry uses in
`app.routes.ts`**, restated rather than inherited, so the file reads on its
own. `''` gets `nav.clients`, exactly as `groups.routes.ts` restates
`nav.groups`.
2. **Every other route takes the key its page already uses for its own
visible heading**, so the tab and the page agree.
3. **Where the heading is a record's name and cannot be a static key, the
route takes a generic key for the screen type.** Only `view/:id` needs this. It
uses `CLIENTS.DETAILS`, matching `GROUPS.GROUP_DETAILS`.
**No new translation keys.** All 21 already existed, because rule 2 selects
a key the page had to have in order to render its own heading. That is a
property of the convention rather than luck about this file, and it is the main
argument for it: a correct title costs nothing to translate and cannot drift
from the heading it was taken from.
| Route | Key | Tab |
|---|---|---|
| `''` | `nav.clients` | Clients |
| `search` | `CLIENT_SEARCH_V2.TITLE` | Advanced Client Search |
| `create` | `CLIENTS.CREATE_CLIENT` | Create Client |
| `edit/:id` | `CLIENTS.EDIT_CLIENT` | Edit Client |
| `view/:id` | `CLIENTS.DETAILS` | Client Details |
| `:clientId/identifiers/create` | `CLIENTS.ADD_IDENTIFIER` | Add Identifier
|
| `:clientId/identifiers/edit/:id` | `CLIENTS.EDIT_IDENTIFIER` | Edit
Identifier |
| `:clientId/addresses/create` | `CLIENTS.ADD_ADDRESS` | Add Address |
| `:clientId/addresses/edit/:id` | `CLIENTS.EDIT_ADDRESS` | Edit Address |
| `:clientId/family-members/create` | `CLIENTS.ADD_FAMILY_MEMBER` | Add
Family Member |
| `:clientId/family-members/edit/:id` | `CLIENTS.EDIT_FAMILY_MEMBER` | Edit
Family Member |
| `:clientId/notes/create` | `CLIENTS.ADD_NOTE` | Add Note |
| `:clientId/notes/edit/:id` | `CLIENTS.EDIT_NOTE` | Edit Note |
| `:clientId/documents/create` | `CLIENTS.ADD_DOCUMENT` | Add Document |
| `:clientId/documents/edit/:id` | `CLIENTS.EDIT_DOCUMENT` | Edit Document |
| `:clientId/charges` | `CLIENT_CHARGES.TITLE` | Client Charges |
| `:clientId/charges/create` | `CLIENT_CHARGES.CREATE` | Add Charge |
| `:clientId/collaterals` | `CLIENT_COLLATERAL.TITLE` | Client Collateral |
| `:clientId/collaterals/create` | `CLIENT_COLLATERAL.CREATE` | Add
Collateral |
| `:clientId/collaterals/edit/:id` | `CLIENT_COLLATERAL.EDIT` | Edit
Collateral |
| `:clientId/transactions` | `CLIENT_TRANSACTIONS.TITLE` | Client
Transactions |
## Verification
**In a browser, against the real `en.json`.** A unit spec with a fake
catalogue can only prove the wiring, not that the tabs read correctly, so both
states were measured with Playwright against the `mocked` project. All 21
routes, after:
```
/clients Clients · Fineract
/clients/search Advanced Client Search · Fineract
/clients/create Create Client · Fineract
/clients/edit/1 Edit Client · Fineract
/clients/view/1 Client Details · Fineract
/clients/1/identifiers/create Add Identifier · Fineract
/clients/1/identifiers/edit/1 Edit Identifier · Fineract
/clients/1/addresses/create Add Address · Fineract
/clients/1/addresses/edit/1 Edit Address · Fineract
/clients/1/family-members/create Add Family Member · Fineract
/clients/1/family-members/edit/1 Edit Family Member · Fineract
/clients/1/notes/create Add Note · Fineract
/clients/1/notes/edit/1 Edit Note · Fineract
/clients/1/documents/create Add Document · Fineract
/clients/1/documents/edit/1 Edit Document · Fineract
/clients/1/charges Client Charges · Fineract
/clients/1/charges/create Add Charge · Fineract
/clients/1/collaterals Client Collateral · Fineract
/clients/1/collaterals/create Add Collateral · Fineract
/clients/1/collaterals/edit/1 Edit Collateral · Fineract
/clients/1/transactions Client Transactions · Fineract
```
The before block at the top of this description was produced by the same run
with only `clients.routes.ts` reverted to this branch's parent, so the two are
directly comparable. That harness was throwaway and is not in the diff; say the
word if you would rather it shipped as a permanent e2e spec.
One thing worth recording, since it would have produced a confidently wrong
result. Reading `document.title` straight after `page.goto` proves nothing
here, and neither does waiting for it to differ from `index.html`'s: the
strategy writes the app name first and the translations arrive after, so an
early read catches the raw `app.shortTitle` key or a bare `Fineract`. The
settled state for a titled route is `Page · Fineract`, so the check waits for
the separator.
| | |
|---|---|
| `npx ng test fineract-backoffice-ui` | **1100 of 1100 SUCCESS**, real
headless Chromium |
| `clients.routes.spec.ts` alone | 7 of 7 SUCCESS |
| `npm run lint` | exit 0, no new warnings |
| `npm run lint:prune` | exit 0 |
| `npm run format:check` | exit 0 |
| `node scripts/check-translations.mjs` | exit 0, 1532 keys checked |
**The guard was tested in three directions**, since a spec over data that
only ever passes proves nothing:
| Change | Result |
|---|---|
| delete the title on `:clientId/identifiers/edit/:id` | 3 failed, naming
the route |
| replace a key with the phrase `'Client Details'` | 1 failed, the key-shape
test |
| point `collaterals/create` at `CLIENT_COLLATERAL.EDIT` | 1 failed,
`CLIENT_COLLATERAL.EDIT · Fineract` where `Add Collateral · Fineract` was
expected |
| restore | 7 of 7 SUCCESS |
Not exercised against a real Fineract. This adds a `title` to route
definitions and changes no request, no behaviour and nothing rendered in the
page body.
## Screenshots
Not applicable. The change is in the browser tab, and the measured titles
above are the evidence a screenshot of the page body would not be.
## Two things noticed while in the file, neither changed here
**`:clientId/collaterals` has no guard.** It is the only route in the file
with no `canActivate` and no `data.permissions`, where its two siblings
`collaterals/create` and `collaterals/edit/:id` both carry `authGuard,
permissionGuard`. That looks like an oversight rather than a decision, but it
is a permissions question and not a titles one, so I have left it alone. Happy
to open a separate issue, or a one-line PR if you already know what it should
declare.
**The clients list heading and its nav entry disagree.**
`clients-list.component.ts` renders `MODULES.CLIENTS_CONTRACTS` ("Clients &
Contracts") while the nav item and now the tab say "Clients". I followed rule 1
rather than rule 2 there, on the grounds that a tab which does not match the
nav item that opened it is worse for findability than one which does not match
the heading. Worth flagging because it is the one place in the file where the
two rules disagree.
## The question this PR is really asking
If the convention above is right, the remaining 20 files are mechanical and
I will work through them in the grouping you prefer. If any of the three rules
is wrong, this is the cheapest possible place to say so.
## Checklist
- [x] I did not hand-edit generated files under `src/app/api/`.
- [x] New component or service code uses the adapter boundary in
`src/app/core/adapters/` instead of direct browser globals or imperative
third-party APIs. No new component or service code.
- [x] User-facing strings use translation keys. Every title is a key, and no
new keys were needed.
- [x] I added or updated tests appropriate to this change.
- [x] UI workflow changes include suitable e2e coverage, including
real-backend testing where relevant. No workflow changes. The browser
measurement above was run against the `mocked` project; a real backend would
exercise nothing this change touches.
- [x] Commits are signed.
--
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]
