# Stocklist Import — Repo Migration & Failure Reporting

- **Date:** 2026-09-13
- **Status:** Awaiting testing
- **Status Date:** 2026-09-13
- **Phases:** 7
- **Phases Complete:** 7 (built; 1-6 confirmed working 2026-09-18, phase 7 tests 18-21 outstanding)
- **Notes:** The reported bug — upload succeeds, job sticks on "Import in progress /
  Started" — is a *class* of failure rather than one defect: every way the Python process
  can die before it reaches the jobs table produced exactly that symptom, and the command
  ended in `> /dev/null 2>&1` so none of them left a trace. The build closes that hole
  first; the specific cause is then whatever the new log and `--check` report. See
  **Root-cause analysis**.

## Plan Phases

1. Move the importer into the repo and take its database credentials from the app
2. Make every failure reach the user (per-job log, exit marker, reconciliation, `--check`)
3. Retire the v1 endpoint
4. Fix the identifier fallback (added after test 3 surfaced it)
5. Put the upload behind a requirements modal
6. Move address matching onto `basedata.abp_main` (migration 065)
7. A UPRN exists in one stocklist only — per-UPRN exclusion, per-row reporting

## Problem

From the web app: the file uploads, a job row is created, and the job never moves off
`Started`. Confirmed already: the file reaches `/var/www/netplanner-files/stocklist/<id>/`,
`public.file_uploads` has its row, and `stocklist_import_manager.php` raises no error.

Alongside the fix, the importer moves from `/home/davebasnett/Python/netplanner/` into
`python/` in this repo, so it deploys with the app rather than being maintained by hand on
one server.

## Root-cause analysis

`process_file()` writes progress 20 as its **first** statement, and everything inside it is
caught and reported to the job row. A job stuck on `Started` therefore never reached
`process_file()` at all. Only two things ran before it:

1. **Module import.** `import pandas`, `openpyxl`, `psycopg2`, and `import config` — the
   last from `/home/davebasnett/Python/config.py`, resolved via `sys.path.insert(0, parentdir)`.
2. **`connect_to_database()`.**

Both fail silently, and the silence is over-determined:

- `exec()` ran the command with `> /dev/null 2>&1 &`, so stdout, stderr and any Python
  traceback were discarded. Nothing was written anywhere.
- `update_job_progress()` caught every exception and logged to stderr — i.e. to the same
  `/dev/null`.
- `connect_to_database()`'s failure path calls `update_job_progress(0, "Error", ...)`, which
  needs the *same* credentials that just failed. A bad connection cannot report a bad
  connection.
- PHP marked the job `Started` unconditionally after `exec()`, having no way to know whether
  anything had actually launched.

So a wrong interpreter path, a missing or unreadable script, a `pandas` broken by an
interpreter upgrade, a `config.py` whose credentials had drifted from `db.php`, or a
database that was simply down would all present identically — and none could be told apart
from the outside. **No cause is identified here.** The point of the build is that they stop
being indistinguishable: `--check` reports each of them by name in one command, and any
future failure of this class lands in the job's log and its `error_message` instead of
nowhere.

Two related findings, both fixed here:

- The relocation would have **caused** this failure on its own: `import config` resolves
  against the script's parent directory, which at `python/stocklist_import_v2.py` is the repo
  root, where there is no `config.py`.
- `monitorImportJob()` polled forever with no terminal condition, and
  `getRecentImportJobs()` renders any non-terminal job as "Import in progress" with no upper
  age. A dead job stayed on screen indefinitely.

## Build

### Phase 1 — Move the importer into the repo ✅

`python/stocklist_import_v2.py` is now the only copy.

- `stocklist_import_manager.php` resolves it as `dirname(dirname(__DIR__)) . '/python/stocklist_import_v2.py'`,
  so app and script move together on a `git pull` deploy.
- `import config` / `sys.path.insert()` are gone. **The site now has one definition of how
  to reach the database:** the libpq service file `/etc/geolynx/pg_service.conf`, service
  `[geolynx]`, which already existed for QGIS. `db.php` reads it with `parse_ini_file` and
  still sets the same `$hostname`/`$username`/`$password`/`$dbname` variables, so the ~40
  endpoints that build their own PDO need no change at all; Python passes `service=` to
  libpq and parses nothing. `psql`, QGIS and GeoServer already read it. `db.php` falls back
  to its own literals when the file is absent, so servers migrate one at a time.
- **The manager passes whichever db.php actually used** — `PGSERVICE`/`PGSERVICEFILE` when
  the service file resolved, the explicit `PG*` values when it fell back — through
  `putenv()`, so no password reaches a command line. This pairing is the point: were Python
  to consult the service file independently, a server where db.php could not read it would
  have PHP on the literals and Python on the service, writing progress to one database
  while the job row sat in another. `_resolve_db()` takes explicit `PG*` ahead of the
  default service for exactly that reason.
- `db_settings()` raises a named error saying what to set, and `--check` prints which
  source was used and what libpq resolved it to, so a mismatch is visible rather than
  silent.
- New tracked templates: `www/fn/db.php.example` (db.php is gitignored and per-server) beside
  the existing `pg_service.conf.example`.
- The old `/home/davebasnett/Python/netplanner/stocklist_import_v2.py` should be moved aside
  once this is confirmed working, so nobody edits the copy that is no longer read.

The other scripts in `python/` (`project_export_pdf_v2.py`, `opportunity_export_pdf.py`) are
untouched: they import `db_config`, which is not in the repo, and their endpoints still point
at `/home/davebasnett/Python/netplanner/`. Consolidating those is a separate job, already on
the backlog.

### Phase 2 — Make every failure reach the user ✅

**Nothing goes to `/dev/null`.** Each run writes to `/var/www/netplanner-files/import_logs/<job_id>.log`,
and the shell appends `GEOLYNX_EXIT=<code>` when the process ends:

```
nohup /bin/sh -c '<python> <script> ... >> <log> 2>&1; printf "\nGEOLYNX_EXIT=%s\n" "$?" >> <log>' &
```

**The exit marker is what makes a dead job detectable.** `reconcileJob()` runs on every
`status` and `recent` poll: a job on a non-terminal status whose log carries the marker ended
without reporting, so it is marked `Failed` with the exit code and the log tail as its
`error_message`. A job with no log at all after 120 seconds never spawned, and says so. This
needs no daemon and no migration — the log path is derived from `job_id`.

**Pre-flight before launch.** Interpreter, script, and log directory are checked before
anything is started; a problem marks the job `Failed` with the offending path and returns the
message to the caller, instead of launching into the dark.

**The script now speaks before it can fail.** `main()` writes `Running` / 5% as its first
act, *then* imports pandas and friends. That one write splits the diagnosis in two: a job
that reaches `Running` had a working interpreter and database, so the fault is in the import
itself; a job still on `Started` never got that far.

**Dependencies are imported lazily** (`load_runtime_dependencies()`) rather than at module
scope, so a broken pandas is a reportable error rather than a process that dies before it can
speak. `from __future__ import annotations` keeps the `pd.DataFrame` annotations from being
evaluated while `pd` is still `None`. psycopg2 stays at module scope — without it nothing can
reach the database at all, and that case is covered by the log.

**A bootstrap catches everything else.** Any exception escaping `main()` is logged with its
traceback and written to the job row by `report_job_failure()`, on a fresh connection.
`update_job_progress()` no longer swallows failures silently — it logs the traceback.

**`--check` answers "why did it not start" in one command**, which matters on a server with
no interactive session to hand. With the service file in place it needs no environment:

```
/opt/netplanner_env/bin/python3 python/stocklist_import_v2.py --check
```

One PASS/FAIL line each for the service file, the settings source, interpreter and version,
dependencies (with versions), database connection (naming the database and host libpq
actually resolved), the six tables the import touches, and the file store. Add
`-stocklist N -file <name>` to check one upload.

**Front end.** The failure modal renders `error_message` escaped inside a `<pre>` — a Python
traceback dropped into markup loses its line breaks and reads as one blur — and offers *Show
process log*, backed by a new `mode=log`. Polling now stops after 30 minutes as a backstop,
and `getRecentImportJobs()` reconciles stale jobs so the "Import in progress" banner cannot
outlive the process.

### Phase 4 — Fix the identifier fallback ✅

Added 2026-09-13 after test 3. With the reporting in place the first real import failed
with `No valid UPRN, TOID, or address values found` on a 452-row file carrying **both** a
`UPRN` and an `ADDRESS` column — the UPRN column holding a supplier's own numbering rather
than Ordnance Survey UPRNs. It should have matched on address.

Two defects, compounding:

1. **`find_identifier_columns()` discarded the address column** — `elif 'address' in cl and
   'uprn' not in found` stopped looking as soon as a UPRN column was seen. It was also
   order-dependent: `ADDRESS` before `UPRN` in the sheet recorded both, `UPRN` before
   `ADDRESS` recorded only the UPRN. It now collects every kind present, prefers an exact
   `address` header over a partial one, and excludes `matched_address` — which a previous
   run writes back, and which `'address' in cl` would otherwise pick as this run's source.
2. **`validate_file()` gated the address analysis on the column being absent**, not on the
   UPRN/TOID columns yielding nothing: `if 'address' in id_cols and 'uprn' not in id_cols`.
   Now `... and not details['valid_uprns'] and not details['valid_toids']`, which is both
   the correct fallback and keeps the per-row address parse off the common path.

Deciding *which* identifier to use is validate_file's job, once it knows which actually
produced values; find_identifier_columns only reports what is there.

Two reporting changes came with it. The failure message now names the column and quotes
what it held (`column 'UPRN' held 452 value(s), none of them a UPRN (a UPRN is all digits)
— e.g. …`) rather than the unplaceable "No valid UPRN, TOID, or address values found". And
a run that falls back says so, because an address-matched import and a UPRN-matched one
carry very different confidence and otherwise look identical.

**Still open — numeric foreign identifiers.** A supplier whose own numbering happens to be
all digits passes `validate_uprn()`, becomes the primary identifier, and matches nothing in
`basedata.abp`; the address column beside it is never tried. `assign_premises()` now says so
loudly ("none of the N identifier(s) resolved from this file matched a premise in Ordnance
Survey AddressBase") instead of completing quietly with a count of zero, but it does not
fall back. Whether it should — and on what threshold, given a partial match is normal — is
a policy question for Dave, not a silent default.

### Phase 5 — Upload behind a requirements modal ✅

Added 2026-09-13 at Dave's request. The upload button fired the file picker directly, so
there was nowhere to state what the file must contain — and the two requirements that
matter most are impossible to guess and fail badly when missed: a UPRN column is taken at
face value if it is all digits, and addresses must be comma-separated with a postcode.

Flow is now: **Upload Stocklist** → `#stocklistUploadModal` states the requirements →
**Choose File** opens the picker → on a successful upload and job start the modal closes
and progress reports on the page as before.

- The modal names the accepted headers (`UPRN` / `TOID` / `ADDRESS`), says UPRN must be
  Ordnance Survey UPRNs and what to do if it is not (remove the column and let the
  addresses match), gives a worked address example, and notes that match results come back
  as extra columns in the uploaded file.
- Client-side rejections (wrong type, too large) and upload failures now render **inside**
  the modal and leave it open. They used to be `alert()`s, which dismissed the requirements
  along with the error — the two belong on screen together.
- `setLoadingState()` drives the modal's button, since that is what the user is looking at;
  the page button is disabled behind it to stop a second upload starting.

Fixed in passing: a job that failed to *start* wrote its reason into `#output`, which lives
inside `#routing-progress` and is only un-hidden by `monitorImportJob()` — so the message
was invisible. It now goes through `showImportFailure()`, which matters because the
pre-flight checks from Phase 2 made that a likely path and their messages are multi-line
paths that need the `<pre>`.

### Phase 6 — Move address matching onto `basedata.abp_main` ✅ (migration 065)

Added 2026-09-13 after test 3. Uploads processed, but every address came back unmatched
with `postcode 'CR0 2SU': 0 ABP rows | postcode unfiltered: 0 rows | ABP postcode format
samples (CR0...): []` — while `basedata.abp` held the row perfectly well.

**Cause:** the matcher's 20 query sites read `adhoc.tbl_abp_import`, which is the staging
table `sql/abp_sync.sql` loads `basedata.abp` *from*. Staging had been emptied, so there
was nothing to match against. The application must never read a load table (Dave).

**Why `basedata.abp` could not simply take over:** it is deliberately the flattened address
model — `sao`/`pao`/`street`/`locality`/`town` concatenated into one `address` column, with
only `pao_text` surviving separately as `block`. The six matching phases are built on the
structured fields (`pao_start_number`, `pao_end_number`, `sao BETWEEN`, range and letter-range
matching) and none of them exist there. Adding them back would fight that table's design.

**Migration 065** creates `basedata.abp_main` with the delivery's structure, populated from
`basedata.abp_import` by a separate data job. The script change is then a pure table-name
swap — every column keeps its name — so the matching algorithm is untouched.

**The indexes are designed from the queries, not copied.** Every phase is anchored on
`postcode = %s`, which narrows to the premises in one postcode, so the remaining predicates
are cheap filters over tens of rows and need no index of their own; hence no trigram indexes
despite the `ILIKE`s in phases 4 and 5. Five indexes: `(postcode, street_description)` as the
workhorse, `(postcode, pao_start_number)`, `(postcode, pao)`, `(postcode, sao)`, and
`(parent_uprn)` for child expansion.

Of the six indexes on the old staging table, **only one could ever have been used**. Five
were partial on `row_is_current = 'Y' AND address_logical_status = 1`, a predicate no query
in the importer carries — `ABP_FILTERS` excludes only `building_classification_code = 'PP'`,
despite a diagnostic string that claimed otherwise. Two were on `upper(pao_text)`, a column
the matcher never queries: `pao_text` in the Python is the *parsed input*, matched against
ABP's `pao`.

**The load carries current rows only** (Dave, 2026-09-13), so the new indexes are
non-partial and `ABP_FILTERS` needs nothing added — a `row_is_current` predicate would match
every row in the table. That is an invariant of the data job rather than of the schema: if a
load ever brings non-current rows in, the filter must be added to `ABP_FILTERS` *and* the
indexes rebuilt partial to match, or matching will silently return superseded addresses.
`row_is_current` is `varchar(8)` here, not staging's `varchar(1)`, so `'Y'` is an assumption
worth checking first. `address_logical_status` is filtered by neither the load nor the
matcher, which preserves the importer's existing behaviour.

**Diagnostics corrected.** `STOP: postcode not found — likely format mismatch` fired even
when the sample list came back empty, which is the opposite diagnosis and cost a real
investigation a detour. It now separates "the area is present but this postcode is not" from
"no postcode in this area at all", reporting the table's approximate row count in the second
case. `--check` gained a populated-row-count line for `abp_main`, because an empty table
looks exactly like broken matching.

### Phase 7 — A UPRN exists in one stocklist only ✅

Added 2026-09-18, from Dave checking the matching logic while diagnosing a client's import.
The rule is **a UPRN can only ever exist in one stocklist**, and it lands at two different
granularities. Conflating them was the mistake in the first attempt at this phase.

**Per UPRN — not added if held elsewhere.** The two paths encoded opposite policies:

| Upload as | Premise already in another stocklist |
|---|---|
| UPRN column | **added**, noted "Also in stocklist(s): 12, 47" |
| ADDRESS column | **not added**, marked `address_in_another_stocklist` |

The address path filters upstream in `check_duplicates()`; the UPRN path had no such check,
and `assign_premises()`'s `NOT EXISTS` was scoped `AND sp.stocklist_id = %s` — this
stocklist only. The same 100 properties gave 100 rows one way and 60 the other, and Phase
4's identifier fallback meant one file could switch policy depending on whether its UPRN
column happened to be numeric.

That `NOT EXISTS` is now **unscoped**. It sits in `assign_premises()` rather than beside
`check_duplicates()` because that is the one statement both paths pass through, so they
cannot drift apart again; on the address path it is a harmless second guard.

**Per row — the row reports all of its UPRNs.** `5 Added, 3 In Another Stocklist`.

**A row is not one premise.** A UPRN is one-to-one, but an address resolves to as many
UPRNs as it covers, so eight flats with three held elsewhere is an addition *and* an
exclusion — no single verdict describes it. `_get_address_feedback()` collapsed a
multi-UPRN row by keeping whichever UPRN sorted first (`ORDER BY source_row, uprn NULLS
LAST`, then `if sr not in feedback`), so that row read `added_ok` and the exclusion vanished
from the feedback file entirely.

`summarise_import_actions()` now counts the outcomes per row and renders them in a fixed
order. Both feedback functions go through it, so the column reads the same whichever way the
file matched. Counts appear when the row resolved to at least one UPRN; a row that resolved
to none — an address that matched nothing, a line that would not parse — gets the bare
label, since "1 Unmatched" would be counting something that does not exist. Unknown actions
pass through under their own name rather than being dropped, which is how the previous
version lost exclusions.

Verified against the real shapes:

```
address: 8 flats, 5 free 3 held  ->  5 Added, 3 In Another Stocklist
address: mixed 3 ways            ->  4 Added, 2 Already In This Stocklist, 1 In Another Stocklist
address: matched nothing         ->  Unmatched
uprn:    held elsewhere          ->  1 In Another Stocklist
toid:    never resolved          ->  No UPRN
```

Also in this phase:

- `assign_premises()` counts and reports the held-back premises ("N held by another
  stocklist") — the number that explains a premise missing from an import that otherwise
  looks clean, which is what prompted the question.
- The zero-match AddressBase warning no longer fires when the batch was held back for this
  reason instead; it would have sent the next person to the address data rather than the
  stocklists.
- `uprns_matched` means *matched* on both paths now, not *added*. The UPRN path was
  reporting 0 for a premise it had resolved perfectly well but not added; `import_action`
  carries added-versus-excluded.
- The two premise counts moved to `COUNT(DISTINCT uprn)`: a file can name a UPRN twice and
  the insert is `SELECT DISTINCT`.

**Enforced in the importer only.** No unique constraint on
`stocklists.stocklist_premises.uprn` — no table changes at this point (Dave). So
`stocklist_address_search.php` (three inserts) and `opportunity_manage.php` (one) can still
put a UPRN in a second stocklist, and pre-existing duplicates are untouched.

**The TOID path is unimplemented**, found while tracing this. Nothing resolves a TOID to
UPRNs — no query joins TOID to AddressBase — so `prepare_data_for_insert()` writes
`uprn = NULL`, `assign_premises()` requires `uprn IS NOT NULL`, and every row reports
`No UPRN`. A TOID file imports nothing. Not fixed here; it is a feature, not a regression.

### Phase 3 — Retire the v1 endpoint ✅

`www/fn/stocklist_process.php` deleted: no caller in `www/js/` or `www/html/`, and the script
it shelled out to (`stocklist_import_prod.py`) was never in the repo. Recorded in
`docs/unusedfiles.md`.

## Testing checklist

1. ✅ *(2026-09-13)* On the dev server, run `--check` as the web user with no environment
   set and confirm every line reads PASS. **Passed as `uid=33`** — Python 3.10.12,
   pandas 2.2.3, openpyxl 3.1.5, psycopg2 2.9.10, `cdifflib` present, all six tables found,
   settings resolved through `service geolynx`. The runtime was never the problem, which
   rules out a broken dependency, a bad interpreter and an unreachable database.
2. ✅ *(2026-09-13)* Confirm the database `--check` reports is the one the app uses —
   `netplanner as postgres on localhost:5432`, matching db.php's literals, so the service
   entry and the fallback agree.
3. ✅ *(2026-09-13)* Re-run the 452-row file with both UPRN and ADDRESS columns and confirm
   it now falls back to address matching instead of failing validation. **Passed** — it
   processed, and the seven feedback columns were appended to the uploaded file with the
   match results. The end-to-end path is working.
4. Upload a UPRN stocklist and confirm the job runs through to Complete with the premises
   count matching.
5. Upload an address stocklist and confirm address matching completes and the feedback
   columns are written back to the file.
6. Confirm `/var/www/netplanner-files/import_logs/<job_id>.log` exists and ends with
   `GEOLYNX_EXIT=0`.
7. Break it deliberately — point `IMPORT_PYTHON_BIN` at a path that does not exist — and
   confirm the job is marked Failed immediately with that path named, not left on Started.
8. Break it later — rename the script after the job starts, or `kill` the Python process
   mid-run — and confirm the next poll marks it Failed with the exit code and log tail.
9. Confirm *Show process log* opens the log in the failure modal.
10. Confirm a previously stuck job clears from the "Import in progress" banner rather than
   showing forever.
11. Confirm the password appears nowhere in `ps auxww` while an import is running.
12. Confirm `sudo -u www-data cat /etc/geolynx/pg_service.conf` succeeds — PHP and the
    importer both read it as the web user.
13. Click Upload Stocklist and confirm the requirements modal opens, Choose File opens the
    picker, and the modal closes by itself once the job starts.
14. Select a .docx and an over-20MB file and confirm each is refused inside the modal, with
    the modal staying open.
15. Point `IMPORT_PYTHON_BIN` at a bad path and confirm the failure modal shows the
    pre-flight message, rather than it disappearing into the hidden progress panel.
16. Apply migration 065, load `basedata.abp_main`, and confirm `--check` reports it
    populated with a plausible row count.
17. Re-run the 452-row Croydon file and confirm `386 London Road, Croydon, CR0 2SU` now
    matches, and that flats 1-8 resolve rather than the parent block alone.
18. Upload a UPRN file whose premises are already in a different stocklist and confirm none
    are added, each row reads `1 In Another Stocklist` naming the holding stocklist in
    notes, and the job message reports "N held by another stocklist".
19. Upload an ADDRESS file with a row covering several flats where some are already in
    another stocklist, and confirm the free ones are added, the held ones are not, and the
    row reads `5 Added, 3 In Another Stocklist` rather than a single verdict.
20. Upload the same properties once as UPRNs and once as addresses and confirm both give
    the same premise count and the same wording in `import_action`.
21. Confirm a premise already in THIS stocklist reads `Already In This Stocklist`, not the
    another-stocklist wording.

## Risks

- **The service file becomes load-bearing for more than QGIS.** It is `root:www-data 640`,
  which is right, but a permission or syntax change to it now affects the whole app rather
  than one export. db.php's fallback to its literals is what stops that being fatal.
- **The password must stay alphanumeric.** `parse_ini_file` treats `;` and `#` as comments
  and libpq does not strip quotes from service-file values, so a password containing
  `; # " '` would be read differently by PHP and by libpq — PHP would connect and Python
  would not, or worse, both would connect as different users. Worth checking before any
  future password rotation.
- **`$port` is only honoured by db.php's own `$pdo`.** Around forty endpoints hardcode
  `port=5432` in their DSN strings, so a service file naming another port would half-work.
  Logged on the backlog.
- `putenv()` is the one new assumption about the PHP install. If it is disabled, the child
  inherits nothing; with the service file present it falls through to the default service
  and still reaches the right database, and without one it raises the named
  `db_settings()` error into the log and the failure modal. Either way, not a silent stall.
- The log directory is created by PHP on first use. If `/var/www/netplanner-files` is not
  writable by the web user, pre-flight reports it by name.
- `reconcileJob()` reads a file on every status poll. It is one `is_file()` plus at most a
  16 KB tail read, on a poll the page was already making.
- `distance_analysis_manager.php:316` spawns its worker with the same
  `> /dev/null 2>&1 &` pattern and will have the same blind spot. Not touched here; logged on
  the backlog.