From e66baee2f6d981c52e2b70c0e47f318767c0d8c1 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Fri, 26 Jun 2026 14:09:59 -0700 Subject: [PATCH] Standardize per-row action links across Data Health checks (#1405) Every Data Health check now gives each flagged row an admin action link, so admins stay one click from the fix. Previously only 3 of 10 checks did. - HealthCheck base gains link_model / link_id_key attrs and a default row_link() that deep-links a row to its admin change page. - Wire the 7 linkless checks in: publication-quality, duplicate-people, news-health, project-health, project-leadership (default link), position-integrity (person_id key), and url-name-collisions (custom link to the Person changelist filtered by the offending url_name). - Add url_name to PersonAdmin.search_fields so that filtered link resolves. - Test pins that every registered check provides an action link. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../data_health/checks/duplicate_people.py | 1 + .../admin/data_health/checks/news_health.py | 1 + .../data_health/checks/position_integrity.py | 2 ++ .../data_health/checks/project_health.py | 1 + .../data_health/checks/project_leadership.py | 1 + .../data_health/checks/publication_quality.py | 1 + .../data_health/checks/url_name_collisions.py | 11 +++++++ website/admin/data_health/registry.py | 29 +++++++++++++++---- website/admin/person_admin.py | 6 ++-- website/tests/test_data_health.py | 20 ++++++++++++- 10 files changed, 65 insertions(+), 8 deletions(-) diff --git a/website/admin/data_health/checks/duplicate_people.py b/website/admin/data_health/checks/duplicate_people.py index 70bc9db4..faa4fccc 100644 --- a/website/admin/data_health/checks/duplicate_people.py +++ b/website/admin/data_health/checks/duplicate_people.py @@ -31,6 +31,7 @@ class DuplicatePeopleCheck(HealthCheck): 'delete shell.' ) group = 'People' + link_model = 'person' columns = [ 'cluster_key', 'id', 'first_name', 'middle_name', 'last_name', 'url_name', 'email', 'personal_website', 'github', 'linkedin', diff --git a/website/admin/data_health/checks/news_health.py b/website/admin/data_health/checks/news_health.py index 52f2d887..d2418a5d 100644 --- a/website/admin/data_health/checks/news_health.py +++ b/website/admin/data_health/checks/news_health.py @@ -18,6 +18,7 @@ class NewsHealthCheck(HealthCheck): title = 'News health' description = "News items missing a slug or an author." group = 'People' + link_model = 'news' columns = ['id', 'title', 'date', 'missing_slug', 'has_author'] def get_rows(self): diff --git a/website/admin/data_health/checks/position_integrity.py b/website/admin/data_health/checks/position_integrity.py index 6d58269d..adf75374 100644 --- a/website/admin/data_health/checks/position_integrity.py +++ b/website/admin/data_health/checks/position_integrity.py @@ -22,6 +22,8 @@ class PositionIntegrityCheck(HealthCheck): 'themselves.' ) group = 'People' + link_model = 'person' + link_id_key = 'person_id' columns = ['person_id', 'name', 'issue', 'detail'] def get_rows(self): diff --git a/website/admin/data_health/checks/project_health.py b/website/admin/data_health/checks/project_health.py index b0b7c113..ebdbc2b7 100644 --- a/website/admin/data_health/checks/project_health.py +++ b/website/admin/data_health/checks/project_health.py @@ -23,6 +23,7 @@ class ProjectHealthCheck(HealthCheck): 'site), with no currently-active members, or with no umbrella.' ) group = 'Projects' + link_model = 'project' columns = [ 'id', 'name', 'short_name', 'is_visible', 'has_thumbnail', 'has_publication', 'active_member_count', 'has_umbrella', 'issues', diff --git a/website/admin/data_health/checks/project_leadership.py b/website/admin/data_health/checks/project_leadership.py index f155b411..a961456f 100644 --- a/website/admin/data_health/checks/project_leadership.py +++ b/website/admin/data_health/checks/project_leadership.py @@ -30,6 +30,7 @@ class ProjectLeadershipCheck(HealthCheck): 'have all ended (no currently-active PI).' ) group = 'Projects' + link_model = 'project' columns = [ 'id', 'name', 'short_name', 'is_visible', 'has_ended', 'pi_count', 'active_pi_count', 'copi_count', 'pis', 'issues', diff --git a/website/admin/data_health/checks/publication_quality.py b/website/admin/data_health/checks/publication_quality.py index ab255601..3131a75b 100644 --- a/website/admin/data_health/checks/publication_quality.py +++ b/website/admin/data_health/checks/publication_quality.py @@ -27,6 +27,7 @@ class PublicationQualityCheck(HealthCheck): 'and publications that share a normalized title (possible duplicates).' ) group = 'Artifacts' + link_model = 'publication' columns = ['id', 'title', 'date', 'missing_fields', 'dup_title'] def get_rows(self): diff --git a/website/admin/data_health/checks/url_name_collisions.py b/website/admin/data_health/checks/url_name_collisions.py index c15caac2..42cbf415 100644 --- a/website/admin/data_health/checks/url_name_collisions.py +++ b/website/admin/data_health/checks/url_name_collisions.py @@ -8,6 +8,9 @@ """ from collections import defaultdict +from urllib.parse import urlencode + +from django.urls import reverse from website.admin.data_health.registry import HealthCheck, register_check from website.models import Person @@ -45,3 +48,11 @@ def get_rows(self): # Worst offenders first, then alphabetical. rows.sort(key=lambda r: (-r['count'], r['url_name'])) return rows + + def row_link(self, row): + """Each row is a cluster of people (not a single object), so link to the + Person changelist searched for the offending url_name — surfacing the + rows to renumber/merge. (``url_name`` is in PersonAdmin.search_fields.) + """ + url = reverse('admin:website_person_changelist') + return ('Open in admin →', f"{url}?{urlencode({'q': row['url_name']})}") diff --git a/website/admin/data_health/registry.py b/website/admin/data_health/registry.py index a681dd42..8aa92581 100644 --- a/website/admin/data_health/registry.py +++ b/website/admin/data_health/registry.py @@ -13,6 +13,7 @@ import io from django.http import HttpResponse +from django.urls import reverse from django.utils import timezone @@ -34,18 +35,36 @@ class HealthCheck: group = 'Other' #: Ordered column keys; used as table headers and the CSV header row. columns = [] + #: Lowercase ``website`` model name (e.g. ``"publication"``) whose admin + #: change page each row links to. Leave ``None`` for checks that build + #: their own link (or none) by overriding :meth:`row_link`. + link_model = None + #: Row key holding the primary key used to build the default link. + link_id_key = 'id' def get_rows(self): """Return a list of row dicts (read-only). Override in subclasses.""" raise NotImplementedError def row_link(self, row): - """Optional: a ``(label, url)`` pair to act on ``row`` from the detail - page, or ``None``. Lets a check wire its read-only findings to a fixer - elsewhere in the admin (e.g. the keyword-merge changelist). Default: no - link. Only affects the on-screen table — the CSV export is unchanged. + """Return an ``(label, url)`` pair to act on ``row`` from the detail + page, or ``None``. + + Default behavior keeps every flagged row one click from its fix: when + the subclass sets :attr:`link_model` and the row carries a primary key + under :attr:`link_id_key`, deep-link to that object's admin change page. + Checks with a non-trivial target — clusters, per-row model types, or a + filtered changelist — override this instead (e.g. the keyword-merge + changelist). Only affects the on-screen table; the CSV export is + unchanged. """ - return None + if not self.link_model: + return None + obj_id = row.get(self.link_id_key) + if not obj_id: + return None + url = reverse(f'admin:website_{self.link_model}_change', args=[obj_id]) + return ('Open →', url) def count(self): """Number of flagged rows. Override for a cheaper count if available.""" diff --git a/website/admin/person_admin.py b/website/admin/person_admin.py index e7da8523..60aba53f 100644 --- a/website/admin/person_admin.py +++ b/website/admin/person_admin.py @@ -175,8 +175,10 @@ def save_model(self, request, obj, form, change): # see: https://docs.djangoproject.com/en/1.11/ref/contrib/admin/#inlinemodeladmin-objects inlines = [PositionInline, ProjectRoleInline] - # We must define search_fields in order to use the autocomplete_fields option - search_fields = ['first_name', 'last_name',] + # We must define search_fields in order to use the autocomplete_fields option. + # url_name is included so the Data Health "url_name collisions" check can deep-link + # here with ?q= to surface the colliding rows. + search_fields = ['first_name', 'last_name', 'url_name',] def get_search_results(self, request, queryset, search_term): """Role-filter the admin autocomplete results for advisor/mentor fields (#1126). diff --git a/website/tests/test_data_health.py b/website/tests/test_data_health.py index f082b58a..052c8c80 100644 --- a/website/tests/test_data_health.py +++ b/website/tests/test_data_health.py @@ -14,7 +14,7 @@ from django.test import SimpleTestCase, override_settings from django.urls import reverse -from website.admin.data_health.registry import get_check +from website.admin.data_health.registry import REGISTRY, HealthCheck, get_check from website.tests.base import DatabaseTestCase from website.utils.name_utils import normalize_person_name, is_default_person_image @@ -331,6 +331,24 @@ def test_missing_file_row_links_to_admin_edit(self): ) +class ActionLinkStandardizationTests(SimpleTestCase): + """Every registered check must give its rows an action link (issue #1405), + keeping admins one click from the fix. A check qualifies by either declaring + ``link_model`` (default deep-link to the object's admin change page) or + overriding ``row_link`` (custom target). This pins the standardization so a + future check can't silently ship without one.""" + + def test_every_check_provides_an_action_link(self): + offenders = [ + c.slug for c in REGISTRY + if c.link_model is None + and type(c).row_link is HealthCheck.row_link + ] + self.assertEqual( + offenders, [], f"checks lacking a per-row action link: {offenders}" + ) + + class DataHealthReadOnlyTests(DatabaseTestCase): def test_get_rows_does_not_mutate_db(self): from website.models import Person, Publication