Skip to content
This repository was archived by the owner on Jul 6, 2026. It is now read-only.

Add 10 year impacts to app#2502

Merged
nikhilwoodruff merged 69 commits into
masterfrom
feat/add-10-year-impacts-to-app
May 24, 2025
Merged

Add 10 year impacts to app#2502
nikhilwoodruff merged 69 commits into
masterfrom
feat/add-10-year-impacts-to-app

Conversation

@anth-volk

@anth-volk anth-volk commented Apr 19, 2025

Copy link
Copy Markdown
Collaborator

Description

Fixes #2472. Requires merging of #2499.

Changes

Adds a component to the PolicyRightSidebar component to enable multi-year impacts runs, then makes modifications where necessary to use the tools added in previous PRs to actually run these impacts.

There are some clear code quality issues with this PR. The best thing to do with this component would be to refactor FetchAndDisplayImpact, DisplayImpact, and tree.js to improve how we handle impact fetching and output display. However, in the interest of time, and accounting for the fact that we're going to be redoing the front end soon, I have not made those changes in this PR.

Screenshots

Please see the preview Vercel deployment. To use:

  1. Open the site
  2. Create a policy
  3. Open the "More options" tab on the left-hand sidebar, then select to "Extend impacts over X years"
  4. Run

Tests

Various added.

Remaining

  • Fix bug within UK policy from Nikhil
  • Add rounding to one decimal for UK policies

@vercel

vercel Bot commented Apr 19, 2025

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
policyengine-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 24, 2025 5:45am

@anth-volk

Copy link
Copy Markdown
Collaborator Author

Placing in draft until changes introduced in #2499 can be integrated

@anth-volk anth-volk marked this pull request as draft May 19, 2025 22:18
Base automatically changed from feat/aggregate-10-year-impacts to master May 20, 2025 10:53
@nikhilwoodruff

Copy link
Copy Markdown
Collaborator

Looks great.
image

Can I request that we round to 1 decimal place? This is the norm in the UK and I don't think it would hurt in the US either.

@nikhilwoodruff nikhilwoodruff marked this pull request as ready for review May 20, 2025 11:15
Copilot AI review requested due to automatic review settings May 20, 2025 11:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces multi–year impacts functionality to the app and updates several related components and API aggregation modules. Key changes include adding multi–year impacts logic to the PolicyPage, updating UI components (Menu/MenuItem/MenuItemGroup) to support a disabled state, and adding/refining aggregation functions and corresponding tests.

Reviewed Changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/pages/PolicyPage.jsx Adds multi-year impacts handling by importing and using determineIfMultiYear.
src/pages/BlogPage.jsx Removes debugging console.log statements.
src/layout/Menu.jsx Updates menu item components to conditionally disable interactions and hover.
src/api/societyWideAggregation/* Introduces and tests new aggregation functions for budget, deciles, and poverty.
src/api/makeSequentialSimulationRequests.js Replaces the legacy API call with asyncApiCall and adapts request validation.
src/tests/* Adds extensive tests for multi-year impact components and aggregation utilities.

@anth-volk

Copy link
Copy Markdown
Collaborator Author

A bit stonewalled on this due to bugs emerging from the FetchAndDisplayImpact.jsx component. This component contains quite a bit of non-canonical effect hook usage that makes modification quite difficult.

@anth-volk

Copy link
Copy Markdown
Collaborator Author

@nikhilwoodruff On the bug you're seeing, the API is outputting the following impacts:

{
  "budget": {
    "baseline_net_income": 1726524008655.007,
    "benefit_spending_impact": 4564467561.186462,
    "budgetary_impact": -4564467561.186462,
    "households": 34606884.05322167,
    "state_tax_revenue_impact": 0,
    "tax_revenue_impact": 0
  },

This is true for the single-year impact page, as well. The "Costs £X" display only shows the budgetary_impact value from the impacts, while the multi-year reports that as the "Tax" row in the UK. The benefit_spending_impact and tax_revenue_impact are not reported at all in the single-year impacts page, while they show up as the relevant rows in the 10-year impacts.

Is it possible that:

  • There's a bug in -uk causing this?
  • The multi-year output component is conflating impact values and the "Tax" row should be, e.g., tax_revenue_impact?

Would love to post-scrum on this

@anth-volk anth-volk marked this pull request as ready for review May 21, 2025 00:21
@anth-volk

Copy link
Copy Markdown
Collaborator Author

Ready for review once issues from @nikhilwoodruff are addressed

@nikhilwoodruff

nikhilwoodruff commented May 21, 2025

Copy link
Copy Markdown
Collaborator

On the bug you're seeing, the API is outputting the following impacts

If this is true then the UI is not matching the impacts- it's showing zero budgetary impact and incorrect tax revenue impacts (they should be zero).
image

image

The benefit_spending_impact and tax_revenue_impact are not reported at all in the single-year impacts page

This isn't actually true: they are Budgetary impact / Overall (the tax revenue bar is just at zero and skipped).

There's a bug in -uk causing this?

Not possible because the single-year impacts are correct.

I think this is a straightforward fix:

{
  "budget": {
    "baseline_net_income": 1726524008655.007,
    "benefit_spending_impact": 4564467561.186462,
    "budgetary_impact": -4564467561.186462,
    "households": 34606884.05322167,
    "state_tax_revenue_impact": 0,
    "tax_revenue_impact": 0
  },

In the above code snippet, the table should be putting tax_revenue_impact into the tax row, benefit_spending_impact into the benefit row, and budgetary_impact into the budgetary impact row.

@anth-volk

Copy link
Copy Markdown
Collaborator Author

Now updated to properly assign impact categories. UK:
Screen Shot 2025-05-23 at 4 08 03 PM

US:
Screen Shot 2025-05-23 at 4 11 24 PM

{
header: "Federal tax",
budgetKey: "budgetary_impact",
budgetKey: "tax_revenue_impact",

@nikhilwoodruff nikhilwoodruff May 23, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is still inconsistent I believe: federal tax revenue impact is tax_revenue_impact- state_tax_revenue_impact.

I would look at this part of the codebase to see exactly which variables map to which concepts and just mirror that. Apologies for missing this in earlier reviews if this was part of one of the chained PRs: I think if in future we could just combine PR dependent on each other in the same repo that would make it easier to review.

@github-project-automation github-project-automation Bot moved this from Todo to PR: Review Requested in policyengine-app May 23, 2025
@nikhilwoodruff nikhilwoodruff merged commit c712729 into master May 24, 2025
4 checks passed
@github-project-automation github-project-automation Bot moved this from PR: Review Requested to Done in policyengine-app May 24, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Enable multi-year impacts and modify society-wide outputs page so that it reflects the multi-year nature of these sorts of calculations

3 participants