From 98caf26d2fa81da3895ce3dad2dc7a2ee048fdbf Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 27 Jul 2026 11:54:19 -0700 Subject: [PATCH 1/3] Expose school on people and position-during-role on project people (#1426) Project Sidewalk's new About page hydrates its team section from our public API. It couldn't render affiliations on the current-team cards (the API exposed current_title but not the school) or credentials in its ~150-person all-contributors roll call (the project-people endpoint exposed the free-text project role but not what the person *was* at the time). Everything needed was already on Position, so this is serializers + prefetching -- no model fields, no migration, and v1 stays additive-only. - PersonSerializer: add current_school and current_department, mirroring the existing current_title (all three are cached_property, not methods). - ProjectRoleSerializer: add position_title, position_school, and position_school_abbreviated -- the Position held when the role started, so a 2015 stint reads "Undergrad, UMD" even if that person is a professor today. Named position_* rather than a bare `title` so they don't read as part of `role`, the editor-written free-text description sitting next to them. - ProjectRole.get_position_during_role(): the resolution rule, documented -- the position containing the role's start date, else the latest one already begun (role started in a gap), else the earliest (role predates every position -- data drift), else None. Filters position_set in Python so it rides a prefetch, mirroring Person.get_latest_position. - Prefetch person__position_set on the project people/leadership endpoints and position_set on the people list, which also fixes the pre-existing N+1 behind current_title. A test pins query count as flat across roster size; without the prefetch it goes 11 -> 21 queries for 10 extra people. - ml_utils.get_school_abbreviated: stop acronyming one-word affiliations. Not every school is a university, and "Easterseals" -> "E" (or "Cornell" -> "C", which the local snapshot renders today) is worse than the name itself. This also fixes the public People page, which already shows these. Co-Authored-By: Claude Opus 5 (1M context) --- docs/API.md | 19 ++++- website/api/serializers.py | 55 ++++++++++++++- website/api/views.py | 6 ++ website/models/project_role.py | 44 ++++++++++++ website/tests/test_api.py | 125 +++++++++++++++++++++++++++++++++ website/tests/test_ml_utils.py | 15 ++++ website/utils/ml_utils.py | 19 ++++- 7 files changed, 276 insertions(+), 7 deletions(-) diff --git a/docs/API.md b/docs/API.md index ff0961b6..6ebf321a 100644 --- a/docs/API.md +++ b/docs/API.md @@ -71,8 +71,15 @@ sub-resources are keyed by `short_name`: - `GET /api/v1/projects//publications/` — the project's pubs. - `GET /api/v1/projects//grants/` — grants funding the project. - `GET /api/v1/projects//people/` — everyone with a role on the - project, each as a `{ person, role, lead_project_role, start_date, end_date, + project, each as a `{ person, role, lead_project_role, position_title, + position_school, position_school_abbreviated, start_date, end_date, is_active }` record (a person may appear more than once for multiple roles). + `role` is the editor-written free-text description of what they did; the + `position_*` fields are what they **were at the time** — the title and school + from the Position they held when the role started, so a 2015 stint reads + `"Undergrad"` / `"UMD"` even if that person is a professor today. If the role + began between (or before) recorded positions, the nearest one is used; all + three are `null` for someone with no Position on record. - `GET /api/v1/projects//leadership/` — **all** leadership across all time (current *and* past), grouped: `{ pis, co_pis, student_leads, postdoc_leads, research_scientist_leads }`, @@ -91,8 +98,14 @@ Funding amounts are intentionally **not** exposed by the API. Actual lab members (people with at least one Position); external co-authors are not listed here even though they appear as publication `authors`. Detail by -`url_name`: `GET /api/v1/people//` — name, current title, bio, -thumbnail, and public social/web links (ORCID, Google Scholar, GitHub, etc.). +`url_name`: `GET /api/v1/people//` — name, `current_title`, +`current_school`, `current_department`, bio, thumbnail, and public social/web +links (ORCID, Google Scholar, GitHub, etc.). + +> **Note:** the `current_*` fields come from the person's *latest* Position, so +> for an alum they describe their last lab position, not their present-day +> employer. For what someone was during a specific project stint, use the +> `position_*` fields on `/projects//people/`. > **Note:** `email` is intentionally **not** exposed by the API to avoid making > it an email-harvesting surface, even where it appears on a member page. diff --git a/website/api/serializers.py b/website/api/serializers.py index 7179954a..35a2bca9 100644 --- a/website/api/serializers.py +++ b/website/api/serializers.py @@ -75,11 +75,18 @@ def get_thumbnail(self, obj): class PersonSerializer(PersonSummarySerializer): """Full person representation for the people detail/list endpoints. - Extends the summary with bio, current title, and the public social/web + Extends the summary with bio, current affiliation, and the public social/web links. ``email`` is intentionally omitted (see module docstring). + + ``current_title`` / ``current_school`` / ``current_department`` all come from + the person's *latest* Position, so for an alum they describe their last lab + position, not their present-day employer. For what someone was during a + specific project stint, use ``ProjectRoleSerializer``'s ``position_*`` fields. """ current_title = serializers.SerializerMethodField() + current_school = serializers.SerializerMethodField() + current_department = serializers.SerializerMethodField() class Meta(PersonSummarySerializer.Meta): fields = PersonSummarySerializer.Meta.fields + [ @@ -87,6 +94,8 @@ class Meta(PersonSummarySerializer.Meta): "middle_name", "last_name", "current_title", + "current_school", + "current_department", "bio", "personal_website", "github", @@ -103,6 +112,14 @@ def get_current_title(self, obj): # Person.get_current_title is a cached_property, not a method. return obj.get_current_title + def get_current_school(self, obj): + # Person.get_current_school is a cached_property, not a method. + return obj.get_current_school + + def get_current_department(self, obj): + # Person.get_current_department is a cached_property, not a method. + return obj.get_current_department + class ProjectSummarySerializer(serializers.ModelSerializer): """Compact project representation, used when nested in publications/grants.""" @@ -262,10 +279,21 @@ def get_bibtex(self, obj): class ProjectRoleSerializer(serializers.ModelSerializer): - """A person's role on a project (start/end, lead type, active flag).""" + """A person's role on a project (start/end, lead type, active flag). + + The ``position_*`` fields describe what the person *was* when they joined the + project -- title and school from the Position held at the role's start date + (#1426) -- so a 2015 stint reads "Undergrad, UMD" even if that person is a + professor today. They're ``null`` for someone with no Position on record. + Named ``position_*`` (not bare ``title``) so they don't read as part of + ``role``, which is the editor-written free-text description of the work. + """ person = PersonSummarySerializer(read_only=True) is_active = serializers.SerializerMethodField() + position_title = serializers.SerializerMethodField() + position_school = serializers.SerializerMethodField() + position_school_abbreviated = serializers.SerializerMethodField() class Meta: model = ProjectRole @@ -273,6 +301,9 @@ class Meta: "person", "role", "lead_project_role", + "position_title", + "position_school", + "position_school_abbreviated", "start_date", "end_date", "is_active", @@ -280,3 +311,23 @@ class Meta: def get_is_active(self, obj): return obj.is_active() + + def _position(self, obj): + # Resolved once per role and stashed on the instance: three fields read + # it, and ProjectRole has no cached_property for it (see the model + # helper's note on riding the view's prefetch). + if not hasattr(obj, "_api_position"): + obj._api_position = obj.get_position_during_role() + return obj._api_position + + def get_position_title(self, obj): + position = self._position(obj) + return position.title if position else None + + def get_position_school(self, obj): + position = self._position(obj) + return position.school if position else None + + def get_position_school_abbreviated(self, obj): + position = self._position(obj) + return position.get_school_abbreviated() if position else None diff --git a/website/api/views.py b/website/api/views.py index 39cd2ab4..7288f128 100644 --- a/website/api/views.py +++ b/website/api/views.py @@ -122,8 +122,12 @@ class PersonViewSet(ReadOnlyModelViewSet): lookup_field = "url_name" def get_queryset(self): + # position_set is prefetched because current_title/current_school/ + # current_department all funnel through Person.get_latest_position, + # which resolves in Python off position_set.all(). return ( Person.objects.filter(position__isnull=False) + .prefetch_related("position_set") .distinct() .order_by("last_name", "first_name") ) @@ -199,6 +203,7 @@ def people(self, request, short_name=None): qs = ( ProjectRole.objects.filter(project=project) .select_related("person") + .prefetch_related("person__position_set") .order_by("person__last_name", "person__first_name", "start_date") ) return self._paginated(qs, ProjectRoleSerializer) @@ -218,6 +223,7 @@ def leadership(self, request, short_name=None): project=project, lead_project_role__in=list(_LEAD_BUCKETS) ) .select_related("person") + .prefetch_related("person__position_set") .order_by("-start_date") ) context = self.get_serializer_context() diff --git a/website/models/project_role.py b/website/models/project_role.py index 63a3fad2..60dd9d74 100644 --- a/website/models/project_role.py +++ b/website/models/project_role.py @@ -70,6 +70,50 @@ def get_date_range_as_str(self): else: return f"{self.start_date.year}-{self.end_date.year}" + def get_position_during_role(self): + """Returns the Position this person held when they started this project role. + + This is deliberately *not* the person's current title: a 2015 undergrad + who is now a professor should read "Undergrad" next to a 2015 project + stint (see issue #1426). Where someone's title changed mid-stint (an + undergrad who stayed on for an MS), we report what they were when they + joined the project. + + Resolution order, most to least faithful: + + 1. The latest-starting Position whose date range contains this role's + ``start_date``. + 2. Failing that, the latest-starting Position that had already begun by + ``start_date`` (the role started in a gap between positions). + 3. Failing that, the earliest Position (the role predates every recorded + position -- data drift, common for older imported roles). + 4. ``None`` only if the person has no Positions at all. + + Filters ``position_set`` in Python rather than issuing a query, so a + caller with ``prefetch_related('person__position_set')`` (e.g. the API's + project-people endpoint) resolves this with zero extra queries. This + mirrors :meth:`Person.get_latest_position`; positions-per-person is tiny. + + Example: + >>> role.get_position_during_role().title + 'Undergrad' + """ + positions = list(self.person.position_set.all()) + if not positions: + return None + + containing = [p for p in positions + if p.start_date <= self.start_date + and (p.end_date is None or p.end_date >= self.start_date)] + if containing: + return max(containing, key=lambda p: p.start_date) + + already_started = [p for p in positions if p.start_date <= self.start_date] + if already_started: + return max(already_started, key=lambda p: p.start_date) + + return min(positions, key=lambda p: p.start_date) + def get_pi_status_index(self): if self.lead_project_role is not None and self.lead_project_role in self.LEAD_PROJECT_ROLE_MAPPING: return self.LEAD_PROJECT_ROLE_MAPPING[self.lead_project_role] diff --git a/website/tests/test_api.py b/website/tests/test_api.py index a6d23e81..effb2f9d 100644 --- a/website/tests/test_api.py +++ b/website/tests/test_api.py @@ -10,6 +10,9 @@ from datetime import date +from django.db import connection +from django.test.utils import CaptureQueriesContext + from website.models import Grant, Position, ProjectRole, Sponsor from website.models.position import Title from website.models.project_role import LeadProjectRoleTypes @@ -185,6 +188,117 @@ def test_project_people_subresource(self): lead_roles = {r["lead_project_role"] for r in body["results"]} self.assertEqual(lead_roles, {"PI", "Co-PI", "Student Lead"}) + # ---- position held during a project role (#1426) ------------------------ + + def _make_role_holder(self, first_name, positions, role_start, + role_end=None): + """Create a person with the given ``(title, school, start, end)`` + positions and a single ProjectRole on self.project. Created per-test + rather than in setUp so the fixture counts other tests assert stay put. + """ + person = self.make_person(first_name=first_name, last_name="Holder") + for title, school, start, end in positions: + Position.objects.create( + person=person, title=title, school=school, + start_date=start, end_date=end, + ) + ProjectRole.objects.create( + person=person, project=self.project, + start_date=role_start, end_date=role_end, + ) + return person + + def _role_record(self, person): + """Fetch the /people/ record for a person (they hold exactly one role).""" + results = self.client.get( + "/api/v1/projects/projectsidewalk/people/?page_size=100" + ).json()["results"] + matches = [r for r in results if r["person"]["id"] == person.id] + self.assertEqual(len(matches), 1) + return matches[0] + + def test_project_people_exposes_position_during_role(self): + """The roll call wants what someone *was* at the time, not their current + title: an undergrad who later did an MS reads 'Undergrad', 'UMD'.""" + person = self._make_role_holder( + "Multi", + positions=[ + (Title.UGRAD, "University of Maryland", + date(2015, 1, 1), date(2017, 5, 31)), + (Title.MS_STUDENT, "University of Washington", + date(2017, 6, 1), date(2019, 6, 1)), + ], + role_start=date(2016, 1, 1), + role_end=date(2018, 1, 1), + ) + record = self._role_record(person) + self.assertEqual(record["position_title"], "Undergrad") + self.assertEqual(record["position_school"], "University of Maryland") + self.assertEqual(record["position_school_abbreviated"], "UMD") + + def test_project_people_position_falls_back_to_earliest(self): + """A role that predates every recorded Position (data drift) reports the + earliest position rather than nothing.""" + person = self._make_role_holder( + "Early", + positions=[(Title.PHD_STUDENT, "University of Washington", + date(2020, 1, 1), None)], + role_start=date(2018, 1, 1), + ) + record = self._role_record(person) + self.assertEqual(record["position_title"], "PhD Student") + self.assertEqual(record["position_school_abbreviated"], "UW") + + def test_project_people_position_falls_back_to_prior_when_in_gap(self): + """A role starting between two positions reports the most recent one that + had already started.""" + person = self._make_role_holder( + "Gap", + positions=[ + (Title.UGRAD, "University of Maryland", + date(2014, 1, 1), date(2015, 1, 1)), + (Title.PHD_STUDENT, "University of Washington", + date(2019, 1, 1), None), + ], + role_start=date(2017, 1, 1), + ) + record = self._role_record(person) + self.assertEqual(record["position_title"], "Undergrad") + + def test_project_people_position_null_without_position(self): + """self.past_lead has a project role but no Position at all.""" + record = self._role_record(self.past_lead) + self.assertIsNone(record["position_title"]) + self.assertIsNone(record["position_school"]) + self.assertIsNone(record["position_school_abbreviated"]) + + def test_project_people_does_not_scale_queries_with_rows(self): + """N+1 guard: resolving each role's Position must ride on the view's + prefetch, so query count is flat as the roster grows (#1426).""" + url = "/api/v1/projects/projectsidewalk/people/?page_size=100" + for i in range(3): + self._make_role_holder( + f"Small{i}", + positions=[(Title.PHD_STUDENT, "University of Washington", + date(2015, 1, 1), None)], + role_start=date(2016, 1, 1), + ) + baseline = self._count_queries(url) + + for i in range(10): + self._make_role_holder( + f"Big{i}", + positions=[(Title.PHD_STUDENT, "University of Washington", + date(2015, 1, 1), None)], + role_start=date(2016, 1, 1), + ) + self.assertEqual(self._count_queries(url), baseline) + + def _count_queries(self, url): + with CaptureQueriesContext(connection) as ctx: + self.assertEqual(self.client.get(url).status_code, 200) + return len(ctx) + def test_project_leadership_subresource(self): resp = self.client.get("/api/v1/projects/projectsidewalk/leadership/") body = resp.json() @@ -222,6 +336,17 @@ def test_person_detail_by_url_name(self): self.assertEqual(resp.status_code, 200) self.assertEqual(resp.json()["name"], "Jon Froehlich") + def test_person_detail_exposes_current_school_and_department(self): + """Affiliation for the team cards -- both come from the latest Position, + like the already-exposed current_title (#1426).""" + body = self.client.get(f"/api/v1/people/{self.jon.url_name}/").json() + self.assertEqual(body["current_title"], "Professor") + self.assertEqual(body["current_school"], "University of Washington") + self.assertEqual( + body["current_department"], + "Allen School of Computer Science and Engineering", + ) + def test_person_email_not_exposed(self): resp = self.client.get(f"/api/v1/people/{self.jon.url_name}/") self.assertNotIn("email", resp.json()) diff --git a/website/tests/test_ml_utils.py b/website/tests/test_ml_utils.py index 3096be91..17c0d17b 100644 --- a/website/tests/test_ml_utils.py +++ b/website/tests/test_ml_utils.py @@ -48,6 +48,21 @@ def test_other_school_falls_back_to_acronym(self): # " of " is dropped before acronyming, so "University of Foo Bar" -> UFB. self.assertEqual(get_school_abbreviated("University of Foo Bar"), "UFB") + def test_uic_acronym(self): + # A real affiliation on the site (the Project Sidewalk UIC subaward). + self.assertEqual( + get_school_abbreviated("University of Illinois Chicago"), "UIC" + ) + + def test_single_word_org_is_not_acronymed(self): + # Non-university affiliations are often one word, where an acronym is a + # single useless letter ("Easterseals" -> "E"). Return the name instead. + self.assertEqual(get_school_abbreviated("Easterseals"), "Easterseals") + self.assertEqual(get_school_abbreviated("Google"), "Google") + + def test_empty_school_is_passed_through(self): + self.assertEqual(get_school_abbreviated(""), "") + class GetDepartmentAbbreviatedTests(SimpleTestCase): def test_cse(self): diff --git a/website/utils/ml_utils.py b/website/utils/ml_utils.py index e4936fa8..0abb19e9 100644 --- a/website/utils/ml_utils.py +++ b/website/utils/ml_utils.py @@ -32,7 +32,21 @@ def slugify_max(text, max_length=100): return trimmed_slug def get_school_abbreviated(school_name): - """Returns the school abbreviation for a given school name""" + """Returns the school abbreviation for a given school name. + + Multi-word names acronym ("University of Illinois Chicago" -> "UIC"; " of " + is dropped first). Names that would acronym to a single letter are returned + unchanged: not every affiliation is a university, and one-word organizations + like "Easterseals" are far more readable than "E". + + Examples: + >>> get_school_abbreviated("University of Washington") + 'UW' + >>> get_school_abbreviated("University of Illinois Chicago") + 'UIC' + >>> get_school_abbreviated("Easterseals") + 'Easterseals' + """ school_low = school_name.lower() if "washington" in school_low: @@ -41,7 +55,8 @@ def get_school_abbreviated(school_name): return "UMD" else: school_low = school_low.replace(" of ", " ") - return create_acronym(school_low).upper() + acronym = create_acronym(school_low).upper() + return acronym if len(acronym) > 1 else school_name def create_acronym(name): """Returns the acronym for a given name""" From cfa2ebd3d4aaf008ab96e27dce70f7e98d029001 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 27 Jul 2026 12:58:45 -0700 Subject: [PATCH 2/3] Bump version to 2.28.0 (API school + position-during-role fields #1426) Co-Authored-By: Claude Opus 5 (1M context) --- makeabilitylab/settings.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index 2f3431f9..2ebfaa55 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -86,8 +86,8 @@ SECURE_PROXY_SSL_HEADER = ('HTTP_X_FORWARDED_PROTO', 'https') # Makeability Lab Global Variables, including Makeability Lab version -ML_WEBSITE_VERSION = "2.27.4" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "Infrastructure-only release: hardens the auto-deploy webhook (auto-deploy/index.php, #1422) after the test server spent Jul 18-22 rebuilding on every push while serving source frozen at Jul 18. Root cause was a one-character bug -- the deploy condition read `$OPERATION = 'TAG'` (assignment) instead of `==`, so ANY tag push satisfied it on EVERY configured host and reassigned $OPERATION for the rest of the request; a tag push therefore drove the branch-tracking test host down the tag code path and left its checkout detached at that tag. It could not recover, because `git pull` aborts from a detached HEAD and the pull ran before the branch checkout. Compounding it, the container build ran regardless of whether the pull or checkout succeeded, so a failed update still produced a successful-looking rebuild -- which is why it went unnoticed for four days. Fixed: `==`; branch hosts now check out the branch before pulling (tags keep fetch-first, since the tag is not local until after the fetch); and the container build is gated on the checked-out HEAD actually matching the pushed commit, aborting with a log line instead of silently shipping stale source. The gate peels annotated tags with ^{commit} (our release tags are annotated, and `after` carries the tag object's sha, not the commit's) and fails safe -- if the sha cannot be resolved locally it deploys as before. Also fixed three more instances of the same =/== footgun (_trace/_debug were always on), a misspelled $reqest variable that made the branch fast-path dead code, and the incoming ref is now validated against refs/(heads|tags)/[A-Za-z0-9._/-]+ before it reaches a shell. No application code changed." +ML_WEBSITE_VERSION = "2.28.0" # Keep this updated with each release and also change the short description below +ML_WEBSITE_VERSION_DESCRIPTION = "Adds five fields to the public v1 REST API so Project Sidewalk's new About page can render its team section straight from this site instead of hand-maintaining a roster (#1426). /api/v1/people// gains current_school and current_department alongside the existing current_title; /api/v1/projects//people/ gains position_title, position_school, and position_school_abbreviated -- what the person WAS when they joined the project, not what they are now, so a 2015 stint reads 'Undergrad, UMD' even though that person may be a professor today. (Jon's own 2012 Sidewalk role now correctly reports Assistant Professor / UMD rather than Professor / UW.) The new fields are named position_* rather than a bare 'title' so they don't read as part of 'role', the editor-written free-text description sitting beside them in the same record. Which position gets picked is an explicit, documented rule on ProjectRole.get_position_during_role(): the position containing the role's start date, else the latest one already begun (the role started in a gap between positions), else the earliest (the role predates every recorded position -- common in older imported data), and null only for someone with no Position at all. Both project sub-resources now prefetch person__position_set and the people list prefetches position_set, so resolving a position per row rides the prefetch instead of costing a query each -- that matters at Sidewalk's ~150-person roster, and it also fixes a pre-existing N+1 behind current_title; a regression test pins the query count as flat as the roster grows. Separately, get_school_abbreviated no longer acronyms one-word affiliations: not every school is a university, and 'Easterseals' -> 'E' (or 'Cornell' -> 'C') is worse than the name itself. That is a visible fix on the public People page too, not just in the API. No model changes and no migration; v1 stays additive-only, so existing consumers are unaffected." DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed MAX_BANNERS = 7 # Maximum number of banners on a page From 0018310d394c450087f0b5f4e4106a3b630993b1 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 27 Jul 2026 13:30:45 -0700 Subject: [PATCH 3/3] Code review follow-ups: deterministic position tie-break, precise UW/UMD match Addresses review findings on #1426. - ProjectRole.position_during_role (renamed from get_position_during_role()) now breaks start_date ties on pk. position_set has no Meta.ordering, so two positions beginning the same day -- concurrent Member/Collaborator rows, or a duplicate entry -- let DB row order pick the winner, which could report a different title for the same role across identical requests. - It's a cached_property now, matching Person.get_current_title, so the API's three fields resolve it once per row from the model rather than via a private attribute stashed on the instance by the serializer. - get_school_abbreviated's UW/UMD special cases match the phrase "university of washington/maryland" instead of a bare substring. The substring check handed UW's initials to Washington State University, George Washington University, and Washington University in St. Louis, and UMD's to UMBC; the generic acronym path gets all four right on its own (WSU/GWU/UMBC), so they fall through to it. Punctuation is now normalized first, so a typed comma can't change the answer. Diffed old vs new across every school value in the local prod snapshot: no change except the two intended one-word fixes (Cornell, IIT-Delhi). - docs/API.md said the "nearest" position is used for a role starting in a gap; it's actually the most recent one already begun, which can be the further of the two. Corrected -- this is the public contract. - The resolution rules move to a new website/tests/test_project_role.py, unit tested directly against the model (all four rules, overlapping positions, the tie-break, and a zero-query assertion under prefetch), so a failure isolates to the logic. test_api.py keeps the wire-contract tests: fields present, nulls, and the N+1 guard. Full suite green: 640 tests. Co-Authored-By: Claude Opus 5 (1M context) --- docs/API.md | 7 +- makeabilitylab/settings.py | 2 +- website/api/serializers.py | 17 ++--- website/models/project_role.py | 31 ++++++--- website/tests/test_api.py | 44 +++---------- website/tests/test_ml_utils.py | 24 +++++++ website/tests/test_project_role.py | 101 +++++++++++++++++++++++++++++ website/utils/ml_utils.py | 26 ++++++-- 8 files changed, 187 insertions(+), 65 deletions(-) create mode 100644 website/tests/test_project_role.py diff --git a/docs/API.md b/docs/API.md index 6ebf321a..4df07614 100644 --- a/docs/API.md +++ b/docs/API.md @@ -77,9 +77,10 @@ sub-resources are keyed by `short_name`: `role` is the editor-written free-text description of what they did; the `position_*` fields are what they **were at the time** — the title and school from the Position they held when the role started, so a 2015 stint reads - `"Undergrad"` / `"UMD"` even if that person is a professor today. If the role - began between (or before) recorded positions, the nearest one is used; all - three are `null` for someone with no Position on record. + `"Undergrad"` / `"UMD"` even if that person is a professor today. Where the + role's start date falls outside every recorded position, the most recent + position already begun by then is used — or, for a role predating them all, + the earliest. All three are `null` for someone with no Position on record. - `GET /api/v1/projects//leadership/` — **all** leadership across all time (current *and* past), grouped: `{ pis, co_pis, student_leads, postdoc_leads, research_scientist_leads }`, diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index 2ebfaa55..87297374 100644 --- a/makeabilitylab/settings.py +++ b/makeabilitylab/settings.py @@ -87,7 +87,7 @@ # Makeability Lab Global Variables, including Makeability Lab version ML_WEBSITE_VERSION = "2.28.0" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "Adds five fields to the public v1 REST API so Project Sidewalk's new About page can render its team section straight from this site instead of hand-maintaining a roster (#1426). /api/v1/people// gains current_school and current_department alongside the existing current_title; /api/v1/projects//people/ gains position_title, position_school, and position_school_abbreviated -- what the person WAS when they joined the project, not what they are now, so a 2015 stint reads 'Undergrad, UMD' even though that person may be a professor today. (Jon's own 2012 Sidewalk role now correctly reports Assistant Professor / UMD rather than Professor / UW.) The new fields are named position_* rather than a bare 'title' so they don't read as part of 'role', the editor-written free-text description sitting beside them in the same record. Which position gets picked is an explicit, documented rule on ProjectRole.get_position_during_role(): the position containing the role's start date, else the latest one already begun (the role started in a gap between positions), else the earliest (the role predates every recorded position -- common in older imported data), and null only for someone with no Position at all. Both project sub-resources now prefetch person__position_set and the people list prefetches position_set, so resolving a position per row rides the prefetch instead of costing a query each -- that matters at Sidewalk's ~150-person roster, and it also fixes a pre-existing N+1 behind current_title; a regression test pins the query count as flat as the roster grows. Separately, get_school_abbreviated no longer acronyms one-word affiliations: not every school is a university, and 'Easterseals' -> 'E' (or 'Cornell' -> 'C') is worse than the name itself. That is a visible fix on the public People page too, not just in the API. No model changes and no migration; v1 stays additive-only, so existing consumers are unaffected." +ML_WEBSITE_VERSION_DESCRIPTION = "Adds five fields to the public v1 REST API so Project Sidewalk's new About page can render its team section straight from this site instead of hand-maintaining a roster (#1426). /api/v1/people// gains current_school and current_department alongside the existing current_title; /api/v1/projects//people/ gains position_title, position_school, and position_school_abbreviated -- what the person WAS when they joined the project, not what they are now, so a 2015 stint reads 'Undergrad, UMD' even though that person may be a professor today. (Jon's own 2012 Sidewalk role now correctly reports Assistant Professor / UMD rather than Professor / UW.) The new fields are named position_* rather than a bare 'title' so they don't read as part of 'role', the editor-written free-text description sitting beside them in the same record. Which position gets picked is an explicit, documented rule on ProjectRole.position_during_role: the position containing the role's start date, else the latest one already begun (the role started in a gap between positions), else the earliest (the role predates every recorded position -- common in older imported data), and null only for someone with no Position at all; ties on start date break on pk so two positions beginning the same day can't let DB row order decide the answer from one request to the next. Both project sub-resources now prefetch person__position_set and the people list prefetches position_set, so resolving a position per row rides the prefetch instead of costing a query each -- that matters at Sidewalk's ~150-person roster, and it also fixes a pre-existing N+1 behind current_title; a regression test pins the query count as flat as the roster grows. Separately, get_school_abbreviated is tightened twice: it no longer acronyms one-word affiliations (not every school is a university, and 'Easterseals' -> 'E' (or 'Cornell' -> 'C') is worse than the name itself), and its UW/UMD special cases now match the phrase 'university of washington/maryland' rather than a bare 'washington'/'maryland' substring, which used to hand UW's initials to Washington State and Washington University in St. Louis, and UMD's to UMBC. That is a visible fix on the public People page too, not just in the API. No model changes and no migration; v1 stays additive-only, so existing consumers are unaffected." DATE_MAKEABILITYLAB_FORMED = datetime.date(2012, 1, 1) # Date Makeability Lab was formed MAX_BANNERS = 7 # Maximum number of banners on a page diff --git a/website/api/serializers.py b/website/api/serializers.py index 35a2bca9..4efc0786 100644 --- a/website/api/serializers.py +++ b/website/api/serializers.py @@ -312,22 +312,17 @@ class Meta: def get_is_active(self, obj): return obj.is_active() - def _position(self, obj): - # Resolved once per role and stashed on the instance: three fields read - # it, and ProjectRole has no cached_property for it (see the model - # helper's note on riding the view's prefetch). - if not hasattr(obj, "_api_position"): - obj._api_position = obj.get_position_during_role() - return obj._api_position - + # ProjectRole.position_during_role is a cached_property, so the three fields + # below resolve it once per row (and ride the view's prefetch -- see the + # model helper). def get_position_title(self, obj): - position = self._position(obj) + position = obj.position_during_role return position.title if position else None def get_position_school(self, obj): - position = self._position(obj) + position = obj.position_during_role return position.school if position else None def get_position_school_abbreviated(self, obj): - position = self._position(obj) + position = obj.position_during_role return position.get_school_abbreviated() if position else None diff --git a/website/models/project_role.py b/website/models/project_role.py index 60dd9d74..60298825 100644 --- a/website/models/project_role.py +++ b/website/models/project_role.py @@ -1,4 +1,5 @@ from django.db import models +from django.utils.functional import cached_property from datetime import date, datetime, timedelta class LeadProjectRoleTypes(models.TextChoices): @@ -70,8 +71,9 @@ def get_date_range_as_str(self): else: return f"{self.start_date.year}-{self.end_date.year}" - def get_position_during_role(self): - """Returns the Position this person held when they started this project role. + @cached_property + def position_during_role(self): + """The Position this person held when they started this project role. This is deliberately *not* the person's current title: a 2015 undergrad who is now a professor should read "Undergrad" next to a 2015 project @@ -89,30 +91,41 @@ def get_position_during_role(self): position -- data drift, common for older imported roles). 4. ``None`` only if the person has no Positions at all. - Filters ``position_set`` in Python rather than issuing a query, so a - caller with ``prefetch_related('person__position_set')`` (e.g. the API's - project-people endpoint) resolves this with zero extra queries. This + Ties on ``start_date`` (a person holding two positions that begin the + same day -- concurrent Member/Collaborator rows, or a duplicate entry) + break on ``pk``, so the answer is stable across requests: ``position_set`` + has no ``Meta.ordering``, so without an explicit tie-break the winner + would follow whatever order the DB happened to return. + + A ``cached_property`` (like :meth:`Person.get_current_title`) because the + API reads it three times per row -- for title, school, and abbreviated + school. Filters ``position_set`` in Python rather than issuing a query, + so a caller with ``prefetch_related('person__position_set')`` (e.g. the + API's project-people endpoint) resolves it with zero extra queries. This mirrors :meth:`Person.get_latest_position`; positions-per-person is tiny. Example: - >>> role.get_position_during_role().title + >>> role.position_during_role.title 'Undergrad' """ positions = list(self.person.position_set.all()) if not positions: return None + # Sorts rather than max()/min() so the pk tie-break is stated once. + positions.sort(key=lambda p: (p.start_date, p.pk)) + containing = [p for p in positions if p.start_date <= self.start_date and (p.end_date is None or p.end_date >= self.start_date)] if containing: - return max(containing, key=lambda p: p.start_date) + return containing[-1] already_started = [p for p in positions if p.start_date <= self.start_date] if already_started: - return max(already_started, key=lambda p: p.start_date) + return already_started[-1] - return min(positions, key=lambda p: p.start_date) + return positions[0] def get_pi_status_index(self): if self.lead_project_role is not None and self.lead_project_role in self.LEAD_PROJECT_ROLE_MAPPING: diff --git a/website/tests/test_api.py b/website/tests/test_api.py index effb2f9d..eca46bab 100644 --- a/website/tests/test_api.py +++ b/website/tests/test_api.py @@ -189,6 +189,16 @@ def test_project_people_subresource(self): self.assertEqual(lead_roles, {"PI", "Co-PI", "Student Lead"}) # ---- position held during a project role (#1426) ------------------------ + # + # These pin the *wire contract* -- that the resolved Position reaches the + # payload, and at what cost. The resolution rules themselves (which Position + # wins for overlapping / gapped / predating dates) are unit-tested directly + # against the model in test_project_role.py, where a failure isolates. + + def _count_queries(self, url): + with CaptureQueriesContext(connection) as ctx: + self.assertEqual(self.client.get(url).status_code, 200) + return len(ctx) def _make_role_holder(self, first_name, positions, role_start, role_end=None): @@ -236,35 +246,6 @@ def test_project_people_exposes_position_during_role(self): self.assertEqual(record["position_school"], "University of Maryland") self.assertEqual(record["position_school_abbreviated"], "UMD") - def test_project_people_position_falls_back_to_earliest(self): - """A role that predates every recorded Position (data drift) reports the - earliest position rather than nothing.""" - person = self._make_role_holder( - "Early", - positions=[(Title.PHD_STUDENT, "University of Washington", - date(2020, 1, 1), None)], - role_start=date(2018, 1, 1), - ) - record = self._role_record(person) - self.assertEqual(record["position_title"], "PhD Student") - self.assertEqual(record["position_school_abbreviated"], "UW") - - def test_project_people_position_falls_back_to_prior_when_in_gap(self): - """A role starting between two positions reports the most recent one that - had already started.""" - person = self._make_role_holder( - "Gap", - positions=[ - (Title.UGRAD, "University of Maryland", - date(2014, 1, 1), date(2015, 1, 1)), - (Title.PHD_STUDENT, "University of Washington", - date(2019, 1, 1), None), - ], - role_start=date(2017, 1, 1), - ) - record = self._role_record(person) - self.assertEqual(record["position_title"], "Undergrad") - def test_project_people_position_null_without_position(self): """self.past_lead has a project role but no Position at all.""" record = self._role_record(self.past_lead) @@ -294,11 +275,6 @@ def test_project_people_does_not_scale_queries_with_rows(self): ) self.assertEqual(self._count_queries(url), baseline) - def _count_queries(self, url): - with CaptureQueriesContext(connection) as ctx: - self.assertEqual(self.client.get(url).status_code, 200) - return len(ctx) - def test_project_leadership_subresource(self): resp = self.client.get("/api/v1/projects/projectsidewalk/leadership/") body = resp.json() diff --git a/website/tests/test_ml_utils.py b/website/tests/test_ml_utils.py index 17c0d17b..1742f0ec 100644 --- a/website/tests/test_ml_utils.py +++ b/website/tests/test_ml_utils.py @@ -48,6 +48,30 @@ def test_other_school_falls_back_to_acronym(self): # " of " is dropped before acronyming, so "University of Foo Bar" -> UFB. self.assertEqual(get_school_abbreviated("University of Foo Bar"), "UFB") + def test_maryland_college_park_is_umd(self): + self.assertEqual( + get_school_abbreviated("University of Maryland, College Park"), "UMD" + ) + + def test_washington_in_another_school_name_is_not_uw(self): + # The special case matches "university of washington", not a bare + # "washington" substring, which used to hand UW's initials to every + # school with Washington in its name. These acronym instead. + self.assertEqual(get_school_abbreviated("Washington State University"), "WSU") + self.assertEqual(get_school_abbreviated("George Washington University"), "GWU") + self.assertNotEqual( + get_school_abbreviated("Washington University in St. Louis"), "UW" + ) + + def test_umbc_is_not_umd(self): + # Same bug on the Maryland side: UMBC is its own school. + self.assertEqual( + get_school_abbreviated("University of Maryland Baltimore County"), "UMBC" + ) + self.assertEqual( + get_school_abbreviated("University of Maryland, Baltimore County"), "UMBC" + ) + def test_uic_acronym(self): # A real affiliation on the site (the Project Sidewalk UIC subaward). self.assertEqual( diff --git a/website/tests/test_project_role.py b/website/tests/test_project_role.py new file mode 100644 index 00000000..52ad9fb3 --- /dev/null +++ b/website/tests/test_project_role.py @@ -0,0 +1,101 @@ +"""Tests for ProjectRole model helpers. + +Covers ``ProjectRole.position_during_role`` (#1426): the rule that decides which +Position a person held when they started a project role. The public REST API +serializes it (``position_title`` / ``position_school`` / +``position_school_abbreviated``), but the rules are exercised here, directly +against the model, so a failure points at the resolution logic rather than at +the API contract. The wire contract is pinned in test_api.py. +""" + +from datetime import date + +from website.models import Position, ProjectRole +from website.models.position import Title +from website.tests.base import DatabaseTestCase + + +class PositionDuringRoleTests(DatabaseTestCase): + def setUp(self): + self.project = self.make_project(name="Project Sidewalk", + short_name="projectsidewalk") + self.person = self.make_person(first_name="Ada", last_name="Holder") + + def _position(self, title, start, end=None, school="University of Maryland"): + return Position.objects.create( + person=self.person, title=title, school=school, + start_date=start, end_date=end, + ) + + def _role(self, start, end=None): + return ProjectRole.objects.create( + person=self.person, project=self.project, + start_date=start, end_date=end, + ) + + def test_returns_position_containing_role_start(self): + """The headline case: what they were when they joined, not what they are + now. An undergrad who later did an MS reads 'Undergrad' on a 2016 stint. + """ + ugrad = self._position(Title.UGRAD, date(2015, 1, 1), date(2017, 5, 31)) + self._position(Title.MS_STUDENT, date(2017, 6, 1), date(2019, 6, 1), + school="University of Washington") + role = self._role(date(2016, 1, 1), date(2018, 1, 1)) + self.assertEqual(role.position_during_role, ugrad) + + def test_overlapping_positions_pick_the_latest_to_start(self): + """Positions can overlap (a title change recorded as a new row without + closing the old one). The most recently started one wins -- it's the + better description of the person at that moment.""" + self._position(Title.UGRAD, date(2015, 1, 1), date(2019, 1, 1)) + ms = self._position(Title.MS_STUDENT, date(2017, 1, 1), date(2019, 1, 1)) + role = self._role(date(2018, 1, 1)) + self.assertEqual(role.position_during_role, ms) + + def test_open_ended_position_contains_later_role(self): + """A position with no end date contains everything after its start.""" + phd = self._position(Title.PHD_STUDENT, date(2015, 1, 1), None) + role = self._role(date(2023, 6, 1)) + self.assertEqual(role.position_during_role, phd) + + def test_falls_back_to_prior_position_when_role_starts_in_a_gap(self): + """A role starting between two positions reports the most recent one + that had already started -- not the nearest in time.""" + ugrad = self._position(Title.UGRAD, date(2014, 1, 1), date(2015, 1, 1)) + self._position(Title.PHD_STUDENT, date(2019, 1, 1), None) + role = self._role(date(2017, 1, 1)) + self.assertEqual(role.position_during_role, ugrad) + + def test_falls_back_to_earliest_when_role_predates_every_position(self): + """Data drift, common in older imported roles: report the earliest + position rather than nothing.""" + phd = self._position(Title.PHD_STUDENT, date(2020, 1, 1), None) + self._position(Title.POST_DOC, date(2025, 1, 1), None) + role = self._role(date(2018, 1, 1)) + self.assertEqual(role.position_during_role, phd) + + def test_none_when_person_has_no_positions(self): + """Real case: external collaborators hold ProjectRoles but no Position.""" + role = self._role(date(2018, 1, 1)) + self.assertIsNone(role.position_during_role) + + def test_same_start_date_ties_break_deterministically(self): + """Two positions starting the same day (concurrent Member/Collaborator + rows, or a duplicate entry) must not let DB row order decide the answer: + position_set has no Meta.ordering, so the tie breaks on pk.""" + self._position(Title.UGRAD, date(2015, 1, 1), None) + later_row = self._position(Title.SOFTWARE_DEVELOPER, date(2015, 1, 1), None) + role = self._role(date(2016, 1, 1)) + self.assertEqual(role.position_during_role, later_row) + + def test_resolves_without_extra_queries_when_prefetched(self): + """The API relies on this riding prefetch_related('person__position_set') + -- see test_api's N+1 guard. Pinned here too since it's a property of the + model helper (it filters in Python), not of the view.""" + self._position(Title.UGRAD, date(2015, 1, 1), None) + self._role(date(2016, 1, 1)) + role = (ProjectRole.objects + .prefetch_related("person__position_set") + .get(project=self.project)) + with self.assertNumQueries(0): + self.assertIsNotNone(role.position_during_role) diff --git a/website/utils/ml_utils.py b/website/utils/ml_utils.py index 0abb19e9..fd5e3490 100644 --- a/website/utils/ml_utils.py +++ b/website/utils/ml_utils.py @@ -34,24 +34,36 @@ def slugify_max(text, max_length=100): def get_school_abbreviated(school_name): """Returns the school abbreviation for a given school name. - Multi-word names acronym ("University of Illinois Chicago" -> "UIC"; " of " - is dropped first). Names that would acronym to a single letter are returned - unchanged: not every affiliation is a university, and one-word organizations - like "Easterseals" are far more readable than "E". + The lab's own two schools are special-cased; everything else acronyms + ("University of Illinois Chicago" -> "UIC"; " of " is dropped first). Names + that would acronym to a single letter are returned unchanged: not every + affiliation is a university, and one-word organizations like "Easterseals" + are far more readable than "E". + + The special cases match the phrase "university of washington/maryland", not + a bare "washington"/"maryland" substring, which used to hand UW's initials + to any school with Washington in its name. The generic acronym path happens + to get those right on its own (Washington State -> WSU, George Washington -> + GWU, UMD Baltimore County -> UMBC), so they're simply left to it. Examples: >>> get_school_abbreviated("University of Washington") 'UW' >>> get_school_abbreviated("University of Illinois Chicago") 'UIC' + >>> get_school_abbreviated("University of Maryland, Baltimore County") + 'UMBC' >>> get_school_abbreviated("Easterseals") 'Easterseals' """ - school_low = school_name.lower() + # Punctuation dropped and whitespace collapsed so the phrase match and the + # acronym both see "university of maryland baltimore county" whether or not + # the editor typed the comma. + school_low = " ".join(school_name.lower().replace(",", " ").split()) - if "washington" in school_low: + if "university of washington" in school_low: return "UW" - elif "maryland" in school_low: + elif "university of maryland" in school_low and "baltimore" not in school_low: return "UMD" else: school_low = school_low.replace(" of ", " ")