fix: address code review issues in flow-to-library sync
- Fix sync trigger: only fire on publish transition, not every PUT - Add TestSyncOnPublish integration tests (2 tests, 16 total passing) - Add group_label to frontend StepContent interface - Guard Library Visibility select to procedure_step nodes only - Block API edits to flow-synced steps (400 read-only guard) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -353,6 +353,12 @@ async def update_step(
|
|||||||
"""Update a step (owner or admin only)."""
|
"""Update a step (owner or admin only)."""
|
||||||
step = await get_step_or_404(step_id, db, current_user, check_edit=True)
|
step = await get_step_or_404(step_id, db, current_user, check_edit=True)
|
||||||
|
|
||||||
|
if step.is_flow_synced:
|
||||||
|
raise HTTPException(
|
||||||
|
status_code=400,
|
||||||
|
detail="Flow-synced steps are read-only. Fork to customize."
|
||||||
|
)
|
||||||
|
|
||||||
# Validate category if being updated
|
# Validate category if being updated
|
||||||
if step_data.category_id:
|
if step_data.category_id:
|
||||||
cat_result = await db.execute(
|
cat_result = await db.execute(
|
||||||
|
|||||||
@@ -641,8 +641,8 @@ async def update_tree(
|
|||||||
if "tree_structure" in update_data:
|
if "tree_structure" in update_data:
|
||||||
tree.version += 1
|
tree.version += 1
|
||||||
|
|
||||||
# Sync steps to step library when publishing
|
# Sync steps to step library on publish transition only
|
||||||
if update_data.get("status") == 'published' or tree.status == 'published':
|
if update_data.get("status") == 'published':
|
||||||
_structure = update_data.get("tree_structure", tree.tree_structure)
|
_structure = update_data.get("tree_structure", tree.tree_structure)
|
||||||
_type = update_data.get("tree_type", tree.tree_type)
|
_type = update_data.get("tree_type", tree.tree_type)
|
||||||
_is_public = update_data.get("is_public", tree.is_public)
|
_is_public = update_data.get("is_public", tree.is_public)
|
||||||
|
|||||||
@@ -138,3 +138,79 @@ class TestExtractStepsForSync:
|
|||||||
}
|
}
|
||||||
results = list(extract_steps_for_sync(tree_structure, tree_type='maintenance'))
|
results = list(extract_steps_for_sync(tree_structure, tree_type='maintenance'))
|
||||||
assert len(results) == 1
|
assert len(results) == 1
|
||||||
|
|
||||||
|
|
||||||
|
class TestSyncOnPublish:
|
||||||
|
"""Integration tests — sync triggered by publishing a tree."""
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_publishing_procedural_tree_creates_library_steps(
|
||||||
|
self, client, auth_headers
|
||||||
|
):
|
||||||
|
# Create a procedural tree with two steps
|
||||||
|
tree_resp = await client.post("/api/v1/trees", json={
|
||||||
|
"name": "Test Procedure",
|
||||||
|
"tree_type": "procedural",
|
||||||
|
"status": "draft",
|
||||||
|
"tree_structure": {
|
||||||
|
"steps": [
|
||||||
|
{"id": "step_1", "type": "procedure_step",
|
||||||
|
"title": "First step", "description": "Do this first"},
|
||||||
|
{"id": "step_2", "type": "procedure_step",
|
||||||
|
"title": "Second step", "description": "Do this second"},
|
||||||
|
{"id": "end_1", "type": "procedure_end", "title": "Done"},
|
||||||
|
]
|
||||||
|
}
|
||||||
|
}, headers=auth_headers)
|
||||||
|
assert tree_resp.status_code == 201
|
||||||
|
tree_id = tree_resp.json()["id"]
|
||||||
|
|
||||||
|
# Publish the tree
|
||||||
|
pub_resp = await client.put(f"/api/v1/trees/{tree_id}", json={"status": "published"}, headers=auth_headers)
|
||||||
|
assert pub_resp.status_code == 200
|
||||||
|
|
||||||
|
# Check library has synced entries
|
||||||
|
lib_resp = await client.get("/api/v1/steps", headers=auth_headers)
|
||||||
|
assert lib_resp.status_code == 200
|
||||||
|
steps = lib_resp.json()
|
||||||
|
synced = [s for s in steps if s.get("is_flow_synced")]
|
||||||
|
assert len(synced) == 2
|
||||||
|
titles = {s["title"] for s in synced}
|
||||||
|
assert "First step" in titles
|
||||||
|
assert "Second step" in titles
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_republishing_updates_existing_library_steps(
|
||||||
|
self, client, auth_headers
|
||||||
|
):
|
||||||
|
# Create a draft tree first, then publish
|
||||||
|
tree_resp = await client.post("/api/v1/trees", json={
|
||||||
|
"name": "Update Test",
|
||||||
|
"tree_type": "procedural",
|
||||||
|
"status": "draft",
|
||||||
|
"tree_structure": {"steps": [
|
||||||
|
{"id": "step_1", "type": "procedure_step",
|
||||||
|
"title": "Original title", "description": "Original desc"},
|
||||||
|
{"id": "end_1", "type": "procedure_end", "title": "Done"},
|
||||||
|
]}
|
||||||
|
}, headers=auth_headers)
|
||||||
|
tree_id = tree_resp.json()["id"]
|
||||||
|
first_pub = await client.put(f"/api/v1/trees/{tree_id}", json={"status": "published"}, headers=auth_headers)
|
||||||
|
assert first_pub.status_code == 200
|
||||||
|
|
||||||
|
# Republish with updated step title
|
||||||
|
second_pub = await client.put(f"/api/v1/trees/{tree_id}", json={
|
||||||
|
"tree_structure": {"steps": [
|
||||||
|
{"id": "step_1", "type": "procedure_step",
|
||||||
|
"title": "Updated title", "description": "Updated desc"},
|
||||||
|
{"id": "end_1", "type": "procedure_end", "title": "Done"},
|
||||||
|
]},
|
||||||
|
"status": "published"
|
||||||
|
}, headers=auth_headers)
|
||||||
|
assert second_pub.status_code == 200
|
||||||
|
|
||||||
|
# Check library shows updated title (not a duplicate)
|
||||||
|
lib_resp = await client.get("/api/v1/steps", headers=auth_headers)
|
||||||
|
synced = [s for s in lib_resp.json() if s.get("is_flow_synced")]
|
||||||
|
assert len(synced) == 1
|
||||||
|
assert synced[0]["title"] == "Updated title"
|
||||||
|
|||||||
@@ -255,8 +255,8 @@ export function StepEditor({ step, stepNumber, onUpdate, onCollapse, availableVa
|
|||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
{/* Library Visibility */}
|
{/* Library Visibility — procedure_step nodes only */}
|
||||||
<div>
|
{step.type === 'procedure_step' && <div>
|
||||||
<label htmlFor="library-visibility" className="mb-1.5 block text-xs font-medium text-muted-foreground">
|
<label htmlFor="library-visibility" className="mb-1.5 block text-xs font-medium text-muted-foreground">
|
||||||
Library Visibility
|
Library Visibility
|
||||||
</label>
|
</label>
|
||||||
@@ -275,7 +275,7 @@ export function StepEditor({ step, stepNumber, onUpdate, onCollapse, availableVa
|
|||||||
<p className="mt-1 text-[10px] text-muted-foreground">
|
<p className="mt-1 text-[10px] text-muted-foreground">
|
||||||
Controls visibility in the step library. Defaults to the flow's own visibility setting.
|
Controls visibility in the step library. Defaults to the flow's own visibility setting.
|
||||||
</p>
|
</p>
|
||||||
</div>
|
</div>}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ export interface StepContent {
|
|||||||
instructions: string
|
instructions: string
|
||||||
help_text?: string
|
help_text?: string
|
||||||
commands?: StepCommand[]
|
commands?: StepCommand[]
|
||||||
|
group_label?: string
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface Step {
|
export interface Step {
|
||||||
|
|||||||
Reference in New Issue
Block a user