F1: Same-user re-import skips existing execution logs
Logs whose id already exists locally were re-inserted as-is, producing a PRIMARY KEY violation per log that surfaced as errors in the import report. They are now counted as skipped (execution logs are immutable history — skip is always the right resolution). Adds the missing regression test: re-import a full export without deleting anything. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
cd8636546a
commit
7e9c825af3
|
|
@ -2,6 +2,12 @@
|
|||
|
||||
Record of architectural and technical decisions made during development.
|
||||
|
||||
## 2026-07-09 — Same-user re-import skips existing execution logs (code review F1)
|
||||
|
||||
**Decision:** In `useJsonImport.js`, execution logs whose id already exists locally (`sameUserCollisions`) are now counted as skipped instead of being re-inserted with their original id. Previously each one hit a PRIMARY KEY violation whose message ended up in the import report's error list, making a routine "re-import my own full export" look like a partially failed import.
|
||||
|
||||
**Rationale:** Execution logs are immutable historical records — unlike exercises/combos/sessions there is nothing to Replace or merge, so a skip is always the correct resolution and no Replace/Skip UI is warranted. A regression test covers the exact scenario the existing round-trip test missed: re-importing a full export *without* deleting anything first must produce zero errors and no duplicate logs.
|
||||
|
||||
## 2026-07-06 — Voice announcements via the Web Speech API (Step 26)
|
||||
|
||||
**Decision:** Added `useSpeech.js`, a thin wrapper around `window.speechSynthesis` exposing `init()` (reads the new `voice_enabled` settings key, default enabled), `speak(text, { volume })` (cancels any pending utterance first, so a new announcement always preempts a stale one, and maps the current i18next language to a BCP-47 tag: `en` → `en-US`, `fr` → `fr-FR`, `da` → `da-DK`), `cancel()`, and `isSupported`. `useSessionExecution.js` calls `speak()` with the exercise title whenever a step becomes current (`loadStepState()`), and pre-announces the *next* exercise ("Next: X") in the two places the on-screen UI already does the same thing silently: 10 s before an EMOM slot ends (hooked into the existing `_emomSounds` tick handler) and the instant a TABATA rest phase opens (`_startRestPhase`). AMRAP and plain (NONE) combos only get the step-change announcement, matching their on-screen behavior of not showing a "next" preview until the step actually changes. To avoid the same exercise being spoken twice in quick succession, the queue index that was just pre-announced is tracked in a closure variable; `loadStepState()` skips the step-change announcement exactly once when it reaches that index. `speechSynthesis.cancel()` is also called on `exit()` and `_finish()` so nothing keeps talking after the user leaves the execution view.
|
||||
|
|
|
|||
|
|
@ -538,10 +538,13 @@ export function useJsonImport() {
|
|||
report,
|
||||
errors,
|
||||
) {
|
||||
// Collect all logs to import from the three analysis buckets
|
||||
// Same-user collisions mean the log already exists locally. Execution logs
|
||||
// are immutable historical records, so re-importing them is always a skip
|
||||
// (re-inserting would only produce PRIMARY KEY violations).
|
||||
report.skipped += (analysis.sameUserCollisions || []).length
|
||||
|
||||
const logsToImport = [
|
||||
...(analysis.noCollision || []).map((log) => ({ log, action: 'insert' })),
|
||||
...(analysis.sameUserCollisions || []).map((e) => ({ log: e.imported, action: 'insert' })),
|
||||
...(analysis.differentUser || []).map((e) => ({ log: e.imported, action: e.action })),
|
||||
]
|
||||
|
||||
|
|
|
|||
|
|
@ -16,6 +16,7 @@ Step 7 — Full Data Import/Export Tests
|
|||
- test_import_as_mine_combo: Import-as-mine button works on combo list
|
||||
- test_import_as_mine_session: Import-as-mine button works on session list
|
||||
- test_execution_logs_same_user_roundtrip: Execution logs survive a same-user export/import cycle
|
||||
- test_execution_logs_same_user_reimport_without_delete: Re-import without delete skips logs cleanly
|
||||
- test_execution_log_id_remapping: Log item exercise_id is remapped when the exercise gets a new_id
|
||||
- test_session_with_combo_item_remapping: Session combo item_id follows remap when combo gets new_id
|
||||
- test_collection_session_remapping: Collection session ref follows remap when session gets new_id
|
||||
|
|
@ -691,6 +692,46 @@ def test_execution_logs_same_user_roundtrip(page: Page, app_url: str):
|
|||
assert result["sessionExists"], "Log session_id must reference an existing session after reimport"
|
||||
|
||||
|
||||
def test_execution_logs_same_user_reimport_without_delete(page: Page, app_url: str):
|
||||
"""Re-importing a full export without deleting anything reports execution
|
||||
logs as skipped (no constraint errors, no duplicate logs)."""
|
||||
_go_to(page, app_url, "#/exercises")
|
||||
_clean_all(page)
|
||||
|
||||
ex_id = _create_exercise(page, "Reimport Exercise")
|
||||
session_id = _create_session(
|
||||
page,
|
||||
"Reimport Session",
|
||||
[{"item_id": ex_id, "item_type": "exercise", "repetitions": 10, "weight": 0}],
|
||||
)
|
||||
_create_execution_log(page, session_id, ex_id)
|
||||
|
||||
json_str = _build_full_export_json(page)
|
||||
|
||||
# Re-import immediately — everything still exists locally
|
||||
_trigger_import_with_json(page, json_str)
|
||||
expect(page.locator('[data-testid="import-apply"]')).to_be_visible(timeout=10000)
|
||||
page.locator('[data-testid="import-apply"]').click()
|
||||
expect(page.locator('[data-testid="import-report-step"]')).to_be_visible(timeout=15000)
|
||||
|
||||
# No constraint errors in the report
|
||||
error_count = page.locator('[data-testid^="import-report-error-"]').count()
|
||||
assert error_count == 0, f"Expected no import errors, got {error_count}"
|
||||
|
||||
# The colliding log is reported as skipped, not imported
|
||||
expect(page.locator('[data-testid="import-report-skipped"]')).to_be_visible()
|
||||
assert page.locator('[data-testid="import-report-logs-imported"]').count() == 0, (
|
||||
"Existing execution log must be skipped, not re-imported"
|
||||
)
|
||||
|
||||
page.click('[data-testid="import-done"]')
|
||||
page.wait_for_timeout(500)
|
||||
|
||||
# No duplicate logs in the database
|
||||
log_count = page.evaluate("async () => (await window.__repos.executionLogs.getAll()).length")
|
||||
assert log_count == 1, f"Expected 1 execution log after reimport, got {log_count}"
|
||||
|
||||
|
||||
def test_execution_log_id_remapping(page: Page, app_url: str):
|
||||
"""When an exercise gets new_id on collision, the execution log item's exercise_id is remapped."""
|
||||
_go_to(page, app_url, "#/exercises")
|
||||
|
|
|
|||
Loading…
Reference in a new issue