fix(ads): a failed job says which step failed (gh-#724) - #768
Merged
Merged
Conversation
STORY-435: a failed write or preview job must read back with the kind that failed (job_failed_kind / failedKind) so the wizard can route the error to the Script or Hear step. Host facts fail on the missing column and DTO field; the Jest spec fails on both steps showing the other's error.
Adds the job_failed_kind column (text, CHECK write|preview, nullable) to station.ad_spot via an idempotent ADD COLUMN IF NOT EXISTS, mirrored in db/06 for fresh installs. A text-equality spec pins the CHECK predicate identical across db/47 and db/06. Story346's marker comment is updated: db/47 creates no table, so setup.sh keeps db/46 as its marker and reports the db/47 gap honestly. Why: job_kind is nulled when a job settles, so the failed step is lost by the time job_error is readable (gh-#724, STORY-435).
AdSpot gains JobFailedKind; AdSpotRepository maps job_failed_kind on every read, nulls it when a fresh job is stamped, and on ClearJobAsync copies the old job_kind into it only when an error is recorded. Ads test fakes mirror the same semantics. Why: job_kind is nulled when a job settles, so without its own column the failed step is lost by the time job_error is readable (gh-#724, STORY-435).
AdSpotJobDto gains FailedKind after Kind; ToJobDto returns a job object whenever any of job_kind, job_error or job_failed_kind is set. The admin-ui AdSpotJobDto type gains failedKind and every spec literal carries it. Why: the wizard needs to know which step's job failed so it can show the error on that step alone (gh-#724, STORY-435).
JobRunner takes the job kind it owns ("write" for ScriptStep, "preview"
for HearStep) and renders job.error only when job.failedKind matches.
A failed preview therefore shows on the Hear step alone and a failed
write on the Script step alone, instead of both steps echoing the same
message (STORY-435 AC6–AC8, gh-#724). The wizard-level alert still
carries request failures only. A legacy row with an error but no
failedKind shows on neither step; the next stamped job clears it.
PR #764 added renderWithinMinutes to AdSpotDto after this branch's wizard-failed-step spec was written, so CI's typecheck:specs failed on the merged tree. The fixture now sets it to null like the other specs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #724 · STORY-435 (a failed job says which step failed) · PLAN T462–T466.
🐛 What was wrong
job_kindis nulled the moment a job settles, success or failure alike, so by the timejob_erroris readable nobody knows whether the write or the preview produced it. The wizard's Script and Hear steps both rendered the samejob.error, so a failed preview showed up on the Script step too.✅ What changed
db/47-ad-spot-job-failed-kind-migration.shaddsstation.ad_spot.job_failed_kind textwithCHECK (IS NULL OR IN ('write','preview')); db/06 carries the identical column for fresh installs (gh-db/06 fresh-init mirror is missing station.schedule_special (db/36) #618 lesson), pinned by a text-equality spec. No new table, so setup.sh's migration marker stays at db/46 and its range message reports "db/47 adds no new table" — the documented degrade, not a bug.AdSpot.JobFailedKind;StampJobAsyncnulls it,ClearJobAsyncsets it to the failing job's kind on failure and null on a clean finish (one SQL statement, reads the oldjob_kind). Fakes mirror the semantics.AdSpotJobDto.FailedKindon the wire asfailedKind;jobisnullonly when none ofjob_kind,job_error,job_failed_kindis set.JobRunnertakes the kind it owns (ScriptStep→write,HearStep→preview) and renders the error only whenjob.failedKindmatches. The wizard-level alert keeps request failures only. A legacy row with an error but nofailedKindshows on neither step; the next stamped job clears it.tests/GenWave.Host.Tests/Specs/Story435_FailedJobKind.cs(11 facts, endpoint driven against an ephemeral Postgres) andadmin-ui/__specs__/wizard-failed-step.spec.tsx. Red at 2b20a02, green now.🔌 Wire evidence (T466, dev stack via
BUILD=1 ./launch.sh, Kokoro stopped)New spot for Wire Diner through the wizard, "Render preview" on the Hear step:
Hear step (accessibility snapshot):
Back to the Script step: the script textarea and voice cast render, no
alertnode. Screenshots of both steps are on the dev box (t466-hear-error.png,t466-script-clean.png) and can be attached on request.📝 Notes for review
job_errorwith a nulljob_failed_kind; the migration does not backfill, so their stale error shows on neither step until the next job runs. Accepted rather than guessed.Do not merge without Dean.