Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portfolio summary move to backend #4885

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

perryr16
Copy link
Contributor

@perryr16 perryr16 commented Nov 21, 2024

Any background context you want to provide?

ICF reported sorting issues on the goal_note columns, leading to duplicate and unsorted results.

What's this PR do?

  • Moves many of the portfolio summary formatting functions to the backend. The property retrieval function was originally reused from the inventory list, however as the portfolio summary has grown in complexity, having a dedicated function for property retrieval will simply the requests.
  • Fixes the portfolio summary sorting on goal_note columns (question, passed checks...). The duplicate records were related to a SQL join when ordering on a 1-many field.

How should this be manually tested?

  • create multiple goals with the same properties. Attempt to sort on all goal_note and historical columns (compare with develop)
  • This introduces a lot of new changes. Double check all existing functionality. There should be no differences in functionality

What are the relevant tickets?

#4886

Screenshots (if appropriate)

@perryr16 perryr16 added the Enhancement Add this label if functionality was generally improved but not a full feature or maintentance. label Nov 21, 2024
@@ -509,39 +509,44 @@ def build_view_filters_and_sorts(
return new_filters, annotations, order_by


def build_related_model_filters_and_sorts(filters: QueryDict, columns: list[dict]) -> tuple[Q, AnnotationDict, list[str]]:
Copy link
Contributor Author

@perryr16 perryr16 Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function was the source of the sorting bugs. Ordering by a related field in a 1-to-many relationship creates a SQL join, which duplicates rows when a property is part of multiple goals or goal_notes.

The new function works by querying and sorting the related models (goal_note & historical_note) independently, then mapping them back to property views. This avoids sorting-related issues at the parent level, simplifies the query, and resolves the duplication bug.

move functionality to backend

dev - meeds pagination

dev - meeds pagination

added pagination

labels functional

naming

logging
@perryr16 perryr16 force-pushed the portfolio-summary-move-to-backend branch from 7c04a45 to 758ca3d Compare November 22, 2024 16:04
@@ -85,14 +85,41 @@ angular.module('SEED.controller.portfolio_summary', [])
// Can only sort based on baseline or current, not both. In the event of a conflict, use the more recent.
let baseline_first = false;

const sort_goals = (goals) => goals.sort((a, b) => (a.name.toLowerCase() < b.name.toLowerCase() ? -1 : 1));
const load_data = (page) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New load_data function fetches goal data in a single request.

@@ -37,8 +37,6 @@ def get_filtered_results(request: Request, inventory_type: Literal["property", "
# check if there is a query parameter for the profile_id. If so, then use that one
profile_id = request.query_params.get("profile_id", profile_id)
shown_column_ids = request.query_params.get("shown_column_ids")
goal_id = request.data.get("goal_id")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

portfolio summary no longer uses get_filtered_results. Removed goal logic

@perryr16 perryr16 marked this pull request as ready for review November 22, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Add this label if functionality was generally improved but not a full feature or maintentance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant