fix(review): carry Measure species into Create title/short-desc
Live overnight slabs after removing the Review species picker had species_id null, so Create title stayed gated off. Require species on Measure Calculate, stop the select sync from wiping choices before options load, and have Review gate/PUT from slab.species_id with a Capture handoff repair so deterministic title/short-desc always see it.
This commit is contained in:
@@ -200,6 +200,30 @@ def test_get_not_found(client, db):
|
||||
assert resp.status_code == 404
|
||||
|
||||
|
||||
|
||||
def test_update_omitting_species_id_preserves_it(client, db):
|
||||
"""Review PUTs omit species_id after Measure owns it — must not null the column."""
|
||||
slab_id = str(uuid.uuid4())
|
||||
create = {
|
||||
"id": slab_id, "sku": "", "length_in": 48, "thickness_in": 2, "sqft": 6.25, "bdft": 12.5,
|
||||
"width_min_in": 10, "width_max_in": 12, "width_avg_in": 11, "species_id": 1,
|
||||
}
|
||||
r = client.post("/api/v1/slabs", json=create)
|
||||
assert r.status_code == 201, r.text
|
||||
assert r.json()["species_id"] == 1
|
||||
rev = r.json()["server_rev"]
|
||||
# PUT like Review commitReview: wood cats only, no species_id key.
|
||||
r2 = client.put(f"/api/v1/slabs/{slab_id}", json={
|
||||
"client_rev": rev,
|
||||
"wood_category_ids": [2],
|
||||
"title": None,
|
||||
"short_description": None,
|
||||
})
|
||||
assert r2.status_code == 200, r2.text
|
||||
assert r2.json()["species_id"] == 1
|
||||
assert r2.json()["wood_category_ids"] == [2]
|
||||
|
||||
|
||||
def test_update_success(client, db):
|
||||
slab_id = str(uuid.uuid4())
|
||||
payload = {"id": slab_id, "sku": "SKU001", "length_in":10,"thickness_in":2,"sqft":20,"bdft":5,"width_min_in":1,"width_max_in":2,"width_avg_in":1.5,"species_id":1,"wood_category_ids":json.dumps([2]),"edge_type_term_id":1,"figure_term_ids":json.dumps([1]),"grade_term_id":1,"thickness_term_id":1,"moisture_term_id":1,"fig_tag_ids":json.dumps([1]),"feat_tag_ids":json.dumps([2]),"price":100,"price_source":"recommendation","title":"Test","short_description":"short","description":"desc","content_source":"manual","species_confidence":0.9,"figure_confidence":0.8,"grade_confidence":0.7,"inference_status":"inferring","calibration_photo_id":None}
|
||||
|
||||
@@ -98,12 +98,16 @@
|
||||
}
|
||||
}
|
||||
|
||||
// Keep local species select in sync when draft restore / session updates it.
|
||||
$: {
|
||||
const next = state.speciesId == null ? '' : String(state.speciesId);
|
||||
if (speciesSel !== next) speciesSel = next;
|
||||
// Sync local select only when session speciesId changes (draft restore / setSpecies).
|
||||
// Do NOT depend on speciesSel — that fights bind:value and can clear a choice
|
||||
// before setSpecies lands (or wipe restored ids while options are still loading).
|
||||
let syncedSpeciesId: number | null | undefined = undefined;
|
||||
$: if (syncedSpeciesId !== state.speciesId) {
|
||||
syncedSpeciesId = state.speciesId;
|
||||
speciesSel = state.speciesId == null ? '' : String(state.speciesId);
|
||||
}
|
||||
|
||||
let speciesOptsReady = false;
|
||||
onMount(() => {
|
||||
void getTaxonomy()
|
||||
.then((tax) => {
|
||||
@@ -111,10 +115,12 @@
|
||||
id: c.id,
|
||||
name: c.name,
|
||||
}));
|
||||
speciesOptsReady = true;
|
||||
})
|
||||
.catch(() => {
|
||||
speciesLoadErr = 'Could not load species list.';
|
||||
speciesOpts = [];
|
||||
speciesOptsReady = true;
|
||||
});
|
||||
});
|
||||
|
||||
@@ -170,7 +176,10 @@
|
||||
setSpecies(speciesSel === '' ? null : Number(speciesSel));
|
||||
};
|
||||
|
||||
$: canCalc = positiveInches(String(lengthIn)) !== null && positiveInches(String(thicknessIn)) !== null;
|
||||
$: canCalc =
|
||||
positiveInches(String(lengthIn)) !== null
|
||||
&& positiveInches(String(thicknessIn)) !== null
|
||||
&& speciesSel !== '';
|
||||
|
||||
const doCalc = async () => {
|
||||
calcBusy = true;
|
||||
@@ -294,19 +303,25 @@
|
||||
|
||||
<div class="field species">
|
||||
<label class="label" for="measure-species">Species</label>
|
||||
<select
|
||||
id="measure-species"
|
||||
class="sel"
|
||||
data-testid="measure-species"
|
||||
aria-label="Species"
|
||||
bind:value={speciesSel}
|
||||
on:change={commitSpecies}
|
||||
>
|
||||
<option value="">Select species…</option>
|
||||
{#each speciesOpts as o (o.id)}
|
||||
<option value={String(o.id)}>{o.name}</option>
|
||||
{/each}
|
||||
</select>
|
||||
{#if !speciesOptsReady}
|
||||
<select id="measure-species" class="sel" data-testid="measure-species" aria-label="Species" disabled>
|
||||
<option value="">Loading species…</option>
|
||||
</select>
|
||||
{:else}
|
||||
<select
|
||||
id="measure-species"
|
||||
class="sel"
|
||||
data-testid="measure-species"
|
||||
aria-label="Species"
|
||||
bind:value={speciesSel}
|
||||
on:change={commitSpecies}
|
||||
>
|
||||
<option value="">Select species…</option>
|
||||
{#each speciesOpts as o (o.id)}
|
||||
<option value={String(o.id)}>{o.name}</option>
|
||||
{/each}
|
||||
</select>
|
||||
{/if}
|
||||
{#if speciesLoadErr}
|
||||
<p class="species-err" role="status">{speciesLoadErr}</p>
|
||||
{/if}
|
||||
@@ -330,6 +345,7 @@
|
||||
<div><span>Sqft</span><span>{state.results.sqft.toFixed(2)}</span></div>
|
||||
<div><span>Bdft</span><span>{state.results.bdft.toFixed(2)}</span></div>
|
||||
<div><span>Scale</span><span>{state.results.scale.toFixed(2)} px/in</span></div>
|
||||
<div data-testid="photos-ready-species"><span>Species</span><span>{speciesOpts.find((o) => o.id === state.speciesId)?.name ?? (state.speciesId != null ? `#${state.speciesId}` : '—')}</span></div>
|
||||
</div>
|
||||
{#if state.results.sourceBelowFloor}
|
||||
<div class="warn">Source is below the 800px floor for the inventory PNG. Retake a larger photo.</div>
|
||||
|
||||
@@ -53,3 +53,9 @@ test('MeasurePanel keeps interactive tape controls (merged Axis)', () => {
|
||||
assert.ok(!/Approve tape line/.test(panelSrc), 'Approve tape folded into Calculate');
|
||||
assert.match(panelSrc, /Calculate/);
|
||||
});
|
||||
|
||||
test('Calculate requires Species (carries to createSlab / Create title)', () => {
|
||||
assert.match(panelSrc, /speciesSel !== ''/);
|
||||
assert.match(panelSrc, /speciesOptsReady/);
|
||||
assert.match(panelSrc, /photos-ready-species/);
|
||||
});
|
||||
|
||||
@@ -6,6 +6,7 @@
|
||||
type Slab, type Taxonomy, type TaxCategory, type TaxTag, type SettingsView, type PhotoRow,
|
||||
} from '../../api/slabs';
|
||||
import type { CalibratedCapture } from '../flow';
|
||||
import { canCreateTitleFromSpecies, effectiveSpeciesId } from './speciesFromCapture';
|
||||
import MultiSelect from './MultiSelect.svelte';
|
||||
import { deriveThicknessTermId } from './thicknessBand';
|
||||
import {
|
||||
@@ -92,7 +93,7 @@
|
||||
|
||||
// --- Editable model (mirrors the live slab) ---
|
||||
// Species is set on Measure / createSlab — Review only reads it (no picker).
|
||||
// Kept as a string so canCreateTitle can gate on a non-empty value from hydrate.
|
||||
// speciesSel mirrors the resolved id for hydrate/debug; gates use effectiveSpeciesId.
|
||||
let speciesSel: string = '';
|
||||
// #90: single-selects bind a STRING var to the native <select>, because
|
||||
// <option value={id}> coerces the option value to a string. Binding a
|
||||
@@ -131,8 +132,9 @@
|
||||
// being enabled.
|
||||
let createBusy = false;
|
||||
let createError = '';
|
||||
$: canCreateTitle =
|
||||
speciesSel !== '' && woodCatIds.length > 0;
|
||||
// Prefer slab.species_id; fall back to Capture handoff if createSlab omitted it.
|
||||
$: resolvedSpeciesId = effectiveSpeciesId(slab?.species_id, capture?.speciesId ?? null);
|
||||
$: canCreateTitle = canCreateTitleFromSpecies(resolvedSpeciesId, woodCatIds.length);
|
||||
$: canCreateDesc =
|
||||
canGenerateContent && title.trim() !== '' && shortDescription.trim() !== '';
|
||||
// Monotonic token: each commit captures the current value and ignores its
|
||||
@@ -318,7 +320,8 @@
|
||||
}
|
||||
|
||||
function hydrate(s: Slab, t: Taxonomy | null) {
|
||||
speciesSel = s.species_id != null ? String(s.species_id) : '';
|
||||
const sid = effectiveSpeciesId(s.species_id, capture?.speciesId ?? null);
|
||||
speciesSel = sid != null ? String(sid) : '';
|
||||
woodCatIds = [...(s.wood_category_ids ?? [])];
|
||||
edgeSel = s.edge_type_term_id != null ? String(s.edge_type_term_id) : '';
|
||||
figureIds = [...(s.figure_term_ids ?? [])];
|
||||
@@ -376,6 +379,14 @@
|
||||
reviewBusy = true;
|
||||
reviewError = '';
|
||||
try {
|
||||
// Repair Measure species onto the row before probing title (review reads the DB).
|
||||
if (resolvedSpeciesId != null && slab.species_id == null) {
|
||||
slab = await updateSlab(slabId, {
|
||||
species_id: resolvedSpeciesId,
|
||||
client_rev: slab.server_rev,
|
||||
});
|
||||
if (myToken !== commitToken) return;
|
||||
}
|
||||
// Run the deterministic review FIRST (reads the current metadata) to get the
|
||||
// title/short_description build before we persist.
|
||||
// Do NOT send price/sku here: review POST persists those overrides and bumps
|
||||
@@ -389,9 +400,9 @@
|
||||
if (probe.title && title === '') title = probe.title;
|
||||
if (probe.short_description && shortDescription === '') shortDescription = probe.short_description;
|
||||
|
||||
// species_id intentionally omitted — set at createSlab from Measure; Review
|
||||
// must not clear or re-edit it.
|
||||
const patch = {
|
||||
// Always echo known species_id (slab or Capture repair) so Review PUTs never
|
||||
// leave a Measure-chosen species off the row; never send null (would wipe).
|
||||
const patch: Record<string, unknown> = {
|
||||
wood_category_ids: woodCatIds,
|
||||
edge_type_term_id: edgeSel === '' ? null : Number(edgeSel),
|
||||
figure_term_ids: figureIds,
|
||||
@@ -407,7 +418,8 @@
|
||||
sku: sku || null,
|
||||
client_rev: slab.server_rev,
|
||||
};
|
||||
const fresh = await updateSlab(slabId, patch);
|
||||
if (resolvedSpeciesId != null) patch.species_id = resolvedSpeciesId;
|
||||
const fresh = await updateSlab(slabId, patch as Parameters<typeof updateSlab>[1]);
|
||||
if (myToken !== commitToken) return; // superseded; don't clobber the latest
|
||||
slab = fresh;
|
||||
// Re-run the review against the just-persisted row (authoritative ready/missing).
|
||||
@@ -418,6 +430,9 @@
|
||||
});
|
||||
if (myToken !== commitToken) return;
|
||||
if (review.primary_wood_category_id != null) primaryCatId = review.primary_wood_category_id;
|
||||
// Post-PUT review may newly have species+wood cats — fill empty Create fields.
|
||||
if (review.title && title === '') title = review.title;
|
||||
if (review.short_description && shortDescription === '') shortDescription = review.short_description;
|
||||
} catch (e) {
|
||||
if (myToken !== commitToken) return;
|
||||
reviewError = e instanceof ApiError ? e.message : 'Could not review. Try again.';
|
||||
@@ -437,6 +452,14 @@
|
||||
createBusy = true;
|
||||
createError = '';
|
||||
try {
|
||||
// 0. Repair: if Measure chose a species but createSlab omitted it, write it
|
||||
// before the deterministic build (review reads species_id from the row).
|
||||
if (resolvedSpeciesId != null && slab.species_id == null) {
|
||||
slab = await updateSlab(slabId, {
|
||||
species_id: resolvedSpeciesId,
|
||||
client_rev: slab.server_rev,
|
||||
});
|
||||
}
|
||||
// 1. Run the deterministic review to get title / short_description.
|
||||
const res = await reviewSlab(slabId, {
|
||||
primary_wood_category_id: primaryCatId === '' ? undefined : Number(primaryCatId),
|
||||
@@ -447,10 +470,11 @@
|
||||
if (res.short_description) shortDescription = res.short_description;
|
||||
// 3. Persist the generated prose (single PUT, fresh client_rev).
|
||||
if ((title || shortDescription) && (title !== (slab as Slab).title || shortDescription !== (slab as Slab).short_description)) {
|
||||
const prosePatch = {
|
||||
const prosePatch: Parameters<typeof updateSlab>[1] = {
|
||||
title: title || null,
|
||||
short_description: shortDescription || null,
|
||||
client_rev: (slab as Slab).server_rev,
|
||||
...(resolvedSpeciesId != null ? { species_id: resolvedSpeciesId } : {}),
|
||||
};
|
||||
const fresh = await updateSlab(slabId, prosePatch);
|
||||
slab = fresh;
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
import assert from 'node:assert/strict';
|
||||
import { test } from 'node:test';
|
||||
import { canCreateTitleFromSpecies, effectiveSpeciesId } from './speciesFromCapture';
|
||||
|
||||
test('effectiveSpeciesId prefers slab over capture', () => {
|
||||
assert.equal(effectiveSpeciesId(281, 11), 281);
|
||||
assert.equal(effectiveSpeciesId(null, 11), 11);
|
||||
assert.equal(effectiveSpeciesId(undefined, 11), 11);
|
||||
assert.equal(effectiveSpeciesId(null, null), null);
|
||||
assert.equal(effectiveSpeciesId(undefined, undefined), null);
|
||||
});
|
||||
|
||||
test('effectiveSpeciesId ignores non-finite values', () => {
|
||||
assert.equal(effectiveSpeciesId(Number.NaN, 11), 11);
|
||||
assert.equal(effectiveSpeciesId(null, Number.NaN), null);
|
||||
});
|
||||
|
||||
test('canCreateTitleFromSpecies requires species + ≥1 wood category', () => {
|
||||
assert.equal(canCreateTitleFromSpecies(281, 1), true);
|
||||
assert.equal(canCreateTitleFromSpecies(281, 0), false);
|
||||
assert.equal(canCreateTitleFromSpecies(null, 2), false);
|
||||
assert.equal(canCreateTitleFromSpecies(null, 0), false);
|
||||
});
|
||||
@@ -0,0 +1,26 @@
|
||||
/** Resolve Woo species leaf for Review Create-title gate + PUTs.
|
||||
*
|
||||
* Species is chosen on Measure and stored on the slab at createSlab. Review has
|
||||
* no picker; Create title/short-desc read species from the slab (Capture
|
||||
* handoff is a repair fallback when create omitted it).
|
||||
*/
|
||||
|
||||
export function effectiveSpeciesId(
|
||||
slabSpeciesId: number | null | undefined,
|
||||
captureSpeciesId: number | null | undefined,
|
||||
): number | null {
|
||||
if (slabSpeciesId != null && Number.isFinite(Number(slabSpeciesId))) {
|
||||
return Math.round(Number(slabSpeciesId));
|
||||
}
|
||||
if (captureSpeciesId != null && Number.isFinite(Number(captureSpeciesId))) {
|
||||
return Math.round(Number(captureSpeciesId));
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
export function canCreateTitleFromSpecies(
|
||||
speciesId: number | null,
|
||||
woodCategoryCount: number,
|
||||
): boolean {
|
||||
return speciesId != null && woodCategoryCount > 0;
|
||||
}
|
||||
@@ -324,6 +324,8 @@ async function completeForReady(page: Page) {
|
||||
await page.getByTestId('create-title').click();
|
||||
// Wait for the build to complete (title field populated = deterministic copy).
|
||||
await expect(page.getByTestId('title')).not.toHaveValue('');
|
||||
await expect(page.getByTestId('title')).toHaveValue(/Cedar/);
|
||||
await expect(page.getByTestId('short-desc')).toHaveValue(/Cedar/);
|
||||
await page.getByTestId('sku').fill('SLAB-0001');
|
||||
// SKU commits on blur — press Tab to move focus off it (deterministic, single
|
||||
// commit; a body click can re-focus the SKU and re-fire blur).
|
||||
@@ -339,9 +341,12 @@ test.describe('M3 Review→Publish flow (#77)', () => {
|
||||
await expect(page.getByTestId('ready-status')).toContainText(/Not ready|Ready/);
|
||||
await expect(page.getByTestId('title')).toHaveValue('');
|
||||
await completeForReady(page);
|
||||
// Species from Measure draft must land on the slab (Create title reads it).
|
||||
expect(slab.species_id).toBe(SPECIES_ID);
|
||||
// #89: the Create build populated the deterministic copy on the slab row.
|
||||
expect(slab.title).toBeTruthy();
|
||||
expect(slab.short_description).toBeTruthy();
|
||||
expect(String(slab.title)).toMatch(/Cedar/);
|
||||
// Continue to Publish is enabled and hands off.
|
||||
await expect(page.getByTestId('continue-publish')).toBeEnabled();
|
||||
await page.getByTestId('continue-publish').click();
|
||||
|
||||
Reference in New Issue
Block a user