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

Improve budget results #1738

Merged
merged 6 commits into from
Jul 10, 2017
Merged

Improve budget results #1738

merged 6 commits into from
Jul 10, 2017

Conversation

bertocq
Copy link
Collaborator

@bertocq bertocq commented Jul 10, 2017

Where

What

  1. Highlight current heading
  2. Calculate winners synchronously
  3. Do not display incompatible investment's table if empty
  4. Show incompatibility checkbox only if winner or incompatible (not for compatible non-winners)
  5. Always recalculate heading winners on incompatibility change

How

  1. Adding a new "bold" class for menu.vertical.li and applying it based on heading_id param value 49f48dd
  2. Making use of .delay explicitly, avoiding the handle_asynchronously that modified all calls. efacd0d
  3. Not rendering the results table with incompatible investments if none present 2fba9de
  4. Checking that investment is either winner or incompatible 741a342
  5. Removing extra unnecessary conditions for recalculation 2b85dea

Screenshots

1.Highlight current heading && 3. Do not display incompatible investment's table if empty

screen shot 2017-07-10 at 17 47 27

4. Show incompatibility checkbox only if winner or incompatible (not for compatible non-winners)

screen shot 2017-07-10 at 18 01 48

Test

Extended coverage for all new scenarios. Better check each commit to see in detail

Deployment

As usual

Warnings

None

Why:

* We should recalculate winners also when an incompatible investment is flagged as compatible again

How:

* Removing the condition to recalculate that was checking only for a winner investment flagged as incompatible
* Extending the Budget::Result model spec to cover that new scenario
Why:

* As seen on preproduction and production environments on Madrid's fork. Budget::Result#calculate_winners is very costly when done to all headings for a given budget (as requested on Admin::BudgetsController#calculate_winners) but its not when done individually for only a heading (as requested on Budget::Investment#recalculate_heading_winners)

How:

* Removing `handle_asynchronously :calculate_winners` from bellow Budget::Result#calculate_winners definition, to avoid making any call delayed. And explicitly calling `.delay` only when needed (on Admin::BudgetsController#calculate_winners)
Why:

* A non-winner but compatible investment shouldn't be marked as incompatible

How:

* Show incompatilibility checkbox only if investment is winner or incompatible
@bertocq bertocq mentioned this pull request Jul 10, 2017
4 tasks
@bertocq
Copy link
Collaborator Author

bertocq commented Jul 10, 2017

BONUS:

Included non-winner and incompatible investments hidden by default as requested at #1737 (comment)
(specs updated as well)
out

@bertocq bertocq merged commit 6285ad3 into master Jul 10, 2017
@bertocq bertocq deleted the feature/improve_budget_results branch July 10, 2017 20:57
javierm pushed a commit to javierm/consul that referenced this pull request Dec 7, 2018
…upstream_missing_spec

[Upstream] Add missing feature spec: Proposal Notifications In-app notifications from the proposal's author group notifications for the same proposal
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 this pull request may close these issues.

None yet

2 participants