Repair a failed import row from the place form #158
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-219-repair-failed-import-rows"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes Vikunja task 219. ADR-0135.
Summary
PartialPlacetype (Name,Description,Latitude,Longitude, all optional) —PlaceImportRow.Invalidtakes it as a required third argument,PlaceImportRowFailure.Partialcarries it into the report. Rows the API refused, or ones never attempted, stay read-only (Partialisnull).PlaceImportReportnow carriesGroupId— the group the import targeted.PlaceFormgains a third seeding surface,InitialValues(aPartialPlace?), alongside the existingInitialCoordinates/DefaultGroupId— either coordinate may be present without the other, unlike a map-click seed. Follows the same "seeded once, never re-derived" shape, with its own_seededInitialValuesguard.ImportReportBannerrenders a real<button class="btn-ghost import-report-repair">for each repairable failure, hosting its ownPlaceFormPanelinstance (the same patternHome/Draweralready use for their own add-mode entry points).PlaceFormPanelgainsOnPlaceSaved, distinct fromOnClose, since the banner needs to know specifically that a save happened.CallerGroupsState.EnsureLoadedAsyncis deferred to the repair button's own click handler, not loaded unconditionally on every page — this banner is mounted everywhere, authenticated or not.ImportReportState.Show's existing behaviour — deliberate, recorded in the ADR, not an oversight.See ADR-0135 for the full reasoning and alternatives considered.
Test plan
dotnet build PlaceMark.slnxclean (SDK 10.0.100)PlaceMark.WebUI.Tests: 754 total, 750 passed, 4 skipped (pre-existing), stable across repeated full runsPlaceMark.Architecture.Tests: 123 total, all passed (ADR index, ButtonWeightCoverage, FieldErrors validation)RemoveRepairedRow, the banner's repairable/non-repairable gate, theIsDirtyrepair-seed baseline, and the hidden-groupEnsureGroupVisibleunhide — each reddened under a targeted mutation and was restoredWaitForAssertionis ADR-0119's own class of gap (OverlayFocusTrap.ExitAsyncscheduled asynchronously after a panel closes), confirmed empirically — reddens 9/10 in isolation without it, and an isolated no-typing/no-discard repro reddens 8/8 on nothing but "close a panel, open a new one straight after" — not aStartRepairordering defect22901ac3c836c98efc6d36c98efc6d625ff4b067Verdict: mergeable
Verified independently, from a clean clone of the PR head (not the author's worktree), with the pinned SDK:
dotnet buildclean;PlaceMark.WebUI.Tests754/750/4,PlaceMark.Architecture.Tests123/123 — matches the PR body exactly.Mutation-tested five of the claimed guards myself rather than trusting the ADR's Consequences section: nulling
CsvPlaceImportParser.BuildRow'sPartialPlaceconstruction reddens exactly 4 tests; widening the banner's repair-button gate reddensImportReportBanner_FailureWithNoPartialData_OffersNoRepairButton; dropping the_seededInitialValuesterm fromIsDirtyreddensPlaceForm_CreateWithInitialValues_IsNotDirty; removingEnsureGroupVisiblefromPlaceFormPanel.HandleSavedAsyncreddens the hidden-group repair test. All confirmed, then reverted.Checked the invariant directly:
PlaceImportRunner.RunAsync's API-refusal and systemic-abort branches never passPartial, only the parser-rejection branch does —Request/Error/Partialstay correctly exclusive, nothing downstream re-derives validity fromPartial.PlaceForm.SubmitAsyncstill buildsCreatePlaceRequest/UpdatePlaceRequestfresh from typed state, never fromPartialPlace. NoPlaceMark.Api/PlaceMark.Contractsfile is touched by this PR at all.Also independently checked the
WaitForAssertionfinding, since it's the most interesting claim here. Removed it and ran the reseed test 10/10 in isolation — all 10 failed identically:#place-form-namesimply absent, not present with a stale/wrong value. That failure shape corroborates a render-timing gap (ADR-0119's own documented mechanism:OverlayFocusTrap.ExitAsynconly fires fromModalOverlay.DisposeAsync, scheduled after the removing render) rather than a data-ordering defect inStartRepair— a realStartRepairbug would more likely show the wrong value, not no element at all. Also confirmedCallerGroupsState.EnsureLoadedAsync's guard is checked before anyawait, so it returns synchronously once loaded and can't be the intermittent cause either — consistent with the author's own disproof of their first explanation. Separately, I mutatedCloseRepairto also callRemoveRepairedRow(modelling the ADR's own rejected "infer save from close" alternative) and the reseed test caught it immediately — so the test isn't purely tautological-by-construction, it does guard a real regression class.ADR-0135 and
docs/adr/README.mdcheck out: correctly slotted between 0134/0140, README diff is exactly one added row, otherwise byte-identical.One doc-only nit, not blocking:
PartialPlace.cs'scref="Contracts.Places.CreatePlaceRequest"won't actually resolve (nousing PlaceMark.Contracts.Places, and the project has noGenerateDocumentationFileso nothing catches it at build time) — harmless today since crefs aren't validated here, but worthcref="PlaceMark.Contracts.Places.CreatePlaceRequest"in a follow-up.