From 37c9f216def773268f9059e2981c2c2c845870b0 Mon Sep 17 00:00:00 2001 From: Mathieu Benoit Date: Thu, 20 Aug 2026 01:52:35 -0400 Subject: [PATCH] [ADD] migration quality: name the missing files, and show every delta MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit « 254 attachment files missing » does not say which. « m » now lists them, grouped by model AND FIELD — the field is the most useful thing in the row, because it says which field lost its image. On a real migration it splits into 234 res.country/image flags, 13 payment.provider logos, and a handful of scattered attachments including one 255 kB event photo: the first two are module data a reinstall restores, the last is not, and the raw list put them on the same footing. The metadata is read ONLY when asked. One more query per database would lengthen a survey that runs in four seconds, for something one looks at by asking for it. And every figure of a step now carries its change against the previous one. « 2283 views » is a number; « +61 » is information. The first step gets none: inventing a zero there would claim a comparison that does not exist. --- FR --- [ADD] qualité de migration : nommer les fichiers absents, et montrer les écarts « 254 fichiers de pièces jointes absents » ne dit pas lesquels. « m » en donne la liste, groupée par modèle ET PAR CHAMP — le champ est le renseignement le plus utile de la ligne, car il dit lequel a perdu son image. Sur une vraie migration : 234 drapeaux res.country/image, 13 logos de fournisseurs de paiement, et une poignée de pièces jointes éparses dont une photo d'événement de 255 ko. Les deux premiers groupes sont des données de module qu'une réinstallation restaure, le dernier non. Les métadonnées ne sont lues QU'À LA DEMANDE : une requête de plus par base allongerait un parcours qui tient en quatre secondes. Et chaque chiffre d'un palier porte son écart avec le précédent. « 2283 vues » est un nombre, « +61 » est une information. Le premier palier n'en a pas : y inventer un zéro annoncerait une comparaison qui n'existe pas. Assisted-by: Claude Opus 5 --- script/analyse/check_migration_quality.py | 104 ++++++++ script/analyse/check_migration_quality_tui.py | 110 +++++++-- script/todo/todo_i18n.py | 28 +++ test/test_check_migration_quality.py | 228 ++++++++++++++++++ 4 files changed, 454 insertions(+), 16 deletions(-) diff --git a/script/analyse/check_migration_quality.py b/script/analyse/check_migration_quality.py index ff08063..bfdb363 100755 --- a/script/analyse/check_migration_quality.py +++ b/script/analyse/check_migration_quality.py @@ -226,9 +226,113 @@ def inspect(database): ) absents = missing_files(database, [ligne[0] for ligne in stockees or []]) etat["attachment_missing"] = None if absents is None else len(absents) + # Les NOMS, pas seulement le compte : sans eux, « 254 fichiers + # absents » ne dit pas lesquels, et l'on ne peut ni juger de la + # gravité ni retrouver ce qui a disparu. Bornés, car une base peut en + # aligner des dizaines de milliers et l'écran n'en montrera jamais tant. + etat["attachment_missing_list"] = (absents or [])[:MAX_MISSING] return etat +MAX_MISSING = 5000 + + +def missing_detail(database, lst_store_fname, limit=400): + """Les métadonnées des pièces jointes dont le fichier a disparu. + + À la DEMANDE, jamais pendant l'inspection : une requête de plus par + base allongerait un parcours qui tient aujourd'hui en quatre secondes, + pour une information qu'on ne regarde qu'en la demandant. + + `res_field` est le renseignement le plus utile du lot : il dit QUEL + champ a perdu son image — l'`image_1920` d'un pays n'a pas le même + poids qu'une pièce jointe de facture. + """ + if not lst_store_fname: + return [] + lst = list(lst_store_fname)[:limit] + valeurs = ", ".join("'" + nom.replace("'", "''") + "'" for nom in lst) + lignes = run_psql( + database, + "SELECT store_fname, coalesce(res_model, '-')," + " coalesce(res_field, '-'), coalesce(res_id::text, '-')," + " coalesce(mimetype, '-'), coalesce(file_size::text, '0')," + " coalesce(name, '-')" + f" FROM ir_attachment WHERE store_fname IN ({valeurs})" + " ORDER BY res_model, res_field, name", + ) + return [ + { + "store_fname": ligne[0], + "model": ligne[1], + "field": ligne[2], + "res_id": ligne[3], + "mimetype": ligne[4], + "size": int(ligne[5]) if ligne[5].isdigit() else 0, + "name": ligne[6], + } + for ligne in lignes or [] + if len(ligne) >= 7 + ] + + +def render_missing(etat, colour=False, limit=60): + """Ce qui manque, groupé d'abord, détaillé ensuite. + + Le groupement d'abord parce qu'il tranche : deux cent trente-trois + drapeaux de pays et treize logos de fournisseurs de paiement sont des + images livrées par les modules, qu'une mise à jour restaure. Huit + pièces jointes éparses, non. La liste brute mettait les deux sur le + même plan. + """ + from script.todo.migration_status import paint + + absents = etat.get("attachment_missing") or 0 + if not absents: + return f"✅ {t('every attachment file is present')}" + lignes = [ + paint( + f"❌ {absents}" + f" {t('attachment files missing from the filestore')}", + "fail", + colour, + ), + f" {etat['database']}", + "", + ] + detail = missing_detail( + etat["database"], etat.get("attachment_missing_list") or [] + ) + if not detail: + lignes.append(t("Could not read their metadata.")) + return "\n".join(lignes) + + groupe = {} + for item in detail: + cle = (item["model"], item["field"], item["mimetype"]) + groupe[cle] = groupe.get(cle, 0) + 1 + lignes.append(f"── {t('by model and field')} ──") + for (modele, champ, mime), nombre in sorted( + groupe.items(), key=lambda x: -x[1] + ): + lignes.append( + f" {nombre:>5} {paint(modele, 'step', colour):<34}" + f" {champ:<22} {mime}" + ) + lignes.append("") + lignes.append(f"── {t('one by one')} ──") + for item in detail[:limit]: + lignes.append( + f" {item['model']}#{item['res_id']}" + f" {paint(item['field'], 'dim', colour)}" + f" {item['size']:>8} o {item['name'][:44]}" + ) + lignes.append(f" {paint(item['store_fname'], 'cmd', colour)}") + if len(detail) > limit: + lignes.append(f" … {len(detail) - limit} {t('more')}") + return "\n".join(lignes) + + def compare(avant, apres): """Ce qui a été gagné et ce qui a été perdu entre deux paliers.""" if not avant.get("exists") or not apres.get("exists"): diff --git a/script/analyse/check_migration_quality_tui.py b/script/analyse/check_migration_quality_tui.py index dd8d5a9..fee2360 100644 --- a/script/analyse/check_migration_quality_tui.py +++ b/script/analyse/check_migration_quality_tui.py @@ -64,6 +64,9 @@ def rows(lst_snapshot): if rang: precedent = presents[rang - 1] diff = quality.compare(precedent, etat) if precedent else None + # Le précédent voyage avec la ligne : le panneau en a besoin pour + # écrire « 171 (−43) », et le recalculer là-bas ferait deux + # sources pour la même comparaison. perdu = ( 0 if diff is None or diff.get("unavailable") @@ -76,6 +79,7 @@ def rows(lst_snapshot): "detail": str(perdu) if perdu else "", "data": etat, "diff": diff, + "previous": precedent, } ) if len(presents) >= 2: @@ -107,34 +111,87 @@ def head_text(lst_snapshot): ) -def pane_text(lst_snapshot, row, colour=False): +def statistics(etat, precedent): + """[(libellé, valeur, écart)] pour un palier. L'écart est None au départ. + + Un chiffre seul ne dit rien : 2283 vues est un nombre, « +61 » est une + information. Le premier palier n'a pas de précédent, et inventer un + écart de zéro y laisserait croire à une comparaison qui n'existe pas. + """ + lst = [ + ( + t("modules"), + len(etat["installed"]), + None if not precedent else len(precedent["installed"]), + ), + ( + t("models"), + len(etat["model"]), + None if not precedent else len(precedent["model"]), + ), + ( + t("views"), + etat["view"], + None if not precedent else precedent["view"], + ), + ( + " · COW", + etat["view_cow"], + None if not precedent else precedent["view_cow"], + ), + ( + t("menus"), + etat["menu"], + None if not precedent else precedent["menu"], + ), + ( + t("attachments"), + etat["attachment"], + None if not precedent else precedent["attachment"], + ), + ] + return [ + (libelle, valeur, None if avant is None else valeur - avant) + for libelle, valeur, avant in lst + ] + + +def pane_text(lst_snapshot, row, colour=False, show_missing=False): """Le détail du palier choisi, ou le bilan d'ensemble.""" if row is None: return t("Nothing to show yet.") if row["kind"] == "missing": return f"⚠️ {row['data']['database']} : {t('database not found')}" - lignes = [] etat = row["data"] + if show_missing: + if not etat: + return t("Pick a step to see its missing files.") + return quality.render_missing(etat, colour) + lignes = [] if etat: lignes.append( status.paint(f"{etat['odoo']} {etat['database']}", "step", colour) ) lignes.append("") - for libelle, valeur in ( - (t("modules"), len(etat["installed"])), - (t("models"), len(etat["model"])), - (t("views"), etat["view"]), - (" · COW", etat["view_cow"]), - (t("menus"), etat["menu"]), - (t("attachments"), etat["attachment"]), - ): - lignes.append(f" {libelle:<28} {valeur:>7}") + for libelle, valeur, ecart in statistics(etat, row.get("previous")): + if ecart is None: + marque = "" + else: + marque = status.paint( + f"{ecart:+d}", "ok" if ecart >= 0 else "warn", colour + ) + lignes.append(f" {libelle:<28} {valeur:>7} {marque}") if etat.get("attachment_missing"): lignes.append( - f" {status.paint('❌ ' + t('attachment files missing from' - ' the filestore'), 'fail', colour)}" - f" {etat['attachment_missing']}" + " " + + status.paint( + f"❌ {etat['attachment_missing']}" + f" {t('attachment files missing from the filestore')}", + "fail", + colour, + ) ) + lignes.append(f" {t('press m to list them')}") lignes.append("") diff = row.get("diff") if diff: @@ -151,13 +208,17 @@ def build_app(lst_snapshot): class QualityApp(App): CSS = globals()["CSS"] - BINDINGS = [("q,escape", "quit", t("Quit"))] + BINDINGS = [ + ("q,escape", "quit", t("Quit")), + ("m", "toggle_missing", t("Missing files")), + ] def __init__(self, lst_snapshot): super().__init__() self.lst_snapshot = lst_snapshot self.lst_row = rows(lst_snapshot) self.index = 0 + self.show_missing = False def compose(self) -> ComposeResult: yield Header() @@ -186,9 +247,26 @@ def build_app(lst_snapshot): else None ) self.query_one("#content", Static).update( - Text.from_ansi(pane_text(self.lst_snapshot, row, colour=True)) + Text.from_ansi( + pane_text( + self.lst_snapshot, + row, + colour=True, + show_missing=self.show_missing, + ) + ) ) + def action_toggle_missing(self): + """Basculer entre les chiffres du palier et ses fichiers absents. + + Les métadonnées ne sont lues qu'ICI : une requête de plus par + base allongerait un parcours qui tient en quatre secondes, pour + une information qu'on ne regarde qu'en la demandant. + """ + self.show_missing = not self.show_missing + self._show() + def on_data_table_row_highlighted(self, event): if event.data_table.id == "left" and self.lst_row: self.index = event.cursor_row diff --git a/script/todo/todo_i18n.py b/script/todo/todo_i18n.py index 03f72e4..58198f3 100644 --- a/script/todo/todo_i18n.py +++ b/script/todo/todo_i18n.py @@ -5755,6 +5755,34 @@ TRANSLATIONS = { "fr": "processus.", "en": "process instead.", }, + "Missing files": { + "fr": "Fichiers absents", + "en": "Missing files", + }, + "every attachment file is present": { + "fr": "tous les fichiers de pièces jointes sont présents", + "en": "every attachment file is present", + }, + "Could not read their metadata.": { + "fr": "Impossible de lire leurs métadonnées.", + "en": "Could not read their metadata.", + }, + "by model and field": { + "fr": "par modèle et par champ", + "en": "by model and field", + }, + "one by one": { + "fr": "un par un", + "en": "one by one", + }, + "press m to list them": { + "fr": "m pour en voir la liste", + "en": "press m to list them", + }, + "Pick a step to see its missing files.": { + "fr": "Choisissez un palier pour voir ses fichiers absents.", + "en": "Pick a step to see its missing files.", + }, "Clean the database before testing the pages?": { "fr": "Nettoyer la base avant de tester les pages ?", "en": "Clean the database before testing the pages?", diff --git a/test/test_check_migration_quality.py b/test/test_check_migration_quality.py index eb264e6..21a8c49 100644 --- a/test/test_check_migration_quality.py +++ b/test/test_check_migration_quality.py @@ -284,6 +284,234 @@ class TestTheReportItself(Base): self.assertLess(texte.index("12.0"), texte.index("From start")) +class TestTheStatisticsCarryTheirDelta(Base): + """Un chiffre seul ne dit rien. + + « 2283 vues » est un nombre ; « +61 » est une information. C'est + l'écart qu'on lit, pas la valeur. + """ + + def test_each_figure_gets_its_change(self): + lignes = qtui.statistics( + snapshot(view=2283, installed=["a"]), + snapshot(view=2222, installed=["a", "b"]), + ) + par_nom = {libelle: ecart for libelle, _v, ecart in lignes} + self.assertEqual(par_nom["views"], 61) + self.assertEqual(par_nom["modules"], -1) + + def test_the_first_step_has_NO_delta(self): + # Inventer un écart de zéro laisserait croire à une comparaison + # qui n'existe pas : il n'y a rien avant le premier palier. + lignes = qtui.statistics(snapshot(), None) + self.assertTrue(all(ecart is None for _l, _v, ecart in lignes)) + + def test_an_unchanged_figure_shows_zero_not_nothing(self): + # Zéro est une réponse : « rien n'a bougé » se distingue de « on + # ne sait pas ». + lignes = qtui.statistics(snapshot(view=100), snapshot(view=100)) + par_nom = {libelle: ecart for libelle, _v, ecart in lignes} + self.assertEqual(par_nom["views"], 0) + + def test_every_figure_of_the_pane_is_covered(self): + libelles = [ + libelle + for libelle, _v, _e in qtui.statistics(snapshot(), snapshot()) + ] + for attendu in ("modules", "models", "views", "menus", "attachments"): + self.assertIn(attendu, libelles) + + def test_the_pane_prints_the_sign(self): + row = { + "kind": "step", + "data": snapshot(view=2283), + "previous": snapshot(view=2222), + "diff": None, + } + texte = qtui.pane_text([], row) + self.assertIn("+61", texte) + + +class TestListingTheMissingFiles(Base): + """« 254 fichiers absents » ne dit pas lesquels. + + Le groupement tranche : deux cent trente-quatre drapeaux de pays sont + des images livrées par un module, qu'une mise à jour restaure. Une + pièce jointe d'événement de 255 ko, non. La liste brute mettait les + deux sur le même plan. + """ + + DETAIL = [ + { + "store_fname": "aa/1", + "model": "res.country", + "field": "image", + "res_id": "1", + "mimetype": "image/png", + "size": 100, + "name": "fr", + }, + { + "store_fname": "aa/2", + "model": "res.country", + "field": "image", + "res_id": "2", + "mimetype": "image/png", + "size": 100, + "name": "ca", + }, + { + "store_fname": "aa/4", + "model": "res.country", + "field": "image_128", + "res_id": "1", + "mimetype": "image/png", + "size": 50, + "name": "fr128", + }, + { + "store_fname": "bb/3", + "model": "calendar.event", + "field": "-", + "res_id": "1", + "mimetype": "image/png", + "size": 255603, + "name": "photo.png", + }, + ] + + def render(self, etat=None, **kw): + original = quality.missing_detail + quality.missing_detail = lambda db, lst, limit=400: self.DETAIL + self.addCleanup(setattr, quality, "missing_detail", original) + return quality.render_missing( + etat + or snapshot( + attachment_missing=3, + attachment_missing_list=["aa/1", "aa/2", "bb/3"], + ), + **kw, + ) + + def test_nothing_missing_says_so_plainly(self): + texte = quality.render_missing(snapshot(attachment_missing=0)) + self.assertIn("every attachment file is present", texte) + + def test_the_grouping_comes_first(self): + texte = self.render() + self.assertLess( + texte.index("by model and field"), texte.index("one by one") + ) + + def test_the_grouping_counts_by_model_AND_field(self): + """Le champ est le renseignement le plus utile du lot. + + Il dit QUEL champ a perdu son image : l'`image_1920` d'un pays n'a + pas le même poids qu'une pièce jointe de facture. Grouper sur le + seul modèle fondrait `image` et `image_128` en une ligne, et l'on + perdrait exactement ce qu'on venait chercher. + """ + entete = self.render().split("one by one")[0] + lignes = [ + ligne for ligne in entete.splitlines() if "res.country" in ligne + ] + self.assertEqual(len(lignes), 2, entete) + self.assertTrue(any("image_128" in ligne for ligne in lignes)) + + def test_the_loudest_group_comes_first(self): + texte = self.render().split("one by one")[0] + self.assertLess(texte.index("res.country"), texte.index("calendar")) + + def test_each_file_names_its_record_and_its_path(self): + texte = self.render() + self.assertIn("calendar.event#1", texte) + self.assertIn("bb/3", texte) + self.assertIn("photo.png", texte) + + def test_a_long_list_is_cut_and_SAYS_so(self): + texte = self.render(limit=1) + self.assertIn("more", texte) + + def test_unreadable_metadata_is_admitted(self): + original = quality.missing_detail + quality.missing_detail = lambda db, lst, limit=400: [] + self.addCleanup(setattr, quality, "missing_detail", original) + texte = quality.render_missing( + snapshot(attachment_missing=3, attachment_missing_list=["a"]) + ) + self.assertIn("Could not read", texte) + + def test_the_metadata_is_read_ONLY_on_demand(self): + # Une requête de plus par base allongerait un parcours qui tient + # en quatre secondes, pour ce qu'on ne regarde qu'en le demandant. + import inspect + + self.assertNotIn("missing_detail", inspect.getsource(quality.inspect)) + self.assertIn( + "missing_detail", inspect.getsource(quality.render_missing) + ) + + def test_the_names_are_kept_but_bounded(self): + # Une base peut en aligner des dizaines de milliers ; l'écran n'en + # montrera jamais tant, et les garder toutes coûterait pour rien. + import inspect + + source = inspect.getsource(quality.inspect) + self.assertIn("attachment_missing_list", source) + self.assertIn("MAX_MISSING", source) + + def test_a_quote_in_a_filename_cannot_break_the_query(self): + vu = {} + original = quality.run_psql + quality.run_psql = lambda db, sql: vu.setdefault("sql", sql) and [] + self.addCleanup(setattr, quality, "run_psql", original) + quality.missing_detail("db", ["aa/o'brien"]) + self.assertIn("o''brien", vu["sql"]) + + +class TestTheMissingFilesButton(Base): + def test_m_is_bound(self): + app = qtui.build_app([snapshot()]) + touches = { + touche + for entree in app.BINDINGS + for touche in entree[0].split(",") + } + self.assertIn("m", touches) + + def test_the_action_exists(self): + app = qtui.build_app([snapshot()]) + self.assertTrue(hasattr(app, "action_toggle_missing")) + + def test_the_pane_switches_to_the_list(self): + original = quality.missing_detail + quality.missing_detail = lambda db, lst, limit=400: [] + self.addCleanup(setattr, quality, "missing_detail", original) + row = { + "kind": "step", + "data": snapshot( + attachment_missing=3, attachment_missing_list=["a"] + ), + "previous": None, + "diff": None, + } + chiffres = qtui.pane_text([], row, show_missing=False) + liste = qtui.pane_text([], row, show_missing=True) + self.assertIn("modules", chiffres) + self.assertNotIn("modules", liste) + + def test_the_pane_tells_you_the_key_exists(self): + # Une touche que rien n'annonce est une touche que personne ne + # presse. + row = { + "kind": "step", + "data": snapshot(attachment_missing=254), + "previous": None, + "diff": None, + } + self.assertIn("press m", qtui.pane_text([], row)) + + class TestItNeverWrites(Base): """Les bases de palier sont parfois la seule copie d'un état."""