From 3f30e2c7bd2c407a0b2aad2e37efd6d43b82e883 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Tue, 25 Aug 2026 09:40:20 -0400 Subject: [PATCH] Aug 25 - Fix inspection lost photos --- JanitorialQC/API/APIClient.swift | 25 +++++++++++ JanitorialQC/CLAUDE.md | 45 ++++++++++++++++++++ JanitorialQC/Models/PendingPhoto.swift | 10 +++++ JanitorialQC/Sync/SyncManager.swift | 58 ++++++++++++++++++++++++++ 4 files changed, 138 insertions(+) diff --git a/JanitorialQC/API/APIClient.swift b/JanitorialQC/API/APIClient.swift index 2089ec4..33c87f7 100644 --- a/JanitorialQC/API/APIClient.swift +++ b/JanitorialQC/API/APIClient.swift @@ -350,6 +350,31 @@ actor APIClient { ) } + // ── Attach a late photo to an ALREADY-SUBMITTED inspection ──────────── + // The inspection twin of updateIssuePhotos above, and the piece that was + // missing: an inspection photo that exhausted its upload attempts was + // submitted with its field blanked, and nothing could ever put it back. + // attachServerPath() wrote the recovered path into LOCAL form data only, + // so the server copy stayed empty forever even after a successful retry. + // + // Server-side (app/api/inspections.py, update_inspection) form_data is + // merged field-by-field via _merge_form_data: a non-empty incoming value + // wins, and existing 'uploads/...' paths are never blanked. Sending only + // the recovered fields is therefore safe and idempotent. + // + // `status` is deliberately NOT sent: including it would re-run the + // draft→completed transition server-side, which is what fulfils a linked + // schedule. Omitting it leaves status, score and schedule untouched. + func updateInspectionFormData(inspectionId: Int, + fields: [String: String]) async throws { + struct R: Decodable, Sendable { let id: Int } + let _: R = try await request( + "/api/v1/inspections/\(inspectionId)", + method: "PATCH", + body: ["form_data": fields] + ) + } + // ── Upload a resolution photo (entity_type = issue_result) ──────────── // Saves to issue_result_photos subfolder on the server — same bucket // as photos uploaded via the web update form. diff --git a/JanitorialQC/CLAUDE.md b/JanitorialQC/CLAUDE.md index 56d1cc8..7717c9c 100644 --- a/JanitorialQC/CLAUDE.md +++ b/JanitorialQC/CLAUDE.md @@ -265,6 +265,51 @@ guard isOnline, let context = modelContext, AuthManager.shared.isAuthenticated e 7. **`pollNotifications`** — fetches new notifications since `lastNotificationFetch` cursor. +### Photo loss on inspections — the recovery loop (Aug 2026) + +An inspection can reach the server with its photo fields BLANK while the files +sit safely on the device. The chain: + +1. `processPhotoQueue` upload fails → `uploadRetryCount++`; after + `maxPhotoUploadAttempts` (5) the row goes `uploadStatus = "failed"`, which is + **terminal**. +2. `processInspectionQueue` treats `"failed"` as ready — deliberate, so a dead + photo cannot block a submission forever — and submits. +3. `APIClient.submitInspection` rewrites any surviving `local://` value to `""`, + so the field lands **blank on the server**. +4. The inspection is marked `synced`. Nothing revisits it. + +**Step 4 was a dead end until this fix.** "Retry Failed Items" resets the photo +to `pending` and the re-upload can succeed, but `attachServerPath()` writes the +recovered path into **local** form data only — and the inspection is already +synced, so nothing carried it across. The photo was recoverable in principle and +unreachable in practice, which is what the Photo Diagnostic screen reports as +*"LOST ON SERVER … File present at stored path — recoverable"*. + +`pushLateInspectionPhotoIfNeeded()` closes it, mirroring +`pushLateIssuePhotoIfNeeded()`: + +| | Issue | Inspection | +|---|---|---| +| late-attach call | `PATCH /api/v1/issues//photos` | `PATCH /api/v1/inspections/` with `form_data` | +| helper | `pushLateIssuePhotoIfNeeded` | `pushLateInspectionPhotoIfNeeded` | + +Server-side `_merge_form_data` makes this safe: a non-empty incoming value wins, +and an existing `uploads/...` path is never blanked by an empty one — so the +PATCH is idempotent and cannot erase a good path. **`status` is deliberately not +sent**: including it would re-run the draft→completed transition, which is what +fulfils a linked schedule. + +Normal path is unaffected — `processPhotoQueue` runs before +`processInspectionQueue`, so a first-time inspection has no `serverId` yet and +the helper no-ops; only a recovery reaches it. + +`PendingPhoto.lastUploadError` records **why** the last attempt failed. Nothing +recorded it before: a photo could burn all five attempts with the reason visible +nowhere — the device showed only "failed", and the server logged only +*successful* uploads (now fixed: `app/api/photos.py` logs every rejection and any +storage-write failure at WARNING/ERROR with the username). + ### Server-pulled issue identification Records inserted by `pullAssignedIssues` are identified by: `syncStatus == "synced"` AND `inspectionLocalId == ""`. These are the only records safe to delete during reconciliation. diff --git a/JanitorialQC/Models/PendingPhoto.swift b/JanitorialQC/Models/PendingPhoto.swift index 48e02aa..f5567ff 100644 --- a/JanitorialQC/Models/PendingPhoto.swift +++ b/JanitorialQC/Models/PendingPhoto.swift @@ -38,6 +38,16 @@ final class PendingPhoto { var uploadRetryCount: Int = 0 var createdAt: Date + /// Why the last upload attempt failed, for diagnosis. + /// + /// Nothing recorded this before: a photo could burn all five attempts and + /// cost an inspection its evidence with no trace anywhere of the reason — + /// the device showed only "failed" and the server logs only SUCCESSFUL + /// uploads. Cleared on a successful upload. + /// + /// Optional so existing SwiftData stores migrate lightweight (rule 8). + var lastUploadError: String? + // ── Capture metadata (sent to the server, burned into the photo) ─────── // Recorded when the shutter fires, NOT when the upload runs — the app is // offline-first, so a photo taken at 09:14 may not sync until 16:00 and diff --git a/JanitorialQC/Sync/SyncManager.swift b/JanitorialQC/Sync/SyncManager.swift index 63f043c..e55f486 100644 --- a/JanitorialQC/Sync/SyncManager.swift +++ b/JanitorialQC/Sync/SyncManager.swift @@ -367,10 +367,14 @@ class SyncManager: ObservableObject { photo.uploadRetryCount = 0 uploadedPaths[photo.localFilePath] = serverPath + photo.lastUploadError = nil attachServerPath(serverPath, for: photo, inspections: allInspections, issues: allIssues) await pushLateIssuePhotoIfNeeded(serverPath, for: photo, issues: allIssues, context: context) + await pushLateInspectionPhotoIfNeeded(serverPath, for: photo, + inspections: allInspections, + context: context) try? context.save() } catch { @@ -386,6 +390,9 @@ class SyncManager: ObservableObject { // photo permanently, silently, with the inspection still // reported as successfully synced. photo.uploadRetryCount += 1 + // Keep the reason. Without it a lost evidence photo is + // undiagnosable after the fact — see PendingPhoto.lastUploadError. + photo.lastUploadError = error.localizedDescription if photo.uploadRetryCount >= Self.maxPhotoUploadAttempts { photo.uploadStatus = "failed" syncError = "A photo failed to upload after " @@ -403,6 +410,7 @@ class SyncManager: ObservableObject { dup.serverPath = serverPath dup.uploadStatus = "uploaded" dup.uploadRetryCount = 0 + dup.lastUploadError = nil attachServerPath(serverPath, for: dup, inspections: allInspections, issues: allIssues) await pushLateIssuePhotoIfNeeded(serverPath, for: dup, @@ -455,6 +463,56 @@ class SyncManager: ObservableObject { try? context.save() } + /// Push a recovered photo onto an inspection that has ALREADY been submitted. + /// + /// The inspection counterpart of pushLateIssuePhotoIfNeeded, and the gap + /// that made inspection photo loss permanent: + /// + /// 1. the upload fails `maxPhotoUploadAttempts` times -> row goes "failed" + /// 2. "failed" counts as ready, so processInspectionQueue submits anyway + /// and APIClient.submitInspection rewrites the surviving local:// value + /// to "" — the field lands BLANK on the server + /// 3. the inspection is marked synced; nothing ever revisits it + /// + /// Step 3 was the dead end. "Retry Failed Items" could re-upload the file + /// successfully, but attachServerPath only wrote the path into LOCAL form + /// data, which no longer goes anywhere — the inspection was already synced. + /// The photo sat on the device, recoverable in principle and unreachable in + /// practice. This closes the loop by PATCHing the server copy. + /// + /// Normal path: processPhotoQueue runs BEFORE processInspectionQueue, so a + /// first-time inspection has no serverId yet and this does nothing — the + /// path travels in the submit body as usual. Only a recovery reaches here. + /// + /// Best-effort: a failure leaves the photo attached locally and retried on + /// the next pass, exactly like the issue version. + private func pushLateInspectionPhotoIfNeeded( + _ serverPath: String, + for photo: PendingPhoto, + inspections: [LocalInspection], + context: ModelContext + ) async { + guard photo.entityType == "inspection", + let fieldId = photo.fieldId + else { return } + let entityId = photo.entityLocalId + guard let inspection = inspections.first(where: { $0.localId == entityId }), + let inspectionServerId = inspection.serverId + else { return } + + do { + try await APIClient.shared.updateInspectionFormData( + inspectionId: inspectionServerId, + fields: [fieldId: serverPath] + ) + inspection.syncErrorMessage = nil + } catch { + inspection.syncErrorMessage = + "A recovered photo could not be attached: \(error.localizedDescription)" + } + try? context.save() + } + /// Write a freshly uploaded server path onto whichever record owns the photo. /// Both collections are pre-fetched by the caller — see processPhotoQueue. private func attachServerPath(