mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 03:01:21 +02:00
y fix(backup): don't count a stale selection, and say when archive links are dropped (#2656)
Two smaller restore-path fixes from the same review.
The modal's footer counted `selected` raw while the checkboxes rendered
`selected && isAvailable`. Switching commits keeps `selected` on purpose (it is
only pruned once the new preview lands), so for as long as the new commit's
preview was in flight — with the category list replaced by its spinner — the
footer still read "2 selected" over an enabled Restore button, and clicking it
restored the newly-picked commit with the previous commit's categories, none of
which the user had seen an item count for. The count and the POST body now come
from one `selectedCategories` memo gated on availability, exactly as the
checkboxes are, so both go empty until the preview lands.
Restoring Spool inventory without Print archives leaves archive_id_map empty,
so every usage -> archive link is nulled even where the archive exists locally.
It can't be resolved here (the archives payload isn't fetched for a category
that wasn't selected) and a later archives-only restore won't repair it either,
since the usage dedupe key doesn't include archive_id and those rows read as
already-present. So it gets a note naming the remedy while the user can still
redo the run with both categories ticked.
This commit is contained in:
@@ -848,6 +848,7 @@ class GitHubRestoreService:
|
||||
|
||||
valid_printers = set((await db.execute(select(Printer.id))).scalars().all())
|
||||
unresolved = 0
|
||||
unlinked_archives = 0
|
||||
|
||||
for entry in usage:
|
||||
if not isinstance(entry, dict):
|
||||
@@ -887,6 +888,16 @@ class GitHubRestoreService:
|
||||
|
||||
old_archive_id = entry.get("archive_id")
|
||||
archive_id = archive_id_map.get(old_archive_id) if isinstance(old_archive_id, int) else None
|
||||
if archive_id is None and isinstance(old_archive_id, int):
|
||||
# Restoring spools without archives leaves archive_id_map empty,
|
||||
# so every "this print consumed that spool" link is dropped — the
|
||||
# local archive may well exist, but its payload wasn't fetched,
|
||||
# so there is no natural key here to match it on. Nor is it
|
||||
# repairable by a later archives-only restore: the dedupe key
|
||||
# above doesn't include archive_id, so these rows are recognised
|
||||
# as already-present and skipped. Worth telling the user while
|
||||
# they can still redo the run with both categories ticked.
|
||||
unlinked_archives += 1
|
||||
|
||||
row = SpoolUsageHistory(
|
||||
spool_id=spool_id,
|
||||
@@ -908,6 +919,11 @@ class GitHubRestoreService:
|
||||
f"{unresolved} usage record(s) skipped — their spool is not in this backup's "
|
||||
"spool list, so there is nothing to attach them to."
|
||||
)
|
||||
if unlinked_archives:
|
||||
tally.note(
|
||||
f"{unlinked_archives} usage record(s) restored without their print-history link — "
|
||||
"select Print archives alongside Spool inventory to keep it."
|
||||
)
|
||||
|
||||
async def _restore_settings(
|
||||
self,
|
||||
|
||||
@@ -439,6 +439,51 @@ class TestRestoreSpools:
|
||||
rows = (await db_session.execute(select(SpoolUsageHistory))).scalars().all()
|
||||
assert len(rows) == 1
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_dropped_archive_link_is_explained(self, db_session):
|
||||
"""Spools without archives nulls every usage -> archive link, silently."""
|
||||
tally = _CategoryTally()
|
||||
inventory = {"spools": [self._spool_entry(id=41)]}
|
||||
usage = {
|
||||
"usage_history": [
|
||||
{"spool_id": 41, "archive_id": 7, "weight_used": 1.0, "created_at": "2026-02-01 09:00:00"},
|
||||
{"spool_id": 41, "archive_id": 8, "weight_used": 2.0, "created_at": "2026-02-01 10:00:00"},
|
||||
{"spool_id": 41, "weight_used": 3.0, "created_at": "2026-02-01 11:00:00"},
|
||||
]
|
||||
}
|
||||
|
||||
# Empty archive_id_map: the archives category wasn't selected, so its
|
||||
# payload was never fetched and there is nothing to match against.
|
||||
await _service()._restore_spools(db_session, inventory, usage, False, tally, {})
|
||||
await db_session.commit()
|
||||
|
||||
rows = (await db_session.execute(select(SpoolUsageHistory))).scalars().all()
|
||||
assert len(rows) == 3
|
||||
assert all(row.archive_id is None for row in rows)
|
||||
# Only the two that had a link to lose are counted.
|
||||
assert any("2 usage record(s) restored without their print-history link" in n for n in tally.notes)
|
||||
assert any("select Print archives alongside" in n for n in tally.notes)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_no_note_when_every_archive_link_resolves(self, db_session):
|
||||
tally = _CategoryTally()
|
||||
inventory = {"spools": [self._spool_entry(id=41)]}
|
||||
usage = {
|
||||
"usage_history": [
|
||||
{"spool_id": 41, "archive_id": 7, "weight_used": 1.0, "created_at": "2026-02-01 09:00:00"}
|
||||
]
|
||||
}
|
||||
archive = PrintArchive(filename="linked.3mf", file_path="", file_size=1)
|
||||
db_session.add(archive)
|
||||
await db_session.flush()
|
||||
|
||||
await _service()._restore_spools(db_session, inventory, usage, False, tally, {7: archive.id})
|
||||
await db_session.commit()
|
||||
|
||||
row = (await db_session.execute(select(SpoolUsageHistory))).scalar_one()
|
||||
assert row.archive_id == archive.id
|
||||
assert not any("print-history link" in note for note in tally.notes)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_dangling_printer_id_is_cleared(self, db_session):
|
||||
tally = _CategoryTally()
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { screen, waitFor } from '@testing-library/react';
|
||||
import userEvent from '@testing-library/user-event';
|
||||
import { http, HttpResponse } from 'msw';
|
||||
import { delay, http, HttpResponse } from 'msw';
|
||||
import { QueryClient, useQuery, useQueryClient } from '@tanstack/react-query';
|
||||
import { render } from '../utils';
|
||||
import { server } from '../mocks/server';
|
||||
@@ -198,6 +198,34 @@ describe('GitHubRestoreModal', () => {
|
||||
expect(screen.getByText('1 usage record(s) skipped')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('drops the selection while a newly-picked commit is still being inspected', async () => {
|
||||
// Switching commits keeps `selected` (it is only pruned once the new preview
|
||||
// lands), so the footer must not keep counting it: the categories belong to
|
||||
// the commit that was switched away from, and the user has not seen an item
|
||||
// count for the new one.
|
||||
let previewCalls = 0;
|
||||
server.use(
|
||||
http.get('/api/v1/github-backup/restore/preview', async () => {
|
||||
previewCalls += 1;
|
||||
// The second commit's preview never resolves, holding the modal in the
|
||||
// in-flight state the assertions below describe.
|
||||
if (previewCalls > 1) await delay('infinite');
|
||||
return HttpResponse.json(mockPreview as unknown as JsonBody);
|
||||
})
|
||||
);
|
||||
render(<GitHubRestoreModal onClose={vi.fn()} />);
|
||||
|
||||
const checkboxes = await waitFor(() => screen.getAllByRole('checkbox') as HTMLInputElement[]);
|
||||
await userEvent.click(checkboxes[1]);
|
||||
await waitFor(() => expect(screen.getByText('1 selected')).toBeInTheDocument());
|
||||
|
||||
await userEvent.selectOptions(screen.getByLabelText('Backup commit'), mockCommits.commits[1].sha);
|
||||
|
||||
await waitFor(() => expect(screen.getByText('Reading backup contents...')).toBeInTheDocument());
|
||||
expect(screen.getByText('0 selected')).toBeInTheDocument();
|
||||
expect(screen.getByRole('button', { name: /Restore$/ })).toBeDisabled();
|
||||
});
|
||||
|
||||
it('sends overwrite_existing when the toggle is on', async () => {
|
||||
let body: Record<string, unknown> | null = null;
|
||||
server.use(
|
||||
|
||||
@@ -74,11 +74,35 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
|
||||
// the one whose contents the user just approved.
|
||||
const resolvedRef = previewQuery.data?.success ? previewQuery.data.ref : selectedRef;
|
||||
|
||||
const availability = useMemo(() => {
|
||||
const map: Record<string, { available: boolean; itemCount: number; detail: string | null }> = {};
|
||||
previewQuery.data?.categories?.forEach((c) => {
|
||||
map[c.category] = { available: c.available, itemCount: c.item_count, detail: c.detail };
|
||||
});
|
||||
return map;
|
||||
}, [previewQuery.data]);
|
||||
|
||||
// What a Restore click would actually send. `selected` on its own is not that:
|
||||
// it survives a commit switch by design (the pruning effect below only runs
|
||||
// once the new preview lands), so between picking a commit and its preview
|
||||
// resolving, `selected` still describes the *previous* commit while the
|
||||
// checkbox list is replaced by a spinner. Counting it raw put "2 selected"
|
||||
// and an enabled Restore button under that spinner, and clicking restored the
|
||||
// new commit with the old commit's categories — none of which the user had
|
||||
// seen an item count for. Gating on availability, exactly as the checkboxes
|
||||
// do, empties the list until the preview says otherwise, which also disables
|
||||
// the button.
|
||||
const selectedCategories = useMemo(
|
||||
() => CATEGORIES.filter((c) => selected[c.id] && availability[c.id]?.available).map((c) => c.id),
|
||||
[selected, availability]
|
||||
);
|
||||
const selectedCount = selectedCategories.length;
|
||||
|
||||
const restoreMutation = useMutation({
|
||||
mutationFn: () =>
|
||||
api.restoreFromGitHub({
|
||||
ref: resolvedRef,
|
||||
categories: CATEGORIES.filter((c) => selected[c.id]).map((c) => c.id),
|
||||
categories: selectedCategories,
|
||||
overwrite_existing: overwriteExisting,
|
||||
}),
|
||||
onSuccess: (data) => {
|
||||
@@ -177,14 +201,6 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
|
||||
return () => window.removeEventListener('beforeunload', handler);
|
||||
}, [isRestoring]);
|
||||
|
||||
const availability = useMemo(() => {
|
||||
const map: Record<string, { available: boolean; itemCount: number; detail: string | null }> = {};
|
||||
previewQuery.data?.categories?.forEach((c) => {
|
||||
map[c.category] = { available: c.available, itemCount: c.item_count, detail: c.detail };
|
||||
});
|
||||
return map;
|
||||
}, [previewQuery.data]);
|
||||
|
||||
// Selecting a category that isn't in the newly-picked commit would send a
|
||||
// request the backend rejects, so drop those whenever the preview changes.
|
||||
useEffect(() => {
|
||||
@@ -198,7 +214,6 @@ export function GitHubRestoreModal({ onClose }: GitHubRestoreModalProps) {
|
||||
});
|
||||
}, [previewQuery.data, availability]);
|
||||
|
||||
const selectedCount = CATEGORIES.filter((c) => selected[c.id]).length;
|
||||
const commits = commitsQuery.data?.commits ?? [];
|
||||
|
||||
const formatCommitLabel = (sha: string, message: string, date: string) => {
|
||||
|
||||
Reference in New Issue
Block a user