feat: Adding missing project details for list view#5258
feat: Adding missing project details for list view#5258rajohnson90 wants to merge 6 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the Projects list endpoint to support pagination and to return additional project-level metadata needed by the list view (start/end date, fiscal-year totals, overall total, and agreement name list), with accompanying test and OpenAPI updates.
Changes:
- Wrap
GET /projectsresponses in{ data, count, limit, offset }and add request pagination params. - Add list-view metadata fields to the project list schemas (and compute them via
project_list_metadata). - Update backend tests and OpenAPI docs to reflect the new response shape and filtering semantics.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/ops_api/ops/services/projects.py | Adds pagination inputs/metadata and adjusts search filter behavior. |
| backend/ops_api/ops/resources/projects.py | Wraps list response with pagination metadata. |
| backend/ops_api/ops/schemas/projects.py | Extends list response schema with metadata fields and pre_dump mapping. |
| backend/models/projects.py | Adds project_list_metadata property computing totals/date ranges/agreement list. |
| backend/openapi.yml | Documents pagination + wrapped response and new list-item fields. |
| backend/ops_api/tests/ops/project/test_project.py | Updates list endpoint tests and adds metadata-focused test cases. |
| backend/ops_api/tests/ops/research_project/test_research_project.py | Updates research project list tests for new response format. |
| backend/ops_api/tests/ops/administrative_and_support_project/test_administrative_and_support_project.py | Updates admin/support list tests for new response format. |
| backend/models/services_components.py | Removes an unused import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_list( | ||
| self, data: dict[str, Any] | None = {} | ||
| ) -> tuple[Sequence[ResearchProject], Sequence[AdministrativeAndSupportProject]]: | ||
| ) -> tuple[Sequence[ResearchProject], Sequence[AdministrativeAndSupportProject], dict[str, Any]]: |
There was a problem hiding this comment.
get_list uses a mutable default argument (data = {}), which can leak state between calls. Use data: dict[str, Any] | None = None and initialize an empty dict inside the method when data is None.
| # Combine results for pagination | ||
| all_projects = research_projects + administrative_and_support_projects | ||
|
|
||
| # Calculate total count before pagination | ||
| total_count = len(all_projects) | ||
|
|
||
| # Apply pagination | ||
| limit_value = filters.limit[0] if filters.limit else 10 | ||
| offset_value = filters.offset[0] if filters.offset else 0 | ||
|
|
||
| paginated_projects = all_projects[offset_value : offset_value + limit_value] | ||
|
|
||
| # Separate paginated results back into their types for serialization | ||
| paginated_research = [p for p in paginated_projects if isinstance(p, ResearchProject)] | ||
| paginated_admin = [p for p in paginated_projects if isinstance(p, AdministrativeAndSupportProject)] | ||
|
|
There was a problem hiding this comment.
Offset/limit pagination is applied to a concatenation of two independent queries without any explicit ordering. Without a deterministic ORDER BY, pagination results can be unstable between requests; define a consistent sort (e.g., by Project.id or updated_on) and apply it consistently when paginating.
| # Verify metadata fields are populated | ||
| assert project_data["start_date"] == "2023-01-01" | ||
| assert project_data["end_date"] == "2045-06-13" | ||
| assert project_data["project_total"] is not None | ||
| assert isinstance(project_data["project_total"], int) |
There was a problem hiding this comment.
This test hard-codes end_date to "2045-06-13" even though the test only adds a services component ending in 2023-12-31. Unless the fixture guarantees a later period_end, this is brittle and can break if seed data changes. Prefer asserting relative behavior (e.g., end_date equals the max of the dates you create in the test) or explicitly create the service component that sets the expected max.
| # Note: projects-group returns all projects (research + admin/support), so we need to filter | ||
| admin_support_projects = [ | ||
| p for p in response.json if p.get("project_type") == ProjectType.ADMINISTRATIVE_AND_SUPPORT.name | ||
| p for p in response.json["data"] if p.get("project_type") == ProjectType.ADMINISTRATIVE_AND_SUPPORT.name | ||
| ] |
There was a problem hiding this comment.
The comment says projects-group returns all projects and needs filtering, but this test now passes project_type=ADMINISTRATIVE_AND_SUPPORT, so the endpoint should already be filtered. Either drop the extra client-side filter or update the comment to reflect current behavior.
| - name: limit | ||
| in: query | ||
| description: Maximum number of projects to return (1-50, default 10) | ||
| schema: | ||
| type: array | ||
| items: | ||
| type: integer | ||
| minimum: 1 | ||
| maximum: 50 | ||
| default: 10 | ||
| style: form | ||
| explode: true | ||
| example: [10] |
There was a problem hiding this comment.
For these query parameters, the schema is type: array, but the default is specified under items. In OpenAPI, defaults should be defined on the array schema itself (e.g., default: [10]) or the parameter should be modeled as a single integer (like other endpoints). As written, tooling may ignore the default.
| # Combine results for pagination | ||
| all_projects = research_projects + administrative_and_support_projects | ||
|
|
||
| # Calculate total count before pagination | ||
| total_count = len(all_projects) | ||
|
|
||
| # Apply pagination | ||
| limit_value = filters.limit[0] if filters.limit else 10 | ||
| offset_value = filters.offset[0] if filters.offset else 0 | ||
|
|
||
| paginated_projects = all_projects[offset_value : offset_value + limit_value] | ||
|
|
There was a problem hiding this comment.
Pagination is currently applied in Python after executing .all() for both queries, which loads the full result set into memory even when the client only requests the first page. This is a significant performance regression for large datasets; apply limit/offset at the SQL level and compute count via a separate COUNT(*) query instead of len(all_projects).
| if hasattr(data, "project_list_metadata"): | ||
| metadata = data.project_list_metadata | ||
|
|
||
| class ProjectListResponse(Schema): | ||
| # Map total to project_total (convert Decimal to int) | ||
| data.project_total = int(metadata["total"]) if metadata["total"] else None | ||
|
|
There was a problem hiding this comment.
project_total is derived via int(metadata["total"]) if metadata["total"] else None. This will incorrectly turn a valid total of Decimal('0') into None, and int() will truncate cents from the Decimal agreement totals. Use an explicit is not None check and serialize totals without losing precision (e.g., decimal-as-string or cents as integer) to avoid incorrect financial values.
| @pre_dump | ||
| def extract_metadata(self, data, **kwargs): | ||
| """Extract fields from project_list_metadata property and map to schema fields.""" | ||
| if hasattr(data, "project_list_metadata"): | ||
| metadata = data.project_list_metadata | ||
|
|
||
| class ProjectListResponse(Schema): | ||
| # Map total to project_total (convert Decimal to int) | ||
| data.project_total = int(metadata["total"]) if metadata["total"] else None | ||
|
|
||
| # Map date ranges | ||
| data.start_date = metadata["project_start"] | ||
| data.end_date = metadata["project_end"] | ||
|
|
||
| # Map fiscal year breakdown | ||
| data.fiscal_year_totals = metadata["by_fiscal_year"] | ||
| data.agreement_name_list = metadata.get("agreement_name_list", []) |
There was a problem hiding this comment.
The @pre_dump hook reads data.project_list_metadata, which traverses agreements -> budget_line_items/services_components. With the current list queries not eager-loading those relationships, this will trigger N+1 (or worse) lazy-loads during list serialization and can negate the earlier performance optimization claim for the list endpoint. Consider computing these fields in the list query/service layer with appropriate eager loading/aggregation instead of per-row relationship traversal during serialization.
| @property | ||
| def project_list_metadata(self) -> dict: | ||
| """ | ||
| Calculate project totals, fiscal year breakdown, and date range. | ||
|
|
||
| Returns a dict with: | ||
| - total: Total value of all agreements (sum of agreement_total for each) | ||
| - by_fiscal_year: Dict mapping fiscal year to total BLI value for that year (non-DRAFT BLIs only) | ||
| - project_start: Earliest period_start across all services_components in all agreements | ||
| - project_end: Latest period_end across all services_components in all agreements | ||
| - agreement_name_list: List of dicts with agreement id and name (nick_name if available, otherwise title) | ||
| """ | ||
| from collections import defaultdict | ||
| from models.budget_line_items import BudgetLineItemStatus | ||
|
|
||
| total = Decimal("0") | ||
| by_fiscal_year = defaultdict(lambda: Decimal("0")) | ||
| start_dates = [] | ||
| end_dates = [] | ||
| agreement_name_list = [] | ||
|
|
||
| for agreement in self.agreements: | ||
| # Add agreement total to overall total | ||
| total += agreement.agreement_total | ||
| if agreement.nick_name: | ||
| agreement_name_list.append({"id": agreement.id, "name": agreement.nick_name}) | ||
| else: | ||
| agreement_name_list.append({"id": agreement.id, "name": agreement.name}) | ||
|
|
||
| # Add BLI amounts by fiscal year (only non-DRAFT or OBE BLIs) | ||
| for bli in agreement.budget_line_items: | ||
| if (bli.is_obe or bli.status != BudgetLineItemStatus.DRAFT) and bli.fiscal_year is not None: | ||
| # Include amount + fees for the BLI | ||
| bli_total = (bli.amount or Decimal("0")) + (bli.fees or Decimal("0")) | ||
| by_fiscal_year[bli.fiscal_year] += bli_total | ||
|
|
||
| # Collect all services_component dates | ||
| for sc in agreement.services_components: | ||
| if sc.period_start is not None: | ||
| start_dates.append(sc.period_start) | ||
| if sc.period_end is not None: | ||
| end_dates.append(sc.period_end) | ||
|
|
There was a problem hiding this comment.
project_list_metadata iterates through self.agreements and then each agreement’s budget_line_items and services_components. When accessed on a list of projects, this pattern can cause severe N+1 query behavior and expensive in-Python aggregation. Consider moving this computation to a service/query that uses database aggregation (MIN/MAX/SUM with GROUP BY) and/or batched eager loading (selectinload) so list views don’t trigger per-project relationship loads.
| assert project_data["fiscal_year_totals"] is not None | ||
| assert isinstance(project_data["fiscal_year_totals"], dict) | ||
| # Fiscal year totals should have entries for FY 2023 and 2024 | ||
| assert "2023" in project_data["fiscal_year_totals"] or "2024" in project_data["fiscal_year_totals"].keys() |
There was a problem hiding this comment.
This assertion is logically incorrect for the stated intent (“should have entries for FY 2023 and 2024”): it uses or, so it will pass even if only one fiscal year is present. Update it to require both expected keys (and ensure you’re comparing against the actual key type returned in JSON, typically strings).
| assert "2023" in project_data["fiscal_year_totals"] or "2024" in project_data["fiscal_year_totals"].keys() | |
| assert "2023" in project_data["fiscal_year_totals"] and "2024" in project_data["fiscal_year_totals"] |
What changed
Added project_start, project_end, a dictionary of year to fiscal year totals, the total value of a project and the list of agreements
Issue
OPS-4169
How to test
A11y impact
A11Y-SUPPRESSIONmetadata (owner, expires, rationale)Definition of Done Checklist