From 45b9eccfd8f605f3d9546a2d6033950cdb983560 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 9 Feb 2023 20:56:45 +0100 Subject: [PATCH] Show valuator group investments to their valuators When accessing the valuation area, we were only displaying the investments directly assigned to the current valuator, but we weren't displaying the investments assigned to that valuator's group. Using the `assigned_investments_ids` method, which takes the valuator group into account, solves the issue. We've also found an issue on our development machines: since we don't have a unique index per `investment_id` and `valuator_id` in the `budget_valuator_assignments` table, we've found duplicate records on this table. When that happened, we were displaying the same investment several times. Since now we no longer join this table in the query returning the investment, this issue is also solved, and we're adding a test for it. We can now remove the call to the `distinct` method when calculating the number of investments per heading. --- .../budget_investments_controller.rb | 2 +- app/models/budget/investment.rb | 2 +- .../valuation/budgets/row_component_spec.rb | 20 ++++++++ spec/models/budget/investment_spec.rb | 49 +++++++++++++++++++ 4 files changed, 71 insertions(+), 2 deletions(-) diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb index 4fcc990f1..3460b6d20 100644 --- a/app/controllers/valuation/budget_investments_controller.rb +++ b/app/controllers/valuation/budget_investments_controller.rb @@ -73,7 +73,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController end def heading_filters - investments = @budget.investments.visible_to_valuator(current_user.valuator).distinct + investments = @budget.investments.visible_to_valuator(current_user.valuator) investment_headings = Budget::Heading.where(id: investments.pluck(:heading_id)).sort_by(&:name) all_headings_filter = [ diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index 306f89793..051c569bb 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -100,7 +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 :visible_to_valuator, ->(valuator) { visible_to_valuators.where(id: valuator&.assigned_investment_ids) } scope :for_render, -> { includes(:heading) } diff --git a/spec/components/valuation/budgets/row_component_spec.rb b/spec/components/valuation/budgets/row_component_spec.rb index 384ec05e6..63d0e7370 100644 --- a/spec/components/valuation/budgets/row_component_spec.rb +++ b/spec/components/valuation/budgets/row_component_spec.rb @@ -17,6 +17,16 @@ describe Valuation::Budgets::RowComponent do expect(page).to have_selector ".investments-count", text: "1" end + it "counts investments assigned to the valuator group" do + budget = create(:budget, :valuating) + valuator_group = create(:valuator_group, valuators: [valuator]) + create(:budget_investment, :visible_to_valuators, budget: budget, valuator_groups: [valuator_group]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + expect(page).to have_selector ".investments-count", text: "1" + end + it "does not count investments with valuation finished" do budget = create(:budget, :valuating) create(:budget_investment, :visible_to_valuators, @@ -58,6 +68,16 @@ describe Valuation::Budgets::RowComponent do expect(page).to have_link "Evaluate" end + it "is shown when the investments are assigned to the valuator group" do + budget = create(:budget, :valuating) + valuator_group = create(:valuator_group, valuators: [valuator]) + create(:budget_investment, :visible_to_valuators, budget: budget, valuator_groups: [valuator_group]) + + render_inline Valuation::Budgets::RowComponent.new(budget: budget) + + 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, diff --git a/spec/models/budget/investment_spec.rb b/spec/models/budget/investment_spec.rb index a0d4e2ef4..7dede4c03 100644 --- a/spec/models/budget/investment_spec.rb +++ b/spec/models/budget/investment_spec.rb @@ -414,6 +414,55 @@ describe Budget::Investment do end end + describe ".visible_to_valuator" do + let(:valuator) { create(:valuator) } + + it "returns investments assigned to the valuator" do + investment = create(:budget_investment, :visible_to_valuators, valuators: [valuator]) + + expect(Budget::Investment.visible_to_valuator(valuator)).to eq [investment] + end + + it "does not return investments assigned to other valuators" do + create(:budget_investment, :visible_to_valuators, valuators: [create(:valuator)]) + + expect(Budget::Investment.visible_to_valuator(valuator)).to be_empty + end + + it "does not return duplicate investments when they're assigned more than once" do + investment = create(:budget_investment, :visible_to_valuators) + 2.times { Budget::ValuatorAssignment.create!(valuator: valuator, investment: investment) } + + expect(Budget::Investment.visible_to_valuator(valuator)).to eq [investment] + end + + it "does not return duplicate investments when assigned to both a valuator and their group" do + valuator_group = create(:valuator_group, valuators: [valuator]) + investment = create(:budget_investment, :visible_to_valuators, valuators: [valuator], + valuator_groups: [valuator_group]) + + expect(Budget::Investment.visible_to_valuator(valuator)).to eq [investment] + end + + it "returns investments assigned to the valuator's group" do + valuator_group = create(:valuator_group, valuators: [valuator]) + investment = create(:budget_investment, :visible_to_valuators, valuator_groups: [valuator_group]) + + expect(Budget::Investment.visible_to_valuator(valuator)).to eq [investment] + end + + it "does not return investments assigned to other valuator groups" do + valuator_group = create(:valuator_group, valuators: [create(:valuator)]) + create(:budget_investment, :visible_to_valuators, valuator_groups: [valuator_group]) + + expect(Budget::Investment.visible_to_valuator(valuator)).to be_empty + end + + it "returns an empty relation when valuator is nil" do + expect(Budget::Investment.visible_to_valuator(nil)).to be_empty + end + end + describe ".scoped_filter" do let(:budget) { create(:budget, :balloting, slug: "budget_slug") } let(:investment) { create(:budget_investment, budget: budget) }