From 9cd1ad14164816709f75509a7431fab88a3e2126 Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Mon, 15 Jun 2026 06:36:38 -0700 Subject: [PATCH] Refactor view_project_people: fix director hardcoding + N+1 queries (#1284) The internal /view-project-people/ page builds a JSON snapshot of every person and project for client-side rendering. It was written quickly and an audit flagged it for cleanup. This addresses three issues: - Correctness: the lab director was resolved two ways that could disagree (a Froehlich-surname query for the advisee check vs. a separate surname string compare for is_director). Now resolved once by Title.DIRECTOR position; both flags derive from that single Person. - Performance: removed the per-person N+1 storm. Earliest/latest position and publication indicators now come from prefetched data, and the PhD-advisee set is precomputed in 3 bulk queries (mirroring Person.is_phd_advisee_of) instead of ~5-6 queries per person. - Readability: decomposed into documented helpers, switched to DjangoJSONEncoder for date serialization, removed leftover instructional comments, a no-op select_related(), a dead payload field, and an unnecessary hasattr guard. Adds website/tests/test_view_project_people.py with 5 integration tests locking the JSON payload contract (render, project-role aggregation, director-by-title, PhD-advisee logic, publication indicators). The template's JSON contract is unchanged, so there is no UI change. Co-Authored-By: Claude Opus 4.8 (1M context) --- website/tests/test_view_project_people.py | 135 +++++++ website/views/view_project_people.py | 449 ++++++++++++---------- 2 files changed, 388 insertions(+), 196 deletions(-) create mode 100644 website/tests/test_view_project_people.py diff --git a/website/tests/test_view_project_people.py b/website/tests/test_view_project_people.py new file mode 100644 index 00000000..55342197 --- /dev/null +++ b/website/tests/test_view_project_people.py @@ -0,0 +1,135 @@ +""" +Regression tests for the internal "view project people" page +(:func:`website.views.view_project_people.view_project_people`). + +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. +""" + +import json +from datetime import date, timedelta + +from django.urls import reverse + +from website.models import Person, Project, Publication +from website.models.position import Position, Role, Title +from website.models.project_role import ProjectRole +from website.models.publication import PubType +from website.tests.base import DatabaseTestCase + + +class ViewProjectPeopleTests(DatabaseTestCase): + """Integration tests that exercise the real view, queryset, and template.""" + + def _get_people_by_id(self): + """GET the page and return (response, {person_id: person_payload}).""" + response = self.client.get(reverse("website:view_project_people")) + self.assertEqual(response.status_code, 200) + people = json.loads(response.context["people_json"]) + return response, {p["id"]: p for p in people} + + def _add_position(self, person, title, start, end=None, **kwargs): + """Create a Position for `person` (Member role unless overridden).""" + kwargs.setdefault("role", Role.MEMBER) + return Position.objects.create( + person=person, title=title, start_date=start, end_date=end, **kwargs + ) + + def test_page_renders_and_payloads_parse(self): + """The view returns 200 and all three context JSON blobs are valid JSON.""" + person = self.make_person(first_name="Ada", last_name="Lovelace") + self._add_position(person, Title.PHD_STUDENT, date(2020, 1, 1)) + + response = self.client.get(reverse("website:view_project_people")) + self.assertEqual(response.status_code, 200) + for key in ("people_json", "projects_json", "abstracted_titles_json"): + parsed = json.loads(response.context[key]) + self.assertIsInstance(parsed, list) + + def test_project_role_aggregation(self): + """project_roles aggregates total days and start/end dates per project.""" + person = self.make_person(first_name="Grace", last_name="Hopper") + self._add_position(person, Title.PHD_STUDENT, date(2019, 1, 1)) + project = Project.objects.create(name="Test Project", short_name="TestProj") + start, end = date(2020, 1, 1), date(2020, 12, 31) + ProjectRole.objects.create( + person=person, project=project, start_date=start, end_date=end + ) + + _, people = self._get_people_by_id() + roles = people[person.id]["project_roles"] + self.assertIn("TestProj", roles) + self.assertEqual(roles["TestProj"]["total_days"], (end - start).days) + 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)) + 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_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)) + + current = self.make_person(first_name="Cur", last_name="Rent") + self._add_position(current, Title.PHD_STUDENT, date(2022, 1, 1), advisor=director) + + past = date.today() - timedelta(days=400) + past_no_diss = self.make_person(first_name="Past", last_name="Nodiss") + self._add_position( + past_no_diss, Title.PHD_STUDENT, date(2015, 1, 1), end=past, advisor=director + ) + + past_with_diss = self.make_person(first_name="Past", last_name="Withdiss") + self._add_position( + past_with_diss, Title.PHD_STUDENT, date(2015, 1, 1), end=past, co_advisor=director + ) + dissertation = self.make_publication( + title="A Dissertation", pub_venue_type=PubType.PHD_DISSERTATION + ) + dissertation.authors.add(past_with_diss) + + _, people = self._get_people_by_id() + self.assertTrue(people[current.id]["is_phd_advisee"]) + self.assertFalse(people[past_no_diss.id]["is_phd_advisee"]) + self.assertTrue(people[past_with_diss.id]["is_phd_advisee"]) + # Sanity-check against the canonical model method. + self.assertEqual( + people[current.id]["is_phd_advisee"], current.is_phd_advisee_of(director) + ) + self.assertEqual( + people[past_no_diss.id]["is_phd_advisee"], + past_no_diss.is_phd_advisee_of(director), + ) + + def test_publication_indicators(self): + """has_any_publication and projects_published_on reflect authored pubs.""" + project = Project.objects.create(name="Pub Project", short_name="PubProj") + author = self.make_person(first_name="Wri", last_name="Ter") + self._add_position(author, Title.PHD_STUDENT, date(2020, 1, 1)) + pub = self.make_publication(title="Authored Paper") + pub.authors.add(author) + pub.projects.add(project) + + non_author = self.make_person(first_name="Non", last_name="Author") + self._add_position(non_author, Title.UGRAD, date(2021, 1, 1)) + + _, people = self._get_people_by_id() + self.assertTrue(people[author.id]["has_any_publication"]) + self.assertIn("PubProj", people[author.id]["projects_published_on"]) + self.assertFalse(people[non_author.id]["has_any_publication"]) + self.assertEqual(people[non_author.id]["projects_published_on"], []) diff --git a/website/views/view_project_people.py b/website/views/view_project_people.py index a1ee717f..327913c5 100644 --- a/website/views/view_project_people.py +++ b/website/views/view_project_people.py @@ -1,214 +1,271 @@ """ -View for displaying project people with customizable sidebar controls. - -This internal page allows lab members to view people associated with projects -with options for filtering, sorting, grouping, and display customization. -Useful for generating screenshots for talks, papers, and presentations. - -URL Parameters (for state persistence): - - projects: Comma-separated list of project short_names - - sort: Sort order (time_on_project, start_date, seniority) - - group: Grouping (none, position) - - show_title: Whether to show titles (1 or 0) - - show_dates: Whether to show date range (1 or 0) - - show_school: Whether to show school (1 or 0) - - full_school: Whether to show full school name vs abbreviated (1 or 0) - - show_pub: Whether to highlight people who published on selected projects (1 or 0) +View for the internal "project people" page. + +This page lets lab members browse the people associated with projects and tweak +the display (filter / sort / group / show-or-hide fields) from a collapsible +sidebar. Its main use is generating acknowledgment grids of headshots for talks +and papers. All filtering, sorting, and grouping happens client-side for instant +updates, so the view's job is simply to emit a complete JSON snapshot of the data. + +Three JSON blobs are placed in the template context: + - projects_json: every project, for the sidebar (id, name, short_name, + is_active, people_count). + - people_json: every Person, with position, school/department, lab dates, + per-project role durations, and publication indicators. + - abstracted_titles_json: the ordered abstracted-title groups used for the + "group by position" mode. + +The client persists its display options in the URL query string; the server does +not read those parameters — it always returns the full dataset. """ +import json +import logging + +from django.core.serializers.json import DjangoJSONEncoder +from django.db.models import Count, Q from django.shortcuts import render -from django.http import JsonResponse -from website.models import Project, Person, Publication + +# easy-thumbnails generates the per-person headshot thumbnails server-side. +from easy_thumbnails.files import get_thumbnailer + +from website.models import Person, Project, Publication from website.models.position import Position, Title +from website.models.publication import PubType from datetime import date -import json -# For generating thumbnail URLs server-side (using easy-thumbnails) -from easy_thumbnails.files import get_thumbnailer +_logger = logging.getLogger(__name__) + +# Matches PERSON_THUMBNAIL_SIZE in website/models/person.py. +PERSON_THUMBNAIL_SIZE = (245, 245) + + +def _build_projects_payload(): + """ + Builds the sidebar project list. + + Returns a list of dicts (one per project, ordered by name). people_count is + annotated in the query so this does not issue a query per project. + """ + projects = ( + Project.objects.annotate(num_people=Count("projectrole__person", distinct=True)) + .order_by("name") + ) + return [ + { + "id": project.id, + "name": project.name, + "short_name": project.short_name, + "is_active": not project.has_ended(), + "people_count": project.num_people, + } + for project in projects + ] + + +def _build_phd_advisee_ids(director): + """ + Returns the set of Person ids who are PhD advisees of `director`. + + This mirrors :meth:`website.models.person.Person.is_phd_advisee_of` (a person + advised or co-advised by the director as a PhD student, counted only while the + position is active or once they have a dissertation) but computes it in a few + bulk queries instead of one query per person. + """ + if director is None: + return set() + + phd_positions = Position.objects.filter(title=Title.PHD_STUDENT).filter( + Q(advisor=director) | Q(co_advisor=director) + ) + advised_ids = set(phd_positions.values_list("person_id", flat=True)) + active_advised_ids = set( + phd_positions.filter( + Q(end_date__isnull=True) | Q(end_date__gte=date.today()) + ).values_list("person_id", flat=True) + ) + dissertation_author_ids = set( + Publication.objects.filter( + pub_venue_type=PubType.PHD_DISSERTATION + ).values_list("authors__id", flat=True) + ) + + # Active advisees always count; past advisees only once they have a dissertation. + return { + person_id + for person_id in advised_ids + if person_id in active_advised_ids or person_id in dissertation_author_ids + } + + +def _build_thumbnail_url(person): + """ + Returns the URL of `person`'s cropped headshot thumbnail. + + Uses easy-thumbnails together with django-image-cropping (the crop box, if set, + lives on person.cropping). Falls back to the original image URL if thumbnail + generation fails, or to '' if the person has no image. + """ + if not person.image: + return "" + + options = { + "size": PERSON_THUMBNAIL_SIZE, + "crop": True, + "upscale": True, + "detail": True, + } + if person.cropping: + options["box"] = person.cropping + + try: + thumbnail = get_thumbnailer(person.image).get_thumbnail(options) + return thumbnail.url if thumbnail else "" + except Exception: + _logger.warning( + "Thumbnail generation failed for %s; falling back to original image.", + person, + exc_info=True, + ) + return person.image.url + + +def _build_project_roles(person, today): + """ + Aggregates `person`'s ProjectRoles into per-project totals. + + Returns a dict keyed by project short_name with total_days worked and the + overall (earliest start, latest end) span. A person can have several roles on + the same project, so durations are summed and the span is widened across them. + Dates are left as date objects; DjangoJSONEncoder serializes them to ISO + 'YYYY-MM-DD' strings, which the client parses. + """ + project_roles = {} + for role in person.projectrole_set.all(): + short_name = role.project.short_name + start = role.start_date + end = role.end_date or today + + entry = project_roles.get(short_name) + if entry is None: + project_roles[short_name] = { + "total_days": (end - start).days, + "start_date": start, + "end_date": end, + } + else: + entry["total_days"] += (end - start).days + entry["start_date"] = min(entry["start_date"], start) + entry["end_date"] = max(entry["end_date"], end) + return project_roles + + +def _build_person_payload(person, director_id, is_phd_advisee, today): + """Builds the JSON-serializable dict for a single Person.""" + # The prefetched positions let us pick earliest/latest in Python (sorted by + # start_date) without the per-person queries the model's cached properties make. + positions = sorted(person.position_set.all(), key=lambda p: p.start_date) + earliest_position = positions[0] if positions else None + latest_position = positions[-1] if positions else None + + if latest_position: + abstracted_title = Position.get_abstracted_title(latest_position) + title_index = latest_position.get_title_index() + else: + abstracted_title = "Unknown" + title_index = Position.TITLE_ORDER_MAPPING[Title.UNKNOWN] + + # Projects this person has published on (drives the "published" highlight). + projects_published_on = sorted( + { + project.short_name + for pub in person.publication_set.all() + for project in pub.projects.all() + } + ) + + return { + "id": person.id, + "first_name": person.first_name, + "last_name": person.last_name, + "full_name": person.get_full_name(), + "url_name": person.url_name, + "is_director": person.id == director_id, + "is_phd_advisee": is_phd_advisee, + # Pre-computed thumbnail URL. + "image_url": _build_thumbnail_url(person), + # Position info. + "title": latest_position.title if latest_position else "Unknown", + "abstracted_title": abstracted_title or "Unknown", + # School / department info. + "school": latest_position.school if latest_position else "", + "school_abbreviated": ( + latest_position.get_school_abbreviated() if latest_position else "" + ), + "department": latest_position.department if latest_position else "", + "department_abbreviated": ( + latest_position.get_department_abbreviated() if latest_position else "" + ), + # Lab dates. + "lab_start_date": earliest_position.start_date if earliest_position else None, + "lab_end_date": latest_position.end_date if latest_position else None, + "date_range_str": ( + latest_position.get_date_range_as_str() if latest_position else "" + ), + # Seniority: lower index = more senior (see Position.TITLE_ORDER_MAPPING). + "seniority_index": title_index, + # Per-project role durations. + "project_roles": _build_project_roles(person, today), + "projects_published_on": projects_published_on, + "has_any_publication": bool(person.publication_set.all()), + # Role (Member vs Collaborator), used by the client-side filters. + "role": latest_position.role if latest_position else "Unknown", + } def view_project_people(request): """ - Renders the project people page with sidebar controls. - - This page displays people associated with selected projects and provides - a collapsible sidebar for customizing the display. All filtering, sorting, - and grouping is done client-side for instant updates. - + Renders the project-people page with the full people/projects dataset. + Args: - request: Django HTTP request object - + request: Django HTTP request object. + Returns: - Rendered HTML template with context containing all projects and people data + Rendered ``website/view_project_people.html`` with three JSON context + blobs (projects, people, abstracted titles) for client-side rendering. """ - # Fetch all projects for the sidebar, ordered by name - all_projects = Project.objects.all().order_by('name') - - # Build project data for the sidebar - projects_data = [] - for project in all_projects: - projects_data.append({ - 'id': project.id, - 'name': project.name, - 'short_name': project.short_name, - 'is_active': not project.has_ended() if hasattr(project, 'has_ended') else True, - 'people_count': project.get_people_count(), - }) - - # Get ALL people in the system. This includes people with project roles, - # people who authored publications on projects, and anyone else in the - # Person table (needed for the "Show all people" mode). - all_people_on_projects = Person.objects.all().distinct().select_related().prefetch_related( - 'position_set', - 'projectrole_set', - 'projectrole_set__project', - 'publication_set', - 'publication_set__projects' + 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() ) + director_id = director.id if director else None + phd_advisee_ids = _build_phd_advisee_ids(director) + + # Every Person (the page includes a "show all people" mode). Prefetch the + # relations the payload reads so building it stays query-free per person. + all_people = Person.objects.prefetch_related( + "position_set", + "projectrole_set__project", + "publication_set__projects", + ) + people_data = [ + _build_person_payload(person, director_id, person.id in phd_advisee_ids, today) + for person in all_people + ] + + # Abstracted-title groups (mix of plain strings and Title enum members). + abstracted_titles = [ + t.value if hasattr(t, "value") else str(t) + for t in Position.get_sorted_abstracted_titles() + ] - # Near the top of view_project_people, after fetching all_people_on_projects: - director = Person.objects.filter(last_name='Froehlich').first() - - # Build comprehensive people data for client-side operations - people_data = [] - for person in all_people_on_projects: - latest_position = person.get_latest_position - earliest_position = person.get_earliest_position - - # Get abstracted title for grouping - abstracted_title = None - title_order = 999 - if latest_position: - abstracted_title = Position.get_abstracted_title(latest_position) - title_order = latest_position.get_title_index() - - # Build project roles data for this person - # We store dates as date objects during iteration, then convert to ISO strings at the end - project_roles_raw = {} - for role in person.projectrole_set.all(): - proj_short_name = role.project.short_name - if proj_short_name not in project_roles_raw: - project_roles_raw[proj_short_name] = { - 'total_days': 0, - 'start_date': None, - 'end_date': None, - } - - # Calculate days on project for this role - end = role.end_date or date.today() - start = role.start_date - days = (end - start).days - project_roles_raw[proj_short_name]['total_days'] += days - - # Track earliest start and latest end (comparing date objects) - if project_roles_raw[proj_short_name]['start_date'] is None or start < project_roles_raw[proj_short_name]['start_date']: - project_roles_raw[proj_short_name]['start_date'] = start - if project_roles_raw[proj_short_name]['end_date'] is None or end > project_roles_raw[proj_short_name]['end_date']: - project_roles_raw[proj_short_name]['end_date'] = end - - # Convert dates to ISO strings for JSON serialization - project_roles = {} - for proj_short_name, role_data in project_roles_raw.items(): - project_roles[proj_short_name] = { - 'total_days': role_data['total_days'], - 'start_date': role_data['start_date'].isoformat() if role_data['start_date'] else None, - 'end_date': role_data['end_date'].isoformat() if role_data['end_date'] else None, - } - - # Build set of projects this person has published on - # This is used for the "highlight published" indicator feature - projects_published_on = set() - for pub in person.publication_set.all(): - for proj in pub.projects.all(): - projects_published_on.add(proj.short_name) - - # Generate thumbnail URL for this person's image - # Uses easy-thumbnails with django-image-cropping - image_url = '' - if person.image: - try: - thumbnailer = get_thumbnailer(person.image) - thumbnail_options = { - 'size': (245, 245), # PERSON_THUMBNAIL_SIZE from person.py - 'crop': True, - 'upscale': True, - 'detail': True, - } - # Add cropping box if available (from django-image-cropping) - if person.cropping: - thumbnail_options['box'] = person.cropping - - thumb = thumbnailer.get_thumbnail(thumbnail_options) - image_url = thumb.url if thumb else '' - except Exception: - # Fallback to original image URL if thumbnail generation fails - image_url = person.image.url if person.image else '' - - # Build person data object - person_obj = { - 'id': person.id, - 'first_name': person.first_name, - 'last_name': person.last_name, - 'full_name': person.get_full_name(), - 'url_name': person.url_name, - - # Is Froehlich's PhD advisee - 'is_phd_advisee': person.is_phd_advisee_of(director) if director else False, - - # Image URL (pre-computed thumbnail) - 'image_url': image_url, - - # Position info - 'title': latest_position.title if latest_position else 'Unknown', - 'abstracted_title': abstracted_title or 'Unknown', - 'title_order': title_order, - - # School/department info - 'school': latest_position.school if latest_position else '', - 'school_abbreviated': latest_position.get_school_abbreviated() if latest_position else '', - 'department': latest_position.department if latest_position else '', - 'department_abbreviated': latest_position.get_department_abbreviated() if latest_position else '', - - # Date info - 'lab_start_date': earliest_position.start_date.isoformat() if earliest_position else None, - 'lab_end_date': latest_position.end_date.isoformat() if latest_position and latest_position.end_date else None, - 'date_range_str': latest_position.get_date_range_as_str() if latest_position else '', - - # Seniority (lower = more senior based on TITLE_ORDER_MAPPING) - 'seniority_index': title_order, - - # Project-specific data - 'project_roles': project_roles, - - # Projects this person has published on (for highlight indicator) - 'projects_published_on': list(projects_published_on), - - # Whether this person has any publication at all in our system - 'has_any_publication': person.publication_set.exists(), - - # Special case for lab director - 'is_director': person.last_name == 'Froehlich', - - # Role info (for filtering collaborators vs members) - 'role': latest_position.role if latest_position else 'Unknown', - } - - people_data.append(person_obj) - - # Get sorted abstracted titles for grouping - abstracted_titles = list(Position.get_sorted_abstracted_titles()) - # Convert Title enum values to their string representations - abstracted_titles_clean = [] - for t in abstracted_titles: - if hasattr(t, 'value'): - abstracted_titles_clean.append(t.value) - else: - abstracted_titles_clean.append(str(t)) - context = { - 'projects_json': json.dumps(projects_data), - 'people_json': json.dumps(people_data), - 'abstracted_titles_json': json.dumps(abstracted_titles_clean), + "projects_json": json.dumps(_build_projects_payload(), cls=DjangoJSONEncoder), + "people_json": json.dumps(people_data, cls=DjangoJSONEncoder), + "abstracted_titles_json": json.dumps(abstracted_titles, cls=DjangoJSONEncoder), } - - return render(request, 'website/view_project_people.html', context) \ No newline at end of file + return render(request, "website/view_project_people.html", context)