diff --git a/DEVELOPING.md b/DEVELOPING.md index 16c3ac6..bda2f7a 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -251,7 +251,9 @@ Editor saves without inputs use `PATCH /api/projects/:id/editor`. Its root field updates leave stored `inputConfig` untouched, including a concurrent Setup update. Do not copy an earlier Setup snapshot into a replacement write. Full GET/PATCH remains the Setup and legacy detail contract; full PATCH keeps -its replacement semantics. All editor candidates remain available for counts, +its replacement semantics, except that an omitted `storyboard` leaves stored +scenes unchanged. Other omitted root fields, including `inputConfig`, are removed. +All editor candidates remain available for counts, selection, generation and composition. The dedicated editor PATCH path is also a rollback safety boundary: a diff --git a/orch.py b/orch.py index 58ee33d..0522100 100644 --- a/orch.py +++ b/orch.py @@ -1175,22 +1175,23 @@ def _write_project_doc( precondition, scene writes retain their existing set behavior, and the stale-scene prune scan is skipped: a brand-new project has no prior scenes to prune, and callers cap create at _MAX_CREATE_SCENES so this always fits - one atomic batch. + one atomic batch. When create is false, an omitted or non-list storyboard + (e.g. null) leaves the scenes subcollection untouched; only an explicit list + (including []) writes and prunes scenes. """ - scenes = payload.get('storyboard') - if not isinstance(scenes, list): - scenes = [] root = dict(payload) root['storyboard'] = [] - scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) ops = [('create' if create else 'set', doc_ref, root)] - for index, scene in enumerate(scenes): - ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) - if not create: - keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} - for snapshot in scenes_ref.stream(): - if snapshot.id not in keep_ids: - ops.append(('delete', scenes_ref.document(snapshot.id), None)) + scenes = payload.get('storyboard') + if isinstance(scenes, list): + scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) + for index, scene in enumerate(scenes): + ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) + if not create: + keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} + for snapshot in scenes_ref.stream(): + if snapshot.id not in keep_ids: + ops.append(('delete', scenes_ref.document(snapshot.id), None)) _commit_in_batches(ui_db, ops) @@ -1205,29 +1206,31 @@ def _write_editor_project_doc( The root update is deliberately field-based: unlike a read-modify-write replacement, it cannot copy a stale inputConfig snapshot over a concurrent Setup save. Fields omitted by the editor payload retain the legacy full-save - replacement behavior through DELETE_FIELD updates. + replacement behavior through DELETE_FIELD updates, except storyboard: the + root always keeps the [] placeholder, and an omitted or non-list storyboard + (e.g. null) leaves the scenes subcollection untouched. """ root_updates = { field_path.FieldPath(key).to_api_repr(): firestore.DELETE_FIELD for key in stored - if key not in payload and key != 'inputConfig' + if key not in payload and key not in ('inputConfig', 'storyboard') } root_updates.update({ field_path.FieldPath(key).to_api_repr(): value for key, value in payload.items() }) - scenes = payload.get('storyboard') - if not isinstance(scenes, list): - scenes = [] root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] - scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) - ops = [('update', doc_ref, root_updates)] - for index, scene in enumerate(scenes): - ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) - keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} - for snapshot in scenes_ref.stream(): - if snapshot.id not in keep_ids: - ops.append(('delete', scenes_ref.document(snapshot.id), None)) + ops = [] + scenes = payload.get('storyboard') + if isinstance(scenes, list): + scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) + for index, scene in enumerate(scenes): + ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) + keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} + for snapshot in scenes_ref.stream(): + if snapshot.id not in keep_ids: + ops.append(('delete', scenes_ref.document(snapshot.id), None)) + ops.insert(0, ('update', doc_ref, root_updates)) _commit_in_batches(ui_db, ops) @@ -1394,7 +1397,9 @@ def project_detail_handler( The default PATCH is the faithful port of the UI's whole-document autosave: a full set() with createdBy stripped from the payload (immutable; the stored - owner is preserved) and lastEdited refreshed server-side. PATCH + owner is preserved) and lastEdited refreshed server-side. An omitted + storyboard preserves scenes; other omitted mutable root fields are removed. + PATCH ?view=editor is the legacy form of the explicit editor exception. The dedicated PATCH /api/projects//editor route is the preferred form; both use field updates so the omitted Setup inputConfig cannot be overwritten by diff --git a/test/test_frontdoor_data.py b/test/test_frontdoor_data.py index ce74d55..853604e 100644 --- a/test/test_frontdoor_data.py +++ b/test/test_frontdoor_data.py @@ -31,6 +31,8 @@ import threading import uuid +import pytest + from google.api_core import exceptions as google_exceptions from google.cloud import firestore @@ -1662,6 +1664,235 @@ def test_project_patch_prunes_removed_scenes(monkeypatch, orchestrator_module): body = client.get('/api/projects/shrink').get_json() assert [s['d'] for s in body['storyboard']] == ['0'] +@pytest.mark.parametrize( + 'path_template', + [ + '/api/projects/{id}', + '/api/projects/{id}/editor', + '/api/projects/{id}?view=editor', + ], +) +def test_project_patch_omitting_storyboard_leaves_scenes_intact( + monkeypatch, orchestrator_module, path_template +): + del orchestrator_module + orch, fake_db, _ = _load_app(monkeypatch) + client = orch.app.test_client() + project_id = f'keep-scenes-{uuid.uuid4().hex[:8]}' + + assert ( + client.post( + '/api/projects', + json={ + 'id': project_id, + 'name': 'Original', + 'inputConfig': {'productDescription': 'Keep for editor saves'}, + 'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}], + }, + ).status_code + == 200 + ) + assert len(_scenes_docs(fake_db, project_id)) == 3 + + # PATCH {"name": "Renamed"} must leave all 3 scenes intact (SM-6 fix). + url = path_template.format(id=project_id) + response = client.patch(url, json={'name': 'Renamed'}) + assert response.status_code == 200 + + # The GET below rebuilds storyboard from the subcollection, so check the + # stored root placeholder directly. + assert fake_db.collection('projects').docs[project_id]['storyboard'] == [] + scenes = _scenes_docs(fake_db, project_id) + assert len(scenes) == 3 + assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2'] + + body = client.get(f'/api/projects/{project_id}').get_json() + assert body['name'] == 'Renamed' + assert [s['d'] for s in body['storyboard']] == ['0', '1', '2'] + + if path_template == '/api/projects/{id}': + # Full PATCH replaces root fields; only an omitted storyboard is preserved. + assert 'inputConfig' not in body + else: + assert body['inputConfig'] == { + 'productDescription': 'Keep for editor saves' + } + + +@pytest.mark.parametrize( + 'path_template', + [ + '/api/projects/{id}', + '/api/projects/{id}/editor', + '/api/projects/{id}?view=editor', + ], +) +def test_project_patch_empty_payload_leaves_scenes_intact( + monkeypatch, orchestrator_module, path_template +): + del orchestrator_module + orch, fake_db, _ = _load_app(monkeypatch) + client = orch.app.test_client() + project_id = f'empty-patch-{uuid.uuid4().hex[:8]}' + + assert ( + client.post( + '/api/projects', + json={ + 'id': project_id, + 'name': 'Original', + 'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}], + }, + ).status_code + == 200 + ) + assert len(_scenes_docs(fake_db, project_id)) == 3 + + url = path_template.format(id=project_id) + response = client.patch(url, json={}) + assert response.status_code == 200 + + scenes = _scenes_docs(fake_db, project_id) + assert len(scenes) == 3 + body = client.get(f'/api/projects/{project_id}').get_json() + assert [s['d'] for s in body['storyboard']] == ['0', '1', '2'] + + +@pytest.mark.parametrize('storyboard', [None, 'not-a-list', {'d': '0'}]) +@pytest.mark.parametrize( + 'path_template', + [ + '/api/projects/{id}', + '/api/projects/{id}/editor', + '/api/projects/{id}?view=editor', + ], +) +def test_project_patch_non_list_storyboard_leaves_scenes_intact( + monkeypatch, orchestrator_module, path_template, storyboard +): + del orchestrator_module + orch, fake_db, _ = _load_app(monkeypatch) + client = orch.app.test_client() + project_id = f'null-storyboard-{uuid.uuid4().hex[:8]}' + + assert ( + client.post( + '/api/projects', + json={ + 'id': project_id, + 'name': 'Original', + 'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}], + }, + ).status_code + == 200 + ) + + # Clients that serialize unset optional fields as null must not wipe scenes. + url = path_template.format(id=project_id) + response = client.patch(url, json={'name': 'n', 'storyboard': storyboard}) + assert response.status_code == 200 + + assert fake_db.collection('projects').docs[project_id]['storyboard'] == [] + scenes = _scenes_docs(fake_db, project_id) + assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2'] + + +@pytest.mark.parametrize( + 'path_template', + [ + '/api/projects/{id}', + '/api/projects/{id}/editor', + '/api/projects/{id}?view=editor', + ], +) +def test_project_patch_explicit_empty_storyboard_deletes_all_scenes( + monkeypatch, orchestrator_module, path_template +): + del orchestrator_module + orch, fake_db, _ = _load_app(monkeypatch) + client = orch.app.test_client() + project_id = f'clear-scenes-{uuid.uuid4().hex[:8]}' + + assert ( + client.post( + '/api/projects', + json={ + 'id': project_id, + 'name': 'Original', + 'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}], + }, + ).status_code + == 200 + ) + assert len(_scenes_docs(fake_db, project_id)) == 3 + + url = path_template.format(id=project_id) + response = client.patch(url, json={'storyboard': []}) + assert response.status_code == 200 + + assert len(_scenes_docs(fake_db, project_id)) == 0 + body = client.get(f'/api/projects/{project_id}').get_json() + assert body['storyboard'] == [] + + +@pytest.mark.parametrize( + 'path_template', + [ + '/api/projects/{id}', + '/api/projects/{id}/editor', + '/api/projects/{id}?view=editor', + ], +) +def test_project_patch_shrinking_storyboard_deletes_trailing_scenes( + monkeypatch, orchestrator_module, path_template +): + del orchestrator_module + orch, fake_db, _ = _load_app(monkeypatch) + client = orch.app.test_client() + project_id = f'shrink-5-to-3-{uuid.uuid4().hex[:8]}' + + assert ( + client.post( + '/api/projects', + json={ + 'id': project_id, + 'name': 'Original', + 'storyboard': [{'d': str(i)} for i in range(5)], + }, + ).status_code + == 200 + ) + assert len(_scenes_docs(fake_db, project_id)) == 5 + + url = path_template.format(id=project_id) + response = client.patch( + url, + json={ + 'storyboard': [{'d': f'{i}-updated'} for i in range(3)], + }, + ) + assert response.status_code == 200 + + # Scenes live only in the subcollection, never inline on the root doc. + assert fake_db.collection('projects').docs[project_id]['storyboard'] == [] + scenes = _scenes_docs(fake_db, project_id) + assert len(scenes) == 3 + assert set(scenes.keys()) == {'000000', '000001', '000002'} + assert [scenes[f'{i:06d}']['d'] for i in range(3)] == [ + '0-updated', + '1-updated', + '2-updated', + ] + assert '000003' not in scenes + assert '000004' not in scenes + + body = client.get(f'/api/projects/{project_id}').get_json() + assert [s['d'] for s in body['storyboard']] == [ + '0-updated', + '1-updated', + '2-updated', + ] + def test_project_delete_clears_scenes_subcollection( monkeypatch, orchestrator_module