From 5bd63e0c4b8bf8e10517eb11c7dd722e53b9b5dc Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Wed, 17 Jun 2026 16:09:16 -0700 Subject: [PATCH 1/2] fix(view-project-people): resolve director by name so PhD-advisee filter works (#1325) The "Only my PhD advisees" filter showed no advisees. The #1284 refactor resolved the lab director via a Title.DIRECTOR position, but Jon holds a professor title (not a "Director" position), so the lookup matched nobody, director was None, and every is_phd_advisee was false. The lab has exactly one director/founder for its lifetime, so identify him by name in one canonical place: - Add Person.DIRECTOR_NAME + Person.get_director() as the single source of truth; the view's is_director flag and advisee check both derive from it. - Point website/admin/utils.get_active_professors_queryset() at the same constant, removing a second independent hardcoding of the name. - Add a regression test reproducing the production scenario (director holds a professor title); update the two tests that fabricated a Title.DIRECTOR director to use the canonical director. Co-Authored-By: Claude Opus 4.8 (1M context) --- website/admin/utils.py | 2 +- website/models/person.py | 24 +++++++++++++ website/tests/test_view_project_people.py | 42 ++++++++++++++++++----- website/views/view_project_people.py | 10 +++--- 4 files changed, 63 insertions(+), 15 deletions(-) diff --git a/website/admin/utils.py b/website/admin/utils.py index 9f86ec48..7c68a7fd 100644 --- a/website/admin/utils.py +++ b/website/admin/utils.py @@ -16,7 +16,7 @@ from website.models.position import Title -def get_active_professors_queryset(prioritized_name=("Jon", "Froehlich")): +def get_active_professors_queryset(prioritized_name=Person.DIRECTOR_NAME): """ Build a queryset of currently active professors, optionally prioritizing a specific person. diff --git a/website/models/person.py b/website/models/person.py index ee3fdc25..a4ac0764 100644 --- a/website/models/person.py +++ b/website/models/person.py @@ -102,10 +102,34 @@ def get_upload_to_for_person_easter_egg(person, original_filename, force_unique= class Person(models.Model): UPLOAD_DIR = 'person/' # relative path + # The Makeability Lab has exactly one director/founder for its lifetime: + # Jon Froehlich. We identify him by name because that is the real invariant — + # he holds a professor title (not a "Director" position), so there is no + # title- or flag-based marker to key off of. This is the single source of + # truth for "who is the director"; see get_director() and #1284. + DIRECTOR_NAME = ("Jon", "Froehlich") + @staticmethod # use as decorator def get_thumbnail_size_as_str(): return f"{PERSON_THUMBNAIL_SIZE[0]}x{PERSON_THUMBNAIL_SIZE[1]}" + @classmethod + def get_director(cls): + """ + Returns the lab director/founder Person (Jon Froehlich), or None if that + Person is not in the database yet. + + Resolved from :attr:`DIRECTOR_NAME` rather than a position title or flag + because the lab has a single director for its lifetime. Use this anywhere + the director needs to be identified (e.g. the is_director flag and the + "Only my PhD advisees" filter) so the answer cannot disagree between call + sites. + """ + first, last = cls.DIRECTOR_NAME + return cls.objects.filter( + first_name__iexact=first, last_name__iexact=last + ).first() + first_name = models.CharField(max_length=40) middle_name = models.CharField(max_length=50, blank=True, null=True) last_name = models.CharField(max_length=50) diff --git a/website/tests/test_view_project_people.py b/website/tests/test_view_project_people.py index 55342197..23422c77 100644 --- a/website/tests/test_view_project_people.py +++ b/website/tests/test_view_project_people.py @@ -5,8 +5,8 @@ This page builds a JSON payload of every Person plus per-project role data and renders an entirely client-side grid (used to generate acknowledgment slides for talks). These tests lock the contract of that payload — project-role aggregation, -director identification (by ``Title.DIRECTOR`` position), PhD-advisee flagging, and -the publication indicators — so the view can be refactored safely. +director identification (by name, via ``Person.get_director()``), PhD-advisee +flagging, and the publication indicators — so the view can be refactored safely. """ import json @@ -66,10 +66,11 @@ def test_project_role_aggregation(self): self.assertEqual(roles["TestProj"]["start_date"], "2020-01-01") self.assertEqual(roles["TestProj"]["end_date"], "2020-12-31") - def test_director_flagged_by_title(self): - """is_director is true for whoever holds a Title.DIRECTOR position.""" - director = self.make_person(first_name="Dee", last_name="Rector") - self._add_position(director, Title.DIRECTOR, date(2010, 1, 1)) + def test_director_flagged_by_name(self): + """is_director is true only for the canonical director (Person.get_director()).""" + first, last = Person.DIRECTOR_NAME + director = self.make_person(first_name=first, last_name=last) + self._add_position(director, Title.FULL_PROF, date(2010, 1, 1)) other = self.make_person(first_name="Reg", last_name="Ular") self._add_position(other, Title.PHD_STUDENT, date(2020, 1, 1)) @@ -77,13 +78,38 @@ def test_director_flagged_by_title(self): self.assertTrue(people[director.id]["is_director"]) self.assertFalse(people[other.id]["is_director"]) + def test_director_resolved_when_holding_professor_title(self): + """ + Regression for #1284: in production the lab director holds a professor + title (not a ``Title.DIRECTOR`` position), and other people hold professor + titles too (collaborators). The director — and therefore the PhD-advisee + set the "Only my PhD advisees" filter relies on — must still resolve to the + canonical director and no one else. + """ + # Another professor who is not the director and must not be flagged. + other_prof = self.make_person(first_name="Alice", last_name="Collaborator") + self._add_position(other_prof, Title.FULL_PROF, date(2010, 1, 1)) + + first, last = Person.DIRECTOR_NAME + director = self.make_person(first_name=first, last_name=last) + self._add_position(director, Title.FULL_PROF, date(2010, 1, 1)) + + advisee = self.make_person(first_name="Phd", last_name="Student") + self._add_position(advisee, Title.PHD_STUDENT, date(2022, 1, 1), advisor=director) + + _, people = self._get_people_by_id() + self.assertTrue(people[director.id]["is_director"]) + self.assertFalse(people[other_prof.id]["is_director"]) + self.assertTrue(people[advisee.id]["is_phd_advisee"]) + def test_phd_advisee_logic_matches_model(self): """ is_phd_advisee mirrors Person.is_phd_advisee_of: current advisee -> true, past advisee without dissertation -> false, past advisee with one -> true. """ - director = self.make_person(first_name="Dee", last_name="Rector") - self._add_position(director, Title.DIRECTOR, date(2010, 1, 1)) + first, last = Person.DIRECTOR_NAME + director = self.make_person(first_name=first, last_name=last) + self._add_position(director, Title.FULL_PROF, date(2010, 1, 1)) current = self.make_person(first_name="Cur", last_name="Rent") self._add_position(current, Title.PHD_STUDENT, date(2022, 1, 1), advisor=director) diff --git a/website/views/view_project_people.py b/website/views/view_project_people.py index 327913c5..92e761fc 100644 --- a/website/views/view_project_people.py +++ b/website/views/view_project_people.py @@ -236,12 +236,10 @@ def view_project_people(request): """ today = date.today() - # The lab director is identified by their Title.DIRECTOR position. Both the - # is_director flag and the PhD-advisee check derive from this single Person so - # they cannot disagree. - director = ( - Person.objects.filter(position__title=Title.DIRECTOR).distinct().first() - ) + # The lab director drives both the is_director flag and the PhD-advisee check. + # Person.get_director() is the single source of truth, so the two flags cannot + # disagree (#1284). + director = Person.get_director() director_id = director.id if director else None phd_advisee_ids = _build_phd_advisee_ids(director) From 8c1f5c4d51819f75c15dbb1deb7a9d51536a3c18 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Wed, 17 Jun 2026 16:16:27 -0700 Subject: [PATCH 2/2] docs(view-project-people): explain the Saugstad advisee special case The "Only my PhD advisees" filter includes `last_name === 'Saugstad'` explicitly. Document why so it isn't mistaken for a bug: Mikey was Jon's MS student at UMD, chose not to pursue a PhD, and has since been the primary lead engineer on Project Sidewalk, so he's included in the acknowledgment view despite not being a PhD advisee by the formal rule. Co-Authored-By: Claude Opus 4.8 (1M context) --- website/templates/website/view_project_people.html | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/website/templates/website/view_project_people.html b/website/templates/website/view_project_people.html index 6462dc5b..c42ec107 100644 --- a/website/templates/website/view_project_people.html +++ b/website/templates/website/view_project_people.html @@ -565,9 +565,15 @@

Project People Viewer

filtered = filtered.filter(person => person.role !== 'Collaborator'); } - // Apply "only my PhD advisees" filter + // Apply "only my PhD advisees" filter. + // The `last_name === 'Saugstad'` clause is a deliberate special case, not + // a bug: Mikey Saugstad was Jon's MS student at UMD and chose not to pursue + // a PhD, but has since been the primary/only lead engineer on Project + // Sidewalk. He isn't a PhD advisee by the formal rule (server-side + // is_phd_advisee), so we include him here explicitly for acknowledgment + // grids. Revisit if he ever leaves the lab or another Saugstad joins. if (state.onlyMyAdvisees) { - filtered = filtered.filter(person => + filtered = filtered.filter(person => person.is_phd_advisee || person.last_name === 'Saugstad' ); }