diff --git a/script/odoo/migration/database_cleanup.py b/script/odoo/migration/database_cleanup.py index 4e6f7fd..4bdaa65 100755 --- a/script/odoo/migration/database_cleanup.py +++ b/script/odoo/migration/database_cleanup.py @@ -79,53 +79,90 @@ END = "ERPLIBRE_CLEANUP_END" SHELL_SCRIPT = """ import json + +try: + from odoo.exceptions import UserError +except Exception: + class UserError(Exception): + pass + ORDER = %(order)r MAX_ROUND = %(max_round)d DRY_RUN = %(dry_run)s report = {"rounds": [], "missing": [], "failed": []} -for index in range(MAX_ROUND): - this_round = [] - purged_this_round = 0 - for label, model in ORDER: - if model not in env: - if label not in report["missing"]: - report["missing"].append(label) - continue - ok = 0 - errors = [] - would = [] - try: - wizard = env[model].create({}) - lines = wizard.purge_line_ids - except Exception as exc: - report["failed"].append([label, "-", str(exc)[:200]]) - continue - for line in lines: - name = line.name or str(line.id) - if DRY_RUN: - would.append(name) + + +def note(label, name, exc): + report["failed"].append([label, name, str(exc)[:200]]) + + +try: + for index in range(MAX_ROUND): + this_round = [] + purged_this_round = 0 + for label, model in ORDER: + if model not in env: + if label not in report["missing"]: + report["missing"].append(label) continue + ok = 0 + errors = [] + would = [] try: - # Un point de reprise par ENTRÉE : un refus n'emporte que la - # sienne, et la passe continue. Sans cela, le premier échec - # ferait perdre tout ce que la passe avait réparé. + # La CRÉATION et la lecture des noms sont dans le point de + # reprise, pas seulement la purge. Un échec ici laissait la + # transaction avortée ; la lecture suivante mourait dessus, + # hors de tout garde, et le rapport entier était perdu. + # Les noms sont matérialisés tout de suite : après un retour + # arrière, les relire relancerait une requête. with env.cr.savepoint(): - line.purge() - ok += 1 + wizard = env[model].create({}) + todo = [ + (line, line.name or str(line.id)) + for line in wizard.purge_line_ids + ] + except UserError: + # « Aucun modèle orphelin trouvé » : le module signale le + # VIDE par une exception. Le compter comme un échec faisait + # passer une base saine pour une base cassée. + this_round.append({"kind": label, "purged": 0, + "errors": [], "would": []}) + continue except Exception as exc: - errors.append([name, str(exc)[:160]]) - if not DRY_RUN and ok: - env.cr.commit() - purged_this_round += ok - this_round.append({"kind": label, "purged": ok, - "errors": errors, "would": would}) - report["rounds"].append(this_round) - # On s'arrête quand une passe ENTIÈRE n'a plus rien réparé : ce qui - # résistait au tour d'avant résistera encore. En simulation, une seule - # passe suffit — rien ne change, donc rien ne se libère. - if purged_this_round == 0 or DRY_RUN: - break + note(label, "-", exc) + continue + for line, name in todo: + if DRY_RUN: + would.append(name) + continue + try: + # Un point de reprise par ENTRÉE : un refus n'emporte que + # la sienne, et la passe continue. Sans cela, le premier + # échec ferait perdre tout ce que la passe avait réparé. + with env.cr.savepoint(): + line.purge() + ok += 1 + except Exception as exc: + errors.append([name, str(exc)[:160]]) + if not DRY_RUN and ok: + try: + env.cr.commit() + except Exception as exc: + note(label, "commit", exc) + purged_this_round += ok + this_round.append({"kind": label, "purged": ok, + "errors": errors, "would": would}) + report["rounds"].append(this_round) + # On s'arrête quand une passe ENTIÈRE n'a plus rien réparé : ce qui + # résistait au tour d'avant résistera encore. En simulation, une + # seule passe suffit — rien ne change, donc rien ne se libère. + if purged_this_round == 0 or DRY_RUN: + break +except Exception as exc: + # Le rapport de CE QUI A ÉTÉ FAIT vaut plus que la trace de ce qui a + # cassé : sans lui, on ne sait même pas si la base a été touchée. + note("*", "fatal", exc) print("%(start)s") print(json.dumps(report)) @@ -200,6 +237,47 @@ def require_matching_version(database): return None +def module_state(database, module="database_cleanup"): + """L'état du module dans cette base, ou None si on ne peut pas lire.""" + env = os.environ.copy() + env["PGOPTIONS"] = "-c default_transaction_read_only=on" + env["PSQLRC"] = "" + done = subprocess.run( + [ + "psql", + "-X", + "-w", + "-d", + database, + "-tAc", + f"SELECT state FROM ir_module_module WHERE name='{module}';", + ], + capture_output=True, + text=True, + env=env, + ) + if done.returncode: + return None + return done.stdout.strip() or None + + +def install_module(database, module="database_cleanup", timeout=1800): + """Poser le module avant de s'en servir. + + Sans lui, aucun assistant n'existe et l'outil rend « rien à faire » sur + une base qui en aurait eu besoin — un silence qu'on prend pour un + succès. La migration l'installait plus tard, à l'étape 3 ; l'attendre + revenait à nettoyer trop tard. + """ + done = subprocess.run( + ["./script/addons/install_addons.sh", database, module], + capture_output=True, + text=True, + timeout=timeout, + ) + return done.returncode, done.stdout + done.stderr + + def run_shell(database, config_path, script, timeout=3600): """Pousser le script dans « odoo-bin shell » et rendre son rapport. @@ -267,6 +345,15 @@ def render(report, database): by_kind[kind] = by_kind.get(kind, 0) + 1 for kind, count in by_kind.items(): lines.append(f" - {t(LABEL.get(kind, kind))} : {count}") + # Les échecs comptent AUSSI en simulation : une catégorie qui ne + # s'ouvre même pas est une information, pas un silence. + for kind, name, message in report.get("failed", []): + lines.append(f" ⚠ [{kind}] {name} : {message[:90]}") + for kind in report.get("missing", []): + lines.append( + f" ℹ {t(LABEL.get(kind, kind))} :" + f" {t('no such wizard in this version, skipped.')}" + ) return "\n".join(lines) + "\n" total = 0 for index, this_round in enumerate(report.get("rounds", []), start=1): @@ -333,6 +420,18 @@ def main(argv=None): print(f"⛔ {mismatch}") return 2 + state = module_state(config.database) + if state != "installed": + print( + f"⧖ {t('database_cleanup is not installed on this base;')}" + f" {t('installing it first.')}" + ) + code, output = install_module(config.database) + if code: + print(output.strip()[-1500:]) + print(f"❌ {t('Could not install database_cleanup.')}") + return 2 + print(f"⧖ {t('Cleaning')} '{config.database}'…") try: report = run_shell( diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index 70b1f72..d24cee4 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -5266,6 +5266,18 @@ TRANSLATIONS = { "fr": "(défaut)", "en": "(default)", }, + "database_cleanup is not installed on this base;": { + "fr": "database_cleanup n'est pas installé sur cette base ;", + "en": "database_cleanup is not installed on this base;", + }, + "installing it first.": { + "fr": "installation préalable.", + "en": "installing it first.", + }, + "Could not install database_cleanup.": { + "fr": "Impossible d'installer database_cleanup.", + "en": "Could not install database_cleanup.", + }, "Nothing to decide yet": { "fr": "Rien à décider pour l'instant", "en": "Nothing to decide yet", diff --git a/script/todo/todo_upgrade.py b/script/todo/todo_upgrade.py index 7aec353..123104e 100755 --- a/script/todo/todo_upgrade.py +++ b/script/todo/todo_upgrade.py @@ -1767,32 +1767,12 @@ class TodoUpgrade: self.print_step(msg) self.add_comment_progression(msg) - if not self.dct_progression.get("state_3_install_clean_database"): - status, cmd_executed = self.todo_upgrade_execute( - f"./script/addons/install_addons.sh {database_name} database_cleanup", - single_source_odoo=True, - ) - if not status: - self.dct_progression["state_3_install_clean_database"] = True - self.write_config() - + # Le nettoyage est fait à l'étape 2, par l'outil, et il pose lui-même + # le module s'il manque. Redemander ici « avez-vous fini de nettoyer + # dans l'interface ? » ferait faire à la main ce qui vient d'être + # fait — et l'installation qui suivait arrivait APRÈS l'usage. if not self.dct_progression.get("state_3_clean_database"): - print( - f"✨ {t('Go to Settings / Technical / Cleanup... / Purge and')}" - f" {t('purge the obsolete modules')}" - ) - status = self.ask_gate( - "💬 Did you finish to clean database? Press y/Y to open" - " server with selenium, else ignore it" - f" {t('(b = go back to a previous step)')} : " - ).strip() - - if status.lower().strip() == "y": - self.todo.prompt_execute_selenium_and_run_db(database_name) - status = self.ask( - f"💬 {t('Press enter to continue step 3')} : " - ).strip() - + self.dct_progression["state_3_install_clean_database"] = True self.dct_progression["state_3_clean_database"] = True self.write_config() diff --git a/test/test_database_cleanup.py b/test/test_database_cleanup.py index 37d2587..f57ca3f 100755 --- a/test/test_database_cleanup.py +++ b/test/test_database_cleanup.py @@ -89,6 +89,39 @@ class FakeEnv(dict): self.cr = FakeCursor(journal) +class UserError(Exception): + """Celle que le script poussé importera : on fournit odoo.exceptions. + + Refaire une classe de son côté ne servirait à rien — « except » compare + des identités, pas des noms, et le test passerait à côté. + """ + + +def install_fake_odoo_exceptions(case): + """Rendre `from odoo.exceptions import UserError` possible ici.""" + import types + + odoo = sys.modules.get("odoo") or types.ModuleType("odoo") + exceptions = types.ModuleType("odoo.exceptions") + exceptions.UserError = UserError + avant_odoo = sys.modules.get("odoo") + avant_exc = sys.modules.get("odoo.exceptions") + sys.modules["odoo"] = odoo + sys.modules["odoo.exceptions"] = exceptions + + def remettre(): + for nom, valeur in ( + ("odoo", avant_odoo), + ("odoo.exceptions", avant_exc), + ): + if valeur is None: + sys.modules.pop(nom, None) + else: + sys.modules[nom] = valeur + + case.addCleanup(remettre) + + def run_script(models, max_round=10, dry_run=False): """Exécuter le script réellement poussé, et rendre (rapport, journal).""" import json @@ -146,7 +179,9 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): ] models = {cleanup.ORDER[0][1]: FakeModel(lines)} _report, got = run_script(models, max_round=1) - self.assertEqual(got.count(("savepoint", "enter")), 2) + # Un point de reprise par entrée, PLUS un pour la création et la + # lecture des noms : c'est là que la transaction s'était avortée. + self.assertEqual(got.count(("savepoint", "enter")), 3) def test_a_refusal_rolls_back_only_its_own(self): journal = [] @@ -161,7 +196,8 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): self.assertEqual(entry["purged"], 2) self.assertEqual([name for name, _msg in entry["errors"]], ["bad"]) self.assertEqual(got.count(("savepoint", "rollback")), 1) - self.assertEqual(got.count(("savepoint", "release")), 2) + # Deux entrées purgées + la création : trois relâchements. + self.assertEqual(got.count(("savepoint", "release")), 3) def test_a_wizard_that_cannot_even_be_created_is_recorded(self): models = {cleanup.ORDER[0][1]: FakeModel([], raise_on_create="boom")} @@ -170,6 +206,55 @@ class TestOneEntryCannotSinkThePass(unittest.TestCase): self.assertIn("boom", report["failed"][0][2]) +class TestTheReportSurvivesAnything(unittest.TestCase): + """Sans rapport, on ne sait même pas si la base a été touchée. + + Vécu : `create({})` échouait, l'erreur était notée mais la transaction + restait AVORTÉE. La lecture de nom suivante mourait dessus, hors de tout + garde, et le script entier s'arrêtait — aucun rapport, juste une trace. + """ + + def test_reading_the_names_is_inside_the_savepoint(self): + # C'est la lecture des noms qui déclenche la requête, pas la + # création : la laisser dehors était le défaut. + source = cleanup.build_script(1, False) + creation = source.index("wizard = env[model].create({})") + garde = source.rindex("with env.cr.savepoint():", 0, creation) + noms = source.index("line.name or str(line.id)") + self.assertLess(garde, creation) + self.assertLess(creation, noms) + + def test_a_failure_on_create_does_not_kill_the_run(self): + models = { + cleanup.ORDER[0][1]: FakeModel([], raise_on_create="boom"), + cleanup.ORDER[2][1]: FakeModel([FakeLine("colonne")]), + } + report, _got = run_script(models, max_round=1) + # La catégorie suivante a bien travaillé malgré l'échec de la + # première. + purged = {e["kind"]: e["purged"] for e in report["rounds"][0]} + self.assertEqual(purged.get("columns"), 1) + self.assertEqual(report["failed"][0][0], "models") + + def test_an_unexpected_failure_still_yields_a_report(self): + class Explosive(dict): + def __init__(self, journal): + super().__init__() + self.cr = FakeCursor(journal) + + def __contains__(self, key): + raise RuntimeError("registre en miettes") + + import json as _json + + journal = [] + namespace = {"env": Explosive(journal)} + exec(cleanup.build_script(1, False), namespace) # noqa: S102 + report = _json.loads(_json.dumps(namespace["report"])) + self.assertEqual(report["failed"][0][:2], ["*", "fatal"]) + self.assertIn("miettes", report["failed"][0][2]) + + class TestGoingRoundAgain(unittest.TestCase): def test_what_one_pass_refused_the_next_may_take(self): # LE point, et la raison même de boucler : une entrée refuse TANT QUE @@ -385,6 +470,94 @@ class TestTheMigrationRunsItBeforeTheSmokeTest(unittest.TestCase): self.assertIn("-d db_upgrade_18", lst_cmd[0]) +class TestNothingToPurgeIsNotAFailure(unittest.TestCase): + """Le module signale le VIDE par une exception. + + `raise UserError("No orphaned models found")` : le compter comme un + échec faisait passer une base saine pour une base cassée, avec quatre + avertissements sur cinq catégories. + """ + + def test_the_pushed_script_separates_it(self): + source = cleanup.build_script(1, False) + self.assertIn("except UserError:", source) + vide = source.index("except UserError:") + echec = source.index("except Exception as exc:\n note") + self.assertLess(vide, echec, "l'ordre des except décide") + + def test_it_is_reported_as_zero_not_as_an_error(self): + install_fake_odoo_exceptions(self) + + class Empty(FakeModel): + def create(self, values): + raise UserError("No orphaned models found") + + models = {cleanup.ORDER[0][1]: Empty([])} + report, _got = run_script(models, max_round=1) + entry = report["rounds"][0][0] + self.assertEqual(entry["purged"], 0) + self.assertEqual(entry["errors"], []) + self.assertEqual(report["failed"], []) + + +class TestTheModuleIsInstalledFirst(unittest.TestCase): + """Sans le module, aucun assistant n'existe et l'outil dit « rien ». + + Ce silence se lit comme un succès. La migration l'installait à l'étape + 3, donc APRÈS le nettoyage de l'étape 2 : l'ordre rendait l'outil + inutile au premier passage. + """ + + def test_it_checks_the_state_before_cleaning(self): + import inspect + + source = inspect.getsource(cleanup.main) + self.assertIn("module_state(", source) + self.assertLess( + source.index("module_state("), source.index("run_shell(") + ) + + def test_it_installs_when_absent(self): + import inspect + + source = inspect.getsource(cleanup.main) + self.assertIn('state != "installed"', source) + self.assertIn("install_module(", source) + + def test_a_failed_install_stops_there(self): + # Nettoyer sans le module rendrait « rien à faire » sur une base qui + # en avait besoin. + import inspect + + source = inspect.getsource(cleanup.main) + install = source.index("install_module(") + self.assertIn("return 2", source[install : install + 400]) + + +class TestTheMigrationNoLongerAsksTwice(unittest.TestCase): + def test_the_manual_cleanup_prompt_is_gone(self): + # Le faire à la main après l'avoir fait automatiquement. + import inspect + + from script.todo.todo_upgrade import TodoUpgrade + + source = inspect.getsource(TodoUpgrade.execute_odoo_upgrade) + self.assertNotIn("Did you finish to clean database", source) + self.assertNotIn("Go to Settings / Technical / Cleanup", source) + + def test_the_late_install_is_gone_too(self): + # Elle arrivait à l'étape 3, après l'usage de l'étape 2 : l'outil + # pose désormais le module lui-même, au bon moment. + import inspect + + from script.todo.todo_upgrade import TodoUpgrade + + source = inspect.getsource(TodoUpgrade.execute_odoo_upgrade) + self.assertNotIn( + "install_addons.sh {database_name} database_cleanup", source + ) + + class TestItRefusesTheWrongOdooVersion(unittest.TestCase): """Un Odoo plus ancien sur une base plus récente ÉCRIT avant d'échouer."""