Fix "Only my PhD advisees" filter on view-project-people (#1325)

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.
 

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)

website/templates/website/view_project_people.html

@@ -565,9 +565,15 @@ <h1 id="page-title">Project People Viewer</h1>
           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'
           );
         }

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,24 +66,50 @@ 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))
 
         _, people = self._get_people_by_id()
         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)

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)