diff --git a/CalculateEstimatedDates/CalculateEstimatedDates.gpr.py b/CalculateEstimatedDates/CalculateEstimatedDates.gpr.py index 1a86ecaeb..59cf33989 100644 --- a/CalculateEstimatedDates/CalculateEstimatedDates.gpr.py +++ b/CalculateEstimatedDates/CalculateEstimatedDates.gpr.py @@ -9,7 +9,7 @@ id="calculateestimateddates", name=_("Calculate Estimated Dates"), description=_("Calculates estimated dates for birth and death."), - version = '0.90.41', + version = '0.90.42', gramps_target_version="6.0", status=STABLE, # not yet tested with python 3 fname="CalculateEstimatedDates.py", diff --git a/CalculateEstimatedDates/CalculateEstimatedDates.py b/CalculateEstimatedDates/CalculateEstimatedDates.py index 7e2c166a2..297cdce6c 100644 --- a/CalculateEstimatedDates/CalculateEstimatedDates.py +++ b/CalculateEstimatedDates/CalculateEstimatedDates.py @@ -29,6 +29,7 @@ # python modules # #------------------------------------------------------------------------ +import logging import time #------------------------------------------------------------------------ @@ -43,6 +44,7 @@ from gramps.gen.lib import (Date, Event, EventType, EventRef, Source, Citation, Note, NoteType) from gramps.gen.db import DbTxn +from gramps.gen.errors import DatabaseError from gramps.gen.config import config from gramps.gen.display.name import displayer as name_displayer from gramps.gen.plug.report.utils import get_person_filters @@ -53,6 +55,8 @@ from gramps.gen.utils.alive import probably_alive_range from gramps.gen.datehandler import displayer as date_displayer from gramps.gen.const import GRAMPS_LOCALE as glocale + +LOG = logging.getLogger(__name__) try: _trans = glocale.get_addon_translator(__file__) except ValueError: @@ -250,140 +254,183 @@ def run(self): self.MAX_AGE_PROB_ALIVE = self.options.handler.options_dict['MAX_AGE_PROB_ALIVE'] self.AVG_GENERATION_GAP = self.options.handler.options_dict['AVG_GENERATION_GAP'] if remove_old: - with DbTxn("", self.db, batch=True) as self.trans: - self.db.disable_signals() - self.results_write(_("Removing old estimations... ")) - self.progress.set_pass((_("Removing '%s'...") % source_text), - num_people) - supdate = None - for person_handle in people: - self.progress.step() - pupdate = 0 - person = self.db.get_person_from_handle(person_handle) - birth_ref = person.get_birth_ref() - if birth_ref: - birth = self.db.get_event_from_handle(birth_ref.ref) - for citation_handle in birth.get_citation_list(): - citation = self.db.get_citation_from_handle(citation_handle) - source_handle = citation.get_reference_handle() - #print "birth handle:", source_handle - source = self.db.get_source_from_handle(source_handle) - if source: - if source.get_title() == source_text: - #print("birth event removed from:", - # person.gramps_id) - person.set_birth_ref(None) - person.remove_handle_references('Event',[birth_ref.ref]) - # remove note - note_list = birth.get_referenced_note_handles() - birth.remove_handle_references('Note', - [note_handle for (obj_type, note_handle) in note_list]) - for (obj_type, note_handle) in note_list: - self.db.remove_note(note_handle, self.trans) - self.db.remove_event(birth_ref.ref, self.trans) - self.db.remove_citation(citation_handle, - self.trans) - pupdate = 1 - supdate = source # found the source. - break - death_ref = person.get_death_ref() - if death_ref: - death = self.db.get_event_from_handle(death_ref.ref) - for citation_handle in death.get_citation_list(): - citation = self.db.get_citation_from_handle(citation_handle) - source_handle = citation.get_reference_handle() - #print "death handle:", source_handle - source = self.db.get_source_from_handle(source_handle) - if source: - if source.get_title() == source_text: - #print("death event removed from:", - # person.gramps_id) - person.set_death_ref(None) - person.remove_handle_references('Event',[death_ref.ref]) - # remove note - note_list = death.get_referenced_note_handles() - death.remove_handle_references('Note', - [note_handle for (obj_type, note_handle) in note_list]) - for (obj_type, note_handle) in note_list: - self.db.remove_note(note_handle, self.trans) - self.db.remove_event(death_ref.ref, self.trans) - self.db.remove_citation(citation_handle, - self.trans) - pupdate = 1 - supdate = source # found the source. - break - if pupdate == 1: - self.db.commit_person(person, self.trans) - if supdate: - self.db.remove_source(supdate.handle, self.trans) - self.results_write(_("done!\n")) - self.db.enable_signals() + skipped = 0 + try: + with DbTxn("", self.db, batch=True) as self.trans: + self.db.disable_signals() + self.results_write(_("Removing old estimations... ")) + self.progress.set_pass((_("Removing '%s'...") % source_text), + num_people) + supdate = None + for person_handle in people: + self.progress.step() + try: + pupdate = 0 + person = self.db.get_person_from_handle(person_handle) + birth_ref = person.get_birth_ref() + if birth_ref: + birth = self.db.get_event_from_handle(birth_ref.ref) + for citation_handle in birth.get_citation_list(): + citation = self.db.get_citation_from_handle(citation_handle) + source_handle = citation.get_reference_handle() + #print "birth handle:", source_handle + source = self.db.get_source_from_handle(source_handle) + if source: + if source.get_title() == source_text: + #print("birth event removed from:", + # person.gramps_id) + person.set_birth_ref(None) + person.remove_handle_references('Event',[birth_ref.ref]) + # remove note + note_list = birth.get_referenced_note_handles() + birth.remove_handle_references('Note', + [note_handle for (obj_type, note_handle) in note_list]) + for (obj_type, note_handle) in note_list: + self.db.remove_note(note_handle, self.trans) + self.db.remove_event(birth_ref.ref, self.trans) + self.db.remove_citation(citation_handle, + self.trans) + pupdate = 1 + supdate = source # found the source. + break + death_ref = person.get_death_ref() + if death_ref: + death = self.db.get_event_from_handle(death_ref.ref) + for citation_handle in death.get_citation_list(): + citation = self.db.get_citation_from_handle(citation_handle) + source_handle = citation.get_reference_handle() + #print "death handle:", source_handle + source = self.db.get_source_from_handle(source_handle) + if source: + if source.get_title() == source_text: + #print("death event removed from:", + # person.gramps_id) + person.set_death_ref(None) + person.remove_handle_references('Event',[death_ref.ref]) + # remove note + note_list = death.get_referenced_note_handles() + death.remove_handle_references('Note', + [note_handle for (obj_type, note_handle) in note_list]) + for (obj_type, note_handle) in note_list: + self.db.remove_note(note_handle, self.trans) + self.db.remove_event(death_ref.ref, self.trans) + self.db.remove_citation(citation_handle, + self.trans) + pupdate = 1 + supdate = source # found the source. + break + if pupdate == 1: + self.db.commit_person(person, self.trans) + except Exception: + skipped += 1 + LOG.warning( + "Failed to remove estimated dates for person" + " handle %s; skipping.", + person_handle, exc_info=True) + continue + if supdate: + try: + self.db.remove_source(supdate.handle, self.trans) + except Exception: + LOG.warning( + "Failed to remove estimated-dates source;" + " continuing.", exc_info=True) + self.results_write(_("done!\n")) + if skipped: + self.results_write( + _("Skipped %d people due to errors (see log).\n") + % skipped) + finally: + self.db.enable_signals() self.db.request_rebuild() if add_birth or add_death: self.results_write(_("Selecting... \n\n")) self.progress.set_pass(_('Selecting...'), num_people) row = 0 + skipped = 0 for person_handle in people: self.progress.step() - person = self.db.get_person_from_handle(person_handle) - birth_ref = person.get_birth_ref() - death_ref = person.get_death_ref() - add_birth_event, add_death_event = False, False - if not birth_ref or not death_ref: - date1, date2, explain, other = self.calc_estimates(person) - if birth_ref: - ev = self.db.get_event_from_handle(birth_ref.ref) - date1 = ev.get_date_object() - elif not birth_ref and add_birth and date1: - if date1.match( current_date, "<"): - add_birth_event = True - date1.make_vague() + try: + person = self.db.get_person_from_handle(person_handle) + birth_ref = person.get_birth_ref() + death_ref = person.get_death_ref() + add_birth_event, add_death_event = False, False + if not birth_ref or not death_ref: + try: + date1, date2, explain, other = \ + self.calc_estimates(person) + except DatabaseError as err: + skipped += 1 + LOG.warning( + "Could not estimate dates for %s (%s): %s" + " — skipping.", + name_displayer.display(person), + person.gramps_id, err) + continue + if birth_ref: + ev = self.db.get_event_from_handle(birth_ref.ref) + date1 = ev.get_date_object() + elif not birth_ref and add_birth and date1: + if date1.match( current_date, "<"): + add_birth_event = True + date1.make_vague() + else: + date1 = Date() else: date1 = Date() - else: - date1 = Date() - if death_ref: - ev = self.db.get_event_from_handle(death_ref.ref) - date2 = ev.get_date_object() - elif not death_ref and add_death and date2: - if date2.match( current_date, "<"): - add_death_event = True - date2.make_vague() + if death_ref: + ev = self.db.get_event_from_handle(death_ref.ref) + date2 = ev.get_date_object() + elif not death_ref and add_death and date2: + if date2.match( current_date, "<"): + add_death_event = True + date2.make_vague() + else: + date2 = Date() else: date2 = Date() - else: - date2 = Date() - # Describe - if add_birth_event and add_death_event: - action = _("Add birth and death events") - elif add_birth_event: - action = _("Add birth event") - elif add_death_event: - action = _("Add death event") - else: - continue - #stab.columns(_("Select"), _("Person"), _("Action"), - # _("Birth Date"), _("Death Date"), _("Evidence"), _("Relative")) - if add_birth == 1 and not birth_ref: # no date - date1 = Date() - if add_death == 1 and not death_ref: # no date - date2 = Date() - if person == other: - other = None - stab.row("checkbox", - person, - action, - date1, - date2, - explain or "", - other or "") - if add_birth_event: - stab.set_cell_markup(3, row, "%s" % date_displayer.display(date1)) - if add_death_event: - stab.set_cell_markup(4, row, "%s" % date_displayer.display(date2)) - self.action[person.handle] = (add_birth_event, add_death_event) - row += 1 + # Describe + if add_birth_event and add_death_event: + action = _("Add birth and death events") + elif add_birth_event: + action = _("Add birth event") + elif add_death_event: + action = _("Add death event") + else: + continue + #stab.columns(_("Select"), _("Person"), _("Action"), + # _("Birth Date"), _("Death Date"), _("Evidence"), _("Relative")) + if add_birth == 1 and not birth_ref: # no date + date1 = Date() + if add_death == 1 and not death_ref: # no date + date2 = Date() + if person == other: + other = None + stab.row("checkbox", + person, + action, + date1, + date2, + explain or "", + other or "") + if add_birth_event: + stab.set_cell_markup(3, row, "%s" % date_displayer.display(date1)) + if add_death_event: + stab.set_cell_markup(4, row, "%s" % date_displayer.display(date2)) + self.action[person.handle] = (add_birth_event, add_death_event) + row += 1 + except Exception: + skipped += 1 + LOG.warning( + "Unexpected error processing person handle %s;" + " skipping.", + person_handle, exc_info=True) + continue + if skipped: + self.results_write( + _("Skipped %d people due to errors (see log).\n") + % skipped) if row > 0: self.results_write(" ") for text, function in BUTTONS: @@ -430,75 +477,90 @@ def apply_selection(self, *args, **kwargs): # Do not add birth or death event if one exists, no matter what if self.table.treeview.get_model() is None: return - with DbTxn("", self.db, batch=True) as self.trans: - self.pre_run() - source_text = self.options.handler.options_dict['source_text'] - select_col = self.table.model_index_of_column[_("Select")] - source = self.get_or_create_source(source_text) - self.db.disable_signals() - self.results_write(_("Selecting... ")) - self.progress.set_pass((_("Adding events '%s'...") % source_text), - len(self.table.treeview.get_model())) - count = 0 - for row in self.table.treeview.get_model(): - self.progress.step() - select = row[select_col] # live select value - if not select: - continue - pupdate = False - index = row[0] # order put in - row_data = self.table.get_raw_data(index) - person = row_data[1] # check, person, action, date1, date2 - date1 = row_data[3] # date - date2 = row_data[4] # date - evidence = row_data[5] # evidence - other = row_data[6] # other person - if other: - other_name = self.sdb.name(other) - else: - other_name = None - add_birth_event, add_death_event = self.action[person.handle] - birth_ref = person.get_birth_ref() - death_ref = person.get_death_ref() - if not birth_ref and add_birth_event: - if other_name: - explanation = _("Added birth event based on %(evidence)s, from %(name)s") % { - 'evidence' : evidence, 'name' : other_name } - else: - explanation = _("Added birth event based on %s") % evidence - modifier = self.get_modifier("birth") - birth = self.create_event(_("Estimated birth date"), - EventType.BIRTH, - date1, source, explanation, modifier) - event_ref = EventRef() - event_ref.set_reference_handle(birth.get_handle()) - person.set_birth_ref(event_ref) - pupdate = True - count += 1 - if not death_ref and add_death_event: - if other_name: - explanation = _("Added death event based on %(evidence)s, from %(person)s") % { - 'evidence' : evidence, 'person' : other_name } - else: - explanation = _("Added death event based on %s") % evidence - modifier = self.get_modifier("death") - death = self.create_event(_("Estimated death date"), - EventType.DEATH, - date2, source, explanation, modifier) - event_ref = EventRef() - event_ref.set_reference_handle(death.get_handle()) - person.set_death_ref(event_ref) - pupdate = True - count += 1 - if pupdate: - self.db.commit_person(person, self.trans) - self.results_write(_(" Done! Committing...")) - self.results_write("\n") - self.db.enable_signals() + count = 0 + skipped = 0 + try: + with DbTxn("", self.db, batch=True) as self.trans: + self.pre_run() + source_text = self.options.handler.options_dict['source_text'] + select_col = self.table.model_index_of_column[_("Select")] + source = self.get_or_create_source(source_text) + self.db.disable_signals() + self.results_write(_("Selecting... ")) + self.progress.set_pass( + (_("Adding events '%s'...") % source_text), + len(self.table.treeview.get_model())) + for row in self.table.treeview.get_model(): + self.progress.step() + select = row[select_col] # live select value + if not select: + continue + try: + pupdate = False + index = row[0] # order put in + row_data = self.table.get_raw_data(index) + person = row_data[1] # check, person, action, date1, date2 + date1 = row_data[3] # date + date2 = row_data[4] # date + evidence = row_data[5] # evidence + other = row_data[6] # other person + if other: + other_name = self.sdb.name(other) + else: + other_name = None + add_birth_event, add_death_event = self.action[person.handle] + birth_ref = person.get_birth_ref() + death_ref = person.get_death_ref() + if not birth_ref and add_birth_event: + if other_name: + explanation = _("Added birth event based on %(evidence)s, from %(name)s") % { + 'evidence' : evidence, 'name' : other_name } + else: + explanation = _("Added birth event based on %s") % evidence + modifier = self.get_modifier("birth") + birth = self.create_event(_("Estimated birth date"), + EventType.BIRTH, + date1, source, explanation, modifier) + event_ref = EventRef() + event_ref.set_reference_handle(birth.get_handle()) + person.set_birth_ref(event_ref) + pupdate = True + count += 1 + if not death_ref and add_death_event: + if other_name: + explanation = _("Added death event based on %(evidence)s, from %(person)s") % { + 'evidence' : evidence, 'person' : other_name } + else: + explanation = _("Added death event based on %s") % evidence + modifier = self.get_modifier("death") + death = self.create_event(_("Estimated death date"), + EventType.DEATH, + date2, source, explanation, modifier) + event_ref = EventRef() + event_ref.set_reference_handle(death.get_handle()) + person.set_death_ref(event_ref) + pupdate = True + count += 1 + if pupdate: + self.db.commit_person(person, self.trans) + except Exception: + skipped += 1 + LOG.warning( + "Failed to apply estimated dates for row %s;" + " skipping.", + row[0] if row else "?", exc_info=True) + continue + self.results_write(_(" Done! Committing...")) + self.results_write("\n") + finally: + self.db.enable_signals() + self.progress.close() self.db.request_rebuild() self.results_write(_("Added %d events.") % count) + if skipped: + self.results_write( + _(" (Skipped %d rows due to errors; see log.)") % skipped) self.results_write("\n\n") - self.progress.close() def get_modifier(self, event_type): setting = self.options.handler.options_dict['dates'] diff --git a/CalculateEstimatedDates/tests/test_calculate_estimated_dates.py b/CalculateEstimatedDates/tests/test_calculate_estimated_dates.py new file mode 100644 index 000000000..e318faeb7 --- /dev/null +++ b/CalculateEstimatedDates/tests/test_calculate_estimated_dates.py @@ -0,0 +1,262 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +""" +Tests for the Calculate Estimated Dates tool — lock in the error +handling added in response to ``gramps-project/bugs#7898`` (ancestry +loops surfaced as ``DatabaseError`` from ``probably_alive_range`` were +tearing down the whole tool). + +The tool's ``__init__`` pulls in the full Gramps GUI stack, so these +tests build stub instances via ``__new__`` and exercise the isolated +helpers plus the ``.gpr.py`` registration. +""" + +# ------------------------ +# Python modules +# ------------------------ +import os +import sys +import unittest +from typing import Any +from unittest import mock + +# The addon imports Gtk at module load — skip cleanly if gi/Gtk are not +# available. On systems where both GTK3 and GTK4 are present, pin Gtk to +# 3.0 before any gramps import (mirrors what gramps.grampsapp does at +# startup); otherwise PyGObject loads GTK4 and the gramps.gui import +# chain crashes on Gtk.IconSize.MENU (a GTK3-only enum). +try: + import gi + + gi.require_version("Gtk", "3.0") + gi.require_version("Gdk", "3.0") +except (ImportError, ValueError, AttributeError) as err: + raise unittest.SkipTest("GTK 3.0 / PyGObject not available: %s" % err) + +# ------------------------ +# Gramps modules +# ------------------------ +# addons-source/ goes on sys.path so ``from CalculateEstimatedDates import +# CalculateEstimatedDates`` resolves package→submodule. With the addon +# directory itself on sys.path instead, ``CalculateEstimatedDates`` binds +# to ``CalculateEstimatedDates.py`` directly, and the submodule lookup +# fails. ADDON_DIR is retained for the ``.gpr.py`` path in the registration +# test. +ADDON_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +ADDONS_SOURCE_DIR = os.path.dirname(ADDON_DIR) +if ADDONS_SOURCE_DIR not in sys.path: + sys.path.insert(0, ADDONS_SOURCE_DIR) + +try: + import gramps +except ImportError as err: + raise unittest.SkipTest("gramps package not available: %s" % err) + +if "GRAMPS_RESOURCES" not in os.environ: + os.environ["GRAMPS_RESOURCES"] = os.path.dirname( + os.path.dirname(gramps.__file__) + ) + +from gramps.gen.errors import DatabaseError # noqa: E402 +from gramps.gen.lib import Date # noqa: E402 + +# ------------------------ +# Gramps specific +# ------------------------ +# The addon module pulls in the full Gramps GUI stack at import time. On +# environments where GTK is missing or version-mismatched the import +# fails; skip the whole module cleanly in that case so collection does +# not surface spurious errors. +try: + from CalculateEstimatedDates import CalculateEstimatedDates as ced_module +except Exception as err: # noqa: BLE001 — environment guard + raise unittest.SkipTest( + "CalculateEstimatedDates module unavailable: %s" % err + ) + + +# ------------------------------------------------------------ +# +# _FakeOptionsHandler +# +# ------------------------------------------------------------ +class _FakeOptionsHandler: + """Stub for ``CalcToolManagedWindow.options.handler`` — exposes only + ``options_dict``, which is all the paths under test read.""" + + def __init__(self, dates: int = 0) -> None: + """ + :param dates: Value stored under ``options_dict["dates"]``. + :type dates: int + """ + self.options_dict: dict[str, int] = {"dates": dates} + + +# ------------------------------------------------------------ +# +# _FakeOptions +# +# ------------------------------------------------------------ +class _FakeOptions: + """Stub for ``CalcToolManagedWindow.options`` with a ``handler`` attribute.""" + + def __init__(self, dates: int = 0) -> None: + """ + :param dates: Value propagated into the handler's ``options_dict``. + :type dates: int + """ + self.handler = _FakeOptionsHandler(dates=dates) + + +def _make_tool(dates: int = 0) -> Any: + """ + Build a ``CalcToolManagedWindow`` via ``__new__`` so its ``__init__`` + (which pulls in GTK) does not run, then attach just enough state for + the isolated helpers to execute. + + :param dates: Value stored on the fake options handler. + :type dates: int + :returns: A stub instance with the minimum attributes set. + :rtype: CalcToolManagedWindow + """ + cls = ced_module.CalcToolManagedWindow + stub = cls.__new__(cls) + stub.options = _FakeOptions(dates=dates) + stub.db = object() + stub.MAX_SIB_AGE_DIFF = 20 + stub.MAX_AGE_PROB_ALIVE = 110 + stub.AVG_GENERATION_GAP = 20 + return stub + + +# ------------------------------------------------------------ +# +# TestGetModifier +# +# ------------------------------------------------------------ +class TestGetModifier(unittest.TestCase): + """Pure-logic coverage for ``get_modifier`` across its four branches.""" + + def test_birth_about_when_dates_zero(self) -> None: + """dates=0 + birth → MOD_ABOUT (the 'approximate' case).""" + tool = _make_tool(dates=0) + self.assertEqual(tool.get_modifier("birth"), Date.MOD_ABOUT) + + def test_birth_after_when_dates_nonzero(self) -> None: + """dates=1 + birth → MOD_AFTER (the 'extremes' case).""" + tool = _make_tool(dates=1) + self.assertEqual(tool.get_modifier("birth"), Date.MOD_AFTER) + + def test_death_about_when_dates_zero(self) -> None: + """dates=0 + death → MOD_ABOUT.""" + tool = _make_tool(dates=0) + self.assertEqual(tool.get_modifier("death"), Date.MOD_ABOUT) + + def test_death_before_when_dates_nonzero(self) -> None: + """dates=1 + death → MOD_BEFORE (upper-bound estimate).""" + tool = _make_tool(dates=1) + self.assertEqual(tool.get_modifier("death"), Date.MOD_BEFORE) + + +# ------------------------------------------------------------ +# +# TestCalcEstimates +# +# ------------------------------------------------------------ +class TestCalcEstimates(unittest.TestCase): + """Regression coverage for bug 7898 — ``calc_estimates`` must let + ``DatabaseError`` from ``probably_alive_range`` escape so the + per-person handler in ``run()`` can log and skip instead of tearing + down the whole tool.""" + + def test_returns_probably_alive_range_result(self) -> None: + """Happy path — the helper is a pass-through to ``probably_alive_range``.""" + tool = _make_tool() + person = object() + expected = (Date(), Date(), "explain", None) + calls: list[tuple] = [] + + def _fake(person_arg: Any, db_arg: Any, max_sib: int, max_age: int, avg_gap: int) -> tuple: + calls.append((person_arg, db_arg, max_sib, max_age, avg_gap)) + return expected + + with mock.patch.object(ced_module, "probably_alive_range", _fake): + result = tool.calc_estimates(person) + + self.assertEqual(result, expected) + self.assertEqual(calls, [(person, tool.db, 20, 110, 20)]) + + def test_propagates_database_error(self) -> None: + """ + When ``probably_alive_range`` raises ``DatabaseError`` (e.g. an + ancestry loop), ``calc_estimates`` must let it escape so the + per-person handler in ``run()`` can log and skip. + """ + tool = _make_tool() + + def _boom(*_args: Any, **_kwargs: Any) -> Any: + raise DatabaseError("loop in Test, Abel's descendants") + + with mock.patch.object(ced_module, "probably_alive_range", _boom): + with self.assertRaisesRegex(DatabaseError, "loop"): + tool.calc_estimates(object()) + + +# ------------------------------------------------------------ +# +# TestGprRegistration +# +# ------------------------------------------------------------ +class TestGprRegistration(unittest.TestCase): + """Catch metadata breakage in the plugin registration file early.""" + + def test_gpr_registration_metadata(self) -> None: + """The .gpr.py file must register a single TOOL with expected keys.""" + gpr_path = os.path.join(ADDON_DIR, "CalculateEstimatedDates.gpr.py") + calls: list[tuple[tuple, dict]] = [] + + namespace: dict[str, Any] = { + "register": lambda *args, **kwargs: calls.append((args, kwargs)), + "TOOL": "TOOL", + "STABLE": "STABLE", + "UNSTABLE": "UNSTABLE", + "TOOL_DBPROC": "TOOL_DBPROC", + "TOOL_MODE_GUI": "TOOL_MODE_GUI", + "_": lambda s: s, + } + with open(gpr_path, encoding="utf-8") as handle: + exec(compile(handle.read(), gpr_path, "exec"), namespace) + + self.assertEqual(len(calls), 1, "expected exactly one register() call") + args, kwargs = calls[0] + self.assertEqual(args, ("TOOL",)) + self.assertEqual(kwargs["id"], "calculateestimateddates") + self.assertEqual(kwargs["fname"], "CalculateEstimatedDates.py") + self.assertEqual(kwargs["gramps_target_version"], "6.0") + self.assertEqual(kwargs["status"], "STABLE") + self.assertEqual(kwargs["toolclass"], "CalcToolManagedWindow") + self.assertEqual(kwargs["optionclass"], "CalcEstDateOptions") + self.assertEqual(kwargs["category"], "TOOL_DBPROC") + self.assertEqual(kwargs["tool_modes"], ["TOOL_MODE_GUI"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/DataEntryGramplet/DataEntryGramplet.gpr.py b/DataEntryGramplet/DataEntryGramplet.gpr.py index 50308593f..dc61a1bab 100644 --- a/DataEntryGramplet/DataEntryGramplet.gpr.py +++ b/DataEntryGramplet/DataEntryGramplet.gpr.py @@ -14,7 +14,7 @@ gramplet_title=_("Data Entry"), detached_width=510, detached_height=480, - version = '1.0.52', + version = '1.0.53', gramps_target_version="6.0", status=STABLE, audience=EXPERT, diff --git a/DataEntryGramplet/DataEntryGramplet.py b/DataEntryGramplet/DataEntryGramplet.py index e00a4e771..bb3964055 100644 --- a/DataEntryGramplet/DataEntryGramplet.py +++ b/DataEntryGramplet/DataEntryGramplet.py @@ -423,6 +423,11 @@ def make_person(self, firstname, surname, gender): def save_data_edit(self, obj): if self._dirty: + if not self.dbstate.is_open(): + from gramps.gui.dialog import ErrorDialog + ErrorDialog(_("No Family Tree is open."), + _("Please open a Family Tree to edit data.")) + return # Save the edits ---------------------------------- person = self._dirty_person # First, get the data: @@ -498,6 +503,10 @@ def add_source(self, obj, source): def add_data_entry(self, obj): from gramps.gui.dialog import ErrorDialog + if not self.dbstate.is_open(): + ErrorDialog(_("No Family Tree is open."), + _("Please open a Family Tree before adding a person.")) + return # First, get the data: if "," in self.de_widgets["NPName"].get_text(): surname, firstname = self.de_widgets["NPName"].get_text().split(",", 1) diff --git a/DataEntryGramplet/tests/test_data_entry_gramplet.py b/DataEntryGramplet/tests/test_data_entry_gramplet.py new file mode 100644 index 000000000..72005d553 --- /dev/null +++ b/DataEntryGramplet/tests/test_data_entry_gramplet.py @@ -0,0 +1,329 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +""" +Tests for the Data Entry Gramplet — covers +``gramps-project/gramps#12691`` (``AttributeError: 'DummyDb' object +has no attribute 'get_undodb'`` when the user presses *Add* or *Save* +without a Family Tree loaded). + +The Gramplet subclass requires a live Gramps GUI to instantiate, so +these tests build a minimal stub via ``__new__`` and invoke the +mutating callbacks directly. That keeps the tests fast and avoids +spinning up GTK, while still exercising the real guard code paths. +""" + +# ------------------------ +# Python modules +# ------------------------ +import os +import sys +import unittest +from typing import Any +from unittest import mock + +# The addon imports Gtk at module load — skip cleanly if gi/Gtk are not +# available. On systems where both GTK3 and GTK4 are present, pin Gtk to +# 3.0 before any gramps import (mirrors what gramps.grampsapp does at +# startup); otherwise PyGObject loads GTK4 and the gramps.gui import +# chain crashes on Gtk.IconSize.MENU (a GTK3-only enum). +try: + import gi + + gi.require_version("Gtk", "3.0") + gi.require_version("Gdk", "3.0") +except (ImportError, ValueError, AttributeError) as err: + raise unittest.SkipTest("GTK 3.0 / PyGObject not available: %s" % err) + +# ------------------------ +# Gramps modules +# ------------------------ +# Addon root goes on sys.path so ``from DataEntryGramplet.DataEntryGramplet +# import DataEntryGramplet`` resolves the class inside the addon module. +# The fully-qualified form matters: when unittest loads this file as +# ``DataEntryGramplet.tests.test_...``, the outer ``DataEntryGramplet`` +# is already a namespace package in ``sys.modules``, so a bare +# ``from DataEntryGramplet import DataEntryGramplet`` would bind the +# submodule instead of the class. The ``tests/`` directory lacks an +# __init__.py, so this ``__file__``-based hack is still the right way +# to make the addon importable during local and CI runs. +ADDON_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if ADDON_DIR not in sys.path: + sys.path.insert(0, ADDON_DIR) + +try: + import gramps +except ImportError as err: + raise unittest.SkipTest("gramps package not available: %s" % err) + +if "GRAMPS_RESOURCES" not in os.environ: + os.environ["GRAMPS_RESOURCES"] = os.path.dirname( + os.path.dirname(gramps.__file__) + ) + +# ------------------------ +# Gramps specific +# ------------------------ +# The addon module pulls in the full Gramps GUI stack at import time. On +# environments where GTK is missing or version-mismatched the import +# fails; skip the whole module cleanly in that case so collection does +# not surface spurious errors. +try: + import gramps.gui.dialog as gramps_dialog # noqa: E402 + from DataEntryGramplet.DataEntryGramplet import DataEntryGramplet # noqa: E402 +except Exception as err: # noqa: BLE001 — environment guard + raise unittest.SkipTest("DataEntryGramplet module unavailable: %s" % err) + + +# ------------------------------------------------------------ +# +# _FakeDbState +# +# ------------------------------------------------------------ +class _FakeDbState: + """Stub for ``Gramplet.dbstate`` — only ``is_open()`` is consulted + by the guards under test.""" + + def __init__(self, is_open: bool = True) -> None: + """ + :param is_open: Value returned from ``is_open()``. + :type is_open: bool + """ + self._is_open = is_open + + def is_open(self) -> bool: + """Return the configured open state.""" + return self._is_open + + +# ------------------------------------------------------------ +# +# _FakeEntry +# +# ------------------------------------------------------------ +class _FakeEntry: + """Stub for ``Gtk.Entry`` — exposes only ``get_text()``.""" + + def __init__(self, text: str = "") -> None: + """ + :param text: Value returned from ``get_text()``. + :type text: str + """ + self._text = text + + def get_text(self) -> str: + """Return the configured text.""" + return self._text + + +# ------------------------------------------------------------ +# +# _FakeCombo +# +# ------------------------------------------------------------ +class _FakeCombo: + """Stub for ``Gtk.ComboBox`` — exposes only ``get_active()``.""" + + def __init__(self, active: int = 0) -> None: + """ + :param active: Value returned from ``get_active()``. + :type active: int + """ + self._active = active + + def get_active(self) -> int: + """Return the configured active index.""" + return self._active + + +def _make_gramplet( + *, + db_open: bool = True, + np_name: str = "", + np_gender: int = 2, # UNKNOWN + np_relation: int = DataEntryGramplet.NO_REL, + dirty: bool = False, +) -> Any: + """ + Build a ``DataEntryGramplet`` via ``__new__`` so its ``__init__`` + (which pulls in GTK) does not run, then attach just enough state + for the isolated guards to execute. + + :param db_open: Whether ``dbstate.is_open()`` reports an open tree. + :type db_open: bool + :param np_name: Value stored in the Name entry. + :type np_name: str + :param np_gender: Value stored in the Gender combo (UNKNOWN by default). + :type np_gender: int + :param np_relation: Value stored in the Relation combo. + :type np_relation: int + :param dirty: Value of the private ``_dirty`` flag. + :type dirty: bool + :returns: A stub instance with the minimum attributes set. + :rtype: DataEntryGramplet + """ + stub = DataEntryGramplet.__new__(DataEntryGramplet) + stub.dbstate = _FakeDbState(db_open) + stub._dirty = dirty + stub._dirty_person = None + stub.de_widgets = { + "NPName": _FakeEntry(np_name), + "NPGender": _FakeCombo(np_gender), + "NPRelation": _FakeCombo(np_relation), + } + # save_data_edit falls through to self.update() even on the guarded path. + stub.update = lambda: None + stub.get_active_object = lambda _type: None + return stub + + +# ------------------------------------------------------------ +# +# _ErrorDialogTestCase +# +# ------------------------------------------------------------ +class _ErrorDialogTestCase(unittest.TestCase): + """Base class that patches ``gramps.gui.dialog.ErrorDialog`` so + tests can inspect calls without opening any GTK dialogs.""" + + def setUp(self) -> None: + """Install the ErrorDialog capture and reset the buffer.""" + self.captured_errors: list[tuple[str, str]] = [] + + def _fake(title: Any, body: Any = "", *_args: Any, **_kwargs: Any) -> None: + self.captured_errors.append((str(title), str(body))) + + patcher = mock.patch.object(gramps_dialog, "ErrorDialog", _fake) + patcher.start() + self.addCleanup(patcher.stop) + + +# ------------------------------------------------------------ +# +# TestBug12691ClosedDb +# +# ------------------------------------------------------------ +class TestBug12691ClosedDb(_ErrorDialogTestCase): + """Regression coverage for bug 12691 — pressing *Add* or *Save* + with no tree open must surface an ErrorDialog instead of crashing + inside ``DbTxn`` with ``AttributeError: 'DummyDb' ... get_undodb``.""" + + def test_add_data_entry_with_closed_db_shows_error(self) -> None: + """*Add* with no tree open surfaces an ErrorDialog, not a crash.""" + stub = _make_gramplet(db_open=False, np_name="Doe, Jane") + + stub.add_data_entry(None) + + self.assertTrue(self.captured_errors, "ErrorDialog was not displayed") + title, body = self.captured_errors[0] + self.assertIn("Family Tree", title) + self.assertIn("open", body.lower()) + + def test_save_data_edit_with_closed_db_shows_error(self) -> None: + """*Save* while dirty with no tree open must not invoke DbTxn.""" + stub = _make_gramplet(db_open=False, dirty=True) + + stub.save_data_edit(None) + + self.assertTrue(self.captured_errors, "ErrorDialog was not displayed") + title, _body = self.captured_errors[0] + self.assertIn("Family Tree", title) + + def test_save_data_edit_noop_when_not_dirty(self) -> None: + """A *Save* click with nothing pending should be a silent no-op.""" + stub = _make_gramplet(db_open=True, dirty=False) + + stub.save_data_edit(None) + + self.assertEqual(self.captured_errors, []) + self.assertFalse(stub._dirty) + + +# ------------------------------------------------------------ +# +# TestInputGuards +# +# ------------------------------------------------------------ +class TestInputGuards(_ErrorDialogTestCase): + """Lock in the pre-existing input validators so future refactors + cannot silently weaken the guardrails around ``add_data_entry``.""" + + def test_add_data_entry_requires_name(self) -> None: + """Empty name with a valid tree should surface the name-required error.""" + stub = _make_gramplet(db_open=True, np_name="") + + stub.add_data_entry(None) + + self.assertTrue(self.captured_errors) + title, _body = self.captured_errors[0] + self.assertIn("name", title.lower()) + + def test_add_data_entry_parent_without_active_person(self) -> None: + """Adding as a parent without an active person surfaces a clear error.""" + stub = _make_gramplet( + db_open=True, + np_name="Doe, Jane", + np_relation=DataEntryGramplet.AS_PARENT, + ) + + stub.add_data_entry(None) + + self.assertTrue(self.captured_errors) + _title, body = self.captured_errors[0] + self.assertIn("parent", body.lower()) + + +# ------------------------------------------------------------ +# +# TestGprRegistration +# +# ------------------------------------------------------------ +class TestGprRegistration(unittest.TestCase): + """Catch metadata breakage in the plugin registration file early.""" + + def test_gpr_registration_metadata(self) -> None: + """The .gpr.py file must register a single gramplet with expected keys.""" + gpr_path = os.path.join(ADDON_DIR, "DataEntryGramplet.gpr.py") + calls: list[tuple[tuple, dict]] = [] + + namespace: dict[str, Any] = { + "register": lambda *args, **kwargs: calls.append((args, kwargs)), + "GRAMPLET": "GRAMPLET", + "STABLE": "STABLE", + "EXPERT": "EXPERT", + "_": lambda s: s, + } + with open(gpr_path, encoding="utf-8") as handle: + exec(compile(handle.read(), gpr_path, "exec"), namespace) + + self.assertEqual(len(calls), 1, "expected exactly one register() call") + args, kwargs = calls[0] + self.assertEqual(args, ("GRAMPLET",)) + self.assertEqual(kwargs["id"], "Data Entry Gramplet") + self.assertEqual(kwargs["gramplet"], "DataEntryGramplet") + self.assertEqual(kwargs["fname"], "DataEntryGramplet.py") + self.assertEqual(kwargs["gramps_target_version"], "6.0") + self.assertEqual(kwargs["status"], "STABLE") + # Navigation type must stay Person — the active object is fetched that way. + self.assertEqual(kwargs["navtypes"], ["Person"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/Form/CensusCheckQuickview.gpr.py b/Form/CensusCheckQuickview.gpr.py index 81ac06e22..9f3cc2b52 100644 --- a/Form/CensusCheckQuickview.gpr.py +++ b/Form/CensusCheckQuickview.gpr.py @@ -8,7 +8,7 @@ id = 'censuscheckquickview', name = _("CensusCheck"), description= _("Check whether any Census events are missing for a person and some of their descendents"), - version = '1.0.1', + version = '1.0.2', gramps_target_version = '6.0', status = STABLE, fname = 'CensusCheckQuickview.py', @@ -22,7 +22,7 @@ id = 'censuscheckupquickview', name = _("CensusCheckUp"), description= _("Check whether any Census events are missing for a person and some of their ancestors"), - version = '1.0.1', + version = '1.0.2', gramps_target_version = '6.0', status = STABLE, fname = 'CensusCheckUpQuickview.py', diff --git a/Form/editform.py b/Form/editform.py index 910cd827c..7e43fdd16 100644 --- a/Form/editform.py +++ b/Form/editform.py @@ -28,8 +28,11 @@ # Python modules # ------------------------------------------------------------------------- from gi.repository import Gdk +import logging import pickle +LOG = logging.getLogger(".FormGramplet") + # ------------------------------------------------------------------------ # # GTK modules @@ -77,6 +80,7 @@ get_section_columns, get_form_citation, ) +from form_validator import split_family_title from entrygrid import EntryGrid # ------------------------------------------------------------------------ @@ -114,6 +118,12 @@ def __init__(self, dbstate, uistate, track, event, citation, callback): self.citation = citation self.callback = callback + LOG.debug( + "Opening EditForm for event %s, citation %s", + event.get_handle() or "", + citation.get_handle() or "", + ) + ManagedWindow.__init__(self, uistate, track, citation) self.widgets = {} @@ -391,10 +401,10 @@ def close(self, *args): """ Close the editor window. """ - (width, height) = self.window.get_size() + width, height = self.window.get_size() self._config.set("interface.form-width", width) self._config.set("interface.form-height", height) - (width, height) = self.window.get_position() + width, height = self.window.get_position() self._config.set("interface.form-horiz-position", width) self._config.set("interface.form-vert-position", height) self._config.save() @@ -700,7 +710,7 @@ def on_drag_data_received( self, widget, context, pos_x, pos_y, sel_data, info, time ): if sel_data and sel_data.get_data(): - (drag_type, idval, handle, val) = pickle.loads(sel_data.get_data()) + drag_type, idval, handle, val = pickle.loads(sel_data.get_data()) person = self.db.get_person_from_handle(handle) if person: self.__person_added(person) @@ -972,7 +982,7 @@ def on_drag_data_received( self, widget, context, pos_x, pos_y, sel_data, info, time ): if sel_data and sel_data.get_data(): - (drag_type, idval, handle, val) = pickle.loads(sel_data.get_data()) + drag_type, idval, handle, val = pickle.loads(sel_data.get_data()) person = self.db.get_person_from_handle(handle) if person: self.__added(person) @@ -1102,7 +1112,15 @@ def __init__(self, dbstate, uistate, track, event, citation, form_id, section): hbox = Gtk.Box() title = get_section_title(form_id, section) - title1, title2 = title.split("/") + title1, title2 = split_family_title(title) + if not title2: + LOG.warning( + "FamilySection for form '%s' section '%s' has title '%s' " + "without the expected 'X/Y' separator; second label will be empty", + form_id, + section, + title, + ) label = Gtk.Label(label="%s" % title1) label.set_use_markup(True) @@ -1158,7 +1176,7 @@ def on_drag_data_received( self, widget, context, pos_x, pos_y, sel_data, info, time ): if sel_data and sel_data.get_data(): - (drag_type, idval, handle, val) = pickle.loads(sel_data.get_data()) + drag_type, idval, handle, val = pickle.loads(sel_data.get_data()) family = self.db.get_family_from_handle(handle) if family: self.__added(family) diff --git a/Form/form.py b/Form/form.py index a2a4c0cf3..81b99352f 100644 --- a/Form/form.py +++ b/Form/form.py @@ -22,6 +22,7 @@ """ Form definitions. """ + # --------------------------------------------------------------- # # Python imports @@ -29,6 +30,8 @@ # --------------------------------------------------------------- import os import xml.dom.minidom +import xml.parsers.expat +import logging # --------------------------------------------------------------- # @@ -37,8 +40,18 @@ # --------------------------------------------------------------- from gramps.gen.datehandler import parser from gramps.gen.config import config -from gramps.gui.dialog import ErrorDialog, WarningDialog -import logging +from gramps.gui.dialog import ErrorDialog + +# --------------------------------------------------------------- +# +# Gramps specific +# +# --------------------------------------------------------------- +from form_validator import ( + get_form_warnings, + validate_form_dom, + validate_form_element, +) LOG = logging.getLogger(".FormGramplet") @@ -111,7 +124,14 @@ class Form: A class to read form definitions from an XML file. """ - def __init__(self): + def __init__(self, definition_dir=None): + """ + :param definition_dir: optional override for the directory the + loader scans for ``form_*.xml`` / ``custom.xml`` files. + Defaults to the directory containing this module. Exposed + primarily so tests can point the loader at an isolated + temporary directory. + """ self.__references = {} self.__dates = {} self.__headings = {} @@ -122,27 +142,91 @@ def __init__(self): self.__names = {} self.__section_types = {} + base_dir = definition_dir or os.path.dirname(__file__) + LOG.debug("Loading form definitions from %s", base_dir) for file_name in definition_files: - full_path = os.path.join(os.path.dirname(__file__), file_name) + full_path = os.path.join(base_dir, file_name) if os.path.exists(full_path): - try: - self.__load_definitions(full_path) - except Exception as e: - WarningDialog( - _("Failed to load Form definition file:\n%s\n") % full_path, - ) - LOG.warning( - "\nERROR: failed to load Form definition file.\n%s\nException:\n%s", - full_path, - str(e), - ) - - def __load_definitions(self, definition_file): - dom = xml.dom.minidom.parse(definition_file) + self.__load_file(full_path) + else: + LOG.debug("Form definition file not present: %s", full_path) + LOG.info( + "Loaded %d form definition(s) from %s", + len(self.__names), + base_dir, + ) + + def __load_file(self, full_path): + """ + Parse and validate a single form definition file, then load any + well-formed ``
`` elements it contains. + + Parse errors and structural validation errors are reported to the + user through an :class:`ErrorDialog` and logged; malformed forms + are skipped while valid forms from the same file are still loaded. + """ + LOG.debug("Parsing form definition file %s", full_path) + try: + dom = xml.dom.minidom.parse(full_path) + except xml.parsers.expat.ExpatError as exc: + ErrorDialog( + _("XML syntax error in Form definition file"), + "%s\n\n%s" % (full_path, exc), + ) + LOG.warning( + "XML syntax error in Form definition file %s: %s", + full_path, + exc, + ) + return + except Exception as exc: + ErrorDialog( + _("Failed to read Form definition file"), + "%s\n\n%s" % (full_path, exc), + ) + LOG.warning("Failed to read Form definition file %s: %s", full_path, exc) + return + + errors = validate_form_dom(dom) + if errors: + ErrorDialog( + _("Invalid Form definition file"), + "%s\n\n%s" % (full_path, "\n".join(errors)), + ) + LOG.warning( + "Invalid Form definition file %s:\n%s", + full_path, + "\n".join(errors), + ) + + for warning in get_form_warnings(dom): + LOG.warning("In %s: %s", full_path, warning) + + try: + self.__load_definitions(dom) + finally: + dom.unlink() + + def __load_definitions(self, dom): top = dom.getElementsByTagName("forms") + if not top: + return for form in top[0].getElementsByTagName("form"): + if validate_form_element(form): + # Errors for this form were already surfaced by the + # file-level validator — skip it so __load_definitions + # never touches a malformed element. + skipped_id = ( + form.attributes["id"].value + if "id" in form.attributes + else "" + ) + LOG.debug("Skipping invalid '%s'", skipped_id) + continue + id = form.attributes["id"].value + LOG.debug("Loading form '%s'", id) self.__names[id] = form.attributes["title"].value self.__types[id] = form.attributes["type"].value if "reference" in form.attributes: @@ -194,7 +278,6 @@ def __load_definitions(self, definition_file): self.__columns[id][role].append( (attr_text, long_text, int(size_text)) ) - dom.unlink() def get_form_ids(self): """Return a list of ids for all form definitions.""" @@ -205,7 +288,7 @@ def get_title(self, form_id): return self.__names[form_id] def get_reference(self, form_id): - """ Return the reference for a given form. """ + """Return the reference for a given form.""" return self.__references[form_id] def get_date(self, form_id): @@ -258,12 +341,14 @@ def get_form_title(form_id): """ return FORM.get_title(form_id) + def get_form_reference(form_id): """ Return the reference for a given form. """ return FORM.get_reference(form_id) + def get_form_date(form_id): """ Return the date for a given form. diff --git a/Form/form_validator.py b/Form/form_validator.py new file mode 100644 index 000000000..5b82cd98c --- /dev/null +++ b/Form/form_validator.py @@ -0,0 +1,238 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +""" +Pure-Python validation for Form addon XML definition files. + +Kept free of GTK/Gramps imports so it can be unit-tested without a GUI +environment. +""" + +# ------------------------ +# Python modules +# ------------------------ +import xml.dom.minidom +import xml.parsers.expat + +VALID_SECTION_TYPES = frozenset({"person", "family", "multi"}) +REQUIRED_FORM_ATTRS = ("id", "title", "type") + + +def split_family_title(title: str) -> tuple[str, str]: + """ + Split a family-section title of the form ``'X/Y'`` into ``(X, Y)``. + + Falls back gracefully when the separator is absent or the input is + empty, so callers never raise ``ValueError`` on malformed XML. + + :param title: the raw title string from the XML (may be empty) + :returns: a two-tuple of title parts; the second element is empty + when the title contains no ``'/'`` + """ + if not title: + return "", "" + parts = title.split("/", 1) + if len(parts) == 2: + return parts[0], parts[1] + return parts[0], "" + + +def validate_form_element(form) -> list[str]: + """ + Validate a single ```` DOM element against the form schema. + + :param form: a DOM element for a single form definition + :returns: a list of human-readable error messages scoped to this + form; empty when the form is valid + """ + errors: list[str] = [] + + form_id = form.attributes["id"].value if "id" in form.attributes else "" + for required in REQUIRED_FORM_ATTRS: + if required not in form.attributes: + errors.append( + "Form '%s': missing required attribute '%s'" % (form_id, required) + ) + + for section in form.getElementsByTagName("section"): + role = section.attributes["role"].value if "role" in section.attributes else "" + if not role: + errors.append( + "Form '%s':
is missing required attribute 'role'" % form_id + ) + continue + + if "type" not in section.attributes: + errors.append( + "Form '%s': section '%s' is missing required attribute 'type'" + % (form_id, role) + ) + continue + + section_type = section.attributes["type"].value + if not section_type: + errors.append( + "Form '%s': section '%s' has an empty 'type' attribute" + % (form_id, role) + ) + continue + + if section_type not in VALID_SECTION_TYPES: + errors.append( + "Form '%s': section '%s' has invalid type '%s' " + "(expected one of: %s)" + % ( + form_id, + role, + section_type, + ", ".join(sorted(VALID_SECTION_TYPES)), + ) + ) + continue + + title = ( + section.attributes["title"].value if "title" in section.attributes else "" + ) + if section_type == "family": + parts = title.split("/") + if len(parts) != 2 or not parts[0].strip() or not parts[1].strip(): + errors.append( + "Form '%s': family section '%s' requires a title " + "of the form 'Name1/Name2' (got '%s')" % (form_id, role, title) + ) + + return errors + + +def validate_form_dom(dom: xml.dom.minidom.Document) -> list[str]: + """ + Validate the structure of a parsed form definitions DOM. + + Checks that: + + * a ```` root element exists and contains at least one + ```` definition; + * each ```` element has ``id``, ``title`` and ``type`` attributes; + * each ``
`` element has non-empty ``role`` and ``type`` + attributes; + * each section's ``type`` is one of ``person``, ``family`` or ``multi``; + * ``family``-type sections declare a title of the form ``'X/Y'`` with + two non-empty parts. + + :param dom: a parsed ``xml.dom.minidom.Document`` + :returns: a list of human-readable error messages; empty when the + document is valid + """ + top = dom.getElementsByTagName("forms") + if not top: + return ["Missing root element"] + + errors: list[str] = [] + forms = top[0].getElementsByTagName("form") + if not forms: + errors.append(" root element contains no definitions") + for form in forms: + errors.extend(validate_form_element(form)) + return errors + + +def get_form_warnings(dom: xml.dom.minidom.Document) -> list[str]: + """ + Collect non-fatal warnings about a parsed form definitions DOM. + + Warnings describe likely authoring mistakes that do not prevent the + form from loading. Currently covers Gramps bug 11010's observation + that a section's ```` ```` values are expected to sum + to 100 — sections that declare explicit sizes on every column but do + not sum to 100 are reported as warnings so callers can log them + without blocking the form from loading. + + Sections without any sized columns, or with only some columns + sized, are skipped because the intent is ambiguous. + + :param dom: a parsed ``xml.dom.minidom.Document`` + :returns: a list of human-readable warning messages; empty when + nothing questionable is detected + """ + warnings: list[str] = [] + top = dom.getElementsByTagName("forms") + if not top: + return warnings + + for form in top[0].getElementsByTagName("form"): + form_id = ( + form.attributes["id"].value + if "id" in form.attributes + else "" + ) + for section in form.getElementsByTagName("section"): + role = ( + section.attributes["role"].value + if "role" in section.attributes + else "" + ) + columns = section.getElementsByTagName("column") + if not columns: + continue + + sizes: list[int] = [] + all_sized = True + for column in columns: + size_nodes = column.getElementsByTagName("size") + if not size_nodes or not size_nodes[0].childNodes: + all_sized = False + break + try: + sizes.append(int(size_nodes[0].childNodes[0].data)) + except ValueError: + all_sized = False + break + if not all_sized: + continue + + total = sum(sizes) + if total != 100: + warnings.append( + "Form '%s': section '%s' column sizes sum to %d " + "(expected 100); form will still load but column " + "widths may not render as intended" + % (form_id, role, total) + ) + return warnings + + +def parse_and_validate(path: str) -> tuple[xml.dom.minidom.Document | None, list[str]]: + """ + Parse ``path`` as XML and validate it against the form schema. + + :param path: filesystem path to a form definitions XML file + :returns: a ``(dom, errors)`` tuple. When parsing fails, ``dom`` is + ``None`` and ``errors`` contains a single description of + the syntax error. When parsing succeeds, ``dom`` is the + parsed document and ``errors`` lists any structural + problems (empty when the file is valid). + """ + try: + dom = xml.dom.minidom.parse(path) + except xml.parsers.expat.ExpatError as exc: + return None, ["XML syntax error: %s" % exc] + except (OSError, ValueError) as exc: + return None, ["Failed to read file: %s" % exc] + return dom, validate_form_dom(dom) diff --git a/Form/formgramplet.gpr.py b/Form/formgramplet.gpr.py index bcdfed128..55d6312d0 100644 --- a/Form/formgramplet.gpr.py +++ b/Form/formgramplet.gpr.py @@ -31,7 +31,7 @@ name=_("Form Gramplet"), description=_("Gramplet interface for Forms"), status=STABLE, - version = '2.0.51', + version = '2.0.52', gramps_target_version="6.0", navtypes=["Person"], fname="formgramplet.py", diff --git a/Form/po/template.pot b/Form/po/template.pot index 91af5203a..9662fd535 100644 --- a/Form/po/template.pot +++ b/Form/po/template.pot @@ -9045,3 +9045,15 @@ msgstr "" #: Form/form_fr.xml.h:144 msgid "Arrived since" msgstr "" + +#: Form/form.py:154 +msgid "XML syntax error in Form definition file" +msgstr "" + +#: Form/form.py:165 +msgid "Failed to read Form definition file" +msgstr "" + +#: Form/form.py:176 +msgid "Invalid Form definition file" +msgstr "" diff --git a/Form/tests/test_form_validator.py b/Form/tests/test_form_validator.py new file mode 100644 index 000000000..0efc350eb --- /dev/null +++ b/Form/tests/test_form_validator.py @@ -0,0 +1,518 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +""" +Unit tests for ``form_validator`` — the pure-Python validation layer +for Form addon XML definition files. + +These tests do not touch Gramps or GTK, so they run in every CI job. + +Run with:: + + python3 -m unittest Form.tests.test_form_validator -v +""" + +# ------------------------ +# Python modules +# ------------------------ +import os +import sys +import tempfile +import textwrap +import unittest +import xml.dom.minidom + +# ------------------------ +# Gramps specific +# ------------------------ +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +from form_validator import ( + get_form_warnings, + parse_and_validate, + split_family_title, + validate_form_dom, + VALID_SECTION_TYPES, +) + + +def _dom_from_string(xml_text: str) -> xml.dom.minidom.Document: + """Parse an XML string into a DOM for validator testing.""" + return xml.dom.minidom.parseString(xml_text) + + +# --------------------------------------------------------------------------- +# split_family_title — belt-and-braces helper used by FamilySection +# --------------------------------------------------------------------------- +class TestSplitFamilyTitle(unittest.TestCase): + """ + Regression coverage for Gramps bug 11707 — ``FamilySection`` used to + crash with ``ValueError: not enough values to unpack (expected 2, + got 1)`` when a form's ``
`` title did not + contain a ``/`` separator. + """ + + def test_two_parts(self): + self.assertEqual(split_family_title("Groom/Bride"), ("Groom", "Bride")) + + def test_no_separator_returns_empty_second(self): + self.assertEqual(split_family_title("Couple"), ("Couple", "")) + + def test_empty_string_returns_two_empties(self): + self.assertEqual(split_family_title(""), ("", "")) + + def test_only_separator(self): + self.assertEqual(split_family_title("/"), ("", "")) + + def test_leading_separator(self): + self.assertEqual(split_family_title("/Bride"), ("", "Bride")) + + def test_trailing_separator(self): + self.assertEqual(split_family_title("Groom/"), ("Groom", "")) + + def test_multiple_separators_only_split_once(self): + self.assertEqual(split_family_title("A/B/C"), ("A", "B/C")) + + def test_whitespace_preserved(self): + self.assertEqual( + split_family_title(" Groom / Bride "), + (" Groom ", " Bride "), + ) + + +# --------------------------------------------------------------------------- +# validate_form_dom — happy path +# --------------------------------------------------------------------------- +class TestValidateFormDomValid(unittest.TestCase): + + def test_minimal_valid_form(self): + dom = _dom_from_string(textwrap.dedent("""\ + + +
+ + + """)) + self.assertEqual(validate_form_dom(dom), []) + + def test_valid_family_section_with_slashed_title(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + self.assertEqual(validate_form_dom(dom), []) + + def test_valid_person_section_without_title(self): + """Person sections may legitimately omit the title attribute.""" + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + self.assertEqual(validate_form_dom(dom), []) + +# --------------------------------------------------------------------------- +# validate_form_dom — structural errors +# --------------------------------------------------------------------------- +class TestValidateFormDomErrors(unittest.TestCase): + + def test_missing_forms_root(self): + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("", errors[0]) + + def test_empty_forms_element_is_rejected(self): + """ + Regression for Gramps bug 11010 — a file whose ```` root + contains no ``
`` definitions used to load silently, leaving + users with no feedback. The validator now reports it as an error. + """ + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("no ", errors[0]) + + def test_forms_root_with_only_comments_is_rejected(self): + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("no ", errors[0]) + + def test_form_missing_id_attribute(self): + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertTrue(any("missing required attribute 'id'" in e for e in errors)) + + def test_form_missing_title_attribute(self): + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertTrue(any("missing required attribute 'title'" in e for e in errors)) + + def test_form_missing_type_attribute(self): + dom = _dom_from_string("") + errors = validate_form_dom(dom) + self.assertTrue(any("missing required attribute 'type'" in e for e in errors)) + + def test_section_missing_role_attribute(self): + dom = _dom_from_string(textwrap.dedent("""\ + + +
+ + + """)) + errors = validate_form_dom(dom) + self.assertTrue(any("'role'" in e for e in errors)) + + def test_section_empty_role_attribute(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertTrue(any("'role'" in e for e in errors)) + + def test_section_missing_type_attribute(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertTrue(any("'type'" in e for e in errors)) + + def test_section_empty_type_attribute(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertTrue(any("'type'" in e for e in errors)) + + def test_section_invalid_type(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("invalid type 'bogus'", errors[0]) + for valid in VALID_SECTION_TYPES: + self.assertIn(valid, errors[0]) + + def test_section_type_is_case_sensitive(self): + """ + A real custom.xml in the wild used ``type='Person'`` instead of + ``type='person'``. The validator rejects it so the user sees a + clear error rather than silently loading a broken form. + """ + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("invalid type 'Person'", errors[0]) + + def test_family_section_without_title_is_rejected(self): + """ + Reproduction of bug 11707 at the validation layer: a family + section without a slashed title produces a clear error instead + of a ValueError crash in the GUI. + """ + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("Name1/Name2", errors[0]) + + def test_family_section_with_single_part_title_is_rejected(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + self.assertIn("Couple", errors[0]) + + def test_family_section_with_blank_part_is_rejected(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + + def test_family_section_with_three_parts_is_rejected(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + errors = validate_form_dom(dom) + self.assertEqual(len(errors), 1) + + def test_multiple_errors_aggregated(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+
+ +
+ + """)) + errors = validate_form_dom(dom) + # 1 for invalid type + 1 for missing role + 1 for missing id + self.assertGreaterEqual(len(errors), 3) + + +# --------------------------------------------------------------------------- +# parse_and_validate — file-level entry point +# --------------------------------------------------------------------------- +class TestParseAndValidate(unittest.TestCase): + + def _write(self, content: str) -> str: + """Write a temporary XML file and return its path.""" + fd, path = tempfile.mkstemp(suffix=".xml") + with os.fdopen(fd, "w", encoding="utf-8") as f: + f.write(content) + self.addCleanup(os.remove, path) + return path + + def test_valid_file(self): + path = self._write(textwrap.dedent("""\ + + +
+ + + """)) + dom, errors = parse_and_validate(path) + self.assertIsNotNone(dom) + self.assertEqual(errors, []) + + def test_xml_syntax_error_reports_cleanly(self): + path = self._write("
") + dom, errors = parse_and_validate(path) + self.assertIsNone(dom) + self.assertEqual(len(errors), 1) + self.assertIn("XML syntax error", errors[0]) + + def test_missing_file_reports_cleanly(self): + dom, errors = parse_and_validate("/nonexistent/path.xml") + self.assertIsNone(dom) + self.assertEqual(len(errors), 1) + + def test_invalid_structure_returns_errors_and_dom(self): + path = self._write(textwrap.dedent("""\ + + +
+ + + """)) + dom, errors = parse_and_validate(path) + self.assertIsNotNone(dom) + self.assertEqual(len(errors), 1) + + +# --------------------------------------------------------------------------- +# Sanity: the shipped built-in definition files must validate +# --------------------------------------------------------------------------- +class TestShippedDefinitionFilesValidate(unittest.TestCase): + """ + Every built-in ``form_*.xml`` file shipped with the addon must pass + validation. If this test fails, the addon would refuse to load one + of its own definition files for end users. + """ + + FORM_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + + def test_all_builtin_form_files_validate(self): + import glob + + files = sorted(glob.glob(os.path.join(self.FORM_DIR, "form_*.xml"))) + self.assertGreater(len(files), 0, "no form_*.xml files discovered") + for path in files: + with self.subTest(form_file=os.path.basename(path)): + dom, errors = parse_and_validate(path) + self.assertIsNotNone(dom, f"failed to parse {path}") + self.assertEqual( + errors, + [], + f"validation errors in {path}:\n" + "\n".join(errors), + ) + + +# --------------------------------------------------------------------------- +# get_form_warnings — non-fatal authoring warnings (Gramps bug 11010) +# --------------------------------------------------------------------------- +class TestGetFormWarnings(unittest.TestCase): + """ + Column sizes are expected to sum to 100. Rather than reject the + form, ``get_form_warnings`` flags suspect sections so the caller can + log them — 78 shipped definition files currently violate this rule + without breaking rendering, so escalating to an error would be + user-hostile. + """ + + def test_section_without_columns_has_no_warning(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ + + """)) + self.assertEqual(get_form_warnings(dom), []) + + def test_columns_without_any_size_have_no_warning(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ <_attribute>Name + <_attribute>Age +
+
+ + """)) + self.assertEqual(get_form_warnings(dom), []) + + def test_columns_summing_to_100_have_no_warning(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ <_attribute>A60 + <_attribute>B40 +
+
+ + """)) + self.assertEqual(get_form_warnings(dom), []) + + def test_columns_not_summing_to_100_emit_warning(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ <_attribute>A25 +
+
+ + """)) + warnings = get_form_warnings(dom) + self.assertEqual(len(warnings), 1) + self.assertIn("sum to 25", warnings[0]) + self.assertIn("F1", warnings[0]) + self.assertIn("Primary", warnings[0]) + + def test_partially_sized_columns_have_no_warning(self): + """ + When only some columns declare a ````, the author's intent + is ambiguous (mixed relative/absolute sizing) so the check is + skipped to avoid false positives. + """ + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ <_attribute>A25 + <_attribute>B +
+
+ + """)) + self.assertEqual(get_form_warnings(dom), []) + + def test_warnings_are_independent_of_errors(self): + """ + Warnings are structurally orthogonal to errors: a malformed form + still produces warnings for its well-formed sibling. + """ + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ +
+
+ <_attribute>A30 +
+
+ + """)) + warnings = get_form_warnings(dom) + self.assertEqual(len(warnings), 1) + self.assertIn("GOOD", warnings[0]) + + def test_multiple_misaligned_sections_all_reported(self): + dom = _dom_from_string(textwrap.dedent("""\ + +
+
+ <_attribute>X40 +
+
+ <_attribute>Y70 +
+
+ + """)) + warnings = get_form_warnings(dom) + self.assertEqual(len(warnings), 2) + + +if __name__ == "__main__": + unittest.main() diff --git a/Form/tests/test_integration_form.py b/Form/tests/test_integration_form.py new file mode 100644 index 000000000..82839c0cf --- /dev/null +++ b/Form/tests/test_integration_form.py @@ -0,0 +1,332 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +""" +Integration tests for the Form addon loader — covers +``gramps-project/gramps#11707`` (*ValueError: not enough values to +unpack* when a form's ``
`` title lacks the +``X/Y`` separator) and ``gramps-project/gramps#11010`` (empty +```` roots used to load silently, and column-size mismatches +had no diagnostic). + +Scenarios covered: + +* Malformed XML produces an ``ErrorDialog`` rather than a bare traceback. +* A partially-broken file still loads its well-formed ``
`` entries. +* Empty ```` roots surface as an ``ErrorDialog`` (bug 11010 item a). +* Column-size mismatches are logged as WARNINGs only (bug 11010 item b). +* The shipped built-in definition files load cleanly without any error + dialogs being raised. + +Run with:: + + python3 -m unittest Form.tests.test_integration_form -v +""" + +# ------------------------ +# Python modules +# ------------------------ +import logging +import os +import shutil +import sys +import tempfile +import textwrap +import unittest +from unittest.mock import patch + +# ------------------------ +# Gramps modules +# ------------------------ +try: + import gi # noqa: F401 + import gramps +except ImportError as exc: + raise unittest.SkipTest( + "Form integration tests require 'gi' and 'gramps': %s" % exc + ) + +if "GRAMPS_RESOURCES" not in os.environ: + os.environ["GRAMPS_RESOURCES"] = os.path.dirname(os.path.dirname(gramps.__file__)) + +# ------------------------ +# Gramps specific +# ------------------------ +ADDON_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +if ADDON_DIR not in sys.path: + sys.path.insert(0, ADDON_DIR) + + +# --------------------------------------------------------------------------- +# Shared harness +# --------------------------------------------------------------------------- +class FormLoaderTestCase(unittest.TestCase): + """ + Base class that imports the Form addon's ``form`` module and + redirects its ``ErrorDialog`` to an in-memory list instead of + opening a GTK dialog. + + Subclasses access :attr:`shown` to inspect captured dialog calls + and use :meth:`_write` to drop XML files into an isolated + temporary directory. + """ + + def setUp(self) -> None: + import form + + self.form = form + self.shown: list[tuple[str, str]] = [] + + def _fake_error_dialog(*args, **kwargs): + title = str(args[0]) if args else "" + body = str(args[1]) if len(args) > 1 else "" + self.shown.append((title, body)) + + error_patch = patch.object(form, "ErrorDialog", _fake_error_dialog) + error_patch.start() + self.addCleanup(error_patch.stop) + + self.tmp_dir = tempfile.mkdtemp(prefix="form-integration-") + self.addCleanup(shutil.rmtree, self.tmp_dir, True) + + def _write(self, filename: str, content: str) -> None: + """Write an XML fixture into the test's temporary directory.""" + path = os.path.join(self.tmp_dir, filename) + with open(path, "w", encoding="utf-8") as handle: + handle.write(content) + + def _patch_definition_files(self, files: list[str]) -> None: + """Redirect the loader to the given filenames (inside tmp_dir).""" + files_patch = patch.object(self.form, "definition_files", files) + files_patch.start() + self.addCleanup(files_patch.stop) + + +# --------------------------------------------------------------------------- +# Error-dialog wiring +# --------------------------------------------------------------------------- +class TestErrorDialogWiring(FormLoaderTestCase): + """ + Exercises the four failure modes the loader now surfaces via + :class:`ErrorDialog` instead of an unhandled traceback. + """ + + def test_malformed_xml_shows_error_dialog(self) -> None: + """XML syntax errors should raise an ErrorDialog, not a traceback.""" + self._write("custom.xml", "") + self._patch_definition_files(["custom.xml"]) + + instance = self.form.Form(definition_dir=self.tmp_dir) + + self.assertTrue(self.shown, "no ErrorDialog was displayed") + title, body = self.shown[0] + self.assertIn("XML syntax error", title) + self.assertIn("custom.xml", body) + self.assertEqual(list(instance.get_form_ids()), []) + + def test_invalid_family_title_shows_error_dialog(self) -> None: + """ + The exact condition from bug 11707 — a family section with a + non-``X/Y`` title — must be surfaced as an ErrorDialog at load + time rather than an unhandled exception when the user later + opens the form. + """ + self._write( + "custom.xml", + textwrap.dedent("""\ + + +
+ + + """), + ) + self._patch_definition_files(["custom.xml"]) + + self.form.Form(definition_dir=self.tmp_dir) + + self.assertTrue( + self.shown, "no ErrorDialog was displayed for invalid family title" + ) + title, body = self.shown[0] + self.assertIn("Invalid Form definition file", title) + self.assertIn("Name1/Name2", body) + + def test_partially_broken_file_still_loads_valid_forms(self) -> None: + """A broken
must not stop sibling elements loading.""" + self._write( + "custom.xml", + textwrap.dedent("""\ + + +
+ +
+
+ + + """), + ) + self._patch_definition_files(["custom.xml"]) + + instance = self.form.Form(definition_dir=self.tmp_dir) + loaded_ids = list(instance.get_form_ids()) + + self.assertIn("GOOD", loaded_ids, "valid form should still load") + self.assertNotIn("BAD", loaded_ids, "invalid form should be skipped") + self.assertTrue(self.shown, "the broken form should have been reported") + + def test_missing_role_attribute_shows_error_dialog(self) -> None: + """A section missing its ``role`` attribute is reported clearly.""" + self._write( + "custom.xml", + textwrap.dedent("""\ + +
+
+ + + """), + ) + self._patch_definition_files(["custom.xml"]) + + self.form.Form(definition_dir=self.tmp_dir) + + self.assertTrue(self.shown) + _, body = self.shown[0] + self.assertIn("role", body) + + def test_invalid_section_type_shows_error_dialog(self) -> None: + """Unknown section types produce a clear error, not a later crash.""" + self._write( + "custom.xml", + textwrap.dedent("""\ + +
+
+ + + """), + ) + self._patch_definition_files(["custom.xml"]) + + instance = self.form.Form(definition_dir=self.tmp_dir) + + self.assertTrue(self.shown) + _, body = self.shown[0] + self.assertIn("bogus", body) + self.assertNotIn("F1", list(instance.get_form_ids())) + + +# --------------------------------------------------------------------------- +# Empty / content-less definition files (Gramps bug 11010 item a) +# --------------------------------------------------------------------------- +class TestEmptyFormsValidation(FormLoaderTestCase): + """ + A definition file whose ```` root contains no ``
`` + children used to load silently. The validator now surfaces it as an + ErrorDialog so the user notices the empty file. + """ + + def test_empty_forms_element_shows_error_dialog(self) -> None: + """Empty ```` must raise an ErrorDialog, not load silently.""" + self._write("custom.xml", "") + self._patch_definition_files(["custom.xml"]) + + instance = self.form.Form(definition_dir=self.tmp_dir) + + self.assertTrue(self.shown, "no ErrorDialog was displayed for empty ") + title, body = self.shown[0] + self.assertIn("Invalid Form definition file", title) + self.assertIn("no ", body) + self.assertEqual(list(instance.get_form_ids()), []) + + +# --------------------------------------------------------------------------- +# Column-size warnings (Gramps bug 11010 item b) — WARNING only, not errors +# --------------------------------------------------------------------------- +class TestColumnSizeWarnings(FormLoaderTestCase): + """ + Sections whose ```` sizes do not sum to 100 must be logged + as warnings only — 78 shipped forms trip this check, so escalating + to an ErrorDialog would harass users on every launch. + """ + + def test_column_size_sum_warning_is_logged_not_dialog(self) -> None: + """Column-size mismatch → WARNING log entry, no ErrorDialog.""" + self._write( + "custom.xml", + textwrap.dedent("""\ + + +
+ <_attribute>A25 +
+ + + """), + ) + self._patch_definition_files(["custom.xml"]) + + with self.assertLogs(".FormGramplet", level=logging.WARNING) as log_ctx: + instance = self.form.Form(definition_dir=self.tmp_dir) + + self.assertFalse( + self.shown, + "column-size mismatch must not produce an ErrorDialog:\n" + + "\n".join("%s: %s" % (t, b) for t, b in self.shown), + ) + self.assertIn( + "F1", + list(instance.get_form_ids()), + "the form must still load despite the size mismatch", + ) + self.assertTrue( + any("sum to 25" in message for message in log_ctx.output), + "expected a column-size warning in the log", + ) + + +# --------------------------------------------------------------------------- +# Shipped files load cleanly +# --------------------------------------------------------------------------- +class TestShippedFilesLoadCleanly(FormLoaderTestCase): + """ + The built-in definition files that ship with the addon must load + without triggering a single ErrorDialog, otherwise end users would + see a popup every time they opened Gramps. + """ + + def test_shipped_files_load_without_errors(self) -> None: + instance = self.form.Form() + + self.assertFalse( + self.shown, + "Built-in definition files triggered ErrorDialog calls:\n" + + "\n".join("%s: %s" % (t, b) for t, b in self.shown), + ) + self.assertTrue( + list(instance.get_form_ids()), + "no forms loaded from built-in definition files", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/ImportMerge/importmerge.gpr.py b/ImportMerge/importmerge.gpr.py index f4be5dad9..f707a0581 100644 --- a/ImportMerge/importmerge.gpr.py +++ b/ImportMerge/importmerge.gpr.py @@ -32,7 +32,7 @@ description=_( "Compares a Gramps XML database with the current one, and allows merging of the differences." ), - version = '0.0.25', + version = '0.0.26', gramps_target_version="6.0", status=STABLE, fname="importmerge.py", diff --git a/ImportMerge/importmerge.py b/ImportMerge/importmerge.py index 2b49e8276..e789e2def 100644 --- a/ImportMerge/importmerge.py +++ b/ImportMerge/importmerge.py @@ -803,8 +803,8 @@ def do_commits(self, status, obj_type, hndl, action, trans): continue # now check the missing list for a match changed += self.check_miss(r_objtype, r_hndl, item) - # check for GID conflict - if item.gramps_id != item1.gramps_id: + # check for GID conflict (Tag is a table object without gramps_id) + if obj_type != 'Tag' and item.gramps_id != item1.gramps_id: if getattr(self.db1, 'has_' + obj_type.lower() + '_gramps_id')(item.gramps_id): item.gramps_id = getattr(self.db1, 'find_next_' + @@ -823,9 +823,10 @@ def do_commits(self, status, obj_type, hndl, action, trans): continue # now check the differences list for a match self.check_diffs(r_objtype, r_hndl, obj_type, item) - # check for GID conflict - if getattr(self.db1, 'has_' + obj_type.lower() + - '_gramps_id')(item.gramps_id): + # check for GID conflict (Tag is a table object without gramps_id) + if obj_type != 'Tag' and getattr( + self.db1, 'has_' + obj_type.lower() + + '_gramps_id')(item.gramps_id): item.gramps_id = getattr(self.db1, 'find_next_' + obj_type.lower() + '_gramps_id')() diff --git a/ImportMerge/tests/__init__.py b/ImportMerge/tests/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/ImportMerge/tests/test_integration_importmerge.py b/ImportMerge/tests/test_integration_importmerge.py new file mode 100644 index 000000000..26b006dff --- /dev/null +++ b/ImportMerge/tests/test_integration_importmerge.py @@ -0,0 +1,187 @@ +# +# Gramps - a GTK+/GNOME based genealogy program +# +# Copyright (C) 2026 Eduard Ralph +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. +# + +"""Integration tests for the Import and Merge tool. + +These tests exercise ``ImportMerge.do_commits`` against a real Gramps SQLite +database. We call the method unbound with a stub ``self`` so the GUI +(ManagedWindow + Gtk dialog) doesn't need to be instantiated — the data-path +logic is what we're verifying. + +Regression coverage: + +* Gramps bug 0014056 — Adding a Tag via the Import Merge tool raised + ``AttributeError: 'SQLite' object has no attribute 'has_tag_gramps_id'`` + because Tag is a table object and has no gramps_id. +""" + +# ------------------------ +# Python modules +# ------------------------ +import os +import shutil +import tempfile +import unittest +from types import SimpleNamespace + +# The ImportMerge module imports Gtk at module load — skip the whole file if +# gi/Gtk aren't available (headless-without-GTK environments). On systems +# where both GTK3 and GTK4 are present, pin Gtk to 3.0 before any gramps +# import (mirrors what gramps.grampsapp does at startup); otherwise +# PyGObject loads GTK4 and the gramps.gui import chain crashes on +# Gtk.IconSize.MENU (a GTK3-only enum). +try: + import gi + + gi.require_version("Gtk", "3.0") + gi.require_version("Gdk", "3.0") +except (ImportError, ValueError, AttributeError) as err: + raise unittest.SkipTest("GTK 3.0 / PyGObject not available: %s" % err) + +# ------------------------ +# Gramps modules +# ------------------------ +from gramps.gen.db import DbTxn +from gramps.gen.db.base import DbReadBase +from gramps.gen.db.utils import make_database +from gramps.gen.lib import Tag + +# ------------------------ +# Gramps specific +# ------------------------ +from ImportMerge.importmerge import ImportMerge, S_ADD, S_DIFFERS, A_ADD + + +def _make_db(suffix: str) -> tuple[DbReadBase, str]: + """Create a fresh on-disk Gramps SQLite DB inside a temp directory. + + :param suffix: Short label used in the temp-directory prefix. + :returns: A ``(db, tmpdir)`` tuple — the caller owns ``tmpdir`` and must + remove it. + """ + tmpdir = tempfile.mkdtemp(prefix="gramps_test_%s_" % suffix) + db_path = os.path.join(tmpdir, "db_%s" % suffix) + os.makedirs(db_path) + db = make_database("sqlite") + db.load(db_path, None) + return db, tmpdir + + +def _add_tag(db: DbReadBase, name: str) -> str: + """Add a Tag to ``db`` and return its handle. + + :param db: The database to add the tag to. + :param name: The tag name. + :returns: The handle of the newly-created tag. + """ + tag = Tag() + tag.set_name(name) + with DbTxn("add tag", db, batch=True) as trans: + handle = db.add_tag(tag, trans) + return handle + + +# ------------------------------------------------------------ +# +# ImportMergeTagTestCase +# +# ------------------------------------------------------------ +class ImportMergeTagTestCase(unittest.TestCase): + """Regression tests for bug 0014056 (Tag handling in do_commits).""" + + def setUp(self) -> None: + self.db1, self.tmp1 = _make_db("db1") + self.db2, self.tmp2 = _make_db("db2") + + def tearDown(self) -> None: + for db in (self.db1, self.db2): + try: + db.close() + except Exception: + pass + shutil.rmtree(self.tmp1, ignore_errors=True) + shutil.rmtree(self.tmp2, ignore_errors=True) + + def _make_stub(self) -> SimpleNamespace: + """Stub ``self`` for ``ImportMerge.do_commits`` — no GUI attributes.""" + return SimpleNamespace( + db1=self.db1, + db2=self.db2, + added={}, + missing={}, + diffs={}, + ) + + def test_add_tag_does_not_crash(self) -> None: + """Regression for bug 0014056 — adding a Tag via do_commits must succeed. + + Before the fix, this raised ``AttributeError: 'SQLite' object has no + attribute 'has_tag_gramps_id'`` because the generic GID-conflict check + didn't special-case Tag (a table object without a gramps_id field). + """ + tag_handle = _add_tag(self.db2, "Imported") + stub = self._make_stub() + + with DbTxn("import merge", self.db1, batch=True) as trans: + ImportMerge.do_commits(stub, S_ADD, "Tag", tag_handle, A_ADD, trans) + + self.assertEqual(self.db1.get_number_of_tags(), 1) + committed = self.db1.get_tag_from_handle(tag_handle) + self.assertIsNotNone(committed) + self.assertEqual(committed.get_name(), "Imported") + + def test_differing_tag_does_not_crash(self) -> None: + """S_DIFFERS branch must also skip the gramps_id check for Tag. + + Same root cause as the S_ADD path: the GID-conflict block at the end + of the S_DIFFERS branch dereferences ``item.gramps_id`` and calls + ``has_tag_gramps_id`` — both fail for Tag objects. + """ + tag_handle = _add_tag(self.db1, "Original") + tag2 = Tag() + tag2.set_handle(tag_handle) + tag2.set_name("Modified") + with DbTxn("seed db2", self.db2, batch=True) as trans: + self.db2.add_tag(tag2, trans) + + stub = self._make_stub() + + def fake_diff_result( + action: int, obj_type: str, hndl: str + ) -> tuple[Tag | None, Tag | None, Tag | None]: + item1 = self.db1.get_tag_from_handle(hndl) + item2 = self.db2.get_tag_from_handle(hndl) + return item1, item2, item2 + + stub.diff_result = fake_diff_result + stub.check_diffs = lambda *a, **kw: False + stub.check_added = lambda *a, **kw: False + stub.check_miss = lambda *a, **kw: False + + with DbTxn("import merge", self.db1, batch=True) as trans: + ImportMerge.do_commits(stub, S_DIFFERS, "Tag", tag_handle, A_ADD, trans) + + committed = self.db1.get_tag_from_handle(tag_handle) + self.assertIsNotNone(committed) + self.assertEqual(committed.get_name(), "Modified") + + +if __name__ == "__main__": + unittest.main() diff --git a/TimelinePedigreeView/TimelinePedigreeView.gpr.py b/TimelinePedigreeView/TimelinePedigreeView.gpr.py index 2d6237670..11bdc514b 100644 --- a/TimelinePedigreeView/TimelinePedigreeView.gpr.py +++ b/TimelinePedigreeView/TimelinePedigreeView.gpr.py @@ -36,7 +36,7 @@ "The view shows a timeline pedigree with ancestors and " "descendants of the selected person" ), - version = '0.1.66', + version = '0.1.67', gramps_target_version="6.0", status=STABLE, fname="TimelinePedigreeView.py", diff --git a/TimelinePedigreeView/TimelinePedigreeView.py b/TimelinePedigreeView/TimelinePedigreeView.py index d490f0242..1926987cf 100644 --- a/TimelinePedigreeView/TimelinePedigreeView.py +++ b/TimelinePedigreeView/TimelinePedigreeView.py @@ -705,7 +705,7 @@ def Tree_Rebuild(self): if Tick[1] > 0 and Tick[1] < RequiredWidth: self.gtklayout_lines.append([Tick[1], int(5*TimeLineHeight/8), Tick[1], int(7*TimeLineHeight/8), 1]) if Tick[0]: - label = Gtk.Label(label=Tick[0]) + label = Gtk.Label(label=str(Tick[0])) label.set_justify(Gtk.Justification.CENTER) label.show() layout_widget.put(label, int(Tick[1]-label.get_preferred_size()[0].width/2), 1*TimeLineHeight/4) @@ -1501,7 +1501,6 @@ def add_settings_to_menu(self, menu): menu.append(item) # Help menu entry - menu.append(item) item = Gtk.MenuItem(label=_("About Timeline Pedigree View")) item.connect("activate", self.on_help_clicked) item.show() diff --git a/WebSearch/WebSearch.gpr.py b/WebSearch/WebSearch.gpr.py index 3c9e12de9..968e254b0 100644 --- a/WebSearch/WebSearch.gpr.py +++ b/WebSearch/WebSearch.gpr.py @@ -37,7 +37,7 @@ "Person, Place, Family, or Source record" ), status=STABLE, - version = '1.10.8', + version = '1.10.9', fname="WebSearch.py", height=20, detached_width=400, diff --git a/WebSearch/tests/test_filetable.py b/WebSearch/tests/test_filetable.py index d88d05666..909a1f4fe 100644 --- a/WebSearch/tests/test_filetable.py +++ b/WebSearch/tests/test_filetable.py @@ -37,8 +37,18 @@ Each test ensures that the file table behaves correctly according to its configuration. """ -import unittest import os +import sys +import unittest + +# Make sure addon modules are importable from the parent directory +# (matches the convention used by TMGimporter/Form tests). Required when +# the test is loaded via its dotted path — e.g. +# `python3 -m unittest WebSearch.tests.test_filetable` from +# addons-source/, the form used by addons-source's own ci.yml — where +# `models`/`constants`/`db_file_table` are otherwise resolved against +# the addons-source root rather than against `WebSearch/`. +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from models import DBFileTableConfig from constants import DuplicateHandlingMode, DB_FILE_TABLE_DIR