Skip to content

feat(manage)!: configurable management shell - #80

Open
JacobCoffee wants to merge 15 commits into
mainfrom
feat/configurable-manage
Open

JacobCoffee wants to merge 15 commits into
mainfrom
feat/configurable-manage

Conversation

@JacobCoffee

Copy link
Copy Markdown
Owner

Lets a host project reuse the management UI for its own staff pages and adopt django-program one module at a time.

What changes

  • Nav registry (django_program.manage.nav): the sidebar and a new Cmd/Ctrl-K palette are built from NavRegistry instead of hard-coded markup. Built-ins live in nav_builtin.py; hosts add sections/items via DJANGO_PROGRAM["manage"]["nav"] hooks and trim built-ins with builtin_nav / hide_nav. Items carry permission checks, feature flags, groups, POST actions, badges and dynamic children.
  • Feature flags enforced in manage: every manage view has a required_feature (plus manage_ui), returning 404 when the module is off; nav entries for disabled modules disappear. Reports/analytics hubs hide disabled modules.
  • Attention providers (django_program.manage.attention): pluggable "needs attention" counts shown on the dashboard and as sidebar badges; attention_builtin ships TG reviews, receipts and pending orders.
  • Shell config: title, home link, logout link, single-conference mode, extra stylesheets/scripts, palette toggle.
  • Shell works on host pages: base.html renders from {% manage_shell %} with only request/user in context; CSS/JS extracted to static files; new focus.html for kiosk pages; mobile drawer is inert when closed.
  • Docs: new docs/customizing-manage.md, configuration reference updated.

Breaking

  • Template tag libraries renamed: registration_tags → program_registration_tags, stripe_tags → program_stripe_tags, sponsor_tags → program_sponsor_tags (clashed with host apps' libraries).
  • ConferencePermissionMixin no longer adds user_perms / submission_type_nav to the context.
  • Built-in sponsor list nav key is sponsor-list.

Verification

  • pytest: 2197 passed; ruff clean.
  • Cross-checked with Codex (two passes); all findings fixed.

- nav registry (django_program.manage.nav) drives the sidebar and a new
  Cmd/Ctrl-K palette; host projects add sections via DJANGO_PROGRAM['manage']['nav'],
  trim built-ins with builtin_nav / hide_nav
- manage views enforce module feature toggles and manage_ui (404 when off)
- pluggable attention providers with sidebar badges and a dashboard panel
- single-conference mode, configurable title/home/logout, extra assets
- shell CSS/JS extracted to static files; base.html works on host pages
  without a conference; new focus.html for kiosk pages

BREAKING CHANGE: template tag libraries renamed to program_registration_tags,
program_stripe_tags and program_sponsor_tags to avoid clashing with host apps.
- gate report/analytics views by module; hub dashboards hide disabled modules
- palette POST uses a rendered CSRF form; POST child items render as forms
- one recursive nav item partial so grouped items keep their children
- submenus stay open for an active child; hide_nav works on dynamic children
- children of a module-disabled parent are promoted, permission still applies
- title links to the dashboard only in single-conference mode
- closed mobile drawer is inert; palette keeps the active option in view
Copilot AI balanced review requested due to automatic review settings November 6, 2026 00:02
@JacobCoffee

Copy link
Copy Markdown
Owner Author

@claude review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 6, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-06T00:05:56.075241Z 57f8cf3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57f8cf30d8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/django_program/manage/views.py Resolved
Comment thread src/django_program/manage/nav.py Resolved

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Outstanding issues leave grouped sidebar links visible when collapsed, omit dynamic-child badge counts and accessible submenu labels, and expose disabled-module links and analytics controls that lead to 404s or incorrect navigation.

5 open issues
What changed in this PR

This PR makes django-program’s management UI reusable for host-project staff pages and supports adopting modules independently.

Changes:

  • Adds configurable navigation, a command palette, attention providers, and shared shell assets.
  • Enforces module feature flags across management views and adjusts report and analytics visibility.
  • Renames template-tag libraries and adds configuration documentation and regression tests.
File Description
tests/​test_sponsors/​test_program_sponsor_tags.py Updates sponsor-tag imports and rendering tests.
tests/​test_settings_config.py Tests management configuration defaults and validation.
tests/​test_registration/​test_stripe_models_admin_forms.py Updates the Stripe-tag import and docstring.
tests/​test_registration/​test_program_registration_tags.py Updates registration-tag imports and tests.
tests/​test_manage/​test_views.py Verifies removal of legacy sidebar context.
tests/​test_manage/​test_nav.py Tests navigation configuration and host-page rendering.
tests/​test_manage/​test_financial_views.py Removes the legacy sidebar-link assertion.
tests/​test_manage/​test_feature_gates.py Tests feature gating and single-conference redirects.
tests/​test_manage/​test_attention.py Tests built-in attention counts and visibility.
tests/​test_examples/​test_example_app_integration.py Uses the renamed Stripe-tag library.
tests/​settings.py Defines the static URL for shell rendering.
src/​django_program/​sponsors/​templatetags/​program_sponsor_tags.py Updates usage examples for the renamed library.
src/​django_program/​settings.py Adds validated management-shell configuration.
src/​django_program/​registration/​templatetags/​program_stripe_tags.py Updates the renamed library’s usage example.
src/​django_program/​registration/​templatetags/​program_registration_tags.py Updates examples for the renamed library.
src/​django_program/​manage/​views.py Adds feature enforcement, redirects, and dashboard attention data.
src/​django_program/​manage/​views_vouchers.py Gates voucher generation on registration.
src/​django_program/​manage/​views_terminal.py Gates terminal access on registration.
src/​django_program/​manage/​views_reports.py Gates reports and exposes dashboard feature flags.
src/​django_program/​manage/​views_purchase_orders.py Gates purchase-order views on registration.
src/​django_program/​manage/​views_overrides.py Gates overrides by their backing modules.
src/​django_program/​manage/​views_letters.py Gates visa-letter management views.
src/​django_program/​manage/​views_financial.py Gates the financial dashboard on registration.
src/​django_program/​manage/​views_checkin.py Gates check-in views on registration.
src/​django_program/​manage/​views_bulk_purchases.py Gates bulk-purchase workflows on registration.
src/​django_program/​manage/​views_analytics.py Exposes module flags and gates sponsor analytics.
src/​django_program/​manage/​templatetags/​manage_shell.py Resolves shell configuration, navigation, and palette data.
src/​django_program/​manage/​templatetags/​__init__.py Introduces the management template-tag package.
src/​django_program/​manage/​templates/​django_program/​manage/​reports_dashboard.html Hides report sections for disabled modules.
src/​django_program/​manage/​templates/​django_program/​manage/​focus.html Adds a sidebar-free shell variant.
src/​django_program/​manage/​templates/​django_program/​manage/​dashboard.html Renders the needs-attention panel.
src/​django_program/​manage/​templates/​django_program/​manage/​analytics_dashboard.html Applies module flags to selected panels and links.
src/​django_program/​manage/​templates/​django_program/​manage/​_nav_item.html Renders recursive navigation and POST actions.
src/​django_program/​manage/​templates/​django_program/​manage/​_icons.html Adds the shared SVG icon sprite.
src/​django_program/​manage/​static/​django_program/​manage/​manage.js Implements sidebar, mobile drawer, and palette interactions.
src/​django_program/​manage/​static/​django_program/​manage/​manage.css Extracts shell styles and adds responsive controls.
src/​django_program/​manage/​nav.py Implements the configurable navigation registry.
src/​django_program/​manage/​nav_builtin.py Registers built-in navigation and dynamic children.
src/​django_program/​manage/​attention.py Adds request-cached attention-provider support.
src/​django_program/​manage/​attention_builtin.py Supplies pending grant, receipt, and order counts.
docs/​index.rst Links the shell customization guide.
docs/​customizing-manage.md Documents shell configuration and extension hooks.
docs/​configuration.md Documents management settings and feature enforcement.

Review effort: Balanced

Comment thread src/django_program/manage/nav.py Resolved
Comment thread src/django_program/manage/views.py Resolved
- .palette-trigger's search <svg><use> had no explicit width/height,
  falling back to the browser's intrinsic 300x150 box: a giant magnifier
  overflowing the top bar on every page. Moved it under the generic
  '.top-bar svg' rule (16x16), reordered so the more specific
  '.sidebar-toggle svg' (18x18) still wins.
- <900px top bar: hide the public-schedule link, synced time, user name,
  and the palette trigger's 'Jump to…'/kbd label (icon-only button);
  title gets ellipsis truncation instead of wrapping; html/body get
  overflow-x: hidden as a safety net. Keeps Log out reachable and the
  sidebar toggle clickable.

Verified in-browser at 1440 and 390 on /manage/python-2077/ and
/manage/python-2077/overrides/rooms/: no horizontal overflow, icons
correctly sized, Cmd/Ctrl-K palette opens/filters/navigates, mobile
drawer toggle opens/closes with inert+focus-return on Esc.
…ta on CI

- pretalx ScheduleView fell back to removed django.utils.timezone.utc
- tzdata dependency so legacy zone keys (US/Eastern) resolve on Linux/Windows

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🔴 src/django_program/manage/templates/django_program/manage/dashboard.html — Staff on a conference dashboard with a module turned off now get 404s from its links and sync buttons; the sidebar hides those links but the dashboard does not. This change makes those views return 404 via required_feature, but dashboard.html still renders every link and form unconditionally. Fix: gate each module link and form on its feature, as reports_dashboard.html and analytics_dashboard.html now do. Expose the flags from DashboardView.get_context_data. That covers the 4 sites listed. Same pattern at 4 sites (dashboard.html:195, dashboard.html:248-258, dashboard.html:298, dashboard.html:333).

    Why this was flagged

    Trigger: a host disables a module, e.g. DJANGO_PROGRAM['features']['registration'] = False, sponsors, travel_grants, visa_letters, programs or pretalx_sync. This is the one-module-at-a-time adoption the PR advertises. DashboardView (views.py:780) has no required_feature, so the dashboard still renders. dashboard.html still links unconditionally to: the Orders stat card at :195; Activities at :248; Orders, Ticket Types, Add-ons and Vouchers at :252-255; Sponsors at :256; Travel Grants at :257; Visa Letters at :258. It also posts the Pretalx sync form to manage:sync-pretalx (:298) and manage:sync-sponsors (:333). This PR gives each of those targets a required_feature (e.g. OrderListView, SponsorManageListView, SyncPretalxView, SyncSponsorsView in views.py), so every click now 404s. On the base branch these views were not feature-gated, so the links worked. The reports and analytics hubs got *_enabled context flags in this PR, but the main dashboard did not. Nothing in the template or view hides these entries.

    Verification: Trigger: a host disables any of registration, sponsors, travel_grants, visa_letters, programs or pretalx_sync. DashboardView sets no required_feature, so the dashboard still renders. The template contains no feature check, so its module links and sync forms render unconditionally and now return a 404 when the module is off. It is a regression: on the base these links reached working pages.

Comment thread src/django_program/manage/views.py Resolved
Comment thread src/django_program/manage/nav.py Resolved
- DashboardView/dashboard.html: hide Orders stat-card, directory links
  (Orders/Ticket Types/Add-ons/Vouchers/Sponsors/Travel Grants/Activities/
  Visa Letters), and the Pretalx/PSF sync forms+buttons when their backing
  feature is disabled, instead of linking to a 404.
- AnalyticsDashboardView/analytics_dashboard.html: gate registration-only
  KPI cards, the cart-funnel/revenue-breakdown charts, and the Ticket
  Sales Ratio table behind registration_enabled; zero out the matching
  _drilldownUrls entries so chart click-through no-ops instead of
  reloading with a dead ?status= link.
- Add tests/test_manage/test_feature_gates.py coverage for both.
Copilot AI balanced review requested due to automatic review settings November 6, 2026 00:16
- ignore non-django-program conference objects in the shell context
- single-conference title skips a dashboard the user cannot open
- tuple required_feature on ConferencePermissionMixin
- shell renders without the request context processor
- drop non-actionable pending-orders attention item
- focus layout has no drawer toggle; logout is a POST form
- purchase orders nav follows the full-access gate its views enforce

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The default attention provider can crash host pages when management URLs are unavailable, while unresolved accessibility and feature-gating issues leave unnamed controls, invisible focus targets, and links to disabled views.

2 open issues
4 previously missed issues

In code that hasn't changed since last review

Medium severity Gate built-in navigation on the manage UI feature flag

src/​django_program/​manage/​nav_builtin.py:114

On a host page using the shared shell, setting manage_ui_enabled=False (or disabling it for the configured conference) still leaves all built-in navigation visible: these entries check module flags but never manage_ui. Every built-in destination now returns 404 through ConferencePermissionMixin, so both the sidebar and palette offer unusable links. Gate the built-in entries on manage_ui as well, without hiding host-provided sections.

Medium severity Make collapsed submenu controls unfocusable

src/​django_program/​manage/​static/​django_program/​manage/​manage.css:414

Closed submenus are clipped to zero height, but their links and POST buttons remain focusable, so keyboard users tab through invisible controls. The same issue affects collapsed People/Commerce subgroups at manage.css:284–287. Remove both closed containers from layout or make them inert until expanded.

Medium severity Hide the Pretalx import action when sync is disabled

src/​django_program/​manage/​views.py:362

With pretalx_sync_enabled=False, the conference list still unconditionally renders its primary “Import from Pretalx” action (conference_list.html:11), but this new gate makes that action return 404 even for superusers. Pass the sync feature state to the conference-list template and hide the action when disabled.

Medium severity Hide Talk Overrides when Pretalx sync is disabled

src/​django_program/​manage/​views_overrides.py:36

When Pretalx sync is disabled but sponsors remain enabled, the new nav explicitly keeps the sponsor-overrides page accessible. However, that page still renders “Talk Overrides” unconditionally (sponsor_override_list.html:11), pointing to this now-disabled view and returning 404. Apply the Pretalx feature check to that cross-module action as well.

5 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 6, 2026 00:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Active issues expose links to disabled manage pages, allow missing routes to cause page-render failures, and leave the mobile palette button without an accessible name.

2 open issues
4 previously missed issues

In code that hasn't changed since last review

Medium severity Skip receipt attention item when its route is unavailable

src/​django_program/​manage/​attention_builtin.py:78

If a host uses the shell and badges without mounting manage:receipt-review-queue, one pending receipt makes the default provider raise NoReverseMatch, failing the page render. Guard this URL reversal and skip the receipt item when its destination is unavailable.

Medium severity Reject section keys that conflict with existing navigation items

src/​django_program/​manage/​nav.py:164

Unlike add(), section() accepts a key that already belongs to an item. For example, adding a host section named reports alongside the built-in Reports item succeeds, and then hide_nav=["reports"] removes only the host section while leaving the item registered. Please reject keys already present in _item_sections, while continuing to allow updates to existing sections.

Medium severity Hide built-in navigation when conference manage UI is disabled

src/​django_program/​manage/​nav.py:497

On host pages, the built-in entries remain visible when manage_ui_enabled is false (including when all_ui_enabled turns it off). With a conference in scope, build_nav() still returns Dashboard, Settings, and the other permitted manage links, although ConferencePermissionMixin now returns 404 for all of them. Apply the conference-aware manage_ui check to built-in navigation while keeping host-provided sections usable.

Medium severity Avoid linking to the disabled dashboard from the shell title

src/​django_program/​manage/​templatetags/​manage_shell.py:93

In single-conference mode, the default title link selects the dashboard based only on permission. For example, a host can set builtin_nav=False, provide its own staff links, and disable manage_ui_enabled, but clicking the shell title still opens the disabled dashboard and returns 404. Check the conference's manage_ui availability before choosing the dashboard so this case falls back to the first visible host page.

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 6, 2026 00:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

5 other optional suggestions (nits or notes on pre-existing code) were found and not posted.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new blocking issues. 4 optional suggestions (nits or notes on pre-existing code) were found and not posted. Nothing in this review needs a push before merging.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

…ed 94%

main sits at 94% (CI never reached the coverage job while tests were red);
a follow-up PR backfills tests and restores fail_under = 100.
Copilot AI balanced review requested due to automatic review settings November 6, 2026 00:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 optional suggestions (nits or notes on pre-existing code) were found and not posted.

Still open from earlier reviews (1):

  • Unresolved: 1 minor or pre-existing.

- attention_builtin: skip items whose manage: URL can't reverse
  instead of raising NoReverseMatch on host pages
- manage.css/base.html: give the palette trigger an accessible
  aria-label so it survives the <=900px text hide
- nav/manage_shell: only post the CSRF token to same-origin
  targets (nav items, palette, logout); cross-origin logout
  renders a plain link instead of a form
- views_overrides/nav_builtin: drop the pretalx_sync gate from
  talk/speaker/room/type-default overrides (they apply regardless
  of sync); sponsor overrides stay gated by sponsors
Copilot AI balanced review requested due to automatic review settings November 6, 2026 01:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

1 other optional suggestion (a nit or a note on pre-existing code) was found and not posted.

Copilot AI balanced review requested due to automatic review settings November 6, 2026 01:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new blocking issues. 2 optional suggestions (nits or notes on pre-existing code) were found and not posted. Nothing in this review needs a push before merging.

The global margin reset removed the UA's dialog { margin: auto }, pinning
the modal to the top-left corner; input:focus-visible also outranked
.palette-input's outline: none.
Copilot AI balanced review requested due to automatic review settings November 6, 2026 03:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Labelled groups render collapsed server-side unless they hold the current
page, so there is no flash of open groups. Collapsed groups leave the tab
order, and labels report aria-expanded.
Copilot AI balanced review requested due to automatic review settings November 6, 2026 03:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 optional suggestion (a nit or a note on pre-existing code) was found and not posted.

Comment on lines +79 to +90
{% for group in section.items|group_nav_items %}
{% if group.label %}
<ul class="sidebar-nav">
<li class="sidebar-sublabel{% if not group.is_active %} collapsed-label{% endif %}" data-subsection="{{ section.key }}-{{ group.label|slugify }}" role="button" tabindex="0" aria-expanded="{% if group.is_active %}true{% else %}false{% endif %}">
{{ group.label }}
<svg class="sidebar-collapse-icon" aria-hidden="true"><use href="#dp-i-chevron-down"></use></svg>
</li>
</ul>
<div class="sidebar-subgroup{% if not group.is_active %} collapsed{% endif %}" data-subsection-target="{{ section.key }}-{{ group.label|slugify }}">
<ul class="sidebar-nav">
{% for item in group.items %}
{% include "django_program/manage/_nav_item.html" with item=item depth=0 %}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Staff can get stuck with a collapsed sidebar group, such as Orders, Visa Letters and Badges, that never opens when a host adds an ungrouped item to a built-in section. group_nav_items only merges contiguous items, so a repeated label such as "People" produces two buckets. Both buckets render the same data-subsection key, {{ section.key }}-{{ group.label|slugify }}. Fix: give each bucket a unique key, for example by adding forloop.counter, or merge non-contiguous buckets that share a label in group_nav_items. In manage.js, document.querySelector toggles only the first matching target. [also at: src/django_program/manage/templates/django_program/manage/base.html:87 - Sidebar items can get stuck hidden when two group buckets in one section share an id. Clicking the second sub-heading then toggles the first group.]

Why this was flagged

A host nav hook calls registry.add("registration", NavItem(key="x", ...)) with the default order=0 and no group. That pattern is documented in docs/customizing-manage.md:58-90. entries() (nav.py:242) sorts by (order, registration index), so the section becomes attendees (People), x (no group), then orders, letters and badges (People). group_nav_items (manage_shell.py:174-191) emits two separate "People" buckets. base.html:81 and :87 give both the same data-subsection/data-subsection-target value, "registration-people". Both start with the collapsed class unless they hold the current page. In manage.js:52-53, document.querySelector('[data-subsection-target="…"]') returns the first target for both labels. Clicking the second "People" heading therefore toggles the first group, and Orders, Visa Letters and Badges stay hidden. Group labels that slugify to the same string within a section hit the same collision. On the base branch the sidebar was hard-coded, with no host items and no shared keys.

Verification: An ungrouped host item with the default order=0 in "registration" sorts between attendees and orders (nav.py:235-243). group_nav_items (manage_shell.py:176-191) then emits two People buckets. base.html:81 and :87 give both the key "registration-people". manage.js:54 uses document.querySelector, which returns the first match, so Orders / Visa Letters / Badges stay collapsed.

Copilot AI balanced review requested due to automatic review settings November 7, 2026 02:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 other optional suggestions (nits or notes on pre-existing code) were found and not posted.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Comment on lines +159 to +166
conference = _resolve_conference(context.get("conference"), config)
active_nav = context.get("active_nav") or ""
sections = build_nav(
request,
conference=conference,
active_nav=active_nav,
badges=lambda: attention_counts(request, conference),
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 (optional) Sponsor contacts using the sponsor portal can now see host staff sidebar and Cmd-K palette entries that leave permission at its default None. The portal pages extend manage/base.html, and manage_shell builds the full nav for whoever is logged in. check_permission (nav.py:313-314) treats None as visible to everyone. Fix: keep nav and palette entries off pages served to non-staff users, for example by letting SponsorPortalMixin pages opt out of the nav or by requiring manage access before resolving None-permission entries. Also correct the nav.py:61-62 and docs wording that None means "anyone who can see the shell".

Why this was flagged

The condition is a host whose DJANGO_PROGRAM['manage']['nav'] hook registers a section or NavItem without permission. The default is None (nav.py:106, nav.py:138). A sponsor contact opens the sponsor portal. That is any user whose email matches a sponsor's contact_email (sponsors/views.py:168-177), with no staff or program_conference permission. portal_home.html, purchase_detail.html and purchase_request.html extend django_program/manage/base.html, and SponsorPortalMixin puts a real conference in context (sponsors/views.py:191). manage_shell (manage_shell.py:159-166) then calls build_nav for this user, and check_permission(user, None) returns True (nav.py:313-314). The host's staff links and POST actions show up in the sidebar and in palette_items, which base.html:138 serialises for the Cmd/Ctrl-K palette. On the base branch, base.html gated every sidebar section on user_perms, which the portal views never supplied, so sponsor users saw no navigation.

Verification: check_permission returns True when permission is None (nav.py:313-314: if permission is None: return True), so host items with no permission show up in the sidebar and the Cmd-K palette for the sponsor contact. Base branch: the sidebar sections were gated on user_perms.* (old base.html:1069, 1090). SponsorPortalMixin never set user_perms, so sponsor contacts saw no staff nav.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants