From 6204789361a1c2c161f58faac0994a8c09c35515 Mon Sep 17 00:00:00 2001 From: nagisml Date: Thu, 1 Oct 2026 18:45:14 +0200 Subject: [PATCH 1/2] Fixing 955 --- src/opensak/db/database.py | 63 ++++++++++++++++--- src/opensak/db/db_settings.py | 46 ++++++++++++++ src/opensak/gui/dialogs/gsak_import_dialog.py | 26 ++++++++ src/opensak/gui/mainwindow.py | 34 ++++++---- src/opensak/gui/settings.py | 45 +++++++------ tests/unit-tests/test_db_settings_659.py | 48 ++++++++++++++ tests/unit-tests/test_distance.py | 35 +++++++++++ tests/unit-tests/test_gsak_import_dialog.py | 58 +++++++++++++++++ 8 files changed, 316 insertions(+), 39 deletions(-) diff --git a/src/opensak/db/database.py b/src/opensak/db/database.py index 520e3e65..eb201c73 100644 --- a/src/opensak/db/database.py +++ b/src/opensak/db/database.py @@ -1119,13 +1119,20 @@ def reload_caches_full(caches: list) -> list: # ── Distance recalculation ──────────────────────────────────────────────────── -def recalculate_distances(lat: float, lon: float) -> int: +def recalculate_distances(lat: float, lon: float, db_path: Path | None = None) -> int: """Recompute distance and bearing for every cache and persist to the DB. Called once whenever the active centre point changes (not on every table refresh). Uses distance_km_batch() which dispatches to Haversine or Vincenty depending on the user's distance_method setting. + *db_path* is the database the open engine points at, when that may not + be the active one — e.g. GsakImportWorker, which switches the engine to + each target database in turn. The centre used is then stored in that + file's own settings (see distances_up_to_date()); without it it goes to + the active database's settings, which while the engine is switched + would be the wrong database. + Returns the number of caches updated. """ from opensak.filters.engine import distance_km_batch @@ -1194,9 +1201,17 @@ def _scalar(la2: float, lo2: float) -> float: # startup if nothing has changed (issue #579). from opensak.gui.settings import get_settings s = get_settings() - s.dist_calc_lat = lat - s.dist_calc_lon = lon - s.dist_calc_method = s.distance_method + if db_path is not None: + from opensak.db import db_settings + db_settings.write_file(db_path, { + "dist_calc_lat": lat, + "dist_calc_lon": lon, + "dist_calc_method": s.distance_method, + }) + else: + s.dist_calc_lat = lat + s.dist_calc_lon = lon + s.dist_calc_method = s.distance_method logger.info( "recalculate_distances: done, %s caches updated (+%.2fs total)", @@ -1212,7 +1227,7 @@ def _scalar(la2: float, lo2: float) -> float: _DISTANCE_EPSILON_KM = 0.01 # 10 m — generous enough to absorb float rounding -def distances_up_to_date(lat: float, lon: float) -> bool: +def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) -> bool: """Check whether the persisted Cache.distance/bearing values are still valid for the given centre point, so a full recalculate_distances() call can be skipped on startup (issue #579). @@ -1225,6 +1240,11 @@ def distances_up_to_date(lat: float, lon: float) -> bool: modified outside this OpenSAK install (e.g. synced from another machine with a different home point, or edited by another tool) without a matching recalculation ever having run here. + 3. No cache with coordinates is missing its distance — e.g. caches a + merge import added after the last recalculation, which the single + spot-check row can't see. + + *db_path*: as for recalculate_distances(). Returns True if it's safe to skip the full recalculation. """ @@ -1248,9 +1268,22 @@ def distances_up_to_date(lat: float, lon: float) -> bool: return True s = get_settings() - calc_lat = s.dist_calc_lat - calc_lon = s.dist_calc_lon - calc_method = s.dist_calc_method + if db_path is not None: + from opensak.db import db_settings + values = { + k: db_settings.peek_value(db_path, k) + for k in ("dist_calc_lat", "dist_calc_lon", "dist_calc_method") + } + try: + calc_lat = float(values["dist_calc_lat"]) + calc_lon = float(values["dist_calc_lon"]) + except (TypeError, ValueError): + calc_lat = calc_lon = None + calc_method = values["dist_calc_method"] + else: + calc_lat = s.dist_calc_lat + calc_lon = s.dist_calc_lon + calc_method = s.dist_calc_method if calc_lat is None or calc_lon is None or calc_method is None: return False @@ -1263,7 +1296,19 @@ def distances_up_to_date(lat: float, lon: float) -> bool: if stored_distance is None: return False fresh_distance = distance_km(lat, lon, row_lat, row_lon) - return abs(fresh_distance - stored_distance) <= _DISTANCE_EPSILON_KM + if abs(fresh_distance - stored_distance) > _DISTANCE_EPSILON_KM: + return False + + # Stops at the first hit; only a database that is up to date pays for + # the full scan. + with get_session() as session: + missing = session.execute( + text( + "SELECT 1 FROM caches WHERE distance IS NULL " + "AND latitude IS NOT NULL AND longitude IS NOT NULL LIMIT 1" + ) + ).fetchone() + return missing is None # ── Health-check helper ─────────────────────────────────────────────────────── diff --git a/src/opensak/db/db_settings.py b/src/opensak/db/db_settings.py index d5c74b8d..be699ad7 100644 --- a/src/opensak/db/db_settings.py +++ b/src/opensak/db/db_settings.py @@ -208,6 +208,52 @@ def read_file(path: Path) -> dict[str, Any]: conn.close() +def peek_value(path: Path, key: str, default: Any = None) -> Any: + """ + Value of path-keyed setting *key* for the database file at *path*, as + get_value() would return it once that database is bound: its own table, + or — while its legacy settings haven't been imported yet — the legacy + opensak.json key that ensure_seeded() would import. Never seeds. + + For code that works on a database that may not be the active one, e.g. + an import running on a temporarily switched engine. + """ + values = read_file(path) if Path(path).is_file() else {} + if key in values: + return values[key] + if not values.get(_IMPORTED_KEY): + value = get_store().get(legacy_db_key(path, key)) + if value is not None and value != "": + return value + return default + + +def write_file(path: Path, values: dict[str, Any]) -> None: + """ + Store *values* in the database file at *path* (open or not). Safe to call + from a worker thread: the cached values of the bound database are + dropped, so the next read sees the new ones. + + Existing keys are replaced. A later ensure_seeded() never overwrites + them (it only inserts missing keys). Does nothing if the file doesn't + exist (it is never created here). + """ + if not Path(path).is_file(): + return + with _lock: + conn = sqlite3.connect(str(path), timeout=30) + try: + conn.execute(_DDL) + conn.executemany( + f"INSERT OR REPLACE INTO {TABLE} (key, value) VALUES (?, ?)", + [(k, json.dumps(v)) for k, v in values.items()], + ) + conn.commit() + finally: + conn.close() + _reset_cache() + + def _reset_cache() -> None: """Forget cached values; the next access reloads them (also for tests).""" global _cache_engine, _cache diff --git a/src/opensak/gui/dialogs/gsak_import_dialog.py b/src/opensak/gui/dialogs/gsak_import_dialog.py index 3cee69f4..b5de6384 100644 --- a/src/opensak/gui/dialogs/gsak_import_dialog.py +++ b/src/opensak/gui/dialogs/gsak_import_dialog.py @@ -23,6 +23,7 @@ from __future__ import annotations +import logging import shutil import tempfile from pathlib import Path @@ -41,6 +42,8 @@ from opensak.lang import tr from opensak.gui.theme import hint_style +logger = logging.getLogger(__name__) + # Table columns COL_GSAK, COL_SIZE, COL_TARGET, COL_STATUS = range(4) @@ -117,6 +120,7 @@ def run(self) -> None: progress_cb=lambda done, total: self.progress.emit(done, total), ) self.result_ready.emit(result) + self._update_distances(self.target_db_path or original_path) except Exception: import traceback self.error.emit(traceback.format_exc()) @@ -124,6 +128,28 @@ def run(self) -> None: if switched and original_path is not None: init_db(db_path=original_path) + @staticmethod + def _update_distances(db_path: Path | None) -> None: + """Bring the imported database's distances up to date while still in + the background, so neither the refresh after the import nor a later + switch to that database has to recalculate them on the GUI thread + (both still check, as a safety net). Runs while the engine points at + *db_path*, so its home point and the centre used are read from and + stored in that file, not in the active database's settings. A failure + here only means that check recalculates later — the import itself + succeeded. + """ + if db_path is None: + return + try: + from opensak.db.database import distances_up_to_date, recalculate_distances + lat, lon = get_settings().home_for_db_file(db_path) + if lat and lon and not distances_up_to_date(lat, lon, db_path=db_path): + recalculate_distances(lat, lon, db_path=db_path) + except Exception: + logger.warning("GSAK import: could not update distances for %s", + db_path, exc_info=True) + class GsakExtractWorker(QThread): """Unpacks the selected members of a GSAK backup .zip in the background. diff --git a/src/opensak/gui/mainwindow.py b/src/opensak/gui/mainwindow.py index 5ed56219..42b28030 100644 --- a/src/opensak/gui/mainwindow.py +++ b/src/opensak/gui/mainwindow.py @@ -1011,12 +1011,12 @@ def _on_database_switched(self, db_info) -> None: self._detail_panel.clear() self._load_sort_for_active_db() self._reload_home_combo() - # Den nye database kan have manglende/forældede distancer — fx en - # database som GSAK-backup-importen har udfyldt i baggrunden (kun - # den aktive database genberegnes i _refresh_after_import()), eller - # et andet hjemmepunkt gemt per database. Samme billige tjek som - # ved opstart (issue #579), så et skift normalt ikke koster en fuld - # genberegning. + # Den nye database kan have manglende/forældede distancer — fx et + # andet hjemmepunkt gemt per database, eller en database som + # GSAK-backup-importen fyldte, hvis GsakImportWorker ikke nåede at + # genberegne den i baggrunden (det gør den normalt — dette er kun + # et sikkerhedsnet). Samme billige tjek som ved opstart (issue + # #579), så et skift normalt ikke koster en fuld genberegning. s = get_settings() if s.home_lat and s.home_lon: from opensak.db.database import recalculate_distances, distances_up_to_date @@ -1894,7 +1894,11 @@ def _open_gsak_import_dialog(self) -> None: return from opensak.gui.dialogs.gsak_import_dialog import GsakImportDialog dlg = GsakImportDialog(self) - dlg.import_completed.connect(self._refresh_after_import) + # GsakImportWorker already brought the distances up to date in the + # background — only recalculate here if that didn't happen. + dlg.import_completed.connect( + lambda: self._refresh_after_import(distances_precomputed=True) + ) dlg.databases_changed.connect(self._reload_db_combo) dlg.filters_imported.connect(self._on_filter_profiles_imported) dlg.exec() @@ -1923,13 +1927,21 @@ def _open_pq_email_check_dialog(self) -> None: dlg.import_completed.connect(self._refresh_after_import) dlg.exec() - def _refresh_after_import(self) -> None: - """Reload both cache table and map after a successful import.""" + def _refresh_after_import(self, distances_precomputed: bool = False) -> None: + """Reload both cache table and map after a successful import. + + *distances_precomputed*: the import already recalculated distances + (GsakImportWorker), so only do it if distances_up_to_date() can't + confirm them. Other imports always recalculate — the cheap check + can't see changed coordinates of existing caches. + """ from opensak.gui.settings import get_settings s = get_settings() if s.home_lat and s.home_lon: - from opensak.db.database import recalculate_distances - recalculate_distances(s.home_lat, s.home_lon) + from opensak.db.database import recalculate_distances, distances_up_to_date + if not (distances_precomputed + and distances_up_to_date(s.home_lat, s.home_lon)): + recalculate_distances(s.home_lat, s.home_lon) self._refresh_cache_list() count = self._cache_table.row_count() self._statusbar.showMessage( diff --git a/src/opensak/gui/settings.py b/src/opensak/gui/settings.py index c1394c69..2a2c5e54 100644 --- a/src/opensak/gui/settings.py +++ b/src/opensak/gui/settings.py @@ -72,19 +72,22 @@ def _db_set(self, key: str, value: Any) -> None: # ── Home location (per database) ────────────────────────────────────────── - @property - def home_lat(self) -> float: - s = get_store() - val = self._db_get("home_lat") - if val is not None and val != "": + @staticmethod + def _home_coord(per_db: Any, global_key: str, default: float) -> float: + """Per-database home coordinate, else the global one, else *default*.""" + if per_db is not None and per_db != "": try: - return float(val) + return float(per_db) except (TypeError, ValueError): pass try: - return float(s.get("location.home_lat", 55.6761)) + return float(get_store().get(global_key, default)) except (TypeError, ValueError): - return 55.6761 + return default + + @property + def home_lat(self) -> float: + return self._home_coord(self._db_get("home_lat"), "location.home_lat", 55.6761) @home_lat.setter def home_lat(self, value: float) -> None: @@ -93,23 +96,27 @@ def home_lat(self, value: float) -> None: @property def home_lon(self) -> float: - s = get_store() - val = self._db_get("home_lon") - if val is not None and val != "": - try: - return float(val) - except (TypeError, ValueError): - pass - try: - return float(s.get("location.home_lon", 12.5683)) - except (TypeError, ValueError): - return 12.5683 + return self._home_coord(self._db_get("home_lon"), "location.home_lon", 12.5683) @home_lon.setter def home_lon(self, value: float) -> None: self._db_set("home_lon", value) get_store().set("location.home_lon", value) # default for new databases + def home_for_db_file(self, path) -> tuple[float, float]: + """ + (home_lat, home_lon) of the database file at *path* — what the two + properties above return once that database is the active one. For + work on a database that isn't active (yet), e.g. GsakImportWorker. + """ + from opensak.db import db_settings + return ( + self._home_coord(db_settings.peek_value(path, "home_lat"), + "location.home_lat", 55.6761), + self._home_coord(db_settings.peek_value(path, "home_lon"), + "location.home_lon", 12.5683), + ) + # ── Globale hjemmepunkter (liste) ───────────────────────────────────────── @property diff --git a/tests/unit-tests/test_db_settings_659.py b/tests/unit-tests/test_db_settings_659.py index f8742f28..9449c30a 100644 --- a/tests/unit-tests/test_db_settings_659.py +++ b/tests/unit-tests/test_db_settings_659.py @@ -327,3 +327,51 @@ def test_falls_back_without_an_open_database(self, manager): set_value("home_lat", "legacy.key", 6.5) assert get_value("home_lat", "legacy.key") == 6.5 assert get_store().get("legacy.key") == 6.5 + + +# ── Files that aren't (or may not be) the active database ─────────────────── + +class TestFileAccess: + """peek_value()/write_file(): used by GsakImportWorker for the database + the engine was switched to, which db_settings doesn't bind.""" + + def test_write_file_goes_to_that_file_only(self, manager, tmp_path): + other = _new_db(manager, "Other", tmp_path) + db_settings.write_file(other.path, {"dist_calc_lat": 47.1}) + assert read_file(other.path)["dist_calc_lat"] == 47.1 + assert get_value("dist_calc_lat", None) is None # active untouched + + def test_write_file_to_active_drops_the_cache(self, manager): + assert get_value("dist_calc_lat", None) is None # loads the cache + db_settings.write_file(manager.active.path, {"dist_calc_lat": 47.1}) + assert get_value("dist_calc_lat", None) == 47.1 + + def test_write_file_survives_seeding(self, manager, tmp_path): + path = _unopened_db(manager, tmp_path / "Old Trip.db") + get_store().set(legacy_db_key(path, "dist_calc_lat"), 60.5) + db_settings.write_file(path, {"dist_calc_lat": 47.1}) + ensure_seeded(path, "Old Trip") + assert read_file(path)["dist_calc_lat"] == 47.1 + + def test_missing_file_is_never_created(self, manager, tmp_path): + path = tmp_path / "nope.db" + db_settings.write_file(path, {"home_lat": 1.0}) + assert db_settings.peek_value(path, "home_lat", "dflt") == "dflt" + assert not path.exists() + + def test_peek_value_reads_the_file(self, manager, tmp_path): + other = _new_db(manager, "Other", tmp_path) + db_settings.write_file(other.path, {"home_lat": 47.1}) + assert db_settings.peek_value(other.path, "home_lat") == 47.1 + + def test_peek_value_sees_legacy_value_before_seeding(self, manager, tmp_path): + path = _unopened_db(manager, tmp_path / "Old Trip.db") + get_store().set(legacy_db_key(path, "home_lat"), 60.5) + assert db_settings.peek_value(path, "home_lat") == 60.5 + assert "db_settings" not in _tables(path) # peeking never seeds + + def test_peek_value_ignores_legacy_value_after_seeding(self, manager, tmp_path): + path = _unopened_db(manager, tmp_path / "Old Trip.db") + ensure_seeded(path, "Old Trip") + get_store().set(legacy_db_key(path, "home_lat"), 60.5) # set too late + assert db_settings.peek_value(path, "home_lat", "dflt") == "dflt" diff --git a/tests/unit-tests/test_distance.py b/tests/unit-tests/test_distance.py index c248ff93..7e452785 100644 --- a/tests/unit-tests/test_distance.py +++ b/tests/unit-tests/test_distance.py @@ -388,3 +388,38 @@ def test_distances_up_to_date_true_on_empty_db(tmp_path): db_path = tmp_path / "empty.db" init_db(db_path=db_path) assert distances_up_to_date(55.0, 12.0) is True + + +def test_distances_up_to_date_false_when_a_distance_is_missing(two_cache_db): + """A cache added after the last recalc (e.g. by a merge import) has no + distance yet. The spot-check row can't see it, so it's checked + separately.""" + from opensak.db.database import recalculate_distances, distances_up_to_date + home_lat, home_lon = 55.6761, 12.5683 + recalculate_distances(home_lat, home_lon) + with get_session() as s: + s.add(Cache(gc_code="GCAAA3", name="Gamma", cache_type="Traditional Cache", + latitude=55.0, longitude=12.0)) + assert distances_up_to_date(home_lat, home_lon) is False + + +# ── db_path: a database the open engine points at, not the active one ──────── + +def test_recalculate_distances_with_db_path_stores_centre_in_that_file(two_cache_db): + """GsakImportWorker recalculates a database the engine was switched to. + The centre must land in that file's settings, not the active database's.""" + from opensak.db import db_settings + from opensak.db.database import recalculate_distances, distances_up_to_date + from opensak.gui.settings import get_settings + home_lat, home_lon = 55.6761, 12.5683 + + assert distances_up_to_date(home_lat, home_lon, db_path=two_cache_db) is False + assert recalculate_distances(home_lat, home_lon, db_path=two_cache_db) == 2 + + stored = db_settings.read_file(two_cache_db) + assert stored["dist_calc_lat"] == home_lat + assert stored["dist_calc_lon"] == home_lon + assert stored["dist_calc_method"] == "haversine" + assert get_settings().dist_calc_lat is None # active settings untouched + assert distances_up_to_date(home_lat, home_lon, db_path=two_cache_db) is True + assert distances_up_to_date(52.5200, 13.4050, db_path=two_cache_db) is False diff --git a/tests/unit-tests/test_gsak_import_dialog.py b/tests/unit-tests/test_gsak_import_dialog.py index 0bfa685e..1fc1f2b8 100644 --- a/tests/unit-tests/test_gsak_import_dialog.py +++ b/tests/unit-tests/test_gsak_import_dialog.py @@ -41,6 +41,10 @@ def _patch_common(self, monkeypatch, active_path=Path("/active.db")): monkeypatch.setattr("opensak.db.manager.get_db_manager", lambda: SimpleNamespace(active_path=active_path)) monkeypatch.setattr("opensak.db.database.get_session", _fake_session) + updated = [] + monkeypatch.setattr(GsakImportWorker, "_update_distances", + staticmethod(updated.append)) + return updated def test_run_success_emits_result(self, monkeypatch): self._patch_common(monkeypatch) @@ -114,6 +118,31 @@ def test_run_replace_clears_target_first(self, monkeypatch): assert calls == ["clear", "import"] assert cleared == [7] + def test_run_updates_distances_of_target(self, monkeypatch): + updated = self._patch_common(monkeypatch, active_path=Path("/active.db")) + monkeypatch.setattr("opensak.importer.gsak_importer.import_gsak_db", + lambda path, session, progress_cb=None: _result()) + monkeypatch.setattr("opensak.db.database.init_db", lambda **k: None) + GsakImportWorker(Path("/gsak.db3"), target_db_path=Path("/other.db")).run() + GsakImportWorker(Path("/gsak.db3")).run() # no target → the active DB + assert updated == [Path("/other.db"), Path("/active.db")] + + def test_run_failed_import_does_not_update_distances(self, monkeypatch): + updated = self._patch_common(monkeypatch) + monkeypatch.setattr( + "opensak.importer.gsak_importer.import_gsak_db", + lambda *a, **k: (_ for _ in ()).throw(RuntimeError("boom")), + ) + GsakImportWorker(Path("/gsak.db3")).run() + assert updated == [] + + def test_update_distances_failure_is_only_logged(self, monkeypatch, caplog): + def boom(*a, **k): + raise RuntimeError("no distances") + monkeypatch.setattr("opensak.db.database.distances_up_to_date", boom) + GsakImportWorker._update_distances(Path("/other.db")) # must not raise + assert "could not update distances" in caplog.text + def test_run_without_replace_does_not_clear(self, monkeypatch): self._patch_common(monkeypatch) calls = [] @@ -392,6 +421,35 @@ def test_backup_creates_one_database_per_folder_and_cleans_up( assert not path.exists() and not path.parent.parent.exists() assert "'AllCH → AllCH'" in dlg._log.toPlainText() + def test_distances_are_calculated_for_each_target_database( + self, dlg, manager, tmp_path, qtbot): + """GsakImportWorker brings each target's distances up to date in the + background, with the centre stored in that database's own settings — + so switching to it later doesn't recalculate on the GUI thread.""" + from opensak.db import db_settings + from opensak.db.database import distances_up_to_date, init_db + from opensak.gui.settings import get_settings + active_before = db_settings.read_file(manager.active_path) + + dlg.set_path(_backup(tmp_path, {"AllCH": "GC1AAA"}, with_settings=False)) + _run(dlg, qtbot) + + path = _db_by_name(manager, "AllCH").path + lat, lon = get_settings().home_for_db_file(path) + conn = sqlite3.connect(path) + try: + distances = [r[0] for r in conn.execute("SELECT distance FROM caches")] + finally: + conn.close() + assert distances and None not in distances + assert db_settings.read_file(path)["dist_calc_lat"] == lat + assert db_settings.read_file(manager.active_path) == active_before + init_db(db_path=path) + try: + assert distances_up_to_date(lat, lon, db_path=path) + finally: + manager.ensure_active_initialised() + def test_only_ticked_databases_are_imported(self, dlg, manager, tmp_path, qtbot): dlg.set_path(_backup(tmp_path, {"A": "GC1AAA", "B": "GC2BBB"}, with_settings=False)) dlg._table.item(1, gdlg.COL_GSAK).setCheckState(Qt.CheckState.Unchecked) From 23f92d92eb41162da029495957a4806b46953b09 Mon Sep 17 00:00:00 2001 From: nagisml Date: Thu, 1 Oct 2026 20:51:56 +0200 Subject: [PATCH 2/2] fix mypy --- src/opensak/db/database.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/opensak/db/database.py b/src/opensak/db/database.py index eb201c73..06fe535a 100644 --- a/src/opensak/db/database.py +++ b/src/opensak/db/database.py @@ -1268,6 +1268,9 @@ def distances_up_to_date(lat: float, lon: float, db_path: Path | None = None) -> return True s = get_settings() + calc_lat: float | None + calc_lon: float | None + calc_method: str | None if db_path is not None: from opensak.db import db_settings values = {