Fix: supervisor picker hid regular active users (only admins showed)
Reported: when creating a new team or project from the friendly UI (/teams/new/ or /projects/new/), the Supervisor dropdown only lists is_staff / is_superuser accounts. Users who should be eligible to supervise (e.g. eendman, supervisor_smoke) are invisible in the picker even though they are active. Root cause: core.forms._supervisor_user_queryset filtered to is_active=True AND (is_staff OR is_superuser OR groups__name='Work Logger') That was strictly more restrictive than the app's own permission helper is_supervisor(user) in views.py, which grants supervisor powers to ANYONE assigned to a team/project (via the team.supervisor FK or project.supervisors M2M), regardless of group membership. On Konrad's dev DB that excluded 2 of 6 active users from the picker (one in a custom group, one in no group) even though both were valid supervisor candidates by the permission model. Fix: Queryset now returns every active user. The act of assigning a user to a team/project is what confers supervisor-ness downstream, so the picker no longer needs a pre-registered allow-list. Inactive users (is_active=False) remain excluded — the one hard guardrail. Docstring rewritten to explain the new behavior and why. Stale comment in TeamForm.__init__ updated to match (the old comment still described the pre-fix Work-Logger-group requirement). Tests: 4 new regression tests in SupervisorPickerQuerysetTests: - regular active user is selectable (the core bug) - user in an unrelated group is selectable - inactive user is still excluded (guardrail) - admin is still selectable (no regression for prior use case) All 28 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
f1e246ce24
commit
0ceceebba4
@ -372,18 +372,23 @@ WorkerWarningFormSet = inlineformset_factory(
|
|||||||
def _supervisor_user_queryset():
|
def _supervisor_user_queryset():
|
||||||
"""Users eligible to supervise a team or project.
|
"""Users eligible to supervise a team or project.
|
||||||
|
|
||||||
Matches the app's role model (see `is_supervisor` in views.py):
|
Returns ALL active users. Any active user can be picked as a supervisor
|
||||||
anyone who is a Django admin (is_staff/is_superuser) OR is a member
|
— the picker doesn't need to pre-filter by group or staff flags because
|
||||||
of the "Work Logger" group. Active accounts only — no deactivated
|
the app's `is_supervisor(user)` helper (in views.py) already grants
|
||||||
users in the picker.
|
supervisor permissions to anyone assigned to a team/project FK/M2M,
|
||||||
|
regardless of their group membership.
|
||||||
|
|
||||||
|
Previously this filter required `is_staff`/`is_superuser` OR membership
|
||||||
|
in a "Work Logger" group, which was strictly more restrictive than the
|
||||||
|
permission model and hid field supervisors from the picker. Fix
|
||||||
|
(2026-04-23): trust the admin making the assignment; any active user
|
||||||
|
can be chosen, and the act of assignment is what confers supervisor
|
||||||
|
powers downstream.
|
||||||
|
|
||||||
|
Inactive users (`is_active=False`) are still excluded — deactivated
|
||||||
|
accounts should never appear in dropdowns.
|
||||||
"""
|
"""
|
||||||
from django.db.models import Q
|
return User.objects.filter(is_active=True).order_by('username')
|
||||||
return (
|
|
||||||
User.objects.filter(is_active=True)
|
|
||||||
.filter(Q(is_staff=True) | Q(is_superuser=True) | Q(groups__name='Work Logger'))
|
|
||||||
.distinct()
|
|
||||||
.order_by('username')
|
|
||||||
)
|
|
||||||
|
|
||||||
|
|
||||||
class TeamForm(forms.ModelForm):
|
class TeamForm(forms.ModelForm):
|
||||||
@ -408,12 +413,11 @@ class TeamForm(forms.ModelForm):
|
|||||||
|
|
||||||
def __init__(self, *args, **kwargs):
|
def __init__(self, *args, **kwargs):
|
||||||
super().__init__(*args, **kwargs)
|
super().__init__(*args, **kwargs)
|
||||||
# Supervisor dropdown — show anyone who is either admin (is_staff/
|
# Supervisor dropdown — show any active user. The app's is_supervisor
|
||||||
# is_superuser) OR a member of the "Work Logger" group. This matches
|
# helper (views.py) grants supervisor powers to whoever is assigned
|
||||||
# the app's role model: team supervisors are typically Work Loggers,
|
# here, regardless of group or staff flags, so the picker doesn't
|
||||||
# not admins, so filtering by is_staff alone hides the people who
|
# need to pre-filter by role. Only is_active=True users appear, so
|
||||||
# actually supervise teams day-to-day. We use `active=True` to drop
|
# deactivated accounts are hidden from the dropdown.
|
||||||
# deactivated accounts from the picker.
|
|
||||||
self.fields['supervisor'].queryset = _supervisor_user_queryset()
|
self.fields['supervisor'].queryset = _supervisor_user_queryset()
|
||||||
self.fields['supervisor'].required = False
|
self.fields['supervisor'].required = False
|
||||||
# Include inactive workers too — matches admin parity. The template
|
# Include inactive workers too — matches admin parity. The template
|
||||||
|
|||||||
@ -416,3 +416,63 @@ class ReportContextFilterInflationTests(TestCase):
|
|||||||
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
|
ctx = self._ctx(project_id=self.project.id, team_id=self.team.id)
|
||||||
totals = {item['type']: item['total'] for item in ctx['adjustment_totals']}
|
totals = {item['type']: item['total'] for item in ctx['adjustment_totals']}
|
||||||
self.assertEqual(totals.get('Bonus'), Decimal('100.00'))
|
self.assertEqual(totals.get('Bonus'), Decimal('100.00'))
|
||||||
|
|
||||||
|
|
||||||
|
# =============================================================================
|
||||||
|
# === TESTS FOR SUPERVISOR PICKER QUERYSET ===
|
||||||
|
# Regression tests for the TeamForm/ProjectForm supervisor dropdown.
|
||||||
|
#
|
||||||
|
# THE BUG (fixed 2026-04-23):
|
||||||
|
# The picker required either admin flags (is_staff/is_superuser) or "Work Logger"
|
||||||
|
# group membership. Any active user NOT pre-enrolled in that specific group
|
||||||
|
# was invisible, even though the app's downstream `is_supervisor()` helper
|
||||||
|
# grants supervisor powers to anyone assigned to a team/project FK/M2M —
|
||||||
|
# regardless of group. The picker was strictly more restrictive than the
|
||||||
|
# permission model, hiding field supervisors from the person trying to assign
|
||||||
|
# them. Fix: allow any active user; let the act of assignment confer
|
||||||
|
# supervisor-ness (matching how `is_supervisor` works).
|
||||||
|
# =============================================================================
|
||||||
|
|
||||||
|
|
||||||
|
class SupervisorPickerQuerysetTests(TestCase):
|
||||||
|
"""Supervisor picker must surface all active users, not just admins."""
|
||||||
|
|
||||||
|
def test_regular_active_user_is_selectable(self):
|
||||||
|
"""A plain active user (no is_staff, no is_superuser, no Work Logger
|
||||||
|
group) must appear. Pre-fix: excluded by the Q filter."""
|
||||||
|
from core.forms import _supervisor_user_queryset
|
||||||
|
regular = User.objects.create_user(
|
||||||
|
username='field_supervisor', password='pass',
|
||||||
|
is_staff=False, is_superuser=False,
|
||||||
|
)
|
||||||
|
self.assertIn(regular, _supervisor_user_queryset())
|
||||||
|
|
||||||
|
def test_user_in_unrelated_group_is_still_selectable(self):
|
||||||
|
"""A user in some random group (not 'Work Logger') is still an active
|
||||||
|
user and must show up. Pre-fix: excluded because the Q filter specifically
|
||||||
|
checked groups__name='Work Logger'."""
|
||||||
|
from django.contrib.auth.models import Group
|
||||||
|
from core.forms import _supervisor_user_queryset
|
||||||
|
user = User.objects.create_user(username='misc_group_user', password='pass')
|
||||||
|
group, _ = Group.objects.get_or_create(name='Some Other Group')
|
||||||
|
user.groups.add(group)
|
||||||
|
self.assertIn(user, _supervisor_user_queryset())
|
||||||
|
|
||||||
|
def test_inactive_user_still_excluded(self):
|
||||||
|
"""Inactive users must NEVER appear in the picker — even if they used
|
||||||
|
to be admins or Work Loggers. Active-only is the one hard guardrail."""
|
||||||
|
from core.forms import _supervisor_user_queryset
|
||||||
|
inactive = User.objects.create_user(
|
||||||
|
username='former_employee', password='pass',
|
||||||
|
is_active=False, is_staff=True,
|
||||||
|
)
|
||||||
|
self.assertNotIn(inactive, _supervisor_user_queryset())
|
||||||
|
|
||||||
|
def test_admin_still_selectable(self):
|
||||||
|
"""Defense-in-depth: admins continue to appear (no regression for the
|
||||||
|
existing use case)."""
|
||||||
|
from core.forms import _supervisor_user_queryset
|
||||||
|
admin = User.objects.create_user(
|
||||||
|
username='an_admin', password='pass', is_staff=True
|
||||||
|
)
|
||||||
|
self.assertIn(admin, _supervisor_user_queryset())
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user