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/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' ); } 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)