Crls/feat/show manager username and counts in catalog admin

[ccantillo]

Show counts and manager usernames in PartnerCatalog admin list

Summary

Improves the PartnerCatalog change list in Django admin so that admins
can see at a glance how many learners and courses each catalog has, and
who is managing it — without leaving the list view.

---

What changed

Column behaviour

The Add Learner, Add Course, and Add Manager columns now work
on two levels:

┌─────────────────────────────────────────────────────────────────┐
│  Column header (clickable)  →  opens a blank add form           │
│  Cell value   (clickable)   →  opens the add form pre-filled    │
│                                with the catalog already selected │
└─────────────────────────────────────────────────────────────────┘

Element	What you see	Where it goes
Column header	Add Learner / Add Course / Add Manager link	Generic add form — no catalog pre-selected
Cell value	Learner count · Course count · Manager username(s)	Add form with the catalog field already filled in

Visual flow:

 ┌──────────────┬─────────────┬──────────────┐
 │ [Add Learner]│ [Add Course]│ [Add Manager]│  ← header links (blank form)
 ├──────────────┼─────────────┼──────────────┤
 │     [42]     │    [10]     │   [jsmith]   │  ← cell links  (pre-filled)
 │     [0]      │    [5]      │     [—]      │
 └──────────────┴─────────────┴──────────────┘

• Learner and course cells show the count as the link text.
• Manager cells show the active manager's username as the link text,
  or — if no active manager is assigned.
• Multiple active managers are listed one per line.

---

Why a custom admin template was added

Django admin assigns CSS classes to table cells automatically
(.field-<method_name> for <td>, .column-<method_name> for <th>),
but there is no way to set text-align on those cells from inside the
cell's own HTML. Any inline style placed on the <a> or wrapper element
inside the cell only affects that inner element — it cannot reach the
<td> or <th> container.

To center the column headers and cell values without touching global
admin CSS, we added a minimal template override:

partner_catalog/templates/admin/partner_catalog/partnercatalog/change_list.html

This file extends Django's stock admin/change_list.html and injects a
small <style> block scoped to #result_list that targets only the
three affected columns:

#result_list .column-add_learner,
#result_list .column-add_course,
#result_list .column-add_manager,
#result_list .field-add_learner,
#result_list .field-add_course,
#result_list .field-add_manager {
  text-align: center;
}

The template is placed in the app's own templates/ directory and is
scoped to the partnercatalog change list only, so it has no effect on
any other admin view.

[ManuelStarDo]

Code Review

Recommendation: Request changes — one blocking issue

🔴 Blocking

add_manager() bypasses HTML escaping — potential stored XSS

def add_manager(self, obj):
    ...
    if active_managers:
        return format_html(
            "<br>".join(
                f'<a href="{full_url}" style="font-weight:bold;">{m.user.username}</a>'
                for m in active_managers
            )
        )

This builds the <a> tag via an f-string first, then passes the already-assembled string to format_html() with no {} placeholders. Since no values are passed as separate arguments, format_html has nothing to escape — m.user.username is inserted raw. If a username ever contains <, >, or &, this is a stored XSS vector rendered in the Django admin change list.

Please use format_html_join, which is the idiomatic Django tool for joining multiple safely-formatted HTML fragments with a separator, and removes the risk of a future edit reintroducing an unescaped value:

from django.utils.html import format_html_join
from django.utils.safestring import mark_safe

def add_manager(self, obj):
    """Display active manager usernames as pre-filled links to add a new manager."""
    add_url = reverse(
        f"admin:{CatalogManager._meta.app_label}_{CatalogManager._meta.model_name}_add"
    )
    full_url = f"{add_url}?catalog={obj.pk}"
    active_managers = [m for m in obj.catalog_managers.all() if m.active]
    if active_managers:
        return format_html_join(
            mark_safe("<br>"),
            '<a href="{}" style="font-weight:bold;">{}</a>',
            ((full_url, m.user.username) for m in active_managers),
        )
    return format_html(
        '<a href="{}" style="font-weight:bold;">—</a>',
        full_url,
    )

Consider this alternative and let me know if you find a better solution.

🟡 Other Suggestions

1. Hardcoded admin URLs vs. reverse() — the three header links use hardcoded string paths:

   add_learner.short_description = mark_safe(
       '<a href="/admin/partner_catalog/cataloglearnerinvitation/add/" style="font-weight:bold;">Add Learner</a>'
   )

   Every other link in this file (including this same method's cell link, one line above) uses reverse(). The hardcoded path does currently resolve correctly, but it will silently drift if the app label, model name, or URL config ever changes. Consider computing these via reverse() at class-definition time for consistency.

2. Python-level filtering of active managers — active_managers = [m for m in obj.catalog_managers.all() if m.active] filters in Python after the prefetch. This works given the current prefetch_related("catalog_managers__user"), but using a Prefetch object with a filtered queryset (CatalogManager.objects.filter(active=True).select_related("user")) would make the filtering intent explicit at the query layer instead of relying on a per-row list comprehension.

3. Discoverability of the "—" no-manager link — the em-dash fallback is still a clickable "add manager" link, but nothing signals that. Consider adding a title="Add manager" attribute so it's clear it's an action, consistent with how the count-as-link pattern communicates intent for add_learner/add_course.

---

@ccantillo
Please address these issues

[ccantillo]

Code Review

Recommendation: Request changes — one blocking issue

🔴 Blocking

add_manager() bypasses HTML escaping — potential stored XSS

def add_manager(self, obj):
    ...
    if active_managers:
        return format_html(
            "<br>".join(
                f'<a href="{full_url}" style="font-weight:bold;">{m.user.username}</a>'
                for m in active_managers
            )
        )

This builds the <a> tag via an f-string first, then passes the already-assembled string to format_html() with no {} placeholders. Since no values are passed as separate arguments, format_html has nothing to escape — m.user.username is inserted raw. If a username ever contains <, >, or &, this is a stored XSS vector rendered in the Django admin change list.

Please use format_html_join, which is the idiomatic Django tool for joining multiple safely-formatted HTML fragments with a separator, and removes the risk of a future edit reintroducing an unescaped value:

from django.utils.html import format_html_join
from django.utils.safestring import mark_safe

def add_manager(self, obj):
    """Display active manager usernames as pre-filled links to add a new manager."""
    add_url = reverse(
        f"admin:{CatalogManager._meta.app_label}_{CatalogManager._meta.model_name}_add"
    )
    full_url = f"{add_url}?catalog={obj.pk}"
    active_managers = [m for m in obj.catalog_managers.all() if m.active]
    if active_managers:
        return format_html_join(
            mark_safe("<br>"),
            '<a href="{}" style="font-weight:bold;">{}</a>',
            ((full_url, m.user.username) for m in active_managers),
        )
    return format_html(
        '<a href="{}" style="font-weight:bold;">—</a>',
        full_url,
    )

Consider this alternative and let me know if you find a better solution.

🟡 Other Suggestions

1. Hardcoded admin URLs vs. reverse() — the three header links use hardcoded string paths:

   add_learner.short_description = mark_safe(
       '<a href="/admin/partner_catalog/cataloglearnerinvitation/add/" style="font-weight:bold;">Add Learner</a>'
   )

   Every other link in this file (including this same method's cell link, one line above) uses reverse(). The hardcoded path does currently resolve correctly, but it will silently drift if the app label, model name, or URL config ever changes. Consider computing these via reverse() at class-definition time for consistency.

2. Python-level filtering of active managers — active_managers = [m for m in obj.catalog_managers.all() if m.active] filters in Python after the prefetch. This works given the current prefetch_related("catalog_managers__user"), but using a Prefetch object with a filtered queryset (CatalogManager.objects.filter(active=True).select_related("user")) would make the filtering intent explicit at the query layer instead of relying on a per-row list comprehension.

3. Discoverability of the "—" no-manager link — the em-dash fallback is still a clickable "add manager" link, but nothing signals that. Consider adding a title="Add manager" attribute so it's clear it's an action, consistent with how the count-as-link pattern communicates intent for add_learner/add_course.

@ccantillo Please address these issues

Hi @ManuelStarDo, Thanks for the review.

• add_manager now uses format_html_join to safely escape usernames
• Header URLs use SimpleLazyObject + reverse() instead of hardcoded paths
• Active manager filtering moved to a Prefetch with filter(active=True)
• title="Add manager" added to the — fallback link

[ManuelStarDo]

@ccantillo Thanks for switching the header links over to reverse() — that resolves the URL-drift concern. However, the way it was implemented introduces a new issue worth addressing:

short_description = SimpleLazyObject(lambda: format_html(...)) on add_learner/add_course/add_manager relies on undocumented Django internals

Why it's a problem: short_description is documented, and used everywhere else in this file (course_count.short_description = 'Total Courses', partner_name.short_description = "Partner", etc.), as a plain string label. Django's admin renders column headers via result_headers() → label_for_field(), which returns attr.short_description as header.text, and the template applies {{ header.text|capfirst }}. capfirst is decorated with @keep_lazy_text, which only defers evaluation for Promise instances — SimpleLazyObject is not a Promise. This currently still renders correctly only because LazyObject proxies __class__ to the wrapped SafeString (so isinstance(x, str) passes) and SafeString's "safe" marking survives the slicing capfirst does. None of that is a guaranteed Django contract — it's incidental behavior of LazyObject/SafeString internals that a future Django version could change without warning, silently breaking these three column headers with no test coverage to catch it.

Change needed: Move the header-link behavior out of short_description entirely. Since this PR already ships a custom change_list.html override (for the column-centering CSS), extend that same approach — inject the three add-URLs via changelist_view(request, extra_context=...), and render the header <a> tags directly in the template (either by overriding the header block in change_list_results.html, or via a small script that swaps the header text for a link on page load). This delivers the same Option C UX, just through a supported customization point instead of short_description.

---

add_manager links every manager's username to the same generic add-manager URL

Why it's a problem: full_url (.../catalogmanager/add/?catalog={obj.pk}) is computed once per row and reused identically for every manager in format_html_join. When a catalog has multiple active managers, clicking any manager's username opens the same generic "add a manager" form — none of the links are specific to the manager that was clicked, which doesn't match what a user would reasonably expect from a clickable name.

Change needed: Either link each manager's username to that specific manager's change page (admin:partner_catalog_catalogmanager_change, using that manager's own pk), or render manager usernames as plain, non-link text.
This also happens to the count values of learner and course, their values a re currently acting as a link to add them where they should be plain text.

---

obj.active_managers_list will AttributeError if add_manager is ever invoked outside this changelist's queryset

Why it's a problem: to_attr="active_managers_list" on the Prefetch only populates that attribute when the object is fetched through PartnerCatalogAdmin.get_queryset. This isn't an active bug today since add_manager is only called from list_display, but it's a latent trap — any future reuse of this method (e.g., exposing it on the detail view, or reusing it elsewhere) will hard-crash instead of degrading gracefully.

Change needed: Guard the access, e.g. active_managers = getattr(obj, "active_managers_list", None) or obj.catalog_managers.filter(active=True).select_related("user").

[ccantillo]

@ccantillo Thanks for switching the header links over to reverse() — that resolves the URL-drift concern. However, the way it was implemented introduces a new issue worth addressing:

short_description = SimpleLazyObject(lambda: format_html(...)) on add_learner/add_course/add_manager relies on undocumented Django internals

Why it's a problem: short_description is documented, and used everywhere else in this file (course_count.short_description = 'Total Courses', partner_name.short_description = "Partner", etc.), as a plain string label. Django's admin renders column headers via result_headers() → label_for_field(), which returns attr.short_description as header.text, and the template applies {{ header.text|capfirst }}. capfirst is decorated with @keep_lazy_text, which only defers evaluation for Promise instances — SimpleLazyObject is not a Promise. This currently still renders correctly only because LazyObject proxies __class__ to the wrapped SafeString (so isinstance(x, str) passes) and SafeString's "safe" marking survives the slicing capfirst does. None of that is a guaranteed Django contract — it's incidental behavior of LazyObject/SafeString internals that a future Django version could change without warning, silently breaking these three column headers with no test coverage to catch it.

Change needed: Move the header-link behavior out of short_description entirely. Since this PR already ships a custom change_list.html override (for the column-centering CSS), extend that same approach — inject the three add-URLs via changelist_view(request, extra_context=...), and render the header <a> tags directly in the template (either by overriding the header block in change_list_results.html, or via a small script that swaps the header text for a link on page load). This delivers the same Option C UX, just through a supported customization point instead of short_description.

add_manager links every manager's username to the same generic add-manager URL

Why it's a problem: full_url (.../catalogmanager/add/?catalog={obj.pk}) is computed once per row and reused identically for every manager in format_html_join. When a catalog has multiple active managers, clicking any manager's username opens the same generic "add a manager" form — none of the links are specific to the manager that was clicked, which doesn't match what a user would reasonably expect from a clickable name.

Change needed: Either link each manager's username to that specific manager's change page (admin:partner_catalog_catalogmanager_change, using that manager's own pk), or render manager usernames as plain, non-link text. This also happens to the count values of learner and course, their values a re currently acting as a link to add them where they should be plain text.

obj.active_managers_list will AttributeError if add_manager is ever invoked outside this changelist's queryset

Why it's a problem: to_attr="active_managers_list" on the Prefetch only populates that attribute when the object is fetched through PartnerCatalogAdmin.get_queryset. This isn't an active bug today since add_manager is only called from list_display, but it's a latent trap — any future reuse of this method (e.g., exposing it on the detail view, or reusing it elsewhere) will hard-crash instead of degrading gracefully.

Change needed: Guard the access, e.g. active_managers = getattr(obj, "active_managers_list", None) or obj.catalog_managers.filter(active=True).select_related("user").

Hi @ManuelStarDo, all points addressed:

• SimpleLazyObject removed — header links are now injected via changelist_view + JavaScript in the existing template override, using supported Django customization points only
• Manager usernames now link to each manager's own change page
• Learner and course counts are plain text
• active_managers_list access guarded with getattr