diff --git a/.env.example b/.env.example new file mode 100644 index 0000000..d4eec84 --- /dev/null +++ b/.env.example @@ -0,0 +1,17 @@ +# Copy to .env only if you need to change something. Nothing here is required: +# docker-compose.yml carries the same default, so a fresh clone with no .env +# publishes PostgreSQL on 55432, which is the port every command in the README names. +# +# cp .env.example .env Git Bash / sh +# Copy-Item .env.example .env PowerShell +# +# .env is ignored by git and .env.example is not, so machine specific values stay +# out of the repository while the list of what can be set stays in it. + +# Host port the database container publishes on. Change it when 55432 is taken too; +# then also pass the new port to the application and to the tests, both of which take +# a full JDBC url rather than a port: +# +# -Dminibank.sql.url=jdbc:postgresql://localhost:55433/minibank +# -Dminibank.test.sql.url=jdbc:postgresql://localhost:55433/minibank_test +MINIBANK_DB_PORT=55432 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..17a2d1d --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,139 @@ +# What has to still be true after a change: the backend compiles and its tests pass against a real +# PostgreSQL, both front ends lint, test and build, and the two commands the README tells a reader +# to type still bring the database up and still bring the migrations to both databases. +# +# The jobs run the scripts rather than repeating what the scripts do. A workflow that spelled the +# steps out again would be a second copy of the setup instructions, free to drift from the ones a +# reader follows, and drift is exactly what this is here to catch. +name: build + +# main and every pull request. A feature branch nobody has opened a pull request for is checked by +# hand through workflow_dispatch; running on every push as well would mean two identical runs for +# every branch that later becomes a pull request. +on: + push: + branches: [main] + pull_request: + workflow_dispatch: + +# Nothing here writes anything back. Stated rather than left to the repository default, which an +# organisation setting can widen without anyone touching this file. +permissions: + contents: read + +# A newer commit on the same branch makes the run in progress pointless. +concurrency: + group: build-${{ github.ref }} + cancel-in-progress: true + +jobs: + backend: + name: backend + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-java@v4 + with: + distribution: temurin + # 21 and not 17, because 21 is the JDK this project is developed and measured on. + # Nothing newer than 17 can slip into the sources anyway: maven-compiler-plugin pins + # release to 17, which bounds the API surface rather than only the bytecode version. + java-version: '21' + cache: maven + + # sh ./mvnw, not ./mvnw. The repository is worked on from Windows with core.filemode false, + # so a file added there carries mode 100644 and the wrapper is not executable in a clone + # made here. Naming the interpreter costs nothing and does not depend on that mode. The + # ./ is not decoration: the wrapper reads its properties from ${0%/*}/.mvn/wrapper, and a + # bare mvnw has no slash to strip, so the path becomes mvnw/.mvn/wrapper and the read fails. + - name: Maven wrapper + run: sh ./mvnw -B -v + + # The stack the README describes, started by the script the README names. Deliberately not + # a service container: db/init is mounted by docker-compose.yml and runs only when compose + # starts PostgreSQL, and it is what creates minibank_test and applies the schema to both + # databases. A service container would leave the second database missing and the SQL tests + # would report as skipped instead of failing. + - name: Start PostgreSQL + run: sh scripts/up.sh + + # Every migration, against both databases, on a volume built moments ago from + # db/init/schema.sql. All of them are written to be safe to run twice, and this is what + # keeps that claim true: one that stopped being re-runnable fails here, rather than on the + # machine of whoever next resets a database. + - name: Apply the migrations + run: sh scripts/migrate.sh + + - name: Build the backend + run: sh ./mvnw -B -DskipTests package + + # The test database is named in full, and that address is not optional. Without the property + # the SQL tests skip and the run goes green over untested code; with an address that names + # some other database they TRUNCATE it, because that is what they do to the database they + # are given. + - name: Test the backend + run: sh ./mvnw -B test "-Dminibank.test.sql.url=jdbc:postgresql://localhost:55432/minibank_test" + + # Surefire redirects test output to files, so a failure prints a summary and keeps the + # detail on disk. On the runner that disk is discarded with the job unless it is collected. + - name: Keep the reports of a failed run + if: failure() + uses: actions/upload-artifact@v4 + with: + name: surefire-reports + path: target/surefire-reports/ + if-no-files-found: ignore + + frontends: + name: front ends + runs-on: ubuntu-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + # The floor both package.json files declare, which is in turn the floor the installed + # Vite declares. Pinned to the minor rather than to '22' so that this file says which + # version the front ends were checked on. + node-version: '22.12' + cache: npm + cache-dependency-path: | + minibank-web/package-lock.json + minibank-fraud-web/package-lock.json + + - name: Install the customer front end + working-directory: minibank-web + run: npm ci + + # Installed even though nothing is run from this directory below: the test step reaches the + # workstation's sources through its own vite config, and that config imports the React + # plugin from here. + - name: Install the workstation + working-directory: minibank-fraud-web + run: npm ci + + - name: Lint the customer front end and the shared layer + working-directory: minibank-web + run: npm run lint + + - name: Lint the workstation + working-directory: minibank-fraud-web + run: npm run lint + + # One runner for the whole tree. vitest is installed once, here, and reaches the workstation + # and frontend-shared through the projects list in minibank-web/vite.config.ts. That is why + # there is no second test step: the workstation has no test script to call, on purpose. + - name: Test + working-directory: minibank-web + run: npm test + + - name: Build the customer front end + working-directory: minibank-web + run: npm run build + + - name: Build the workstation + working-directory: minibank-fraud-web + run: npm run build diff --git a/.gitignore b/.gitignore index c7414f4..992d153 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,5 @@ ### Build output ### target/ -!.mvn/wrapper/maven-wrapper.jar !**/src/main/**/target/ !**/src/test/**/target/ build/ @@ -20,6 +19,9 @@ data/ # Machine specific: docker compose reads MINIBANK_DB_PORT from .env .env .env.* +# The one exception. .env.example holds no machine specific value, it lists what may be +# set and what the defaults already are, so it has to be committable. +!.env.example ### IntelliJ IDEA ### .idea/ diff --git a/.mvn/wrapper/maven-wrapper.properties b/.mvn/wrapper/maven-wrapper.properties new file mode 100644 index 0000000..48a56c9 --- /dev/null +++ b/.mvn/wrapper/maven-wrapper.properties @@ -0,0 +1,19 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +wrapperVersion=3.3.2 +distributionType=only-script +distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.6/apache-maven-3.9.6-bin.zip diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..8167f9a --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 Demid Ostiakov + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md index 555d8b0..93083d0 100644 --- a/README.md +++ b/README.md @@ -9,6 +9,10 @@ What it does: a customer signs in, lists their accounts, sends a payment, confir time password, or cancels it. A payment that looks risky is held for review, and a fraud analyst approves it, records confirmed fraud, or leaves notes. +Every push to `main` and every pull request runs `.github/workflows/ci.yml`: the backend is built +and tested against a real PostgreSQL started by docker compose, the migrations are applied to both +databases, and both front ends are linted, tested and built. + ## What is in here - **Two persistence backends behind one set of repository interfaces**: a JSON document store and @@ -20,8 +24,8 @@ approves it, records confirmed fraud, or leaves notes. - **Lazy Load** wired into the JSON adapter. In SQL mode the lazy navigation methods return nothing rather than loading, which is a known gap rather than a design. - **A REST API** on Spring Boot, and two console entry points that drive the same services. -- **Two web front ends**: a customer application, which also carries the fraud desk for a signed in - analyst, and a standalone analyst application. What they share is in `frontend-shared/`. +- **Two web front ends, each of which serves both roles**: a customer application in a web idiom + and a workstation in a window idiom. What they share is in `frontend-shared/`. - Every JDBC statement that carries a value binds it as a `PreparedStatement` parameter; no value is ever concatenated into SQL text. The only plain `Statement`s are the constant-text `nextval` queries that allocate ids. @@ -31,118 +35,135 @@ approves it, records confirmed fraud, or leaves notes. ## Requirements - Java 17 or newer. The build holds the code to the Java 17 API, so a current JDK works too. -- Maven 3.9 or newer - Docker, for PostgreSQL mode and for the SQL tests -- Node 20 or newer, for either front end +- Node 20.19 or newer, or 22.12 or newer. That is the floor both `package.json` files declare, and + it is in turn the floor the installed Vite declares; the workflow runs 22.12. -## Running it +Maven is not on that list. The repository carries the Maven wrapper, `mvnw`, `mvnw.cmd` and +`.mvn/`, which downloads Maven 3.9.6 on first use, so nothing has to be on `PATH`. -### The fastest path, no database +Every command below is written for **PowerShell**. In Git Bash the wrapper is `./mvnw` rather than +`.\mvnw.cmd`, and the scripts are `scripts/up.sh`, `scripts/migrate.sh` and +`scripts/demo-reset.sh`; nothing else differs. -The API defaults to the JSON store, so nothing else has to be running: +## Getting started + +One command brings the database up and returns only when it is ready to take a connection: ``` -mvn -B spring-boot:run +.\scripts\up.ps1 ``` -It listens on `http://localhost:8080`. The `demo` profile is active by default, so it seeds a -sample customer and creates the demo logins. Watch the first seconds of the log for this line: +It prints the three commands that are left, with the port this run actually published filled in. +There is nothing to watch and no `docker compose ps` to repeat. -``` -WARN [api] Demo profile is active: logins alice/alice123 and fraud/fraud123 are available -and the one time password is a fixed constant. Start with a different profile to disable this. -``` +The database is published on **55432**. That is the default in `docker-compose.yml`, so a clone +with no `.env` and no environment variable connects. Set `MINIBANK_DB_PORT` only when 55432 is +taken too; `.env.example` says what else has to move with it. -The store is written to `storage/data.json`, which is not tracked by git. +The container is `minibank-db-1`, the user and the password are both `minibank`, and it creates two +databases: `minibank` for the application and `minibank_test` for the SQL tests. -One process at a time owns the store. A second one pointed at the same file refuses to start, -naming the sidecar lock file it found held, `storage/data.json.lock`; close the first process, -give the second one a store of its own with `-Dminibank.json.path`, or point both at PostgreSQL, -which is built for more than one writer. The lock file is empty and staying behind after a run is -normal; the claim lives with the running process, so deleting the file frees nothing. +## Running it -### With PostgreSQL +### Starting the API -Start the database from the compose file in this repository: +Against the database that is now up: ``` -docker compose up -d +.\mvnw.cmd -B spring-boot:run "-Dspring-boot.run.jvmArguments=-Dminibank.storage=sql -Dminibank.sql.url=jdbc:postgresql://localhost:55432/minibank" ``` -That publishes port 5432. **On most machines 5432 is already held by a local PostgreSQL**, so -publish a different one instead: +The quotes are required in PowerShell. Without them the argument is split on the colons and Maven +reports a plugin it cannot resolve, which reads like a network problem and is not one. The url is +on the line so that the command still holds when `MINIBANK_DB_PORT` has moved the database. On the +port compose publishes by default it repeats what `minibank.sql.url` already resolves to, and can +be left off. -``` -MINIBANK_DB_PORT=55432 docker compose up -d -``` +If the store is PostgreSQL and nothing answers, the API prints the address it tried, the key that +points it there and the commands to fix it, then stops. There is no stack trace and no half-started +server that answers every request with a failure of its own. -In PowerShell: +It listens on `http://localhost:8080`. The `demo` profile is active by default, so it seeds a +sample customer and creates the demo logins. Watch the first seconds of the log for this line: ``` -$env:MINIBANK_DB_PORT = "55432"; docker compose up -d +WARN [api] Demo profile is active: logins alice/alice123 and fraud/fraud123 are available +and the one time password is a fixed constant. Start with a different profile to disable this. ``` -Put `MINIBANK_DB_PORT=55432` in a `.env` file in the repository root to avoid repeating it. `.env` -is gitignored. +### The fastest path, no database -The container is `minibank-db-1`, the user and the password are both `minibank`, and it creates two -databases: `minibank` for the application and `minibank_test` for the SQL tests. Wait for the -healthcheck: +The API defaults to the JSON store, so nothing else has to be running: ``` -docker compose ps +.\mvnw.cmd -B spring-boot:run ``` -Then start the API against it. Both properties are needed if you moved the port: - -``` -mvn -B spring-boot:run "-Dspring-boot.run.jvmArguments=-Dminibank.storage=sql -Dminibank.sql.url=jdbc:postgresql://localhost:55432/minibank" -``` +The store is written to `storage/data.json`, which is not tracked by git. -The quotes are required in PowerShell. Without them the argument is split on the colons and Maven -reports a plugin it cannot resolve, which reads like a network problem and is not one. +One process at a time owns the store. A second one pointed at the same file refuses to start, +naming the sidecar lock file it found held, `storage/data.json.lock`; close the first process, +give the second one a store of its own with `-Dminibank.json.path`, or point both at PostgreSQL, +which is built for more than one writer. The lock file is empty and staying behind after a run is +normal; the claim lives with the running process, so deleting the file frees nothing. ### As a jar ``` -mvn -B package +.\mvnw.cmd -B package java -jar target/VIS_project_minibank-1.0-SNAPSHOT.jar ``` -`mvn package` produces a runnable jar whose main class is the REST API. +`package` produces a runnable jar whose main class is the REST API. -### The front ends +### The two front ends Each needs its dependencies installed once: ``` -cd minibank-web && npm install && npm run dev +cd minibank-web; npm install; npm run dev ``` ``` -cd minibank-fraud-web && npm install && npm run dev +cd minibank-fraud-web; npm install; npm run dev ``` -The customer application serves `http://localhost:5173` and the analyst one +The customer application serves `http://localhost:5173` and the workstation `http://localhost:5174`. Each proxies `/api` to `http://localhost:8080` through its own vite config, so the API must be running and no cross origin configuration is needed. +The workstation on 5174 serves both roles: a fraud analyst gets the alert desk, and a customer gets +New payment, Waiting authorizations and Payment history in the same window idiom. The customer +application on 5173 serves both roles too, and an analyst who signs into it is taken straight to +the fraud desk and cannot reach the customer screens. + +The skins differ on purpose and stay different: the customer application is dark and web shaped, +the workstation is light and drawn as a window. What the two hold in common is the structure and +the wording rather than the palette, and that common part is `frontend-shared/`. + +The workstation carries a web manifest, so a browser can install it: in Chrome that is the +three dot menu, then Cast, save and share, then Install page as app. The installed copy opens in a +window of its own, titled MiniBank Workstation and carrying the same mark the tab does, with no tab +strip and no address bar. That is the whole of it. There is no service worker, nothing is cached, +and none of the screens work without the API running, so this is a window rather than an offline +application and it is not a packaged binary. The customer application has no manifest. + ### The console applications `App` uses the JSON store and has no login, which is why it is what a bare `exec:java` runs: ``` -mvn -B compile exec:java +.\mvnw.cmd -B compile exec:java ``` An API already running in JSON mode holds `storage/data.json`, so `App` refuses to start beside it; stop the API first, or give `App` a store of its own with `-Dminibank.json.path`. -`AppSql` uses PostgreSQL and signs you in first. Name it, and give it the url if you moved the -port: +`AppSql` uses PostgreSQL and signs you in first. Name it, and give it the url: ``` -mvn -B compile exec:java "-Dexec.mainClass=cz.vsb.minibank.AppSql" "-Dminibank.sql.url=jdbc:postgresql://localhost:55432/minibank" +.\mvnw.cmd -B compile exec:java "-Dexec.mainClass=cz.vsb.minibank.AppSql" "-Dminibank.sql.url=jdbc:postgresql://localhost:55432/minibank" ``` Left at its default, `AppSql` seeds the sample customer and creates the same demo logins the @@ -153,12 +174,26 @@ holds. There is also a scripted end to end run, which prints what it checked at each step: ``` -mvn -B compile exec:java "-Dexec.mainClass=cz.vsb.minibank.demo.DemoRunner" "-Dminibank.demo.reset=true" +.\mvnw.cmd -B compile exec:java "-Dexec.mainClass=cz.vsb.minibank.demo.DemoRunner" "-Dminibank.demo.reset=true" ``` It uses its own store, `storage/demo.json`, so it cannot disturb the application's, and `minibank.demo.reset` discards that store before it runs. +### What the console is, and whether it is kept + +It is a third surface rather than a leftover. `ui/console` is 750 lines, it is what the two entry +points above put in front of an operator, and it drives the same application services the REST API +does under the same role checks: eight commands, seven of them the customer's and one the +analyst's, each visible only to the role that may run it. Two test classes hold that behaviour +still, so a change to the services that broke the console would fail the build rather than be found +by whoever next opened it. + +It is kept for one reason beyond the two entry points that name it. Adding a beneficiary exists +only here: the API offers `GET /api/me/beneficiaries` and no way to create one, so neither front +end can do it, and the saved recipients a payment form offers have to come from somewhere. New work +goes to the web applications, and the console is held at what it does today rather than grown. + ## Signing in The `demo` profile creates two logins on startup: @@ -174,13 +209,6 @@ user interface says so, so a payment cannot be confirmed without being told this These are throwaway local values, not secrets. No screen prints them; the running application announces them once at startup, in the line quoted above. -The password hashing parameters changed with this version, nothing is stored beside a hash to say -which parameters wrote it, and the seeders never overwrite a user they find. A PostgreSQL database -seeded by an older version of this code therefore answers both demo passwords with 401. Delete the -rows with `DELETE FROM users`, or destroy the volume with `docker compose down -v`, and start -again with the demo active to reseed them. JSON mode is untouched: its users live only in memory, -so nothing hashed under the old parameters survives a restart. - The profile is active by default through `spring.profiles.default=demo` in `src/main/resources/application.properties`. Switch it off with any other profile: @@ -191,39 +219,52 @@ java -jar target/VIS_project_minibank-1.0-SNAPSHOT.jar --spring.profiles.active= Under `plain` nothing is seeded and neither login exists, so the API serves only whatever data is already in the store. -An analyst who signs into the customer application is taken straight to the fraud desk and cannot -reach the customer screens. A customer who signs into the analyst application is refused. +The fixed one time password is accepted only under the `demo` profile. Started with +`--spring.profiles.active=plain`, the API builds a validator that refuses every code and no payment +can be authorized; the refusal is announced in one line at startup rather than met at the +confirmation step. + +The password hashing parameters changed with this version, nothing is stored beside a hash to say +which parameters wrote it, and the seeders never overwrite a user they find. A PostgreSQL database +seeded by an older version of this code therefore answers both demo passwords with 401. Empty the +tables with `.\scripts\demo-reset.ps1`, then start the API again with the demo active to reseed +them. JSON mode is untouched: its users live only in memory, so nothing hashed under the old +parameters survives a restart. ## Tests ``` -mvn -B test +.\mvnw.cmd -B test ``` -**This is green while silently skipping every test that needs a database.** Every test in -`MinibankSqlUowTests`, `SqlSchemaPassTest` and `SqlUserRepositoryTest` skips rather than fails -when no database is reachable, so that a fresh clone stays green. To actually run them, point -them at the test database: +The SQL integration tests need a database of their own and there is no default address. Without +one they are skipped, naming the key: ``` -mvn -B test "-Dminibank.test.sql.url=jdbc:postgresql://localhost:55432/minibank_test" +.\mvnw.cmd -B test "-Dminibank.test.sql.url=jdbc:postgresql://localhost:55432/minibank_test" ``` -The quotes are required in PowerShell, for the same reason as above. The user and the password -default to `minibank`, so only the url has to be given. +A run that names no address opens no connection at all, so it cannot reach a PostgreSQL that +happens to be listening on the usual port. The quotes are required in PowerShell, for the reason +given above. The user and the password default to `minibank`, so only the url has to be given. The tests use their own keys, `minibank.test.sql.*`, and refuse to start if pointed at the application database, because they truncate every table. -The front ends have one test runner between them, in `minibank-web`, and it covers the shared -module as well: +There is one JavaScript test runner in the tree. vitest is installed only in `minibank-web`, and it +reaches the workstation and `frontend-shared` through the `projects` list in +`minibank-web/vite.config.ts`: ``` -cd minibank-web && npm test +cd minibank-web; npm test ``` -Every case in it calls a function from `frontend-shared/` with values and checks what comes back: -parsing the amount a customer types, printing money and dates, the error contract, the queue +`minibank-fraud-web` has no `test` script and is not meant to get one, so `npm test` inside that +directory does nothing. That is a decision rather than an oversight: a second install would be a +second vitest to keep in step with the first. + +Every case in that run calls a function from `frontend-shared/` with values and checks what comes +back: parsing the amount a customer types, printing money and dates, the error contract, the queue filters, the wording the two desks share. **Nothing renders a component**, and neither `jsdom` nor a testing library is installed. That is deliberate: the logic worth pinning down was moved out of the screens and into that module, where a plain call reaches it, and a render test would mostly @@ -236,9 +277,10 @@ and the render tests that follow it. ``` db/ - init/ run by the container, unattended, only on an empty volume - migrate/ run by hand, against both databases - reset.sql drops everything, run only on purpose + init/ run by the container, unattended, only on an empty volume + migrate/ run by scripts/migrate, against both databases + demo-reset.sql empties the tables so the next API start seeds them again + reset.sql drops everything, run only on purpose ``` `db/init/schema.sql` is the whole schema: every table, index, constraint and sequence. @@ -246,25 +288,42 @@ db/ drops anything, so `schema.sql` fails on a database that already has the tables rather than emptying it. -`db/migrate/` exists for a volume that already holds data. Each script is an online `ALTER`, is -mirrored back into `schema.sql`, and explains in its own header what it does and why. **A fresh -volume needs none of them**: `schema.sql` already has everything they add. Nothing records which of -them has been run, so their names describe rather than order. +### Migrations + +`db/migrate/` exists for a volume that already holds data. **A fresh volume needs none of them**: +`schema.sql` already has everything they add. -To apply one, run it against **both** databases: +`db/migrate/order.txt` declares the order the scripts run in, oldest first, and ``` -docker compose exec -T db psql -U minibank -d minibank < db/migrate/currency-is-czk.sql -docker compose exec -T db psql -U minibank -d minibank_test < db/migrate/currency-is-czk.sql +.\scripts\migrate.ps1 ``` -In PowerShell, where `<` is a reserved operator: +applies all of them to both `minibank` and `minibank_test`. Pass `-DryRun` to print the plan +without connecting. The script refuses to run if the directory and `order.txt` have drifted apart +in either direction, and every script is safe to run twice. + +Most of them change the shape of a table and are mirrored back into `schema.sql`, but not all of +them are that: `hold-alerted-transfers.sql` only moves rows and has nothing to mirror, +`fraud-alert-comment-and-notes-journal.sql` creates a table and then carries the old values into +it, and `2026-08-24-daily-limit-on-the-customer.sql` copies two columns onto another table before +dropping them. Each states in its own header what it does and why, and `order.txt` names the two to +read before re-running. Nothing records which of them a given database has already had, which is +why each is written to survive being run twice. + +### Demo data + +To get the demo dataset back after it has been paid down or left mid review: ``` -Get-Content db/migrate/currency-is-czk.sql | docker compose exec -T db psql -U minibank -d minibank +.\scripts\demo-reset.ps1 ``` -To start from nothing: +Then restart the API. It empties the seven tables and resets their sequences; the schema, the +second database and every applied migration survive. `docker compose down -v` is no longer the way +to do this: it discards the volume and both databases with it. + +To start from nothing, and only then: ``` docker compose down -v @@ -288,16 +347,34 @@ everywhere. | `minibank.demo.path` | `storage/demo.json` | the demo runner's own store | | `minibank.demo.reset` | unset | `true` discards that store before the demo runs | | `minibank.demo.enabled` | `true` | `false` stops the SQL console seeding the sample data and creating the demo logins; the API is switched by the `demo` profile instead | -| `minibank.sql.url` | `jdbc:postgresql://localhost:5432/minibank` | JDBC url | +| `minibank.sql.url` | `jdbc:postgresql://localhost:55432/minibank` | JDBC url | | `minibank.sql.user` | `minibank` | | | `minibank.sql.password` | `minibank` | | -| `minibank.log.file` | `minibank.log` | where the application log is written | +| `minibank.log.file` | `storage/minibank.log` | where the application log is written | + +The default url names 55432, the port compose publishes, so a clone that has not moved the port +connects with no url given at all. The commands above spell it out anyway, so that they still hold +on a machine where `MINIBANK_DB_PORT` had to be changed. + +`minibank.log.file` behaves like the other keys on the REST API only: there it is resolved from +`application.properties`, the command line or the environment. The console entry points and the +demo runner have no such environment, so for them `-Dminibank.log.file=...` is the only way to move +the log. + +The database keys were renamed. `minibank.jdbcUrl`, `minibank.dbUser` and `minibank.dbPass` are no +longer read: the console entry points refuse to start on them, and the REST API, which resolves +them through placeholders that never see an old name, warns at startup and says which value it is +using instead. ## Runtime state -`storage/`, `data/`, `target/` and `node_modules/` are generated and are not tracked. Deleting +`storage/`, `target/` and `node_modules/` are generated and are not tracked. Deleting `storage/` is safe: under the `demo` profile the sample data is seeded again on the next start. **A store that cannot be read stops the application from starting**, rather than being ignored so that it silently comes up empty. If startup fails on a `storage/data.json` written by an older version of this code, delete the file and start again. + +## Licence + +MIT. See [LICENSE](LICENSE). diff --git a/db/demo-reset.sql b/db/demo-reset.sql new file mode 100644 index 0000000..2f58ed5 --- /dev/null +++ b/db/demo-reset.sql @@ -0,0 +1,65 @@ +-- Empties the MiniBank tables so the next start of the API seeds the demo dataset again. +-- The schema itself is untouched: this is the difference between this script and db/reset.sql, +-- which drops the tables and needs db/init/schema.sql run after it. +-- +-- scripts/demo-reset.sh Git Bash / sh +-- scripts/demo-reset.ps1 PowerShell +-- +-- or by hand: +-- +-- docker compose exec -T db psql -U minibank -d minibank < db/demo-reset.sql +-- +-- Then restart the API. Seeding runs once at startup, under the demo profile, so an empty +-- database on its own shows nothing until the process comes back. +-- +-- Why emptying is what it takes. DemoScenario.seed() decides it has already run by looking for +-- one account, PRIMARY_IBAN, and returns early when it finds it. It therefore cannot notice a +-- fixture that has drifted, been paid down, or been left mid-review by somebody trying the +-- screens, and there was no way to get the dataset back short of docker compose down -v, which +-- discards the volume and both databases with it. +-- +-- users goes with the rest and not because the demo data lives in it. DemoUsersInitializer +-- creates the alice and fraud logins only when they are missing, and binds alice to the customer +-- id seed() just returned. customers.id is referenced by users.customer_id ON DELETE SET NULL, so +-- a run that kept the users would leave alice pointing at nothing, and, being present, she would +-- never be rebound to the new customer. The logins are throwaway values recreated on the same +-- start that seeds the data, so there is nothing here to preserve. +-- +-- Whatever database -d names is the database that is emptied. Naming minibank_test does no harm +-- and no good: the SQL tests truncate what they need themselves. + +\set ON_ERROR_STOP on + +-- One statement, so the tables come back consistent or not at all, and so the foreign keys +-- between them never have to be satisfied half way through. CASCADE reaches nothing beyond the +-- seven, all of which are named anyway, for the same reason db/reset.sql names them: this list +-- has to stay readable as the inverse of schema.sql rather than as a list that happens to work. +-- RESTART IDENTITY puts the sequences back to their starting value; each is OWNED BY the column +-- it numbers, which is what brings it into the scope of this statement. +TRUNCATE TABLE + fraud_alert_notes, + fraud_alerts, + transfers, + beneficiaries, + accounts, + customers, + users +RESTART IDENTITY CASCADE; + +DO $$ +DECLARE + left_over BIGINT; +BEGIN + SELECT (SELECT count(*) FROM customers) + + (SELECT count(*) FROM accounts) + + (SELECT count(*) FROM transfers) + + (SELECT count(*) FROM beneficiaries) + + (SELECT count(*) FROM fraud_alerts) + + (SELECT count(*) FROM fraud_alert_notes) + + (SELECT count(*) FROM users) + INTO left_over; + IF left_over > 0 THEN + RAISE EXCEPTION '% row(s) survived the reset', left_over; + END IF; + RAISE NOTICE 'MiniBank tables are empty. Restart the API to seed the demo dataset again.'; +END $$; diff --git a/db/migrate/one-alert-per-transfer.sql b/db/migrate/one-alert-per-transfer.sql index 0e70e05..2767dad 100644 --- a/db/migrate/one-alert-per-transfer.sql +++ b/db/migrate/one-alert-per-transfer.sql @@ -30,7 +30,15 @@ BEGIN ALTER TABLE fraud_alerts ADD CONSTRAINT fraud_alerts_one_per_transfer UNIQUE (transfer_id); EXCEPTION - WHEN duplicate_object THEN NULL; + -- Two codes, because there are two ways this constraint can already be there and they do + -- not report the same one. duplicate_object is a second run of this script against a + -- database that got the constraint from it. duplicate_table is a database built from + -- db/init/schema.sql, which creates the same constraint under the same name: a UNIQUE + -- constraint is backed by an index that carries its name, and PostgreSQL fails on the index + -- name first, with "relation fraud_alerts_one_per_transfer already exists", before it ever + -- compares constraint names. Only duplicate_object was caught, so scripts/migrate.sh died + -- here on every database that had not been built by these scripts in order. + WHEN duplicate_object OR duplicate_table THEN NULL; END $$; DROP INDEX IF EXISTS idx_fraud_alerts_transfer; diff --git a/db/migrate/order.txt b/db/migrate/order.txt new file mode 100644 index 0000000..099deb0 --- /dev/null +++ b/db/migrate/order.txt @@ -0,0 +1,38 @@ +# The order the scripts in this directory must run in, oldest first. +# +# Why a list and not a numeric prefix on each file: eleven of these thirteen names are already +# quoted, by their own headers, by db/init/schema.sql and by the documentation, and renaming them +# would have made every one of those quotations wrong to buy an ordering this file states in one +# place. Two names do carry a date, and those two are left exactly as they are. +# +# The order is the order the scripts were written in, and it is also a working dependency order: +# stored-fee-and-account-version.sql creates accounts.soft_daily_threshold_czk, which +# 2026-08-24-daily-limit-on-the-customer.sql carries onto the customer and then drops, and it +# creates transfers.settled_at, which transfer-daily-total-index.sql indexes. Anything appended +# here goes at the bottom. +# +# There is still no table of applied migrations, so nothing here records what a given database has +# already had. Every script is written to be safe to run twice: each guards its own change with +# IF NOT EXISTS, a duplicate_object handler or an information_schema test, and each ends by +# raising an exception if what it promised is not there. The two exceptions to read before +# re-running are hold-alerted-transfers.sql and fraud-alert-comment-and-notes-journal.sql, which +# change rows rather than shape; on a database the application has been running against, both +# find nothing to change. +# +# scripts/migrate.sh and scripts/migrate.ps1 read this file, and refuse to run if it and the +# directory have drifted apart in either direction. Blank lines and lines starting with # are +# ignored. + +hold-alerted-transfers.sql +stored-fee-and-account-version.sql +transfer-version.sql +one-alert-per-transfer.sql +sequence-ownership.sql +transfer-daily-total-index.sql +currency-is-czk.sql +fraud-alert-version.sql +fee-sign-and-creation-instants.sql +transfer-dispatch-state.sql +fraud-alert-comment-and-notes-journal.sql +2026-08-24-daily-limit-on-the-customer.sql +2026-08-29-declined-at-on-the-transfer.sql diff --git a/docker-compose.yml b/docker-compose.yml index bb0c214..bf16f84 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -8,9 +8,13 @@ services: POSTGRES_USER: minibank POSTGRES_PASSWORD: minibank ports: - # Overridable because 5432 is usually already taken by a local PostgreSQL: - # MINIBANK_DB_PORT=55432 docker compose up -d - - "${MINIBANK_DB_PORT:-5432}:5432" + # 55432 rather than 5432, because 5432 is usually already taken by a PostgreSQL + # installed on the machine, and publishing onto an occupied port either fails or, + # worse, leaves the reader talking to the wrong server. This is the port every + # command in the README names, so a clone with no .env connects. + # Override it when 55432 is taken too: + # MINIBANK_DB_PORT=55433 docker compose up -d + - "${MINIBANK_DB_PORT:-55432}:5432" volumes: # A directory, not a single file: on Windows, Docker Desktop creates a directory in # place of a missing bind mount source, and the container is then broken for good. diff --git a/frontend-shared/__fixtures__/README.md b/frontend-shared/__fixtures__/README.md new file mode 100644 index 0000000..42cd6c4 --- /dev/null +++ b/frontend-shared/__fixtures__/README.md @@ -0,0 +1,32 @@ +# Captured bodies + +Three answers from the running API, saved exactly as they arrived. They are the wire, not examples +of it: nothing in this directory was typed by hand, and nothing in it may be. + +That rule is the whole point. Every test in this directory used to build its own object and hand it +to the function under test, which proves the function reads the object the test wrote and nothing +about the answer the server sends. One field drifted for exactly that reason: the waiting list DTO +was declared with `beneficiaryIban`, the server had always sent `toIban`, and the screen reached +the value through a cast because the field it wanted was not on the type it had. Tests were green +throughout. See `WaitingTransferItem` in `minibank-web/src/api.ts` for the repair. + +## What is here, and where each came from + +| File | Route | +| --- | --- | +| `fraud-alerts.json` | `GET /api/fraud/alerts?page=0&size=5`, signed in as `fraud` | +| `fraud-alert-detail.json` | `GET /api/fraud/alerts/1`, signed in as `fraud` | +| `waiting-transfers.json` | `GET /api/me/waiting-transfers?page=0&size=10`, signed in as `alice` | + +Captured on 2026-08-30 against the demo data, over `X-Session-Id` from `POST /api/auth/login`. +Read only calls, all three. + +The bytes are the response bodies re-indented to two spaces and nothing else. No value was +shortened, rounded, renamed or invented, and a row was neither added nor removed: the alert queue +really does hold two alerts and the customer really is waiting on two payments. + +## Refreshing them + +Re-fetch. Do not edit. A body that no longer matches what `wire.test.ts` expects is a report about +the wire, and the answer to it is a change to the interfaces in `fraud.ts` or to the server, never +a change to these files. diff --git a/frontend-shared/__fixtures__/fraud-alert-detail.json b/frontend-shared/__fixtures__/fraud-alert-detail.json new file mode 100644 index 0000000..41a24d7 --- /dev/null +++ b/frontend-shared/__fixtures__/fraud-alert-detail.json @@ -0,0 +1,256 @@ +{ + "alert": { + "id": 1, + "state": "NEW", + "decision": null, + "decidedBy": null, + "resolvedAt": null, + "decisionComment": null, + "reason": "New beneficiary + high amount", + "riskScore": 80, + "createdAt": "2026-08-29T20:20:50.290980Z", + "assignee": null, + "tags": [] + }, + "transfer": { + "id": 12, + "code": "TR-12", + "status": "HELD_FOR_REVIEW", + "fromIban": "CZ6508000000192000145399", + "fromBalance": { + "amount": "45338.50", + "currency": "CZK" + }, + "toIban": "CZ9608000000192000145423", + "toIbanInBank": false, + "dispatchState": null, + "amount": { + "amount": "12000.00", + "currency": "CZK" + }, + "feeAmount": { + "amount": "145.00", + "currency": "CZK" + }, + "createdAt": "2026-08-29T20:20:50.290980Z", + "settledAt": null, + "declinedAt": null, + "message": null, + "declineReason": null, + "authMethod": "CARD" + }, + "history": [ + { + "id": 14, + "createdAt": "2026-08-29T20:20:50.290980Z", + "settledAt": null, + "declinedAt": null, + "amount": { + "amount": "16000.00", + "currency": "CZK" + }, + "fee": { + "amount": "185.00", + "currency": "CZK" + }, + "status": "WAITING_AUTH", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ2108000000192000145415", + "toIbanInBank": false, + "message": null, + "declineReason": null + }, + { + "id": 12, + "createdAt": "2026-08-29T20:20:50.290980Z", + "settledAt": null, + "declinedAt": null, + "amount": { + "amount": "12000.00", + "currency": "CZK" + }, + "fee": { + "amount": "145.00", + "currency": "CZK" + }, + "status": "HELD_FOR_REVIEW", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ9608000000192000145423", + "toIbanInBank": false, + "message": null, + "declineReason": null + }, + { + "id": 13, + "createdAt": "2026-08-29T00:20:50.290980Z", + "settledAt": null, + "declinedAt": "2026-08-29T20:20:50.290980Z", + "amount": { + "amount": "11500.00", + "currency": "CZK" + }, + "fee": { + "amount": "140.00", + "currency": "CZK" + }, + "status": "DECLINED", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ9608000000192000145423", + "toIbanInBank": false, + "message": null, + "declineReason": "Withdrawn by the customer while the review was open" + }, + { + "id": 11, + "createdAt": "2026-08-28T16:20:50.290980Z", + "settledAt": "2026-08-28T16:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "50.00", + "currency": "CZK" + }, + "fee": { + "amount": "0.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ2108000000192000145415", + "toIbanInBank": false, + "message": "Coffee", + "declineReason": null + }, + { + "id": 10, + "createdAt": "2026-08-27T09:20:50.290980Z", + "settledAt": "2026-08-27T09:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "980.00", + "currency": "CZK" + }, + "fee": { + "amount": "0.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ9608000000192000145423", + "toIbanInBank": false, + "message": "Deposit for the workshop", + "declineReason": null + }, + { + "id": 9, + "createdAt": "2026-08-26T14:20:50.290980Z", + "settledAt": "2026-08-26T14:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "300.00", + "currency": "CZK" + }, + "fee": { + "amount": "0.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ4308000000192000145407", + "toIbanInBank": true, + "message": null, + "declineReason": null + }, + { + "id": 8, + "createdAt": "2026-08-25T10:20:50.290980Z", + "settledAt": "2026-08-25T10:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "2500.00", + "currency": "CZK" + }, + "fee": { + "amount": "25.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ4308000000192000145407", + "toIban": "CZ6508000000192000145399", + "toIbanInBank": true, + "message": "Back to the current account for the rent", + "declineReason": null + }, + { + "id": 7, + "createdAt": "2026-08-24T17:20:50.290980Z", + "settledAt": "2026-08-24T17:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "1200.00", + "currency": "CZK" + }, + "fee": { + "amount": "12.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ2108000000192000145415", + "toIbanInBank": false, + "message": null, + "declineReason": null + }, + { + "id": 6, + "createdAt": "2026-08-23T12:20:50.290980Z", + "settledAt": "2026-08-23T12:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "420.00", + "currency": "CZK" + }, + "fee": { + "amount": "0.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ4308000000192000145407", + "toIban": "CZ2108000000192000145415", + "toIbanInBank": false, + "message": "Dentist, second visit", + "declineReason": null + }, + { + "id": 5, + "createdAt": "2026-08-22T11:20:50.290980Z", + "settledAt": "2026-08-22T11:20:50.290980Z", + "declinedAt": null, + "amount": { + "amount": "890.00", + "currency": "CZK" + }, + "fee": { + "amount": "0.00", + "currency": "CZK" + }, + "status": "SENT", + "fromIban": "CZ6508000000192000145399", + "toIban": "CZ2108000000192000145415", + "toIbanInBank": false, + "message": null, + "declineReason": null + } + ], + "notes": [ + { + "author": "fraud", + "writtenAt": "2026-08-29T20:20:50.290980Z", + "text": "Payee was added this month and this is the largest amount the account has ever sent to it. Holding until the card scheme comes back on the merchant." + }, + { + "author": "fraud", + "writtenAt": "2026-08-29T20:20:50.290980Z", + "text": "Called the number on file, no answer. Trying again this afternoon." + } + ] +} diff --git a/frontend-shared/__fixtures__/fraud-alerts.json b/frontend-shared/__fixtures__/fraud-alerts.json new file mode 100644 index 0000000..8ac9810 --- /dev/null +++ b/frontend-shared/__fixtures__/fraud-alerts.json @@ -0,0 +1,44 @@ +{ + "alerts": { + "items": [ + { + "id": 1, + "alertCode": "ALERT-1", + "transferCode": "TR-12", + "state": "NEW", + "transferStatus": "HELD_FOR_REVIEW", + "amount": { + "amount": "12000.00", + "currency": "CZK" + }, + "shortReason": "New beneficiary + high amount", + "createdAt": "2026-08-29T20:20:50.290980Z", + "riskScore": 80, + "assignee": null + }, + { + "id": 2, + "alertCode": "ALERT-2", + "transferCode": "TR-13", + "state": "OK", + "transferStatus": "DECLINED", + "amount": { + "amount": "11500.00", + "currency": "CZK" + }, + "shortReason": "New beneficiary + high amount", + "createdAt": "2026-08-29T00:20:50.290980Z", + "riskScore": 65, + "assignee": "fraud" + } + ], + "page": 0, + "size": 5, + "total": 2 + }, + "counters": { + "newCount": 1, + "suspiciousCount": 0, + "okCount": 1 + } +} diff --git a/frontend-shared/__fixtures__/waiting-transfers.json b/frontend-shared/__fixtures__/waiting-transfers.json new file mode 100644 index 0000000..55b53ed --- /dev/null +++ b/frontend-shared/__fixtures__/waiting-transfers.json @@ -0,0 +1,29 @@ +{ + "items": [ + { + "id": 14, + "toIban": "CZ2108000000192000145415", + "amount": { + "amount": "16000.00", + "currency": "CZK" + }, + "createdAt": "2026-08-29T20:20:50.290980Z", + "authMethod": "CARD", + "status": "WAITING_AUTH" + }, + { + "id": 12, + "toIban": "CZ9608000000192000145423", + "amount": { + "amount": "12000.00", + "currency": "CZK" + }, + "createdAt": "2026-08-29T20:20:50.290980Z", + "authMethod": "CARD", + "status": "HELD_FOR_REVIEW" + } + ], + "page": 0, + "size": 10, + "total": 2 +} diff --git a/frontend-shared/alertFilters.ts b/frontend-shared/alertFilters.ts index 5b7429f..5e3ecb0 100644 --- a/frontend-shared/alertFilters.ts +++ b/frontend-shared/alertFilters.ts @@ -14,19 +14,31 @@ export interface AmountBoxes { } /** - * What is wrong with the amount range, or null when it can be sent. + * What is wrong with the filters, or null when they can be sent. * - * This is the affordance, not the guard. It names which two numbers are the wrong way round, - * which the server cannot do: FraudController refuses the same range, but no handler in this + * This is the affordance, not the guard. It names which two values are the wrong way round, + * which the server cannot do: FraudController refuses the same ranges, but no handler in this * project echoes an exception message, so over the wire it is only "the request contains invalid - * or missing values". The server keeps the rule because the endpoint is reachable without this - * screen; this keeps it because an analyst deserves to be told what to change. + * or missing values". The server keeps the rules because the endpoint is reachable without this + * screen; this keeps them because an analyst deserves to be told what to change. * - * An empty queue is how the endpoint reports "nothing matched", so a range that cannot match - * anything must never reach it - otherwise a backwards filter is indistinguishable from a quiet + * An empty queue is how the endpoint reports "nothing matched", so a filter that cannot match + * anything must never reach it - otherwise a backwards range is indistinguishable from a quiet * afternoon, and the counters beside the list go on reporting the whole queue. + * + * TWO PAIRS, ONE FUNCTION. The queue is narrowed by an amount range and by a range of raising + * times, and both go wrong the same way. It was called amountRangeProblem while it knew about one + * of them, which is a name that quietly promises the other pair is somebody else's problem: a + * screen that grows the date boxes would have had to write a second checker, and two checkers is + * how the two pairs come to disagree about what a reversed range is. + * + * ONE COMPLAINT AT A TIME, and which one is not arbitrary. A box the browser could not read comes + * first because it is the only complaint whose evidence is not on screen: the value that would + * explain it is gone, so nothing else said about the filters can be trusted. The dates come next, + * because they narrow what the amounts are even asked about. The reversed amounts come last, which + * is also where the reader's eye is, on the pair they just typed. */ -export function amountRangeProblem( +export function filterProblem( filters: AlertFilters, unreadable: AmountBoxes, ): string | null { @@ -38,6 +50,22 @@ export function amountRangeProblem( return 'That amount could not be read. Enter a number, like 1 500 or 1 500,50.'; } + // The parameters go to the server as instants and are parsed there with Instant.parse, which + // takes nothing else: a bare 2026-08-30 is refused with the same opaque sentence a reversed + // range gets. So the example spells out the shape rather than naming the standard. + const from = readInstant(filters.createdFrom); + const to = readInstant(filters.createdTo); + + if (from === 'unreadable') { + return 'The earliest date could not be read. Use an instant, like 2026-08-30T00:00:00Z.'; + } + if (to === 'unreadable') { + return 'The latest date could not be read. Use an instant, like 2026-08-30T00:00:00Z.'; + } + if (from !== null && to !== null && from > to) { + return `The earliest date (${filters.createdFrom}) is after the latest (${filters.createdTo}).`; + } + const min = filters.minAmount ? Number(filters.minAmount) : null; const max = filters.maxAmount ? Number(filters.maxAmount) : null; @@ -59,3 +87,29 @@ export function amountRangeProblem( return null; } + +/** + * The instant a date box holds, null for a box nobody filled in, and the marker for one that + * cannot be an instant at all. + * + * Three answers rather than a number and a NaN, because NaN compares false against everything: a + * reversed pair with one unreadable half would pass the comparison below and be sent, which is the + * silent wrong answer this whole module exists to refuse. + * + * Date.parse accepts more shapes than the server does, and the ones it accepts and the server + * refuses are exactly the ones worth naming here, so the check is what the server's own parser + * would do rather than what the browser will tolerate. + */ +function readInstant(raw: string | undefined): number | null | 'unreadable' { + if (!raw) { + return null; + } + // The trailing Z or an offset is what makes the string an instant rather than a local time, + // and it is the half a date input never writes. + if (!/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}(:\d{2}(\.\d+)?)?(Z|[+-]\d{2}:\d{2})$/.test(raw)) { + return 'unreadable'; + } + + const at = Date.parse(raw); + return Number.isFinite(at) ? at : 'unreadable'; +} diff --git a/frontend-shared/buildDecisionRequest.test.ts b/frontend-shared/buildDecisionRequest.test.ts new file mode 100644 index 0000000..1a41850 --- /dev/null +++ b/frontend-shared/buildDecisionRequest.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect } from 'vitest'; +import { buildDecisionRequest } from './fraud'; + +/** + * The body a decision is sent as, and specifically which keys are not in it. + * + * Both optional fields mean "leave what is already there alone" by being absent, and an empty + * string is a different instruction to the server. `note` appends one entry to the alert's journal, + * so a blank one files an empty paragraph on every press. `comment` is what the analyst concluded + * about this verdict and it is the sentence the customer is shown when the verdict is a refusal; + * blank, it writes over what a colleague put there. + * + * Both desks built this inline as `box.trim() || undefined`, which leaves the key present holding + * undefined and relies on JSON.stringify dropping it on the way out. That is correct over the wire + * and it means the object in front of a reader and the bytes the server gets are two different + * things, which is exactly the gap a shaping test has to close. So the assertions below are about + * the key, not about the value: `toEqual` treats a missing key and an undefined one as the same + * object and would pass either way. + */ + +const CONCLUSION = 'Beneficiary matches a mule account reported last week.'; +const ENTRY = 'Called the customer, no answer.'; + +describe('the verdict itself', () => { + it.each(['APPROVE', 'DECLINE', 'ANNOTATE'] as const)('rides on every body, here %s', (kind) => { + expect(buildDecisionRequest(kind, '', '').decision).toBe(kind); + }); + + it('is never trimmed and never dropped, being a token rather than typing', () => { + const body = buildDecisionRequest('ANNOTATE', '', ''); + expect(Object.keys(body)).toEqual(['decision']); + }); +}); + +describe('a box nobody typed in', () => { + it('leaves the comment key off the body rather than sending an empty string', () => { + const body = buildDecisionRequest('APPROVE', '', ENTRY); + expect('comment' in body).toBe(false); + }); + + it('leaves the note key off the body rather than sending an empty string', () => { + const body = buildDecisionRequest('APPROVE', CONCLUSION, ''); + expect('note' in body).toBe(false); + }); + + it('sends the verdict alone when both boxes are empty', () => { + expect(Object.keys(buildDecisionRequest('APPROVE', '', ''))).toEqual(['decision']); + }); + + it.each([' ', ' ', '\n', '\t\t'])( + 'reads %o as untyped, in both boxes, so an accidental keystroke files nothing', + (only) => { + const body = buildDecisionRequest('ANNOTATE', only, only); + expect('comment' in body).toBe(false); + expect('note' in body).toBe(false); + }, + ); + + it('survives the round trip to JSON without either key appearing', () => { + // The end of the argument. Whatever the object looks like, this is what leaves the browser. + const wire = JSON.stringify(buildDecisionRequest('APPROVE', '', '')); + expect(wire).toBe('{"decision":"APPROVE"}'); + }); +}); + +describe('a box somebody typed in', () => { + it('carries the comment, trimmed', () => { + const body = buildDecisionRequest('DECLINE', ` ${CONCLUSION}\n`, ''); + expect(body.comment).toBe(CONCLUSION); + }); + + it('carries the note, trimmed', () => { + const body = buildDecisionRequest('ANNOTATE', '', `\t${ENTRY} `); + expect(body.note).toBe(ENTRY); + }); + + it('carries both at once, which is one press and not two', () => { + expect(buildDecisionRequest('DECLINE', CONCLUSION, ENTRY)).toEqual({ + decision: 'DECLINE', + comment: CONCLUSION, + note: ENTRY, + }); + }); + + it('keeps the two apart: the conclusion is not filed as the journal entry', () => { + // They were one field once. `notes` carried the whole journal and the comment was called + // `reason`, and the alert then printed the analyst's sentence joined to the bank's own. + const body = buildDecisionRequest('DECLINE', CONCLUSION, ENTRY); + expect(body.comment).not.toBe(body.note); + }); + + it('sends a comment on a verdict that is not a refusal', () => { + // It rides with all three presses. Nothing may narrow it back to the refusal: it is what + // the analyst concluded, and on an approval that conclusion is the record of why the money + // was let go. + expect(buildDecisionRequest('APPROVE', CONCLUSION, '').comment).toBe(CONCLUSION); + }); +}); + +describe('the keys the wire has and no others', () => { + it('offers nothing the server retired, whatever the boxes hold', () => { + // `tags` and `assignee` left this body and are not coming back, and `notes` was replaced + // by `note`, which is a different field rather than a rename. `notes` sent today is + // ignored by the server, so a desk that still sent it would file nothing and say it had. + const keys = Object.keys(buildDecisionRequest('DECLINE', CONCLUSION, ENTRY)); + for (const retired of ['tags', 'assignee', 'notes', 'reason']) { + expect(keys).not.toContain(retired); + } + }); + + it('never grows a key past the three the request declares', () => { + const keys = Object.keys(buildDecisionRequest('DECLINE', CONCLUSION, ENTRY)).sort(); + expect(keys).toEqual(['comment', 'decision', 'note']); + }); +}); diff --git a/frontend-shared/decisionAllowed.test.ts b/frontend-shared/decisionAllowed.test.ts new file mode 100644 index 0000000..4e6148f --- /dev/null +++ b/frontend-shared/decisionAllowed.test.ts @@ -0,0 +1,117 @@ +import { describe, it, expect } from 'vitest'; +import { decisionAllowed } from './fraud'; +import type { FraudDecision } from './fraud'; + +/** + * Which of the three verdicts an alert in a given state will take. + * + * The rule used to live in six `disabled=` expressions, three per desk, and the two desks agreed + * only because the same person wrote both on the same afternoon. Nothing checked it. Each of the + * three lines is a rule about money: one releases a held payment, one sends the customer a sentence + * that cannot be withdrawn, and one does nothing at all to either. + * + * The alert states are the three the queue counts, from queueCounterCells: NEW is undecided, + * SUSPICIOUS is recorded fraud and OK is cleared. Every case below names one of them, because the + * interesting failures are not "some other state" but the two decided ones, which look alike to a + * reader and are not alike to APPROVE. + */ + +const STATES = ['NEW', 'SUSPICIOUS', 'OK'] as const; +const DECIDED = ['SUSPICIOUS', 'OK'] as const; +const WRITTEN = 'Card used from two countries within the hour.'; + +describe('releasing the payment', () => { + it('is offered on an alert nobody has decided', () => { + expect(decisionAllowed('APPROVE', { state: 'NEW' }, '')).toBe(true); + }); + + it.each(DECIDED)('is refused once a verdict is recorded, here %s', (state) => { + // Not cosmetic. The server refuses a second verdict, and its refusal arrives as the general + // validation sentence, which does not say that a colleague got there first. + expect(decisionAllowed('APPROVE', { state }, WRITTEN)).toBe(false); + }); + + it('does not care what is in the comment box, in either direction', () => { + expect(decisionAllowed('APPROVE', { state: 'NEW' }, '')).toBe(true); + expect(decisionAllowed('APPROVE', { state: 'NEW' }, WRITTEN)).toBe(true); + expect(decisionAllowed('APPROVE', { state: 'OK' }, WRITTEN)).toBe(false); + }); +}); + +describe('refusing the payment', () => { + it('is offered on a new alert once something is written', () => { + expect(decisionAllowed('DECLINE', { state: 'NEW' }, WRITTEN)).toBe(true); + }); + + it('is refused on an alert that already records fraud, which is what it would record', () => { + expect(decisionAllowed('DECLINE', { state: 'SUSPICIOUS' }, WRITTEN)).toBe(false); + }); + + it('is offered on a cleared alert, unlike releasing, because clearing can be reconsidered', () => { + // The asymmetry is the point of having both cases here. OK and SUSPICIOUS are both decided + // states and one of them still takes this verdict. + expect(decisionAllowed('DECLINE', { state: 'OK' }, WRITTEN)).toBe(true); + }); + + it('is refused while the comment box is empty, whatever the state', () => { + expect(decisionAllowed('DECLINE', { state: 'NEW' }, '')).toBe(false); + expect(decisionAllowed('DECLINE', { state: 'OK' }, '')).toBe(false); + }); + + it.each([' ', ' ', '\n', '\t ', ' '])( + 'reads %o as an empty box, so a press cannot send the customer whitespace', + (only) => { + // What the customer is shown is this string. A refusal whose reason is one space is + // the failure the box exists to prevent, and it is a keystroke away from an empty box. + expect(decisionAllowed('DECLINE', { state: 'NEW' }, only)).toBe(false); + }, + ); + + it('counts a comment that is padded but not empty', () => { + expect(decisionAllowed('DECLINE', { state: 'NEW' }, ` ${WRITTEN} `)).toBe(true); + }); +}); + +describe('adding a note', () => { + it.each(STATES)('is offered in every state, here %s', (state) => { + // It takes no verdict and moves no money, so there is no state it is wrong in. An alert + // decided last week is exactly the one somebody comes back to write on. + expect(decisionAllowed('ANNOTATE', { state }, '')).toBe(true); + expect(decisionAllowed('ANNOTATE', { state }, WRITTEN)).toBe(true); + }); +}); + +describe('a state nobody planned for', () => { + it.each(['', 'new', 'PENDING', 'null'])( + 'refuses the verdict that moves money and offers the one that does not, for %o', + (state) => { + // A fourth state added at the server reaches this function as a string. The safe + // reading of an unknown state is that the alert is not the untouched one, and the + // spelling matters: a desk matching case-insensitively would release on `new`. + expect(decisionAllowed('APPROVE', { state }, WRITTEN)).toBe(false); + expect(decisionAllowed('ANNOTATE', { state }, '')).toBe(true); + }, + ); +}); + +describe('the three verdicts as a set', () => { + const KINDS: readonly FraudDecision[] = ['APPROVE', 'DECLINE', 'ANNOTATE']; + + it('answers a boolean for every one of them, so no press falls through undecided', () => { + // The function is a switch over a closed union. A fourth verdict added to the union and + // not to the switch stops the build; a fourth one cast in from elsewhere would fall out + // as undefined, and `disabled={!undefined}` is a live button. + for (const kind of KINDS) { + for (const state of STATES) { + expect(typeof decisionAllowed(kind, { state }, WRITTEN)).toBe('boolean'); + } + } + }); + + it('offers at most one destructive verdict on a decided alert', () => { + // Both desks draw all three buttons always and grey the dead ones, rather than hiding + // them, so this is what an analyst sees rather than an internal invariant. + expect(decisionAllowed('APPROVE', { state: 'SUSPICIOUS' }, WRITTEN)).toBe(false); + expect(decisionAllowed('DECLINE', { state: 'SUSPICIOUS' }, WRITTEN)).toBe(false); + }); +}); diff --git a/frontend-shared/describeDecision.test.ts b/frontend-shared/describeDecision.test.ts new file mode 100644 index 0000000..51b8329 --- /dev/null +++ b/frontend-shared/describeDecision.test.ts @@ -0,0 +1,142 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { DECISION_RESULT_TITLE, DECISION_TITLE, describeDecision } from './glossary'; +import type { AlertDetail, FraudDecision } from './fraud'; + +/** + * What the analyst is told a press did. + * + * The whole point of the function is that the sentence is read off the payment the server sent + * back and not off the button. Deriving it from the button was safe only while every decision did + * the one thing its label said: a DECLINE on a payment that has already gone is now accepted and + * records the verdict without stopping anything, and "Transfer declined" for that would tell the + * analyst the money was held when it is gone. + * + * Both desks wrote these sentences themselves and two had already drifted apart before they were + * moved here, so the same outcome was announced in two different sets of words on the two + * platforms. Nothing was holding either of them to anything. + */ + +const ACTIONS: FraudDecision[] = ['APPROVE', 'DECLINE', 'ANNOTATE']; +const STATUSES = ['CREATED', 'HELD_FOR_REVIEW', 'WAITING_AUTH', 'SENT', 'DECLINED']; + +describe('clearing an alert', () => { + it('says the payment goes back to the customer while it is still waiting on one', () => { + expect(describeDecision('APPROVE', 'WAITING_AUTH')).toBe( + 'Alert cleared. The payment is released to the customer to confirm; no money has moved.', + ); + }); + + it('promises no money has moved, which is the fact the press turns on', () => { + expect(describeDecision('APPROVE', 'WAITING_AUTH')).toContain('no money has moved'); + }); + + it.each(['SENT', 'DECLINED', 'CREATED', 'HELD_FOR_REVIEW'])( + 'says there was nothing to release when the payment was already %s', + (status) => { + expect(describeDecision('APPROVE', status)).toContain('nothing to release'); + expect(describeDecision('APPROVE', status)).not.toContain('released to the customer'); + }, + ); + + it('names what the payment had already become, in the analyst own vocabulary', () => { + expect(describeDecision('APPROVE', 'SENT')).toBe( + 'Alert cleared. The transfer was already sent, so there was nothing to release.', + ); + // Under review, not HELD_FOR_REVIEW: the sentence speaks the status rather than printing + // it, and this is the only place in either application where that is true. + expect(describeDecision('APPROVE', 'HELD_FOR_REVIEW')).toContain('already under review'); + }); +}); + +describe('recording confirmed fraud', () => { + it('says out loud that a payment already gone has NOT been reversed', () => { + // The worse lie the old wording told: "Transfer declined" for money that is gone. + expect(describeDecision('DECLINE', 'SENT')).toBe( + 'Recorded as confirmed fraud. The payment had already been sent and has NOT been reversed.', + ); + }); + + it('does not claim the alert was cleared', () => { + for (const status of STATUSES) { + expect(describeDecision('DECLINE', status)).not.toContain('cleared'); + } + }); + + it('names the state the transfer is left in when the payment was stopped in time', () => { + expect(describeDecision('DECLINE', 'DECLINED')).toBe( + 'Alert recorded as confirmed fraud, and the transfer is declined.', + ); + }); +}); + +describe('saving without deciding', () => { + it.each(STATUSES)('takes no decision whatever the payment is doing, here %s', (status) => { + const said = describeDecision('ANNOTATE', status); + expect(said).toContain('No decision was taken'); + expect(said).toContain('the transfer is unchanged'); + }); + + it('promises nothing this press cannot do', () => { + // It used to promise two. Tags cannot be written over the decision route at all, and the + // assignee is written by a route of its own that no button in the panel calls. + const said = describeDecision('ANNOTATE', 'HELD_FOR_REVIEW'); + expect(said).not.toMatch(/tag/i); + expect(said).not.toMatch(/assign/i); + }); + + it('says the entry was saved on the alert without naming which of the two it was', () => { + // The press may write the comment, the case note, both or neither, so naming one would + // announce the wrong one half the time. + expect(describeDecision('ANNOTATE', 'SENT')).toContain('Saved on the alert'); + }); +}); + +describe('the sentence follows the answer, not the button', () => { + it('gives one press two different sentences for two different answers', () => { + // The single case this function exists for. Same action, same analyst, same click. + expect(describeDecision('DECLINE', 'SENT')).not.toBe( + describeDecision('DECLINE', 'HELD_FOR_REVIEW'), + ); + expect(describeDecision('APPROVE', 'WAITING_AUTH')).not.toBe( + describeDecision('APPROVE', 'SENT'), + ); + }); + + it('reads the status off a body the server really sent', () => { + // A hand written status would prove the branch, not that the branch is reachable with + // what arrives. The captured alert is held for review, so approving it releases nothing. + const detail: AlertDetail = JSON.parse( + readFileSync(new URL('./__fixtures__/fraud-alert-detail.json', import.meta.url), 'utf8'), + ); + expect(detail.transfer.status).toBe('HELD_FOR_REVIEW'); + expect(describeDecision('APPROVE', detail.transfer.status)).toContain( + 'nothing to release', + ); + expect(describeDecision('APPROVE', detail.transfer.status)).not.toContain( + 'no money has moved', + ); + }); +}); + +describe('what no sentence may contain', () => { + it.each(ACTIONS)('prints no raw enum at the analyst after a %s', (action) => { + for (const status of STATUSES) { + // The sentence used to interpolate the token: "the transfer is WAITING_AUTH". + expect(describeDecision(action, status)).not.toMatch(/[A-Z]{2,}_[A-Z]{2,}/); + } + }); + + it.each(ACTIONS)('ends in a full stop after a %s, being prose and not a label', (action) => { + for (const status of STATUSES) { + expect(describeDecision(action, status).endsWith('.')).toBe(true); + } + }); +}); + +describe('the word over the sentence', () => { + it('is not the heading of the panel it sits inside', () => { + // Two headings reading the same word, one inside the other, name nothing. + expect(DECISION_RESULT_TITLE).not.toBe(DECISION_TITLE); + }); +}); diff --git a/frontend-shared/filterProblem.test.ts b/frontend-shared/filterProblem.test.ts new file mode 100644 index 0000000..15bf6bf --- /dev/null +++ b/frontend-shared/filterProblem.test.ts @@ -0,0 +1,158 @@ +import { describe, it, expect } from 'vitest'; +import { filterProblem } from './alertFilters'; +import type { AmountBoxes } from './alertFilters'; + +/** + * The second pair, and the order the complaints come in. + * + * The amount pair has been checked here since the box was a number input; the queue is narrowed by + * a range of raising times as well, and that pair went to the server unchecked. It goes wrong the + * same way and it is answered the same way: an empty queue is how the endpoint reports "nothing + * matched", so a range that cannot match anything must never reach it, or a backwards filter is + * indistinguishable from a quiet afternoon. + * + * The amount cases live beside the ones in the application that owns the screen. What is pinned + * here is what the widening added: the dates, and which complaint is spoken when more than one is + * true at once. + */ + +const READABLE: AmountBoxes = { min: false, max: false }; + +/** An instant as the server's own parser takes one. A date box writes the first ten characters. */ +const AUGUST = '2026-08-01T00:00:00Z'; +const SEPTEMBER = '2026-09-01T00:00:00Z'; + +describe('date ranges that can be sent', () => { + it.each([ + [{}, 'no dates at all'], + [{ createdFrom: AUGUST }, 'an earliest with no latest'], + [{ createdTo: SEPTEMBER }, 'a latest with no earliest'], + [{ createdFrom: AUGUST, createdTo: SEPTEMBER }, 'an ordinary range'], + [{ createdFrom: AUGUST, createdTo: AUGUST }, 'one instant, which both bounds admit'], + [{ createdFrom: '', createdTo: '' }, 'two boxes nobody filled in'], + ])('passes %o - %s', (filters, _case) => { + expect(filterProblem(filters, READABLE)).toBeNull(); + }); + + it('takes an offset as readily as a Z, both being instants', () => { + expect( + filterProblem( + { createdFrom: '2026-08-01T00:00:00+02:00', createdTo: SEPTEMBER }, + READABLE, + ), + ).toBeNull(); + }); + + it('takes the fractional seconds the server sends back on every timestamp', () => { + // Every createdAt on this wire arrives as 2026-08-29T20:20:50.290980Z, so a filter built + // from a row the analyst is looking at has to be sendable. + expect( + filterProblem({ createdFrom: '2026-08-29T20:20:50.290980Z' }, READABLE), + ).toBeNull(); + }); +}); + +describe('a date range that cannot match anything', () => { + it('is refused and names both dates, which the server cannot', () => { + const problem = filterProblem( + { createdFrom: SEPTEMBER, createdTo: AUGUST }, + READABLE, + ); + expect(problem).toBe( + `The earliest date (${SEPTEMBER}) is after the latest (${AUGUST}).`, + ); + }); + + it('names them as they were typed, so the analyst can see which box to change', () => { + const problem = filterProblem( + { createdFrom: SEPTEMBER, createdTo: AUGUST }, + READABLE, + ); + expect(problem).toContain(SEPTEMBER); + expect(problem).toContain(AUGUST); + }); +}); + +describe('a date the server would refuse to parse', () => { + it.each([ + ['2026-08-30', 'what a date input writes, with no zone on it at all'], + ['yesterday', 'a word'], + ['30/08/2026', 'the Czech reading order, which is not a timestamp'], + ['2026-08-30T12:00', 'a local time, which names no instant'], + ])('refuses %o as the earliest - %s', (raw, _case) => { + // Instant.parse takes nothing else, and over the wire its refusal is only "the request + // contains invalid or missing values". + expect(filterProblem({ createdFrom: raw }, READABLE)).toContain( + 'The earliest date could not be read', + ); + }); + + it('refuses the same value in the latest box, and says which box', () => { + expect(filterProblem({ createdTo: '2026-08-30' }, READABLE)).toContain( + 'The latest date could not be read', + ); + }); + + it('teaches the shape rather than naming the standard', () => { + expect(filterProblem({ createdFrom: 'yesterday' }, READABLE)).toContain( + '2026-08-30T00:00:00Z', + ); + }); + + it('does not let an unreadable half slip through the comparison beside it', () => { + // The trap this branch exists for: parsed to NaN, the pair compares false both ways and a + // range nobody can read is sent as a range that is fine. + expect(filterProblem({ createdFrom: 'yesterday', createdTo: AUGUST }, READABLE)) + .not.toBeNull(); + expect(filterProblem({ createdFrom: SEPTEMBER, createdTo: 'tomorrow' }, READABLE)) + .not.toBeNull(); + }); +}); + +describe('which complaint is spoken when more than one is true', () => { + // One filter, everything wrong with it at once. Each case below removes the complaint above it + // and reads what is left, which is what makes this a total order rather than three assertions. + const everything = { + createdFrom: SEPTEMBER, + createdTo: AUGUST, + minAmount: '500', + maxAmount: '100', + }; + + it('speaks the box the browser could not read first, its evidence being the one that is gone', () => { + expect(filterProblem(everything, { min: true, max: false })).toContain( + 'could not be read', + ); + }); + + it('speaks the dates next, because they narrow what the amounts are asked about', () => { + expect(filterProblem(everything, READABLE)).toContain('The earliest date'); + }); + + it('speaks the reversed amounts last, where the reader eye already is', () => { + const { createdFrom: _from, createdTo: _to, ...amountsOnly } = everything; + expect(filterProblem(amountsOnly, READABLE)).toBe( + 'The smallest amount (500) is above the largest (100).', + ); + }); + + it('holds the same order for a date that cannot be read as for one that is backwards', () => { + expect( + filterProblem({ createdFrom: 'yesterday', minAmount: '500', maxAmount: '100' }, READABLE), + ).toContain('The earliest date could not be read'); + }); + + it('answers null once none of the three is true, so the order is an order and not a trap', () => { + expect( + filterProblem( + { + createdFrom: AUGUST, + createdTo: SEPTEMBER, + minAmount: '100', + maxAmount: '500', + }, + READABLE, + ), + ).toBeNull(); + }); +}); diff --git a/frontend-shared/fraud.ts b/frontend-shared/fraud.ts index d127187..781ac4e 100644 --- a/frontend-shared/fraud.ts +++ b/frontend-shared/fraud.ts @@ -452,6 +452,69 @@ export async function fetchAlertHistory( return handle>(res); } +/** + * Whether this verdict may be taken at all, given the alert in front of the analyst. + * + * Written twice before this, once per desk, as three `disabled=` expressions each. The two copies + * happened to agree; nothing made them agree, and each of the three rules is a rule about money. + * + * APPROVE releases a held payment, so it is offered on an alert nobody has decided and on no + * other: pressed on a decided alert the server refuses it, and the refusal arrives as the general + * validation sentence, which does not say that somebody else got there first. + * + * DECLINE is refused on SUSPICIOUS because that state is already the recorded fraud, and it is + * refused on an empty comment because the comment is the sentence the customer is shown. There is + * no second chance to write it: the press cannot be taken back. + * + * ANNOTATE takes no verdict and changes no payment, so there is no state it is wrong in. + * + * The screen's own busy flag is deliberately not a parameter. A request in flight is not a fact + * about the alert, it is a fact about the screen, and folding the two together would let a slow + * network read as a decision already taken. + * + * @param comment the decision comment box as typed, trimmed here rather than by the caller + */ +export function decisionAllowed( + kind: FraudDecision, + alert: { state: string }, + comment: string, +): boolean { + switch (kind) { + case 'APPROVE': + return alert.state === 'NEW'; + case 'DECLINE': + return alert.state !== 'SUSPICIOUS' && comment.trim() !== ''; + case 'ANNOTATE': + return true; + } +} + +/** + * Builds the body of a decision out of the two boxes on the desk. + * + * The point of it is the absent key. Both optional fields mean "leave what is there alone" when + * they are missing, and an empty string is not the same instruction: `note` writes an entry into + * the journal, so a blank one would file an empty paragraph on every press, and a blank `comment` + * would overwrite the sentence a colleague wrote about the verdict with nothing. Both desks got + * this right by writing `|| undefined`, which leaves the key present holding undefined and relies + * on JSON.stringify dropping it on the way out. That works, and it means the object a test can + * inspect and the bytes the server reads are two different things. Here the key is not set. + * + * The verdict itself is never omitted and is never trimmed: it is a token, not typing. + */ +export function buildDecisionRequest( + decision: FraudDecision, + comment: string, + note: string, +): FraudDecisionRequest { + const body: FraudDecisionRequest = { decision }; + const writtenComment = comment.trim(); + if (writtenComment !== '') body.comment = writtenComment; + const writtenNote = note.trim(); + if (writtenNote !== '') body.note = writtenNote; + return body; +} + /** * Records the verdict, and answers the whole alert back. * diff --git a/frontend-shared/navigation.test.ts b/frontend-shared/navigation.test.ts new file mode 100644 index 0000000..0f4f488 --- /dev/null +++ b/frontend-shared/navigation.test.ts @@ -0,0 +1,171 @@ +import { describe, it, expect } from 'vitest'; +import { + ACTIVITY_EVENTS, + ACTIVITY_REFRESH_MS, + NAV_ENTRIES, + NAV_LANDMARK, + NAV_TITLE, + NO_SCREENS_TITLE, + PLANNED_TITLE, + SERVED_ROLES, + SESSION_IDLE_NOTE, + SIGNED_OUT_NOTICE, + SIGNED_OUT_WITH_LOSS, + noScreensNote, + servedRole, +} from './navigation'; +import type { NavEntry, NavRole, NavView } from './navigation'; + +/** + * The column existed three times in hand written markup and the three copies disagreed, which is + * what moved it here. Moving it settled the disagreement and left the list itself unasserted: the + * rule that no application may add a destination, drop one or reorder them is stated in a comment + * and nothing has been holding anybody to it. + */ + +/** Every screen a person can stand on, and the compiler will not let this list be short. */ +const ALL_VIEWS: Record = { + 'new-payment': true, + history: true, + 'waiting-auth': true, + 'fraud-desk': true, +}; + +/** + * The destinations in a column, the planned entries dropped. + * + * flatMap rather than filter and map, because a filter narrows nothing: the planned half of the + * union survives it as far as the compiler is concerned and has no view to read. + */ +function viewsOf(entries: readonly NavEntry[]): NavView[] { + return entries.flatMap((entry) => (entry.kind === 'screen' ? [entry.view] : [])); +} + +describe('which signed in roles these clients have screens for', () => { + it.each(['CUSTOMER', 'FRAUD_ANALYST'])('serves %s', (role) => { + expect(servedRole(role)).toBe(role); + }); + + it.each(['OPERATIONS', 'MANAGEMENT'])('has nothing to draw for %s, which the bank has', (r) => { + // The wire keeps all four names on purpose. An endpoint that can answer OPERATIONS is not + // made honest by a type saying it cannot, so the narrowing happens here and answers null. + expect(servedRole(r)).toBeNull(); + }); + + it.each(['', 'customer', 'ADMIN', 'FRAUD_ANALYST ', 'undefined'])( + 'answers null for %o rather than letting it through a comparison that looks exhaustive', + (junk) => { + // The argument is a plain string because both the sign-in response and the record a + // tab kept for itself are cast rather than validated. + expect(servedRole(junk)).toBeNull(); + }, + ); + + it('admits exactly the roles that have a column, neither more nor fewer', () => { + expect([...SERVED_ROLES].sort()).toEqual(Object.keys(NAV_ENTRIES).sort()); + for (const role of SERVED_ROLES) { + expect(servedRole(role)).toBe(role); + } + }); +}); + +describe('the column each role is offered', () => { + const roles = Object.keys(NAV_ENTRIES) as NavRole[]; + + it.each(roles)('gives %s somewhere to actually stand', (role) => { + // A role with a column of planned entries only would sign in to a shell that goes nowhere, + // which is the state the no-screens panel exists for and not a column. + expect(NAV_ENTRIES[role].some((entry) => entry.kind === 'screen')).toBe(true); + }); + + it.each(roles)('keys the list of %s by ids no two entries share', (role) => { + const ids = NAV_ENTRIES[role].map((entry) => entry.id); + expect(new Set(ids).size).toBe(ids.length); + }); + + it.each(roles)('offers no screen twice in the column of %s', (role) => { + const views = viewsOf(NAV_ENTRIES[role]); + expect(new Set(views).size).toBe(views.length); + }); + + it('offers every screen that exists, to exactly one role', () => { + const offered = viewsOf(Object.values(NAV_ENTRIES).flat()); + expect(offered.sort()).toEqual(Object.keys(ALL_VIEWS).sort()); + }); + + it('hands a planned entry nothing to navigate to, so no renderer can wire one up', () => { + // The union is what enforces this at compile time. Asserted again here because an entry + // built elsewhere and cast reaches the renderers through the same list. + for (const entry of Object.values(NAV_ENTRIES).flat()) { + if (entry.kind === 'planned') { + expect('view' in entry).toBe(false); + } + } + }); + + it('gives every planned entry the same sentence, because the reason does not differ', () => { + const titles = Object.values(NAV_ENTRIES) + .flat() + .flatMap((entry) => (entry.kind === 'planned' ? [entry.title] : [])); + expect(titles.length).toBeGreaterThan(0); + for (const title of titles) { + expect(title).toBe(PLANNED_TITLE); + } + }); + + it('is in product order, not in the order things happened to be built', () => { + // Both columns open on a planned entry and the customer column closes on one, with the + // three built screens between them. A column sorted by what is implemented would tell the + // reader about the state of the project instead of about the bank. + for (const role of roles) { + expect(NAV_ENTRIES[role][0].kind).toBe('planned'); + } + const customer = NAV_ENTRIES.CUSTOMER; + expect(customer[customer.length - 1].kind).toBe('planned'); + }); + + it('labels itself for a reader and for a screen reader without saying the same word twice', () => { + expect(NAV_TITLE).not.toBe(NAV_LANDMARK); + // The element already announces its own kind, so the landmark must not repeat it. + expect(NAV_LANDMARK).not.toMatch(/navigation/i); + }); +}); + +describe('what a role with no screens is told', () => { + it('names the person, which is what makes it a fact about the account', () => { + expect(noScreensNote('opsuser')).toContain('opsuser'); + }); + + it('prints no role, because the client narrowed its type rather than keep a name to print', () => { + const note = `${NO_SCREENS_TITLE} ${noScreensNote('opsuser')}`; + expect(note).not.toMatch(/OPERATIONS|MANAGEMENT/); + }); + + it('reads as a fact about the account and not as a fault in the application', () => { + expect(NO_SCREENS_TITLE).not.toMatch(/error|sorry|wrong|failed/i); + }); +}); + +describe('the session, and the notice for the person who was in the middle of something', () => { + it('carries the plain notice whole, plus the one clause that notice cannot answer', () => { + expect(SIGNED_OUT_WITH_LOSS.startsWith(SIGNED_OUT_NOTICE)).toBe(true); + // Being told you were signed out does not say whether what you typed went through, and + // silence reads as yes. + expect(SIGNED_OUT_WITH_LOSS.slice(SIGNED_OUT_NOTICE.length)).toContain('Nothing'); + }); + + it('says how long a session survives idle, so nobody is judged by a rule they were not told', () => { + expect(SESSION_IDLE_NOTE).toContain('15'); + }); + + it('reads for a keep alive well inside that window', () => { + expect(ACTIVITY_REFRESH_MS).toBeLessThan(15 * 60 * 1000); + }); + + it('does not count a cursor crossing the window as somebody working', () => { + // A pointer on its way somewhere else would keep an empty desk signed in, which is the + // thing the timeout exists to end. + expect(ACTIVITY_EVENTS).not.toContain('mousemove'); + expect(ACTIVITY_EVENTS.length).toBeGreaterThan(0); + }); +}); diff --git a/frontend-shared/navigation.ts b/frontend-shared/navigation.ts index c0829f7..005c7ce 100644 --- a/frontend-shared/navigation.ts +++ b/frontend-shared/navigation.ts @@ -111,8 +111,11 @@ export const NAV_ENTRIES: Record = { ], FRAUD_ANALYST: [ { kind: 'planned', id: 'dashboard', label: 'Dashboard', title: PLANNED_TITLE }, - { kind: 'planned', id: 'settings', label: 'Settings', title: PLANNED_TITLE }, + // The desk before the settings, which is the order the customer's column already reads in: + // the place the work happens, then the place the account is configured. Settings sat above + // it and put the one screen this role has third of three. { kind: 'screen', id: 'fraud-desk', label: 'Fraud desk', view: 'fraud-desk' }, + { kind: 'planned', id: 'settings', label: 'Settings', title: PLANNED_TITLE }, ], }; diff --git a/frontend-shared/paging.test.ts b/frontend-shared/paging.test.ts new file mode 100644 index 0000000..884a789 --- /dev/null +++ b/frontend-shared/paging.test.ts @@ -0,0 +1,184 @@ +import { describe, it, expect } from 'vitest'; +import { + DEFAULT_PAGE_SIZE, + MAX_PAGE_SIZE, + SHOW_MORE, + SHOW_MORE_BUSY, + appendPage, + applyPaging, + hasMore, + nextPage, + showingLine, +} from './paging'; + +/** + * Five lists across two applications grow the same foot: a Show more button, a line saying how + * much is on screen, and the arithmetic that decides both. None of it had a test, which is the + * state a module is in when it is about to be reimplemented in a screen by somebody who did not + * know it was here. + */ + +describe('how much of the list is on screen', () => { + it('names the second number only while there is one worth naming', () => { + expect(showingLine(25, 137)).toBe('Showing: 25 of 137'); + expect(showingLine(137, 137)).toBe('Showing: 137'); + }); + + it('stands in a row of labels, so it carries a colon and no full stop', () => { + // Its neighbours in that strip are "New: 12" and "Cleared: 122". A stop here would be the + // one cell of four that ends a sentence. + for (const line of [showingLine(25, 137), showingLine(137, 137)]) { + expect(line).toContain(': '); + expect(line.endsWith('.')).toBe(false); + } + }); + + it('prints a row count plain, which the money formatter would not', () => { + // Through formatCzech this would read "10,00", and the thousands would be divided with a + // non-breaking space. These are rows. + expect(showingLine(10, 10)).toBe('Showing: 10'); + expect(showingLine(1000, 2000)).toBe('Showing: 1000 of 2000'); + }); + + it('cannot say it holds more than there is, however the two numbers arrive', () => { + // A total that has shrunk under a list already on screen is reachable: rows are deleted + // between two requests. "Showing: 12 of 10" is not. + expect(showingLine(12, 10)).toBe('Showing: 12'); + }); + + it('counts whole rows, because half a row is not on screen', () => { + expect(showingLine(2.7, 10)).toBe('Showing: 2 of 10'); + }); + + it('says nothing about an empty list, which is the screen own sentence', () => { + expect(showingLine(0, 0)).toBe('Showing: 0'); + }); +}); + +describe('whether the button is drawn at all', () => { + it.each([ + [25, 137, true], + [137, 137, false], + [0, 0, false], + [0, 1, true], + ])('holds %i of %i and answers %s', (shown, total, expected) => { + expect(hasMore(shown, total)).toBe(expected); + }); + + it('is false once a list has outgrown the total it was promised', () => { + expect(hasMore(12, 10)).toBe(false); + }); + + it('agrees with the line beside it: no button exactly when there is no second number', () => { + for (const [shown, total] of [ + [25, 137], + [137, 137], + [12, 10], + [0, 0], + ]) { + expect(showingLine(shown, total).includes(' of ')).toBe(hasMore(shown, total)); + } + }); +}); + +describe('the page to ask for next', () => { + it('is counted from the page that arrived and not from the rows on screen', () => { + // The distinction has one witness and this is it: a page whose rows were all dropped as + // repeats. Derived from the row count the request would repeat page 1 forever. + const arrived = { items: [], page: 1, size: 25, total: 137 }; + expect(nextPage(arrived)).toBe(2); + }); + + it('follows the size the server echoed rather than the size that was asked for', () => { + expect(nextPage({ items: [], page: 0, size: 10, total: 40 })).toBe(1); + }); +}); + +describe('adding a page to the rows already held', () => { + const held = [{ id: 1 }, { id: 2 }, { id: 3 }]; + + it('drops the row offset paging handed back twice', () => { + // A payment created between the two requests pushes a row off page one onto page two. + // Kept, it is one payment drawn twice under one React key. + expect(appendPage(held, [{ id: 3 }, { id: 4 }])).toEqual([ + { id: 1 }, + { id: 2 }, + { id: 3 }, + { id: 4 }, + ]); + }); + + it('keeps the order the rows arrived in, the held ones first', () => { + expect(appendPage([{ id: 5 }], [{ id: 9 }, { id: 7 }]).map((r) => r.id)).toEqual([5, 9, 7]); + }); + + it('drops a repeat inside the incoming page too, not only one against the held rows', () => { + expect(appendPage([], [{ id: 1 }, { id: 1 }]).map((r) => r.id)).toEqual([1]); + }); + + it('leaves the array it was given alone, so a stale render cannot see a half merged list', () => { + const before = [{ id: 1 }]; + appendPage(before, [{ id: 2 }]); + expect(before).toEqual([{ id: 1 }]); + }); + + it('answers a page of nothing with the rows already held', () => { + expect(appendPage(held, [])).toEqual(held); + }); + + it('keeps the row it already had rather than the one that arrived under the same id', () => { + // Not an arbitrary choice between two copies: the held row is the one on screen, and + // replacing it would redraw a row under the reader for a change they cannot see. + const merged = appendPage([{ id: 1, state: 'NEW' }], [{ id: 1, state: 'OK' }]); + expect(merged).toEqual([{ id: 1, state: 'NEW' }]); + }); +}); + +describe('what goes on the query string', () => { + it('writes both numbers and hands the same parameters back', () => { + const params = new URLSearchParams(); + expect(applyPaging(params, 2, 25)).toBe(params); + expect(params.get('page')).toBe('2'); + expect(params.get('size')).toBe('25'); + }); + + it('leaves the parameters the caller had already built, which is why it takes them', () => { + // The alert queue writes six of its own before it gets here. A second URLSearchParams to + // merge is how one of the six goes missing. + const params = new URLSearchParams(); + params.set('state', 'NEW'); + params.append('excludeTransferStatus', 'DECLINED'); + applyPaging(params, 0, DEFAULT_PAGE_SIZE); + expect(params.get('state')).toBe('NEW'); + expect(params.getAll('excludeTransferStatus')).toEqual(['DECLINED']); + }); + + it('clamps a size the server would refuse, so a screen gets rows and not an error box', () => { + const params = applyPaging(new URLSearchParams(), 0, MAX_PAGE_SIZE + 1); + expect(params.get('size')).toBe(String(MAX_PAGE_SIZE)); + }); + + it('asks for one row rather than none, because a size of zero is a mistake in a screen', () => { + expect(applyPaging(new URLSearchParams(), 0, 0).get('size')).toBe('1'); + expect(applyPaging(new URLSearchParams(), 0, -5).get('size')).toBe('1'); + }); + + it('never asks for a page before the first', () => { + expect(applyPaging(new URLSearchParams(), -1, 25).get('page')).toBe('0'); + }); + + it('sends the default through untouched, which is what makes it the default', () => { + expect(DEFAULT_PAGE_SIZE).toBeLessThanOrEqual(MAX_PAGE_SIZE); + expect(applyPaging(new URLSearchParams(), 0, DEFAULT_PAGE_SIZE).get('size')).toBe( + String(DEFAULT_PAGE_SIZE), + ); + }); +}); + +describe('the two words on the button', () => { + it('does not widen under the pointer that just pressed it', () => { + // "Loading more" was the alternative and it is longer than the label it replaces, so the + // button grows and the control moves out from under the finger. + expect(SHOW_MORE_BUSY.length).toBeLessThanOrEqual(SHOW_MORE.length); + }); +}); diff --git a/frontend-shared/paymentForm.test.ts b/frontend-shared/paymentForm.test.ts new file mode 100644 index 0000000..7b6d6ae --- /dev/null +++ b/frontend-shared/paymentForm.test.ts @@ -0,0 +1,228 @@ +import { describe, it, expect } from 'vitest'; +import { checkPaymentForm } from './paymentForm'; +import type { + PaymentCheck, + PaymentField, + PaymentFormValues, + PaymentProblems, +} from './paymentForm'; + +/** + * The form's own refusals: all of them on one press, each at the box it is about. + * + * The behaviour under test is the plural. A form that stops at the first thing it finds sends the + * customer round the loop once per mistake, and the two mistakes people actually make together are + * an account they never chose and an amount typed in their own convention. What is checked here is + * that both come back from one press, that each answer names its own box, and that the order the + * caret is sent in is the order the boxes stand in rather than the order the checks ran in. + * + * The amount's own wording is not re-tested here; money.test.ts owns that. What is tested is that + * the parser's reason is passed through as it was given rather than replaced by one generic + * refusal, and that the value it read comes back with it so the caller need not parse twice. + */ + +/** A form somebody has filled in correctly, which each case below then breaks in one place. */ +const FILLED: PaymentFormValues = { + selectedAccountId: 1, + amount: '1 500,00', + beneficiaryId: null, + targetIban: 'CZ6508000000192000145399', + hasSavedBeneficiaries: false, +}; + +/** The same form with every box left alone, which is what a first press on a fresh screen sees. */ +const UNTOUCHED: PaymentFormValues = { + selectedAccountId: null, + amount: '', + beneficiaryId: null, + targetIban: '', + hasSavedBeneficiaries: false, +}; + +const CZECH = 'cs-CZ'; + +/** The boxes a check complained about, named rather than counted. */ +function complained(problems: PaymentProblems): PaymentField[] { + return (Object.keys(problems) as PaymentField[]).sort(); +} + +/** + * How Intl groups thousands in Czech, which is not the space anyone types. + * + * Spelled as an escape rather than pasted in, so that the assertions below say which character + * they mean instead of carrying an invisible one. + */ +const NBSP = '\u00a0'; + +describe('a form with nothing wrong with it', () => { + it('finds no problem, sends the caret nowhere and reads the amount', () => { + const check: PaymentCheck = checkPaymentForm(FILLED, CZECH); + expect(check.problems).toEqual({}); + expect(check.first).toBeNull(); + expect(check.amount).toEqual({ ok: true, value: 1500, czech: `1${NBSP}500,00` }); + }); + + it('is just as happy with a saved payee and no typed destination', () => { + const check = checkPaymentForm( + { ...FILLED, beneficiaryId: 7, targetIban: '', hasSavedBeneficiaries: true }, + CZECH, + ); + expect(check.problems).toEqual({}); + }); +}); + +describe('each box on its own', () => { + it('names the account when none is chosen', () => { + const check = checkPaymentForm({ ...FILLED, selectedAccountId: null }, CZECH); + expect(check.problems.source).toBe('Choose the account this payment leaves.'); + expect(check.problems.target).toBeUndefined(); + expect(check.problems.amount).toBeUndefined(); + }); + + it('names the destination when neither half of it is given', () => { + const check = checkPaymentForm({ ...FILLED, targetIban: '' }, CZECH); + expect(check.problems.target).toBe('Enter the account number this payment goes to.'); + }); + + it.each([' ', '\t', '\n '])( + 'reads %o in the destination box as nothing given', + (only) => { + // An account number cannot be whitespace, and the request would go out with one. + expect(checkPaymentForm({ ...FILLED, targetIban: only }, CZECH).problems.target) + .toContain('Enter the account number'); + }, + ); + + it('names the amount when the box is empty, in the parser own words', () => { + const check = checkPaymentForm({ ...FILLED, amount: '' }, CZECH); + expect(check.problems.amount).toBe('Enter the amount to send.'); + }); + + it('passes the parser reason through rather than replacing it with one generic refusal', () => { + // The point of passing it through: it teaches the shape. The amounts people get wrong are + // the ones where two readings of the same characters differ by a factor of a thousand, and + // "invalid amount" cannot say which reading this bank took. + const check = checkPaymentForm({ ...FILLED, amount: 'a lot' }, CZECH); + expect(check.problems.amount).toContain('1 500,00'); + }); +}); + +describe('the destination sentence when the customer has saved payees', () => { + it('offers the list as the other way out', () => { + const check = checkPaymentForm( + { ...FILLED, targetIban: '', hasSavedBeneficiaries: true }, + CZECH, + ); + expect(check.problems.target).toBe( + 'Enter the account number this payment goes to, or choose a saved beneficiary.', + ); + }); + + it('does not offer a list to somebody who has none, which is an instruction they cannot follow', () => { + const check = checkPaymentForm( + { ...FILLED, targetIban: '', hasSavedBeneficiaries: false }, + CZECH, + ); + expect(check.problems.target).not.toContain('saved beneficiary'); + }); + + it('says nothing about the destination once a saved payee names it', () => { + // Skipped explicitly rather than by luck. The two are never both sent: the server refuses + // a request that names a beneficiary and an account number at once. + const check = checkPaymentForm( + { ...FILLED, beneficiaryId: 7, targetIban: '', hasSavedBeneficiaries: true }, + CZECH, + ); + expect(check.problems.target).toBeUndefined(); + }); +}); + +describe('more than one thing wrong at once', () => { + it('reports all three from one press, which is the whole reason this function exists', () => { + const check = checkPaymentForm(UNTOUCHED, CZECH); + expect(complained(check.problems)).toEqual(['amount', 'source', 'target']); + }); + + it('reports the pair people actually make together', () => { + // No account chosen and an amount in a convention this parser refuses. Answered one at a + // time, that is two presses and two refusals for one filling in of one form. + const check = checkPaymentForm( + { ...FILLED, selectedAccountId: null, amount: 'abc' }, + CZECH, + ); + expect(check.problems.source).toBeDefined(); + expect(check.problems.amount).toBeDefined(); + }); +}); + +describe('where the caret is sent', () => { + it('goes to the account, which stands above the other two', () => { + expect(checkPaymentForm({ ...FILLED, selectedAccountId: null, amount: '' }, CZECH).first) + .toBe('source'); + }); + + it('goes to the destination when the account is chosen and the amount is not readable', () => { + // Screen order, not check order. The amount is parsed second and stands third, so a form + // that sent the caret in the order the checks ran would skip the box in between. + expect(checkPaymentForm({ ...FILLED, targetIban: '', amount: '' }, CZECH).first) + .toBe('target'); + }); + + it('goes to the amount when it is the only thing left', () => { + expect(checkPaymentForm({ ...FILLED, amount: '' }, CZECH).first).toBe('amount'); + }); + + it('goes nowhere when there is nothing to fix', () => { + expect(checkPaymentForm(FILLED, CZECH).first).toBeNull(); + }); + + it('names a box that has a problem in it, every time', () => { + // The trap on the other side: a first field that names a box with no sentence under it + // moves the caret to a box the customer cannot see anything wrong with. + for (const values of [ + UNTOUCHED, + { ...FILLED, selectedAccountId: null }, + { ...FILLED, targetIban: '' }, + { ...FILLED, amount: '' }, + ]) { + const check = checkPaymentForm(values, CZECH); + const first = check.first; + expect(first).not.toBeNull(); + expect(first == null ? undefined : check.problems[first]).toBeDefined(); + } + }); +}); + +describe('the amount the caller gets back', () => { + it('is the reading, so the caller does not parse the same string a second time', () => { + const check = checkPaymentForm({ ...FILLED, amount: '2 000,50' }, CZECH); + expect(check.amount).toEqual({ ok: true, value: 2000.5, czech: `2${NBSP}000,50` }); + }); + + it('carries the Czech spelling the box is rewritten with, so the reading is on screen', () => { + // The echo. Whatever was understood goes back into the box, which is what stops the + // customer having to guess which reading the bank took. + expect(checkPaymentForm({ ...FILLED, amount: '1500' }, CZECH).amount) + .toEqual({ ok: true, value: 1500, czech: `1${NBSP}500,00` }); + }); + + it('reads the one ambiguous shape against the locale it was handed', () => { + // `1,000` is a thousand to a reader whose locale groups with a comma. To a Czech one the + // comma divides, which makes it three digits of hellers and refused for being finer than + // the currency goes. Same characters, two readings, and the locale is what tells them + // apart: the parser that took the second reading unconditionally paid 1 CZK for 1000. + expect(checkPaymentForm({ ...FILLED, amount: '1,000' }, 'en-US').amount) + .toEqual({ ok: true, value: 1000, czech: `1${NBSP}000,00` }); + expect(checkPaymentForm({ ...FILLED, amount: '1,000' }, CZECH).amount.ok).toBe(false); + }); + + it('comes back unreadable rather than absent, so the caller has one thing to test', () => { + const check = checkPaymentForm({ ...FILLED, amount: 'abc' }, CZECH); + expect(check.amount.ok).toBe(false); + }); + + it('is read even while another box is empty, the checks not stopping at the first', () => { + const check = checkPaymentForm({ ...FILLED, selectedAccountId: null }, CZECH); + expect(check.amount).toEqual({ ok: true, value: 1500, czech: `1${NBSP}500,00` }); + }); +}); diff --git a/frontend-shared/paymentForm.ts b/frontend-shared/paymentForm.ts new file mode 100644 index 0000000..f0e4aa3 --- /dev/null +++ b/frontend-shared/paymentForm.ts @@ -0,0 +1,85 @@ +/** + * Everything a new payment can be refused for before the bank is asked, and all of it at once. + * + * Shared because it was written twice, character for character, including the three sentences: the + * customer's form and the analyst workstation's form each carried their own copy of the same block. + * The copies agreed on the day they were written. What they could not agree on afterwards is a + * fourth check, or a reworded sentence, or the order the boxes are visited in. + * + * All the checks run and the verdict is taken after them. The form used to stop at the first thing + * it found, so a customer with no account chosen and an unreadable amount fixed one, pressed again, + * and was refused again for the other one. Each answer belongs at the box it is about; a single + * sentence at the foot of the form cannot say which of three boxes it means. + */ + +import { parseAmount, type ParsedAmount } from './money'; + +/** The three boxes this form can refuse on its own, in the order they stand on the screen. */ +export type PaymentField = 'source' | 'target' | 'amount'; + +/** Every problem the last press found, and never only the first of them. */ +export type PaymentProblems = Partial>; + +/** What the boxes hold. Nothing here is a screen concern: no refs, no setters, no busy flag. */ +export interface PaymentFormValues { + /** The account the money leaves, or null while nothing is chosen. */ + selectedAccountId: number | null; + /** The amount box as typed, in whatever convention the person in front of it uses. */ + amount: string; + /** A saved payee, or null when the destination is typed out instead. */ + beneficiaryId: number | null; + /** The typed destination. Never sent together with a beneficiary: the server refuses both. */ + targetIban: string; + /** Whether this customer has saved payees, which decides how the destination is asked for. */ + hasSavedBeneficiaries: boolean; +} + +export interface PaymentCheck { + problems: PaymentProblems; + /** + * The box the caret belongs in, or null when there is nothing to fix. + * + * The first box a reader coming down the form would have reached, which is not the first check + * that ran: the amount is parsed second and stands third. + */ + first: PaymentField | null; + /** + * The amount as it was read, whether or not it could be. + * + * Handed back rather than parsed again by the caller, because the caller needs both halves of + * it: the number to send, and the Czech spelling to write into the box so that what the bank + * understood is on screen rather than only in the review beneath it. + */ + amount: ParsedAmount; +} + +/** Screen order, which is what decides where the caret lands. */ +const PAYMENT_FIELDS: readonly PaymentField[] = ['source', 'target', 'amount']; + +export function checkPaymentForm(values: PaymentFormValues, locale: string): PaymentCheck { + const problems: PaymentProblems = {}; + + if (values.selectedAccountId == null) { + problems.source = 'Choose the account this payment leaves.'; + } + + // The parser's own reason is passed through rather than replaced by one generic refusal. It + // names what is wrong with this particular string, and the amounts people get wrong are the + // ones where two readings of the same characters differ by a factor of a thousand. + const amount = parseAmount(values.amount, locale); + if (!amount.ok) { + problems.amount = amount.reason; + } + + // Skipped explicitly when a saved payee names the destination, rather than by luck. The + // sentence changes with what this customer actually has: offering a saved beneficiary to + // somebody who has none is an instruction they cannot follow. + if (values.beneficiaryId == null && values.targetIban.trim() === '') { + problems.target = values.hasSavedBeneficiaries + ? 'Enter the account number this payment goes to, or choose a saved beneficiary.' + : 'Enter the account number this payment goes to.'; + } + + const first = PAYMENT_FIELDS.find((field) => problems[field] != null) ?? null; + return { problems, first, amount }; +} diff --git a/frontend-shared/route.ts b/frontend-shared/route.ts index cf5c7fd..c22b1d7 100644 --- a/frontend-shared/route.ts +++ b/frontend-shared/route.ts @@ -146,6 +146,21 @@ export function goTo(route: Route) { if (hash !== location.hash) location.hash = hash; } +/** + * A press in the navigation column, from whichever shell drew it. + * + * The three lines it replaces are the guard, the route and the move, and each shell was about to + * write its own copy of them. They are one decision, not three: an entry the role is not served may + * not become an address, whoever pressed it and whichever window they pressed it in. Kept here + * rather than in the navigation module because the guard and the address are both this module's, + * and it is the address that the press turns into. + */ +export function navigateToView(role: NavRole, view: NavView) { + const target = routeFor(view); + if (!allowedForRole(role, target)) return; + goTo(target); +} + /** * A correction the program made: a selection dropped because the payment left the list, an * address nobody can have. diff --git a/frontend-shared/tokens/theme-desktop.css b/frontend-shared/tokens/theme-desktop.css index fc54fdc..e074f58 100644 --- a/frontend-shared/tokens/theme-desktop.css +++ b/frontend-shared/tokens/theme-desktop.css @@ -204,6 +204,17 @@ /* ---- shadow ---- */ --shadow-shell: 0 10px 35px rgb(0 0 0 / 0.15); + /* + * A box pressed INTO the surface rather than standing on it, which is the one piece of depth a + * light window uses that a dark page has no equivalent for. Two layers: a shadow cast from the + * upper edge inwards, which is what reads as a lip above the sunken face, and a hairline of the + * same ink all round to close the well. Inset, so it is drawn inside the radius the box already + * has and nothing about the box's size or position changes when it is applied. + * + * It is a shadow and therefore not there at all under a forced palette; anything relying on it + * has to say the same thing a second way, and the rail's selected row does, with an edge. + */ + --shadow-recessed: inset 0 1px 3px rgb(0 0 0 / 0.2), inset 0 0 0 1px rgb(0 0 0 / 0.05); /* ---- named fixed dimensions. Not on any scale. Named so the desktop customer * screens reuse them rather than invent a second set. The content, form, prose, diff --git a/frontend-shared/tsconfig.tests.json b/frontend-shared/tsconfig.tests.json new file mode 100644 index 0000000..4cac204 --- /dev/null +++ b/frontend-shared/tsconfig.tests.json @@ -0,0 +1,43 @@ +{ + /* How the tests in this directory are type checked, which is not with the applications. + + There is no node_modules here and none above it, so `vitest` and `node:fs` cannot be resolved + from a file in this directory by walking up from it. The application programs include this + whole directory, which is how eight test files came to stop both builds at once with the same + twenty-nine errors: the modules they import do not exist as far as an application is concerned. + The applications now leave the tests out and keep the rest, and the tests are checked here + instead, against the one vitest install the whole tree has. + + Run from minibank-web, whose node_modules this points at: + npx tsc -p ../frontend-shared/tsconfig.tests.json */ + "compilerOptions": { + "target": "ES2022", + "lib": ["ES2022", "DOM", "DOM.Iterable"], + "module": "ESNext", + "moduleResolution": "bundler", + "skipLibCheck": true, + "verbatimModuleSyntax": true, + "moduleDetection": "force", + "noEmit": true, + + /* The same strictness the applications are held to, so a test cannot be written in a dialect + the code it tests would not compile in. */ + "strict": true, + "noUnusedLocals": true, + "noUnusedParameters": true, + "erasableSyntaxOnly": true, + "noFallthroughCasesInSwitch": true, + "noUncheckedSideEffectImports": true, + + /* Both borrowed from the one install. `node` is here because two of these files read the + fixture bodies off disk, which is the whole point of wire.test.ts: it asserts against bytes + the server actually sent rather than against an object the test author wrote. */ + "typeRoots": ["../minibank-web/node_modules/@types"], + "types": ["node"], + "baseUrl": ".", + "paths": { + "vitest": ["../minibank-web/node_modules/vitest"] + } + }, + "include": ["*.test.ts"] +} diff --git a/frontend-shared/wire.test.ts b/frontend-shared/wire.test.ts new file mode 100644 index 0000000..e0055ac --- /dev/null +++ b/frontend-shared/wire.test.ts @@ -0,0 +1,236 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import type { + AlertDetail, + AlertInfo, + AlertNote, + AlertQueueItem, + AlertQueueResponse, + HistoryItem, + TransferInfo, +} from './fraud'; +import type { Page } from './paging'; + +/** + * The three answers in `__fixtures__/` against the types the screens were written from. + * + * Every other test in this directory builds its own object and hands it to the function under + * test, which is the right shape for a test about wording or arithmetic and proves nothing at all + * about the wire: the object was written by the same person as the expectation. That gap is not + * hypothetical here. The waiting list DTO was declared with `beneficiaryIban`, the server had + * never sent anything but `toIban`, the screen read the value through a cast because the field it + * wanted was not on the type it had, and the suite stayed green the whole time. + * + * So this file reads bytes the server actually sent. Two failures are worth having and they fail + * differently on purpose. A field the interface declares and the body does not have fails here, at + * run time, on a real answer. A field the body has and the interface does not is caught by the + * same assertion from the other side. And a field added to an interface without being added to the + * list below stops the build rather than this test, because each list is a mapped type over the + * interface's own keys and TypeScript will not let it be short. + */ + +/** + * Every key of T, and the compiler will not accept a list that misses one or invents one. + * + * The `true` is filler. What is being written down is the set of names, and writing it as a record + * rather than as an array of strings is what buys the exhaustiveness: an array of `keyof T` can be + * empty and still type-check. + */ +type Keys = Record; + +const ALERT_QUEUE_ITEM: Keys = { + id: true, + alertCode: true, + transferCode: true, + state: true, + transferStatus: true, + amount: true, + shortReason: true, + createdAt: true, + riskScore: true, + assignee: true, +}; + +const ALERT_INFO: Keys = { + id: true, + state: true, + decision: true, + decidedBy: true, + resolvedAt: true, + decisionComment: true, + reason: true, + riskScore: true, + createdAt: true, + assignee: true, + tags: true, +}; + +const TRANSFER_INFO: Keys = { + id: true, + code: true, + status: true, + fromIban: true, + fromBalance: true, + toIban: true, + toIbanInBank: true, + dispatchState: true, + amount: true, + feeAmount: true, + createdAt: true, + settledAt: true, + declinedAt: true, + message: true, + declineReason: true, + authMethod: true, +}; + +const HISTORY_ITEM: Keys = { + id: true, + createdAt: true, + settledAt: true, + declinedAt: true, + amount: true, + fee: true, + status: true, + fromIban: true, + toIban: true, + toIbanInBank: true, + message: true, + declineReason: true, +}; + +const ALERT_NOTE: Keys = { + author: true, + writtenAt: true, + text: true, +}; + +const PAGE_ENVELOPE: Keys> = { + items: true, + page: true, + size: true, + total: true, +}; + +/** + * A row of the waiting list, written out rather than derived. + * + * `WaitingTransferItem` lives in `minibank-web/src/api.ts` and not here, so this one list cannot be + * a mapped type over it: the shared directory is what both applications import, and importing an + * application's module into it would point the dependency the wrong way. The names are therefore + * copied, which is the weaker guarantee, and it is still the guarantee that was missing when the + * field this list starts with was called something else. + */ +const WAITING_TRANSFER_ITEM = ['id', 'toIban', 'amount', 'createdAt', 'authMethod', 'status']; + +function captured(name: string): T { + return JSON.parse( + readFileSync(new URL(`./__fixtures__/${name}.json`, import.meta.url), 'utf8'), + ) as T; +} + +/** Names both directions of the difference, so a failure says which field and which way. */ +function expectSameKeys(value: object, declared: object | readonly string[]) { + const want = Array.isArray(declared) ? declared : Object.keys(declared); + expect(Object.keys(value).sort()).toEqual([...want].sort()); +} + +describe('the alert queue, as it answered', () => { + const queue = captured('fraud-alerts'); + + it('is an envelope of a page and the counters, and nothing else', () => { + expectSameKeys(queue, { alerts: true, counters: true }); + expectSameKeys(queue.alerts, PAGE_ENVELOPE); + expectSameKeys(queue.counters, { newCount: true, suspiciousCount: true, okCount: true }); + }); + + it('sends every row with every field the queue table draws', () => { + expect(queue.alerts.items.length).toBeGreaterThan(0); + for (const row of queue.alerts.items) { + expectSameKeys(row, ALERT_QUEUE_ITEM); + } + }); + + it('counts the whole queue in the counters and the matched rows in the total, separately', () => { + // The two numbers are the reason the envelope exists, and the fixture is a queue where + // they happen to agree. What is asserted is that both arrived, not that they are equal: + // a screen that derived one from the other would pass this and still be wrong. + const counted = + queue.counters.newCount + queue.counters.suspiciousCount + queue.counters.okCount; + expect(Number.isInteger(counted)).toBe(true); + expect(Number.isInteger(queue.alerts.total)).toBe(true); + }); +}); + +describe('one alert in full, as it answered', () => { + const detail = captured('fraud-alert-detail'); + + it('is the alert, the payment, the earlier payments and the journal', () => { + expectSameKeys(detail, { alert: true, transfer: true, history: true, notes: true }); + }); + + it('sends the alert with every field the panel lists', () => { + expectSameKeys(detail.alert, ALERT_INFO); + }); + + it('sends the payment with every field the panel lists', () => { + expectSameKeys(detail.transfer, TRANSFER_INFO); + }); + + it('sends both account numbers on every history row, not only the destination', () => { + // A customer can hold more than one account, so a row that named only where the money went + // could not say which account it left. Asserted on real rows because the guarantee is the + // server's: a hand written row proves only that the test author remembered both. + expect(detail.history.length).toBeGreaterThan(0); + for (const row of detail.history) { + expectSameKeys(row, HISTORY_ITEM); + expect(typeof row.fromIban).toBe('string'); + expect(typeof row.toIban).toBe('string'); + } + }); + + it('answers where the beneficiary account is held rather than the two halves of it', () => { + for (const row of detail.history) { + expect(typeof row.toIbanInBank).toBe('boolean'); + } + expect(typeof detail.transfer.toIbanInBank).toBe('boolean'); + }); + + it('sends a fee on every row, including the ones that never settled', () => { + // The wire used to carry the charge alone and null everything unsettled, which on a desk + // is most of the table. The fixture is a desk's table: held, declined and sent together. + const unsettled = detail.history.filter((row) => row.settledAt === null); + expect(unsettled.length).toBeGreaterThan(0); + for (const row of detail.history) { + expect(row.fee).not.toBeNull(); + expect(typeof row.fee.amount).toBe('string'); + expect(row.fee.currency).toBe(row.amount.currency); + } + }); + + it('sends the journal as a list, entry by entry, and never as one blob of text', () => { + expect(Array.isArray(detail.notes)).toBe(true); + for (const note of detail.notes) { + expectSameKeys(note, ALERT_NOTE); + } + }); +}); + +describe('the payments waiting on the customer, as they answered', () => { + // Typed as the envelope alone, because the row type is the application's and not this + // directory's; the keys are checked against the copied list below. + const waiting = captured>>('waiting-transfers'); + + it('is a page like every other page on this wire', () => { + expectSameKeys(waiting, PAGE_ENVELOPE); + }); + + it('calls the destination toIban, which is the name the drift was found under', () => { + expect(waiting.items.length).toBeGreaterThan(0); + for (const row of waiting.items) { + expectSameKeys(row, WAITING_TRANSFER_ITEM); + expect(row).not.toHaveProperty('beneficiaryIban'); + expect(typeof row.toIban).toBe('string'); + } + }); +}); diff --git a/minibank-fraud-web/index.html b/minibank-fraud-web/index.html index 9465c92..9dea84c 100644 --- a/minibank-fraud-web/index.html +++ b/minibank-fraud-web/index.html @@ -10,6 +10,21 @@ title bar, so the tab, the sign-in card and the working window are one mark. --> + +