diff --git a/docs/API.md b/docs/API.md index a86d08b6..619f32f5 100644 --- a/docs/API.md +++ b/docs/API.md @@ -76,12 +76,18 @@ sub-resources are keyed by `short_name`: 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. 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. + `position_*` fields describe them **over the span of that role** — the title + and school from the latest Position overlapping the role's window + (`start_date` → `end_date`, or today for an open role; never past today). So + an ongoing role reads as what the person is *now* (`"Professor"` / `"UW"`), + while a stint that ended in 2016 still reads `"Undergrad"` / `"UMD"` even if + that person is a professor today. Where a role's window falls outside every + recorded position, the most recent position already begun by its end is used — + or, for a role predating them all, the earliest. All three are `null` for + someone with no Position on record. + > Use these rather than `/people//`'s `current_*` fields for a roster: + > they're scoped to the project, so a past contributor reads as what they were + > on the project, not their last lab position. - `GET /api/v1/projects//leadership/` — **all** leadership across all time (current *and* past), grouped: `{ pis, co_pis, student_leads, postdoc_leads, research_scientist_leads }`, @@ -107,8 +113,10 @@ etc.). See *Images* below. > **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/`. +> employer. For someone's title during a specific project stint — including a +> current one — use the `position_*` fields on +> `/projects//people/`; they're project-scoped, and for an active +> role they carry the same present-day title anyway. > **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. @@ -145,6 +153,13 @@ disk and will 404. Ask for a different size in an issue instead. derivative described above. A field named `thumbnail` handing out an 11 MB original was a bug, not a contract — 13 headshots cost one consumer 33.5 MB. The raw file is still available as `image_original`. + - A second, in **2.31.0** (#1435): the `position_*` fields on + `/projects//people/` were pinned to the Position held on the + role's *start* date, so a role open since 2012 still reported a title from + 2012. They now report the latest Position overlapping the role, which only + changes values for roles spanning a title change — and changes them to the + ones a roster actually wants. Finished stints are unaffected unless the + person was promoted mid-stint. - Don't hardcode pagination page sizes as a proxy for "all" — page through `next`, or set `page_size` explicitly (≤100). - URLs in responses (PDFs, thumbnails, page links) are absolute and safe to use diff --git a/makeabilitylab/settings.py b/makeabilitylab/settings.py index 2e7011d4..62dd8bcc 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.30.0" # Keep this updated with each release and also change the short description below -ML_WEBSITE_VERSION_DESCRIPTION = "The public API now serves cropped, sized photos (#1432): a person or project thumbnail honors the crop box you set in the admin instead of handing out the raw upload, which cost one consumer 33.5 MB for 13 headshots. The full-resolution file is still available." +ML_WEBSITE_VERSION = "2.31.0" # Keep this updated with each release and also change the short description below +ML_WEBSITE_VERSION_DESCRIPTION = "A project's roster in the public API now reports each person's title over the span of their role (#1435), so someone on a project since 2012 reads as what they are today rather than the title they held back then. Finished stints still read as what that person was at the time." 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 49907872..45869ce1 100644 --- a/website/api/serializers.py +++ b/website/api/serializers.py @@ -340,12 +340,13 @@ def get_bibtex(self, obj): class ProjectRoleSerializer(serializers.ModelSerializer): """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. + The ``position_*`` fields describe the person *over the span of this role* -- + title and school from the latest Position overlapping it (#1426, #1435). An + ongoing role reports what they are today ("Professor, UW"); a stint that + ended in 2016 still reads "Undergrad, UMD" even if that person is a professor + now. 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) diff --git a/website/models/project_role.py b/website/models/project_role.py index 60298825..016a7fc1 100644 --- a/website/models/project_role.py +++ b/website/models/project_role.py @@ -73,24 +73,47 @@ def get_date_range_as_str(self): @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 - 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. + """The Position that best describes this person over this project role. + + Specifically, the *latest* Position overlapping the role's window -- + ``[start_date, end_date or today]``, with the end never past today. That + gives the two answers a roster wants (#1426, refined in #1435): + + - An **ongoing** role reports what the person is *now*. Someone who has + led a project since 2012 and was promoted along the way reads + "Professor, University of Washington", not the assistant professorship + they held when the stint began. + - A **finished** stint stays frozen in its own time. A 2015 undergrad who + is a professor today still reads "Undergrad" next to that stint, because + the professorship never overlapped it. Where their title changed + mid-stint (an undergrad who stayed on for an MS), the later title wins. + + This is why it is *not* :meth:`Person.get_current_title`, which is the + person's latest Position full stop -- for an alum, their last lab position + rather than either of the above. The one case where the two converge is a + role left open (``end_date`` null) after the person has actually left: its + window runs to today, so it reports their last lab position, same as + ``get_current_title`` would. That's a data-entry gap, not a rule -- both + :meth:`Position.save` (which closes a departing member's open roles) and + the ``auto_close_project_roles`` command exist to keep it from arising. Resolution order, most to least faithful: - 1. The latest-starting Position whose date range contains this role's - ``start_date``. + 1. The latest-starting Position overlapping the role's window. 2. Failing that, the latest-starting Position that had already begun by - ``start_date`` (the role started in a gap between positions). + the window's end (the whole role sits 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. + The window end is clamped twice. Up top, to today, so a Position starting + in the future (a promotion entered ahead of time) never reaches today's + payload -- including on a role carrying a future ``end_date``, which an + editor may enter for a planned wrap-up. Down below, to at least the start + date, so a typo'd ``end_date`` earlier than ``start_date`` degrades to + "position at role start" instead of inverting the range and matching + nothing. + 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`` @@ -105,8 +128,10 @@ def position_during_role(self): mirrors :meth:`Person.get_latest_position`; positions-per-person is tiny. Example: + >>> role.end_date is None # still on the project + True >>> role.position_during_role.title - 'Undergrad' + 'Professor' """ positions = list(self.person.position_set.all()) if not positions: @@ -115,13 +140,19 @@ def position_during_role(self): # 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 containing[-1] + today = date.today() + window_start = self.start_date + # min() first so a future end_date can't admit a not-yet-started + # position; max() second so a typo'd end_date can't invert the window. + window_end = max(min(self.end_date or today, today), window_start) + + overlapping = [p for p in positions + if p.start_date <= window_end + and (p.end_date is None or p.end_date >= window_start)] + if overlapping: + return overlapping[-1] - already_started = [p for p in positions if p.start_date <= self.start_date] + already_started = [p for p in positions if p.start_date <= window_end] if already_started: return already_started[-1] diff --git a/website/tests/test_api.py b/website/tests/test_api.py index ca7e981f..23887476 100644 --- a/website/tests/test_api.py +++ b/website/tests/test_api.py @@ -225,7 +225,7 @@ 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) ------------------------ + # ---- position held during a project role (#1426, #1435) ----------------- # # These pin the *wire contract* -- that the resolved Position reaches the # payload, and at what cost. The resolution rules themselves (which Position @@ -271,8 +271,8 @@ def _role_record(self, person): 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'.""" + """A finished stint reports the latest position that overlapped it -- an + undergrad who moved to an MS mid-stint reads 'MS Student', 'UW'.""" person = self._make_role_holder( "Multi", positions=[ @@ -285,9 +285,28 @@ def test_project_people_exposes_position_during_role(self): 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") + self.assertEqual(record["position_title"], "MS Student") + self.assertEqual(record["position_school"], "University of Washington") + self.assertEqual(record["position_school_abbreviated"], "UW") + + def test_project_people_active_role_reports_present_day_position(self): + """#1435: a still-open role reports what the person is *today*, not what + they were when the role began -- the case that had consumers fetching + /people// per roster row just to get a current title.""" + person = self._make_role_holder( + "Promoted", + positions=[ + (Title.ASSISTANT_PROF, "University of Maryland", + date(2012, 1, 1), date(2017, 8, 31)), + (Title.FULL_PROF, "University of Washington", + date(2017, 9, 1), None), + ], + role_start=date(2012, 2, 1), + ) + record = self._role_record(person) + self.assertEqual(record["position_title"], "Professor") + self.assertEqual(record["position_school"], "University of Washington") + self.assertEqual(record["position_school_abbreviated"], "UW") def test_project_people_position_null_without_position(self): """self.past_lead has a project role but no Position at all.""" diff --git a/website/tests/test_project_role.py b/website/tests/test_project_role.py index 52ad9fb3..59efb8d0 100644 --- a/website/tests/test_project_role.py +++ b/website/tests/test_project_role.py @@ -1,14 +1,15 @@ """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 +Covers ``ProjectRole.position_during_role`` (#1426, revised in #1435): the rule +that decides which Position best describes a person over the span of a project +role -- the latest one that overlaps the role's window. 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 datetime import date, timedelta from website.models import Position, ProjectRole from website.models.position import Title @@ -33,14 +34,34 @@ def _role(self, start, end=None): 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") + def test_ongoing_role_reports_the_present_day_position(self): + """The headline case (#1435): someone who has been on a project since + 2014 and was promoted along the way reads as what they are *now*, not + what they were when the stint began. A still-open role's window runs to + today, so today's position is the latest one overlapping it.""" + self._position(Title.ASSISTANT_PROF, date(2014, 1, 1), date(2018, 8, 31)) + full_prof = self._position(Title.FULL_PROF, date(2018, 9, 1), None, + school="University of Washington") + role = self._role(date(2014, 6, 1)) + self.assertEqual(role.position_during_role, full_prof) + + def test_mid_stint_promotion_reports_the_later_title(self): + """An undergrad who stayed on for an MS mid-stint reads 'MS Student': + of the positions overlapping the role, the latest to start wins.""" + self._position(Title.UGRAD, date(2015, 1, 1), date(2017, 5, 31)) + ms = 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, ms) + + def test_ended_role_ignores_positions_that_start_after_it(self): + """The other half of #1435: a finished stint is still frozen in its own + time. A 2015 undergrad who is a professor today reads 'Undergrad' next + to that stint -- the professorship never overlapped it.""" + ugrad = self._position(Title.UGRAD, date(2015, 1, 1), date(2016, 6, 1)) + self._position(Title.FULL_PROF, date(2022, 1, 1), None, + school="Massachusetts Institute of Technology") + role = self._role(date(2015, 6, 1), date(2016, 6, 1)) self.assertEqual(role.position_during_role, ugrad) def test_overlapping_positions_pick_the_latest_to_start(self): @@ -49,7 +70,7 @@ def test_overlapping_positions_pick_the_latest_to_start(self): 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)) + role = self._role(date(2018, 1, 1), date(2018, 12, 1)) self.assertEqual(role.position_during_role, ms) def test_open_ended_position_contains_later_role(self): @@ -58,20 +79,38 @@ def test_open_ended_position_contains_later_role(self): 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.""" + def test_future_position_is_not_reported_yet(self): + """A promotion entered ahead of time (start date in the future) must not + leak into today's payload: the window stops at today.""" + current = self._position(Title.PHD_STUDENT, date(2020, 1, 1), None) + self._position(Title.POST_DOC, date.today() + timedelta(days=90), None) + role = self._role(date(2021, 1, 1)) + self.assertEqual(role.position_during_role, current) + + def test_future_position_is_not_reported_on_a_role_ending_in_the_future(self): + """The same guard, on the role shape that used to slip past it: an + editor may enter a planned wrap-up date, which puts end_date in the + future. The window end is clamped to today either way, so a promotion + entered ahead of time still doesn't leak into today's payload.""" + current = self._position(Title.PHD_STUDENT, date(2020, 1, 1), None) + self._position(Title.POST_DOC, date.today() + timedelta(days=30), None) + role = self._role(date(2021, 1, 1), date.today() + timedelta(days=365)) + self.assertEqual(role.position_during_role, current) + + def test_falls_back_to_prior_position_when_role_falls_in_a_gap(self): + """A completed role sitting 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)) + role = self._role(date(2016, 1, 1), 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._position(Title.POST_DOC, date(2023, 1, 1), None) + role = self._role(date(2017, 1, 1), date(2018, 1, 1)) self.assertEqual(role.position_during_role, phd) def test_none_when_person_has_no_positions(self): @@ -88,6 +127,13 @@ def test_same_start_date_ties_break_deterministically(self): role = self._role(date(2016, 1, 1)) self.assertEqual(role.position_during_role, later_row) + def test_role_ending_before_it_starts_does_not_crash(self): + """A typo'd end_date (before start_date) would invert the window. It's + clamped, so resolution still answers with the position at role start.""" + ugrad = self._position(Title.UGRAD, date(2015, 1, 1), None) + role = self._role(date(2016, 1, 1), date(2015, 6, 1)) + self.assertEqual(role.position_during_role, ugrad) + 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