From d5cb01bf98312e658708833e3a2240656a7d8592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Feb 2023 15:13:27 +0100 Subject: [PATCH 1/5] Split test in budget valuation component We're going to add more cases and the test for the link to the evaluate would become too big, so we're splitting it. --- .../valuation/budgets/row_component_spec.rb | 90 ++++++++++++------- 1 file changed, 58 insertions(+), 32 deletions(-) diff --git a/spec/components/valuation/budgets/row_component_spec.rb b/spec/components/valuation/budgets/row_component_spec.rb index 4d0f1a7af..5ec3d5258 100644 --- a/spec/components/valuation/budgets/row_component_spec.rb +++ b/spec/components/valuation/budgets/row_component_spec.rb @@ -5,46 +5,72 @@ describe Valuation::Budgets::RowComponent do before { sign_in(valuator.user) } - it "Displays visible and assigned investments count when budget is in valuating phase" do - budget = create(:budget, :valuating, name: "Sports") - create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) - create(:budget_investment, :invisible_to_valuators, budget: budget, valuators: [valuator]) - create(:budget_investment, :visible_to_valuators, budget: budget) + describe "investments count" do + it "counts visible and assigned investments when the budget is in the valuating phase" do + budget = create(:budget, :valuating) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + create(:budget_investment, :invisible_to_valuators, budget: budget, valuators: [valuator]) + create(:budget_investment, :visible_to_valuators, budget: budget) - render_inline Valuation::Budgets::RowComponent.new(budget: budget) + render_inline Valuation::Budgets::RowComponent.new(budget: budget) - expect(page).to have_selector(".investments-count", text: "1") + expect(page).to have_selector ".investments-count", text: "1" + end + + it "displays zero when the budget is not in the valuating phase" do + budget = create(:budget, %i[accepting finished].sample) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).to have_selector ".investments-count", text: "0" + end end - it "Displays zero as investments count when budget is not in valuating phase" do - budget = create(:budget, %i[accepting finished].sample, name: "Sports") - create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + describe "link to evaluate investments" do + it "is shown when the valuator has visible investments assigned in the valuating phase" do + budget = create(:budget, :valuating) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) - render_inline Valuation::Budgets::RowComponent.new(budget: budget) + render_inline Valuation::Budgets::RowComponent.new(budget: budget) - expect(page).to have_selector(".investments-count", text: "0") - end + expect(page).to have_link "Evaluate" + end - it "Displays the link to evaluate investments when valuator has visible investments assigned and budget is - in valuating phase" do - valuating = create(:budget, :valuating) - create(:budget_investment, :visible_to_valuators, budget: valuating, valuators: [valuator]) - valuating_invisible = create(:budget, :valuating) - create(:budget_investment, :invisible_to_valuators, budget: valuating_invisible, valuators: [valuator]) - valuating_unassigned = create(:budget, :valuating) - create(:budget_investment, :visible_to_valuators, budget: valuating_unassigned) - accepting = create(:budget, :accepting) - create(:budget_investment, :visible_to_valuators, budget: accepting, valuators: [valuator]) - finished = create(:budget, :finished) - create(:budget_investment, :visible_to_valuators, budget: finished, valuators: [valuator]) - budgets = [valuating, valuating_invisible, valuating_unassigned, accepting, finished] + it "is not shown when the assigned investments aren't visible to valuators" do + budget = create(:budget, :valuating) + create(:budget_investment, :invisible_to_valuators, budget: budget, valuators: [valuator]) - render_inline Valuation::Budgets::RowComponent.with_collection(budgets) + render_inline Valuation::Budgets::RowComponent.new(budget: budget) - expect(page.find("#budget_#{valuating.id}")).to have_link("Evaluate") - expect(page.find("#budget_#{valuating_invisible.id}")).not_to have_link("Evaluate") - expect(page.find("#budget_#{valuating_unassigned.id}")).not_to have_link("Evaluate") - expect(page.find("#budget_#{accepting.id}")).not_to have_link("Evaluate") - expect(page.find("#budget_#{finished.id}")).not_to have_link("Evaluate") + expect(page).not_to have_link "Evaluate" + end + + it "is not shown when the valuator doesn't have assigned investments" do + budget = create(:budget, :valuating) + create(:budget_investment, :visible_to_valuators, budget: budget) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).not_to have_link "Evaluate" + end + + it "is not shown when the budget hasn't reached the valuating phase" do + budget = create(:budget, :accepting) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).not_to have_link "Evaluate" + end + + it "is not shown when the valuating phase is over" do + budget = create(:budget, :finished) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).not_to have_link "Evaluate" + end end end From ed4b03c3ed1a6de8382ba386e3514229dcd69cfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 4 Feb 2023 16:05:28 +0100 Subject: [PATCH 2/5] Show link to evaluate after evaluation has finished This way it's still possible to access the "evaluation finished" filter in the valuation investments index. --- .../valuation/budgets/row_component.html.erb | 2 +- .../valuation/budgets/row_component.rb | 10 ++++- .../valuation/budgets/row_component_spec.rb | 41 +++++++++++++++++-- 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/app/components/valuation/budgets/row_component.html.erb b/app/components/valuation/budgets/row_component.html.erb index 5ba9933f8..279617bd3 100644 --- a/app/components/valuation/budgets/row_component.html.erb +++ b/app/components/valuation/budgets/row_component.html.erb @@ -6,7 +6,7 @@ <%= budget.current_phase.name %> - <%= investments.count %> + <%= valuation_open_investments_count %> <% if investments.any? %> diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb index 4b75b196e..fd9417bd2 100644 --- a/app/components/valuation/budgets/row_component.rb +++ b/app/components/valuation/budgets/row_component.rb @@ -9,8 +9,14 @@ class Valuation::Budgets::RowComponent < ApplicationComponent end def investments - return Budget::Investment.none unless budget.valuating? + return Budget::Investment.none unless budget.valuating_or_later? - budget.investments.visible_to_valuators.by_valuator(current_user.valuator).valuation_open + budget.investments.visible_to_valuators.by_valuator(current_user.valuator) + end + + def valuation_open_investments_count + return 0 unless budget.valuating? + + investments.valuation_open.count end end diff --git a/spec/components/valuation/budgets/row_component_spec.rb b/spec/components/valuation/budgets/row_component_spec.rb index 5ec3d5258..384ec05e6 100644 --- a/spec/components/valuation/budgets/row_component_spec.rb +++ b/spec/components/valuation/budgets/row_component_spec.rb @@ -17,8 +17,29 @@ describe Valuation::Budgets::RowComponent do expect(page).to have_selector ".investments-count", text: "1" end - it "displays zero when the budget is not in the valuating phase" do - budget = create(:budget, %i[accepting finished].sample) + it "does not count investments with valuation finished" do + budget = create(:budget, :valuating) + create(:budget_investment, :visible_to_valuators, + budget: budget, + valuators: [valuator], + valuation_finished: true) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).to have_selector ".investments-count", text: "0" + end + + it "displays zero when the budget hasn't reached the valuating phase" do + budget = create(:budget, :accepting) + create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).to have_selector ".investments-count", text: "0" + end + + it "displays zero when the valuating phase is over" do + budget = create(:budget, :finished) create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) render_inline Valuation::Budgets::RowComponent.new(budget: budget) @@ -37,6 +58,18 @@ describe Valuation::Budgets::RowComponent do expect(page).to have_link "Evaluate" end + it "is shown when the assigned investments have finished valuation" do + budget = create(:budget, :valuating) + create(:budget_investment, :visible_to_valuators, + budget: budget, + valuators: [valuator], + valuation_finished: true) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).to have_link "Evaluate" + end + it "is not shown when the assigned investments aren't visible to valuators" do budget = create(:budget, :valuating) create(:budget_investment, :invisible_to_valuators, budget: budget, valuators: [valuator]) @@ -64,13 +97,13 @@ describe Valuation::Budgets::RowComponent do expect(page).not_to have_link "Evaluate" end - it "is not shown when the valuating phase is over" do + it "is shown when the valuating phase is over" do budget = create(:budget, :finished) create(:budget_investment, :visible_to_valuators, budget: budget, valuators: [valuator]) render_inline Valuation::Budgets::RowComponent.new(budget: budget) - expect(page).not_to have_link "Evaluate" + expect(page).to have_link "Evaluate" end end end From 1649b9125e25049f1da59a9ddc6ab94dbd2046bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Feb 2023 14:30:00 +0100 Subject: [PATCH 3/5] Add scope to get investments visible to a valuator Using this method makes it more obvious that we're loading the same investments in the budgets index as in the investments index. --- app/components/valuation/budgets/row_component.rb | 2 +- .../valuation/budget_investments_controller.rb | 10 +++++----- app/models/budget/investment.rb | 1 + 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb index fd9417bd2..6795f90d9 100644 --- a/app/components/valuation/budgets/row_component.rb +++ b/app/components/valuation/budgets/row_component.rb @@ -11,7 +11,7 @@ class Valuation::Budgets::RowComponent < ApplicationComponent def investments return Budget::Investment.none unless budget.valuating_or_later? - budget.investments.visible_to_valuators.by_valuator(current_user.valuator) + budget.investments.visible_to_valuator(current_user.valuator) end def valuation_open_investments_count diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index f1ff5d834..37f891617 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -17,7 +17,8 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController def index @heading_filters = heading_filters @investments = if current_user.valuator? && @budget.present? - @budget.investments.visible_to_valuators.scoped_filter(params_for_current_valuator, @current_filter) + @budget.investments.visible_to_valuator(current_user.valuator) + .scoped_filter(filtered_params, @current_filter) .order(cached_votes_up: :desc) .page(params[:page]) else @@ -72,7 +73,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController end def heading_filters - investments = @budget.investments.by_valuator(current_user.valuator&.id).visible_to_valuators.distinct + investments = @budget.investments.visible_to_valuator(current_user.valuator).distinct investment_headings = Budget::Heading.where(id: investments.pluck(:heading_id)).sort_by(&:name) all_headings_filter = [ @@ -92,9 +93,8 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController end end - def params_for_current_valuator - Budget::Investment.filter_params(params).to_h.merge({ valuator_id: current_user.valuator.id, - budget_id: @budget.id }) + def filtered_params + Budget::Investment.filter_params(params).to_h.except(:valuator_id).merge(budget_id: @budget.id) end def valuation_params diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 76fe714f4..306f89793 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -100,6 +100,7 @@ class Budget scope :by_heading, ->(heading_id) { where(heading_id: heading_id) } scope :by_admin, ->(admin_id) { where(administrator_id: admin_id) } scope :by_tag, ->(tag_name) { tagged_with(tag_name).distinct } + scope :visible_to_valuator, ->(valuator) { visible_to_valuators.by_valuator(valuator) } scope :for_render, -> { includes(:heading) } From b6ed11471e024001c3264f2f230804d740428f0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 6 Feb 2023 14:30:44 +0100 Subject: [PATCH 4/5] Simplify investment params filters in valuation We weren't allowing the `budget_id` parameter and then we were adding it manually. We were also allowing other parameters that aren't used in the valuation section. So we're allowing budget and heading, which are the only parameter we're offering filters for in the user interface. Note the `budget_id` parameter doesn't seem to make sense because we're already inside a `@budget.investments` statement, but the `budget_id` parameter is required by the `scoped_filter` method. --- app/controllers/valuation/budget_investments_controller.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index 37f891617..6ebb4dd94 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -18,7 +18,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController @heading_filters = heading_filters @investments = if current_user.valuator? && @budget.present? @budget.investments.visible_to_valuator(current_user.valuator) - .scoped_filter(filtered_params, @current_filter) + .scoped_filter(params.permit(:budget_id, :heading_id), @current_filter) .order(cached_votes_up: :desc) .page(params[:page]) else @@ -93,10 +93,6 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController end end - def filtered_params - Budget::Investment.filter_params(params).to_h.except(:valuator_id).merge(budget_id: @budget.id) - end - def valuation_params params.require(:budget_investment).permit(allowed_params) end From 54fbdf4372b7e866c7d432edf39c387aef12b7ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 9 Feb 2023 19:05:11 +0100 Subject: [PATCH 5/5] Remove unnecessary condition in valuation investments The budget is loaded using a method which raises an exception if it isn't found, so `@budget.present?` will always return true. --- app/controllers/valuation/budget_investments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index 6ebb4dd94..4fcc990f1 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -16,7 +16,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController def index @heading_filters = heading_filters - @investments = if current_user.valuator? && @budget.present? + @investments = if current_user.valuator? @budget.investments.visible_to_valuator(current_user.valuator) .scoped_filter(params.permit(:budget_id, :heading_id), @current_filter) .order(cached_votes_up: :desc)