test: add admin smoke tests for PartnerCatalog changelist

[ccantillo]

Admin smoke tests for PartnerCatalog changelist

Context

Follow-up to the PR that implemented the layout for the
PartnerCatalog admin changelist (ticket nau-technical#938).
During that review, it was noted that no test coverage existed for any
of the behaviour — this PR adds it.

What is tested

Test	What it asserts
test_changelist_returns_200	Page renders without errors for a superuser
test_learner_count_is_plain_integer	add_learner returns a plain int, not HTML
test_course_count_is_plain_integer	add_course returns a plain int, not HTML
test_manager_renders_username_linked_to_change_page	Active manager username is wrapped in a link to that manager's own change page
test_manager_inactive_shows_dash	Inactive manager → em-dash fallback
test_manager_no_managers_shows_dash	No managers at all → em-dash fallback
test_manager_username_is_html_escaped	Username output contains no raw <script> tags (XSS guard)
test_multiple_managers_all_rendered	All active managers appear, separated by <br>
test_changelist_view_injects_add_urls_into_context	changelist_view injects learner_add_url, course_add_url, manager_add_url into the template context

Files changed

File	Change
tests/test_partner_catalog_admin.py	New — 9 smoke tests for PartnerCatalogAdmin

[ManuelStarDo]

Hello @ccantillo , thank you for the tests.

Just a few improvements before merging.

Findings

[HIGH] tests/test_partner_catalog_admin.py:112-120 — test_manager_username_is_html_escaped does not actually test HTML escaping
Risk: The test is named and documented as an "XSS guard" but the username fixture used is "safe_name" — a plain alphanumeric string with no HTML special characters at all. The assertion "<script>" not in result will trivially pass regardless of whether add_manager correctly escapes user input, because no <script> (or any markup) is ever injected into the username. This gives false confidence that XSS is covered when it is not actually exercised. It also duplicates test_manager_renders_username_linked_to_change_page almost verbatim.
Fix: Create the user with a genuinely malicious username, e.g. make_user(username="<script>alert(1)</script>"), and assert the raw tag is absent while the escaped form (e.g. <script>) is present:

user = make_user(username="<script>alert(1)</script>")
CatalogManager.objects.create(catalog=catalog, user=user, active=True)
result = str(_admin().add_manager(catalog))
assert "<script>" not in result
assert "&lt;script&gt;" in result

Note: make_user/User.objects.create_user may reject non-standard usernames depending on AUTH_USER_MODEL/username validators in this project — verify a Django username field accepts special characters here, or escape via a different user-controllable field if usernames are restricted by validators.

[MEDIUM] tests/test_partner_catalog_admin.py — coverage gap on the prefetch-optimized path of add_manager
Risk: add_manager has two code paths: (1) the fast path using obj.active_managers_list (populated only when the object comes through PartnerCatalogAdmin.get_queryset's Prefetch(..., to_attr="active_managers_list")), and (2) the fallback path obj.catalog_managers.filter(active=True) used when the attribute is absent. Every manager-related test in this PR (test_manager_renders_username_linked_to_change_page, test_manager_inactive_shows_dash, test_manager_no_managers_shows_dash, test_multiple_managers_all_rendered, test_manager_username_is_html_escaped) calls _admin().add_manager(catalog) directly on an object that never went through get_queryset, so only the fallback path is exercised. The prefetch path — the one actually used in production via the changelist — is untested for correctness (e.g., whether it correctly excludes inactive managers, whether ordering is preserved).
Fix: Add at least one test that goes through PartnerCatalogAdmin.get_queryset(request) (e.g., admin_view.get_queryset(request).get(pk=catalog.pk)) before calling add_manager, to confirm the prefetched active_managers_list path renders identically to the fallback path.

[LOW] tests/test_partner_catalog_admin.py:test_changelist_view_injects_add_urls_into_context — asserts substring /add/ rather than exact URL
Risk: Not incorrect, just a weaker assertion than possible. The exact expected URLs are computable via reverse(...) the same way the admin code does, and the test already imports reverse for other tests.
Fix: Tighten to assert context["learner_add_url"] == reverse("admin:partner_catalog_cataloglearnerinvitation_add") (and equivalents), which better catches accidental cross-wiring of the three URLs.

[ccantillo]

Hello @ccantillo , thank you for the tests.

Just a few improvements before merging.

Findings

[HIGH] tests/test_partner_catalog_admin.py:112-120 — test_manager_username_is_html_escaped does not actually test HTML escaping Risk: The test is named and documented as an "XSS guard" but the username fixture used is "safe_name" — a plain alphanumeric string with no HTML special characters at all. The assertion "<script>" not in result will trivially pass regardless of whether add_manager correctly escapes user input, because no <script> (or any markup) is ever injected into the username. This gives false confidence that XSS is covered when it is not actually exercised. It also duplicates test_manager_renders_username_linked_to_change_page almost verbatim. Fix: Create the user with a genuinely malicious username, e.g. make_user(username="<script>alert(1)</script>"), and assert the raw tag is absent while the escaped form (e.g. <script>) is present:

user = make_user(username="<script>alert(1)</script>")
CatalogManager.objects.create(catalog=catalog, user=user, active=True)
result = str(_admin().add_manager(catalog))
assert "<script>" not in result
assert "&lt;script&gt;" in result

Note: make_user/User.objects.create_user may reject non-standard usernames depending on AUTH_USER_MODEL/username validators in this project — verify a Django username field accepts special characters here, or escape via a different user-controllable field if usernames are restricted by validators.

[MEDIUM] tests/test_partner_catalog_admin.py — coverage gap on the prefetch-optimized path of add_manager Risk: add_manager has two code paths: (1) the fast path using obj.active_managers_list (populated only when the object comes through PartnerCatalogAdmin.get_queryset's Prefetch(..., to_attr="active_managers_list")), and (2) the fallback path obj.catalog_managers.filter(active=True) used when the attribute is absent. Every manager-related test in this PR (test_manager_renders_username_linked_to_change_page, test_manager_inactive_shows_dash, test_manager_no_managers_shows_dash, test_multiple_managers_all_rendered, test_manager_username_is_html_escaped) calls _admin().add_manager(catalog) directly on an object that never went through get_queryset, so only the fallback path is exercised. The prefetch path — the one actually used in production via the changelist — is untested for correctness (e.g., whether it correctly excludes inactive managers, whether ordering is preserved). Fix: Add at least one test that goes through PartnerCatalogAdmin.get_queryset(request) (e.g., admin_view.get_queryset(request).get(pk=catalog.pk)) before calling add_manager, to confirm the prefetched active_managers_list path renders identically to the fallback path.

[LOW] tests/test_partner_catalog_admin.py:test_changelist_view_injects_add_urls_into_context — asserts substring /add/ rather than exact URL Risk: Not incorrect, just a weaker assertion than possible. The exact expected URLs are computable via reverse(...) the same way the admin code does, and the test already imports reverse for other tests. Fix: Tighten to assert context["learner_add_url"] == reverse("admin:partner_catalog_cataloglearnerinvitation_add") (and equivalents), which better catches accidental cross-wiring of the three URLs.

Hi @ManuelStarDo, Thanks for the review! all points addressed