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

Fix how unitsSummarized uses AMI charts #872

Closed
pbn4 opened this issue Nov 25, 2020 · 7 comments · Fixed by #1925
Closed

Fix how unitsSummarized uses AMI charts #872

pbn4 opened this issue Nov 25, 2020 · 7 comments · Fixed by #1925
Assignees

Comments

@pbn4
Copy link
Contributor

pbn4 commented Nov 25, 2020

When changing AMI chart entity to be a relation of Unit instead of Property I noticed that we are actually not using amiChartId per unit when computing this, but we are retrieving an AMI chart for first Unit in a collection and apply that for the rest.

Notes

  • Unit summarized logic only used AMI of first unit and used that to compute summary table
  • Use range if we have multiple AMI charts
@pbn4
Copy link
Contributor Author

pbn4 commented Jan 7, 2021

Related code

const amiChartId = units[0].amiChartId

@software-project
Copy link
Contributor

For now at least that works as we don't really have multiple charts per listing. If we did there is a matter how would we even show it (just wider income ranges? or something more?). Another question is (I don't remember the answer) if we really need multiple ami charts per one listing. If we will have new property model, maybe one chart per each listing for the property will be enouch?

@pbn4
Copy link
Contributor Author

pbn4 commented Jan 7, 2021

If we did there is a matter how would we even show it (just wider income ranges? or something more?).

OK that I don't know and it probably needs to be discussed more broadly.

Another question is (I don't remember the answer) if we really need multiple ami charts per one listing.

@slowbot If I recognize correctly we want to be able to configure ami chart at unit level, right?

@slowbot
Copy link
Collaborator

slowbot commented Jan 7, 2021

@software-project @pbn4

There are and will be use cases of multiple ami charts per building so we should allow for ami chart to be assigned at the unit level.

@kathyccheng kathyccheng added blocked Further development is blocked waiting for something external to this ticket and removed blocked Further development is blocked waiting for something external to this ticket labels Jul 28, 2021
@emilyjablonski emilyjablonski added the ready for qa Ready to be QA’d label Aug 20, 2021
@emilyjablonski
Copy link
Collaborator

emilyjablonski commented Aug 20, 2021

For QA, ensure that existing HMI tables look as we expect them to. Two new seeds with multiple AMI charts have been added but may not be on dev.

@seanmalbert seanmalbert assigned slowbot and unassigned emilyjablonski Sep 8, 2021
@slowbot
Copy link
Collaborator

slowbot commented Sep 9, 2021

@seanmalbert @emilyjablonski Can you help me get the new multiple AMI charts seeds onto dev so I can QA?

@slowbot
Copy link
Collaborator

slowbot commented Sep 13, 2021

@emilyjablonski Why is there a range expressed for max income per ami level?

I think we can simplify this table and only display a single max max income for each hh size and ami level.

I assume @pbn4 comment had to do with a table where there was only a single column for multiple AMI charts. Since we have dedicated columns for each chart in this example we don't need a range.

https://dev-bloom.netlify.app/listing/977dad6d-9b7c-4db2-bba5-cb95899599f6/test_default_multiple_ami_and_percentages_548_market_street_san_francisco_ca

@slowbot slowbot added failed qa This issue or pull request already exists and removed ready for qa Ready to be QA’d labels Sep 13, 2021
@emilyjablonski emilyjablonski removed failed qa This issue or pull request already exists Object Model labels Oct 8, 2021
@emilyjablonski emilyjablonski linked a pull request Oct 8, 2021 that will close this issue
24 tasks
seanmalbert pushed a commit to housingbayarea/bloom that referenced this issue Apr 12, 2022
Separate two buttons and change color and text on homepage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants