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

Updates to portfolio summary #4862

Merged
merged 14 commits into from
Oct 31, 2024
Merged

Updates to portfolio summary #4862

merged 14 commits into from
Oct 31, 2024

Conversation

perryr16
Copy link
Contributor

@perryr16 perryr16 commented Oct 18, 2024

Any background context you want to provide?

The following is based on user feedback.

What's this PR do?

  • Allows users to sort on portfolio summary columns that represent data from related models (goal_note and historical_note).
  • Fixes bug that ignored 0 values in data quality checks, leading to false "passed checks"
  • Excludes baseline new builds from summary calculations

How should this be manually tested?

  • Attempt to sort on Question, Resolution, Historical Note, and Passed Checks
  • Set EUI to 0 for a property, rerun data quality checks. Property should fail and a label applied
  • Set New Build or Acquired to true for a property, baseline calculations should update to ignore this property (looking at the change in total area is easiest), while current calculations remain unchanged.

What are the relevant tickets?

#4852
#4583

Screenshots (if appropriate)

@perryr16 perryr16 added the Feature Add this label to new features. This will be reflected in the change log when generated. label Oct 18, 2024
@perryr16 perryr16 marked this pull request as ready for review October 22, 2024 16:39
@perryr16 perryr16 requested a review from kflemin October 22, 2024 16:39
else:
target = ""

related_model = Case(When(**{column_name: target}, then=Value(1)), default=Value(0), output_field=IntegerField())
Copy link
Contributor Author

@perryr16 perryr16 Oct 22, 2024

Choose a reason for hiding this comment

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

By defautt, django .order_by(...) will put blank values above "A" values.

  • asc: None, "A", "B", "Z"
  • desc: "Z", "B", "A", None

I thought this was counterintuitive. If a user sorted a text column like historical notes, "A" should be at the top, and blanks should be below "Z". If this doesn't matter, I can simplify this function by removing the related_model annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the way you implemented it!

@perryr16 perryr16 changed the title Allow sorting on Portfolio Summary related model columns Updates to portfolio summary Oct 24, 2024
else:
target = ""

related_model = Case(When(**{column_name: target}, then=Value(1)), default=Value(0), output_field=IntegerField())
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the way you implemented it!

@kflemin kflemin merged commit dc590dc into develop Oct 31, 2024
9 checks passed
@kflemin kflemin deleted the 4852-ps-sort-related-cols branch October 31, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Add this label to new features. This will be reflected in the change log when generated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants