From 2b85deabec96371f51530ca452e9011cc165f901 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 14:24:55 +0200 Subject: [PATCH 1/6] Recalculate heading winners on incompatibility change 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 --- app/models/budget/investment.rb | 2 +- spec/models/budget/result_spec.rb | 20 +++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 5fcf2e882..6a83b28a2 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -204,7 +204,7 @@ class Budget end def recalculate_heading_winners - Budget::Result.new(budget, heading).calculate_winners if incompatible_changed? && winner? && incompatible? + Budget::Result.new(budget, heading).calculate_winners if incompatible_changed? end def set_responsible_name diff --git a/spec/models/budget/result_spec.rb b/spec/models/budget/result_spec.rb index 85eb14f3e..a650e25c5 100644 --- a/spec/models/budget/result_spec.rb +++ b/spec/models/budget/result_spec.rb @@ -15,7 +15,7 @@ describe Budget::Result do investment4 = create(:budget_investment, :selected, heading: heading, price: 500, ballot_lines_count: 600, winner: false) investment5 = create(:budget_investment, :selected, heading: heading, price: 100, ballot_lines_count: 500, winner: false) - Budget::Result.new(budget, heading).calculate_winners + described_class.new(budget, heading).calculate_winners expect(heading.investments.winners.pluck(:id)).to match_array([investment1.id, investment2.id, investment4.id]) end @@ -29,7 +29,7 @@ describe Budget::Result do investment4 = create(:budget_investment, :winner, heading: heading, price: 500, ballot_lines_count: 600) investment5 = create(:budget_investment, :winner, heading: heading, price: 100, ballot_lines_count: 500) - Budget::Result.new(budget, heading).calculate_winners + described_class.new(budget, heading).calculate_winners expect(heading.investments.winners.pluck(:id)).to match_array([investment1.id, investment2.id, investment4.id]) end @@ -49,6 +49,20 @@ describe Budget::Result do expect(heading.investments.winners.pluck(:id)).to match_array([investment1.id, investment2.id, investment4.id]) end end - end + context "When an incompatible is flagged as compatible again" do + it "recalculates winners taking it in consideration" do + investment1 = create(:budget_investment, :winner, heading: heading, price: 200, ballot_lines_count: 900) + investment2 = create(:budget_investment, :winner, heading: heading, price: 300, ballot_lines_count: 800) + investment3 = create(:budget_investment, :incompatible, heading: heading, price: 500, ballot_lines_count: 700) + investment4 = create(:budget_investment, :winner, heading: heading, price: 500, ballot_lines_count: 600) + investment5 = create(:budget_investment, :winner, heading: heading, price: 100, ballot_lines_count: 500) + + investment3.incompatible = false + investment3.save + + expect(heading.investments.winners.pluck(:id)).to match_array([investment1.id, investment2.id, investment3.id]) + end + end + end end From efacd0def32df2850deb19998ddf7d94b788146b Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 15:12:14 +0200 Subject: [PATCH 2/6] Make calculate_winners explicitly delayed 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) --- app/controllers/admin/budgets_controller.rb | 2 +- app/models/budget/result.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/admin/budgets_controller.rb b/app/controllers/admin/budgets_controller.rb index b37351028..7caf4f35c 100644 --- a/app/controllers/admin/budgets_controller.rb +++ b/app/controllers/admin/budgets_controller.rb @@ -20,7 +20,7 @@ class Admin::BudgetsController < Admin::BaseController def calculate_winners return unless @budget.balloting_process? - @budget.headings.each { |heading| Budget::Result.new(@budget, heading).calculate_winners } + @budget.headings.each { |heading| Budget::Result.new(@budget, heading).delay.calculate_winners } redirect_to admin_budget_budget_investments_path(budget_id: @budget.id, filter: 'winners'), notice: I18n.t("admin.budgets.winners.calculated") end diff --git a/app/models/budget/result.rb b/app/models/budget/result.rb index aa6c4c7ac..d034e50ef 100644 --- a/app/models/budget/result.rb +++ b/app/models/budget/result.rb @@ -15,7 +15,6 @@ class Budget set_winner if inside_budget? end end - handle_asynchronously :calculate_winners def investments heading.investments.selected.sort_by_ballots From 2fba9de33b697a9c6ff62045c807a61c61b031e7 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 17:07:00 +0200 Subject: [PATCH 3/6] Don't display incompatible investment's table if empty --- app/views/budgets/results/show.html.erb | 10 ++++++---- spec/features/budgets/results_spec.rb | 10 ++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/views/budgets/results/show.html.erb b/app/views/budgets/results/show.html.erb index 2343e652c..f2b9ae7f8 100644 --- a/app/views/budgets/results/show.html.erb +++ b/app/views/budgets/results/show.html.erb @@ -42,8 +42,10 @@ heading_price: @heading.price, investments: @investments.compatible %> - <%= render 'results_table', results_type: :incompatible, - title: t("budgets.results.incompatibles"), - heading_price: @heading.price, - investments: @investments.incompatible %> + <% if @investments.incompatible.present? %> + <%= render 'results_table', results_type: :incompatible, + title: t("budgets.results.incompatibles"), + heading_price: @heading.price, + investments: @investments.incompatible %> + <% end %> diff --git a/spec/features/budgets/results_spec.rb b/spec/features/budgets/results_spec.rb index cd9cb0912..0e70fdaab 100644 --- a/spec/features/budgets/results_spec.rb +++ b/spec/features/budgets/results_spec.rb @@ -55,4 +55,14 @@ feature 'Results' do expect(page).to have_content "You do not have permission to carry out the action" end + scenario "No incompatible investments", :js do + investment3.incompatible = false + investment3.save + + visit budget_path(budget) + click_link "See results" + + expect(page).not_to have_content "Incompatibles" + end + end From 741a342bb0fd26a385356788c84dc2b788b41426 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 17:34:05 +0200 Subject: [PATCH 4/6] Restrict compatilibility edit on admin investment form Why: * A non-winner but compatible investment shouldn't be marked as incompatible How: * Show incompatilibility checkbox only if investment is winner or incompatible --- .../admin/budget_investments/edit.html.erb | 2 ++ spec/features/admin/budget_investments_spec.rb | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/app/views/admin/budget_investments/edit.html.erb b/app/views/admin/budget_investments/edit.html.erb index 1e21ee706..c33039021 100644 --- a/app/views/admin/budget_investments/edit.html.erb +++ b/app/views/admin/budget_investments/edit.html.erb @@ -63,6 +63,7 @@
+ <% if @investment.incompatible? || @investment.winner? %>

<%= t("admin.budget_investments.edit.compatibility") %>

<%= f.label :incompatible do %> @@ -70,6 +71,7 @@ <%= t("admin.budget_investments.edit.mark_as_incompatible") %> <% end %>
+ <% end %>

<%= t("admin.budget_investments.edit.selection") %>

<%= f.label :selected do %> diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 57dedcb54..925f105b0 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -312,7 +312,7 @@ feature 'Admin budget investments' do context "Edit" do scenario "Change title, incompatible, description or heading" do - budget_investment = create(:budget_investment, :selected) + budget_investment = create(:budget_investment, :incompatible) create(:budget_heading, group: budget_investment.group, name: "Barbate") visit admin_budget_budget_investment_path(budget_investment.budget, budget_investment) @@ -321,7 +321,7 @@ feature 'Admin budget investments' do fill_in 'budget_investment_title', with: 'Potatoes' fill_in 'budget_investment_description', with: 'Carrots' select "#{budget_investment.group.name}: Barbate", from: 'budget_investment[heading_id]' - check "budget_investment_incompatible" + uncheck "budget_investment_incompatible" check "budget_investment_selected" click_button 'Update' @@ -329,10 +329,21 @@ feature 'Admin budget investments' do expect(page).to have_content 'Potatoes' expect(page).to have_content 'Carrots' expect(page).to have_content 'Barbate' - expect(page).to have_content 'Incompatible' + expect(page).to have_content 'Compatibility: Compatible' expect(page).to have_content 'Selected' end + scenario "Compatible non-winner can't edit incompatibility" do + budget_investment = create(:budget_investment, :selected) + create(:budget_heading, group: budget_investment.group, name: "Tetuan") + + visit admin_budget_budget_investment_path(budget_investment.budget, budget_investment) + click_link 'Edit' + + expect(page).not_to have_content 'Compatibility' + expect(page).not_to have_content 'Mark as incompatible' + end + scenario "Add administrator" do budget_investment = create(:budget_investment) administrator = create(:administrator, user: create(:user, username: 'Marta', email: 'marta@admins.org')) From 49f48ddf4be50b932799739143bbbf3455bc410c Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 17:48:04 +0200 Subject: [PATCH 5/6] Highlight current heading on results list --- app/assets/stylesheets/layout.scss | 5 +++++ app/views/budgets/results/show.html.erb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 9673cdc88..e223ffa56 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -236,6 +236,11 @@ a { border-bottom: 2px solid $brand; color: $brand; } + + &.bold { + font-weight: bold; + color: $brand; + } } &.no-margin-top { diff --git a/app/views/budgets/results/show.html.erb b/app/views/budgets/results/show.html.erb index f2b9ae7f8..278ae012b 100644 --- a/app/views/budgets/results/show.html.erb +++ b/app/views/budgets/results/show.html.erb @@ -26,7 +26,8 @@ <% @budget.headings.each do |heading| %> -
  • + <% active_class = heading.id.to_s == params[:heading_id] ? 'bold' : '' %> +
  • <%= link_to heading.name, budget_results_path(@budget, heading_id: heading.id) %>
    • From 11a793d63f201e8ab7a1c81794e3815928a8db28 Mon Sep 17 00:00:00 2001 From: Bertocq Date: Mon, 10 Jul 2017 19:41:04 +0200 Subject: [PATCH 6/6] Hide incompatible and non-winner investments by default --- .../budgets/results/_results_table.html.erb | 8 +++-- app/views/budgets/results/show.html.erb | 2 +- spec/features/budgets/results_spec.rb | 30 +++++++++---------- 3 files changed, 22 insertions(+), 18 deletions(-) diff --git a/app/views/budgets/results/_results_table.html.erb b/app/views/budgets/results/_results_table.html.erb index 993060562..f391e8d50 100644 --- a/app/views/budgets/results/_results_table.html.erb +++ b/app/views/budgets/results/_results_table.html.erb @@ -1,4 +1,6 @@ -
    +

    <%= title %> @@ -28,7 +30,9 @@ <% amount_available = heading_price %> <% investments.each do |investment| %> - + <% if investment.winner? %> diff --git a/app/views/budgets/results/show.html.erb b/app/views/budgets/results/show.html.erb index 278ae012b..eb31124ec 100644 --- a/app/views/budgets/results/show.html.erb +++ b/app/views/budgets/results/show.html.erb @@ -35,7 +35,7 @@

    - <%= link_to t("budgets.results.hide_discarded_link"), "#", class: "js-toggle-link button hollow margin-bottom", data: {'toggle-selector' => '.js-discarded', 'toggle-text' => t("budgets.results.show_all_link")} %> + <%= link_to t("budgets.results.show_all_link"), "#", class: "js-toggle-link button hollow margin-bottom", data: {'toggle-selector' => '.js-discarded', 'toggle-text' => t("budgets.results.hide_discarded_link")} %> <%= render 'results_table', results_type: :compatible, diff --git a/spec/features/budgets/results_spec.rb b/spec/features/budgets/results_spec.rb index 0e70fdaab..3b2d05c65 100644 --- a/spec/features/budgets/results_spec.rb +++ b/spec/features/budgets/results_spec.rb @@ -17,6 +17,21 @@ feature 'Results' do visit budget_path(budget) click_link "See results" + within("#budget-investments-compatible") do + expect(page).to have_content investment1.title + expect(page).to have_content investment2.title + expect(page).not_to have_content investment3.title + expect(page).not_to have_content investment4.title + + expect(investment1.title).to appear_before(investment2.title) + end + end + + scenario "Show non winner & incomaptible investments", :js do + visit budget_path(budget) + click_link "See results" + click_link "Show all" + within("#budget-investments-compatible") do expect(page).to have_content investment1.title expect(page).to have_content investment2.title @@ -31,21 +46,6 @@ feature 'Results' do end end - scenario "Displays non winner investments", :js do - visit budget_path(budget) - click_link "See results" - click_link "Hide discarded" - - within("#budget-investments-compatible") do - expect(page).to have_content investment1.title - expect(page).to have_content investment2.title - expect(page).not_to have_content investment3.title - expect(page).not_to have_content investment4.title - - expect(investment1.title).to appear_before(investment2.title) - end - end - scenario "If budget is in a phase different from finished results can't be accessed" do budget.update phase: (Budget::PHASES - ["finished"]).sample visit budget_path(budget)