Fix terrain recompute on edit, antenna heights, and ADIF/CSV import bugs #21

graham · 2026-09-22T11:23:17-05:00

graham commented

2026-09-22 11:23:17 -05:00

Audit terrain/radio/import group.

Terrain profiles are recomputed after a contact edit (upsert replaces; worker skips only when complete+row exists); vacuous test rewritten.
TerrainProfileWorker uses contact antenna heights and no longer marks :complete on failed upsert.
ADIF header strip uses binary_part — BOM/non-ASCII headers no longer drop the first QSO.
Non-digit QSO_DATE/TIME_ON yields a row error instead of crashing preview.
CSV commit handles {:error, :constraint_error} and logs duplicates.
Grid/band edits reset radar_status and mechanism_status too.
Contacts.source/1 classifies contest-log imports correctly.

Verified: ~300 targeted tests pass; full suite green.

Audit terrain/radio/import group. - Terrain profiles are recomputed after a contact edit (upsert replaces; worker skips only when complete+row exists); vacuous test rewritten. - TerrainProfileWorker uses contact antenna heights and no longer marks :complete on failed upsert. - ADIF header strip uses binary_part — BOM/non-ASCII headers no longer drop the first QSO. - Non-digit QSO_DATE/TIME_ON yields a row error instead of crashing preview. - CSV commit handles {:error, :constraint_error} and logs duplicates. - Grid/band edits reset radar_status and mechanism_status too. - Contacts.source/1 classifies contest-log imports correctly. Verified: ~300 targeted tests pass; full suite green.

graham added 1 commit

2026-09-22 11:23:17 -05:00

Fix terrain recompute on edit, antenna heights, and ADIF/CSV import bugs

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

dfdddc6160

- Terrain profiles are recomputed after a contact edit (upsert now
  replaces; worker only skips when status is complete AND a row exists)
- TerrainProfileWorker uses the contact's height1_ft/height2_ft instead
  of the fixed 10 ft default, and no longer marks :complete on a failed
  upsert
- ADIF header stripping uses binary_part on the byte offset — a BOM or
  non-ASCII byte no longer drops the first QSO
- Non-digit QSO_DATE/TIME_ON yields a row error instead of crashing the
  import preview
- CSV commit path handles {:error, :constraint_error} and logs duplicates
- Grid/band edits now also reset radar_status and mechanism_status
- Contacts.source/1 classifies contest-log imports correctly

skippy-bot reviewed

2026-09-22 11:27:37 -05:00

skippy-bot left a comment

🤖 Skippy PR review

3 findings — 2 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/microwaveprop/workers/terrain_profile_worker.ex:35`	Skip gate still trusts terrain_status, which other code sets from row existence
🟡 Warning	`lib/microwaveprop/radio/contacts.ex:587`	New :constraint_error return is unhandled by two of three callers
🔵 Suggestion	`lib/microwaveprop/workers/terrain_profile_worker.ex:29`	get_contact!/1 turns a deleted contact into a 20-attempt poison job

_{Reviewed dfdddc6160b9. Comment skippy review to re-run.}

### 🤖 Skippy PR review **3 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/microwaveprop/workers/terrain_profile_worker.ex:35` | Skip gate still trusts terrain_status, which other code sets from row existence | | 🟡 Warning | `lib/microwaveprop/radio/contacts.ex:587` | New :constraint_error return is unhandled by two of three callers | | 🔵 Suggestion | `lib/microwaveprop/workers/terrain_profile_worker.ex:29` | get_contact!/1 turns a deleted contact into a 20-attempt poison job | Reviewed `dfdddc6160b9`. Comment `skippy review` to re-run.

lib/microwaveprop/radio/contacts.ex Outdated

					
				@ -569,0 +584,4 @@

				          {:ok, Contact.t()}

				          | {:error, Ecto.Changeset.t()}

				          | {:error, :duplicate, Contact.t()}

				          | {:error, :constraint_error}

🟡 Warning — New :constraint_error return is unhandled by two of three callers

create_contact/2 can return the bare {:error, :constraint_error} (from the Ecto.ConstraintError rescue at line 651), and only the CSV import was taught about it. lib/microwaveprop_web/controllers/api/v1/contact_controller.ex:83 only matches {:ok, _}, {:error, %Ecto.Changeset{}} and {:error, :duplicate, _}, so this raises CaseClauseError and returns 500 instead of 409; lib/microwaveprop_web/live/submit_live.ex:105 binds it to changeset and calls to_form(:constraint_error), which crashes the LiveView. It is reachable because contacts_dedup_idx keys on the coalesced station/grid pair while find_duplicate_contact/1 compares non-null grids, so the post-rescue re-check can come back nil. Add a clause for it both places (or convert :constraint_error to a changeset error inside create_contact/2 so callers only ever see the three original shapes).

**🟡 Warning — New :constraint_error return is unhandled by two of three callers** `create_contact/2` can return the bare `{:error, :constraint_error}` (from the `Ecto.ConstraintError` rescue at line 651), and only the CSV import was taught about it. `lib/microwaveprop_web/controllers/api/v1/contact_controller.ex:83` only matches `{:ok, _}`, `{:error, %Ecto.Changeset{}}` and `{:error, :duplicate, _}`, so this raises `CaseClauseError` and returns 500 instead of 409; `lib/microwaveprop_web/live/submit_live.ex:105` binds it to `changeset` and calls `to_form(:constraint_error)`, which crashes the LiveView. It is reachable because `contacts_dedup_idx` keys on the coalesced station/grid pair while `find_duplicate_contact/1` compares non-null grids, so the post-rescue re-check can come back nil. Add a clause for it both places (or convert `:constraint_error` to a changeset error inside `create_contact/2` so callers only ever see the three original shapes).

skippy-bot marked this conversation as resolved

lib/microwaveprop/workers/terrain_profile_worker.ex Outdated

					
				@ -21,3 +28,2 @@

				    Microwaveprop.Instrument.span([:worker, :terrain_profile], %{contact_id: contact_id}, fn ->

				      if Terrain.has_terrain_profile?(contact_id) do

				        _ = Radio.set_enrichment_status!([contact_id], :terrain_status, :complete)

				      contact = Radio.get_contact!(contact_id)

🔵 Suggestion — get_contact!/1 turns a deleted contact into a 20-attempt poison job

This now runs on every attempt, including the skip path that used to return :ok for a contact with an existing profile. A contact deleted while its terrain job is queued raises Ecto.NoResultsError here, and with max_attempts: 20 plus the 6-hour backoff cap the job fails for days before it is discarded. Repo.get(Contact, contact_id) with a nil -> :ok branch (the shape MechanismClassifyWorker uses) keeps the delete case quiet, and drops the unused :user/:flagged_by_user preload this worker never reads.

**🔵 Suggestion — get_contact!/1 turns a deleted contact into a 20-attempt poison job** This now runs on every attempt, including the skip path that used to return `:ok` for a contact with an existing profile. A contact deleted while its terrain job is queued raises `Ecto.NoResultsError` here, and with `max_attempts: 20` plus the 6-hour backoff cap the job fails for days before it is discarded. `Repo.get(Contact, contact_id)` with a `nil -> :ok` branch (the shape `MechanismClassifyWorker` uses) keeps the delete case quiet, and drops the unused `:user`/`:flagged_by_user` preload this worker never reads.

skippy-bot marked this conversation as resolved

lib/microwaveprop/workers/terrain_profile_worker.ex Outdated

					
				@ -24,0 +32,4 @@

				      # terrain_status is :complete. Edits to grids, band, timestamp or

				      # antenna heights reset the status to :pending, so a pending row

				      # here means "recompute and replace", not "already done".

				      if contact.terrain_status == :complete and Terrain.has_terrain_profile?(contact_id) do

🟠 High — Skip gate still trusts terrain_status, which other code sets from row existence

terrain_status is not a reliable "profile is current" flag, so this still silently skips the recompute the PR is trying to get. Two writers set :complete from the mere existence of a terrain_profiles row: ContactLive.Show.maybe_enqueue_terrain/2 (lib/microwaveprop_web/live/contact_live/show.ex:830-836) and BackfillEnqueueWorker.reconcile_stale_queued/1 (lib/microwaveprop/workers/backfill_enqueue_worker.ex:201-204). Failure path: approve a grid edit (resets to :pending at import.ex:372, recompute deferred to the backfill cron), then open /contacts/:id before that sweep. The stale row flips :pending -> :complete, type_filter no longer selects the contact, and no recompute ever runs. Same trap if the job is already queued and retrying (elevation fetch error keeps the status at :queued): the next 30-min reconcile flips it to :complete and this clause returns :ok. Either compare the row against the contact (profile.updated_at >= contact.updated_at) or stop setting :complete where the only evidence is that a row exists.

**🟠 High — Skip gate still trusts terrain_status, which other code sets from row existence** `terrain_status` is not a reliable "profile is current" flag, so this still silently skips the recompute the PR is trying to get. Two writers set `:complete` from the mere existence of a `terrain_profiles` row: `ContactLive.Show.maybe_enqueue_terrain/2` (`lib/microwaveprop_web/live/contact_live/show.ex:830-836`) and `BackfillEnqueueWorker.reconcile_stale_queued/1` (`lib/microwaveprop/workers/backfill_enqueue_worker.ex:201-204`). Failure path: approve a grid edit (resets to `:pending` at `import.ex:372`, recompute deferred to the backfill cron), then open `/contacts/:id` before that sweep. The stale row flips `:pending` -> `:complete`, `type_filter` no longer selects the contact, and no recompute ever runs. Same trap if the job is already queued and retrying (elevation fetch error keeps the status at `:queued`): the next 30-min reconcile flips it to `:complete` and this clause returns `:ok`. Either compare the row against the contact (`profile.updated_at >= contact.updated_at`) or stop setting `:complete` where the only evidence is that a row exists.

skippy-bot marked this conversation as resolved

graham force-pushed fix/terrain-import from dfdddc6160

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to cc76c83a4d

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

2026-09-22 12:04:18 -05:00

Compare

skippy-bot reviewed

2026-09-22 12:08:09 -05:00

skippy-bot left a comment

🤖 Skippy PR review

2 findings — none blocking.

Severity	Location	Issue
🔵 Suggestion	`lib/microwaveprop/workers/terrain_profile_worker.ex:77`	Antenna heights only reach profiles computed after deploy (no backfill)
🔵 Suggestion	`lib/microwaveprop/radio/contacts.ex:425`	Contest-import sentinel is a second copy of the mix task's literal

Force-push: dfdddc6 is no longer in the branch, so this is a full-diff pass at cc76c83. 3 earlier findings are still open (skip gate vs terrain_status, get_contact!/1 on a deleted contact, unhandled :constraint_error in the API + LiveView callers); 0 resolved.

_{Reviewed cc76c83a4dc0. Comment skippy review to re-run.}

### 🤖 Skippy PR review **2 findings** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `lib/microwaveprop/workers/terrain_profile_worker.ex:77` | Antenna heights only reach profiles computed after deploy (no backfill) | | 🔵 Suggestion | `lib/microwaveprop/radio/contacts.ex:425` | Contest-import sentinel is a second copy of the mix task's literal | Force-push: dfdddc6 is no longer in the branch, so this is a full-diff pass at cc76c83. 3 earlier findings are still open (skip gate vs terrain_status, get_contact!/1 on a deleted contact, unhandled :constraint_error in the API + LiveView callers); 0 resolved. Reviewed `cc76c83a4dc0`. Comment `skippy review` to re-run.

lib/microwaveprop/radio/contacts.ex Outdated

					
				@ -421,0 +422,4 @@

				  # `mix import_contest_logs` (CsvImport.commit/2 with no user_id).

				  # Those rows are bulk contest-log imports, not individual email

				  # submissions, so `source/1` reports them as :archive_import.

				  @contest_import_email "contest-import@ntms.org"

🔵 Suggestion — Contest-import sentinel is a second copy of the mix task's literal

@contest_import_email has to stay byte-identical to the address passed to CsvImport.preview/2 at lib/mix/tasks/import_contest_logs.ex:49, and the new test asserts the same literal, so nothing catches drift: change the address in the task and every contest import silently falls through to :email_submission ("the operator behind the submission is unverified") instead of :archive_import. Define it once (a CsvImport function or a shared module attribute) and reference it from both source/1 and the task. Worth noting too that submitter_email is free text on the anonymous submit form, so classification rests on a user-supplied string; a dedicated column or checking the insert path would be sturdier.

**🔵 Suggestion — Contest-import sentinel is a second copy of the mix task's literal** `@contest_import_email` has to stay byte-identical to the address passed to `CsvImport.preview/2` at `lib/mix/tasks/import_contest_logs.ex:49`, and the new test asserts the same literal, so nothing catches drift: change the address in the task and every contest import silently falls through to `:email_submission` ("the operator behind the submission is unverified") instead of `:archive_import`. Define it once (a `CsvImport` function or a shared module attribute) and reference it from both `source/1` and the task. Worth noting too that `submitter_email` is free text on the anonymous submit form, so classification rests on a user-supplied string; a dedicated column or checking the insert path would be sturdier.

lib/microwaveprop/workers/terrain_profile_worker.ex Outdated

					
				@ -62,0 +74,4 @@

				    analysis =

				      TerrainAnalysis.analyse(profile, dist_km, freq_ghz,

				        ant_ht_a: feet_to_m(contact.height1_ft),

🔵 Suggestion — Antenna heights only reach profiles computed after deploy (no backfill)

Every stored profile written before this change was analysed with TerrainAnalysis.analyse/4's default antenna height of 0.0 m - this worker never passed heights - so verdict and diffraction_db on those rows are height-blind. Nothing re-queues them: the backfill cron only selects :pending/:queued/:failed plus :unavailable older than 24 h, mix reset_enrichment deliberately skips terrain, and the new skip gate (terrain_status == :complete and a row exists) means a :complete row is never recomputed again unless the contact is edited. Net effect: the Terrain card on /contacts/:id keeps showing the old height-blind verdict/diffraction for every contact nobody edits, while an identical contact edited once shows the new numbers - and the CHANGELOG tells users the analysis now uses the heights they entered. Either ship a one-off reset with the deploy (terrain_status -> :pending where a terrain_profiles row exists, then let the queue drain) or scope that changelog line to edited/new contacts.

**🔵 Suggestion — Antenna heights only reach profiles computed after deploy (no backfill)** Every stored profile written before this change was analysed with `TerrainAnalysis.analyse/4`'s default antenna height of 0.0 m - this worker never passed heights - so `verdict` and `diffraction_db` on those rows are height-blind. Nothing re-queues them: the backfill cron only selects `:pending`/`:queued`/`:failed` plus `:unavailable` older than 24 h, `mix reset_enrichment` deliberately skips terrain, and the new skip gate (`terrain_status == :complete` and a row exists) means a `:complete` row is never recomputed again unless the contact is edited. Net effect: the Terrain card on `/contacts/:id` keeps showing the old height-blind verdict/diffraction for every contact nobody edits, while an identical contact edited once shows the new numbers - and the CHANGELOG tells users the analysis now uses the heights they entered. Either ship a one-off reset with the deploy (`terrain_status -> :pending` where a `terrain_profiles` row exists, then let the queue drain) or scope that changelog line to edited/new contacts.

graham force-pushed fix/terrain-import from cc76c83a4d

skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread

Details

to 9915337811

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

2026-09-22 12:18:59 -05:00

Compare

graham commented

2026-09-22 12:19:32 -05:00

All three findings addressed in 99153378:

Skip gate trusts terrain_status — fixed at the writers, not the gate. maybe_enqueue_terrain no longer stamps :complete over a stale row (non-:complete falls through to the enqueue path; an Oban conflict marks :queued, not :unavailable). reconcile_stale_queued now joins terrain_profiles and only flips :queued → :complete when tp.updated_at >= c.updated_at, so a profile that predates the edit stays :queued for the recompute.
:constraint_error unhandled — added clauses in both callers: API returns 409 without an existing row; SubmitLive flashes instead of crashing on to_form(:constraint_error).
get_contact! poison job — worker now uses Repo.get and returns :ok for a deleted contact.

All three findings addressed in 99153378: 1. **Skip gate trusts terrain_status** — fixed at the writers, not the gate. `maybe_enqueue_terrain` no longer stamps `:complete` over a stale row (non-`:complete` falls through to the enqueue path; an Oban conflict marks `:queued`, not `:unavailable`). `reconcile_stale_queued` now joins `terrain_profiles` and only flips `:queued` → `:complete` when `tp.updated_at >= c.updated_at`, so a profile that predates the edit stays `:queued` for the recompute. 2. **`:constraint_error` unhandled** — added clauses in both callers: API returns 409 without an `existing` row; SubmitLive flashes instead of crashing on `to_form(:constraint_error)`. 3. **`get_contact!` poison job** — worker now uses `Repo.get` and returns `:ok` for a deleted contact.

graham merged commit d9518a3f3b into main

2026-09-22 12:19:50 -05:00

graham referenced this pull request from a commit

2026-09-22 12:19:52 -05:00

Merge pull request 'Fix terrain recompute on edit, antenna heights, and ADIF/CSV import bugs' (#21) from fix/terrain-import into main

skippy-bot reviewed

2026-09-22 12:22:32 -05:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟡 Warning	`lib/microwaveprop/workers/terrain_profile_worker.ex:109`	A job that started before an edit still stamps :complete over the pre-edit analysis

Resolved 3 earlier findings fixed in 9915337 (the two row-existence writers of terrain_status: :complete, the deleted-contact poison job, the unhandled :constraint_error at the API and LiveView callers); 2 suggestions still open. Note prev_sha cc76c83 is no longer in the branch (rebased onto the new main), so this pass re-read the full PR plus the address-review commit.

_{Reviewed 9915337811d5. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/microwaveprop/workers/terrain_profile_worker.ex:109` | A job that started before an edit still stamps :complete over the pre-edit analysis | Resolved 3 earlier findings fixed in [9915337](https://git.mcintire.me/graham/prop/commit/9915337811d53db789566badf13d20f1b338b3ec) (the two row-existence writers of `terrain_status: :complete`, the deleted-contact poison job, the unhandled `:constraint_error` at the API and LiveView callers); 2 suggestions still open. Note `prev_sha` cc76c83 is no longer in the branch (rebased onto the new main), so this pass re-read the full PR plus the address-review commit. Reviewed `9915337811d5`. Comment `skippy review` to re-run.

lib/microwaveprop/workers/terrain_profile_worker.ex

					
				@ -80,0 +106,4 @@

				           verdict: analysis.verdict

				         }) do

				      {:ok, _profile} ->

				        _ = Radio.set_enrichment_status!([contact_id], :terrain_status, :complete)

🟡 Warning — A job that started before an edit still stamps :complete over the pre-edit analysis

process/1 reads the contact once and then spends the whole elevation fetch (download: true, seconds) on that snapshot, and nothing re-checks it before this line claims :complete. Edit a grid or an antenna height while a terrain job for that contact is in flight: the edit rewrites pos1/pos2 and resets terrain_status to :pending, then the in-flight job finishes, stores a profile built from the pre-edit struct, and stamps :complete. Every reader now trusts that flag: the skip gate here returns :ok, ContactLive.Show.maybe_enqueue_terrain/2 returns the contact without enqueueing, BackfillEnqueueWorker drops :terrain because already_complete?/2 is true, and reconcile_stale_queued/1 only inspects :queued rows. The card then shows the old geometry until the contact is edited again. Capture contact.updated_at at read time and, before stamping :complete, re-read the row and compare: when it moved, leave the status alone and return :ok so the queue recomputes, or re-run the analysis on the fresh struct. reconcile_stale_queued/1's tp.updated_at >= c.updated_at test has the same blind spot, because a profile written by a stale fetch is newer than the edit that invalidated it.

**🟡 Warning — A job that started before an edit still stamps :complete over the pre-edit analysis** `process/1` reads the contact once and then spends the whole elevation fetch (`download: true`, seconds) on that snapshot, and nothing re-checks it before this line claims `:complete`. Edit a grid or an antenna height while a terrain job for that contact is in flight: the edit rewrites `pos1/pos2` and resets `terrain_status` to `:pending`, then the in-flight job finishes, stores a profile built from the pre-edit struct, and stamps `:complete`. Every reader now trusts that flag: the skip gate here returns `:ok`, `ContactLive.Show.maybe_enqueue_terrain/2` returns the contact without enqueueing, `BackfillEnqueueWorker` drops `:terrain` because `already_complete?/2` is true, and `reconcile_stale_queued/1` only inspects `:queued` rows. The card then shows the old geometry until the contact is edited again. Capture `contact.updated_at` at read time and, before stamping `:complete`, re-read the row and compare: when it moved, leave the status alone and return `:ok` so the queue recomputes, or re-run the analysis on the fresh struct. `reconcile_stale_queued/1`'s `tp.updated_at >= c.updated_at` test has the same blind spot, because a profile written by a stale fetch is newer than the edit that invalidated it.

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

graham/prop!21

Rows
Columns