From ee80b3f4a259c945314d31ab771f1d45a7814f2e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?=
 <15726+Senen@users.noreply.github.com>
Date: Wed, 4 Jan 2023 12:31:17 +0100
Subject: [PATCH 1/5] Extract valuation budget index view to components

---
 .../budgets/index_component.html.erb          | 21 +++++++++++
 .../valuation/budgets/index_component.rb      |  7 ++++
 .../valuation/budgets/row_component.html.erb  | 16 ++++++++
 .../valuation/budgets/row_component.rb        | 13 +++++++
 .../valuation/budgets_controller.rb           |  3 --
 app/views/valuation/budgets/index.html.erb    | 37 +------------------
 6 files changed, 58 insertions(+), 39 deletions(-)
 create mode 100644 app/components/valuation/budgets/index_component.html.erb
 create mode 100644 app/components/valuation/budgets/index_component.rb
 create mode 100644 app/components/valuation/budgets/row_component.html.erb
 create mode 100644 app/components/valuation/budgets/row_component.rb

diff --git a/app/components/valuation/budgets/index_component.html.erb b/app/components/valuation/budgets/index_component.html.erb
new file mode 100644
index 000000000..d4ecb28d5
--- /dev/null
+++ b/app/components/valuation/budgets/index_component.html.erb
@@ -0,0 +1,21 @@
+<h2 class="inline-block"><%= t("valuation.budgets.index.title") %></h2>
+
+<% if budget.present? %>
+  <table>
+    <thead>
+      <tr>
+        <th><%= t("valuation.budgets.index.table_name") %></th>
+        <th><%= t("valuation.budgets.index.table_phase") %></th>
+        <th><%= t("valuation.budgets.index.table_assigned_investments_valuation_open") %></th>
+        <th><%= t("valuation.budgets.index.table_actions") %></th>
+      </tr>
+    </thead>
+    <tbody>
+      <%= render Valuation::Budgets::RowComponent.new(budget) %>
+    </tbody>
+  </table>
+<% else %>
+  <div class="callout primary clear">
+    <%= t("valuation.budgets.index.no_budgets") %>
+  </div>
+<% end %>
diff --git a/app/components/valuation/budgets/index_component.rb b/app/components/valuation/budgets/index_component.rb
new file mode 100644
index 000000000..40138ac88
--- /dev/null
+++ b/app/components/valuation/budgets/index_component.rb
@@ -0,0 +1,7 @@
+class Valuation::Budgets::IndexComponent < ApplicationComponent
+  attr_reader :budget
+
+  def initialize(budget)
+    @budget = budget
+  end
+end
diff --git a/app/components/valuation/budgets/row_component.html.erb b/app/components/valuation/budgets/row_component.html.erb
new file mode 100644
index 000000000..be3fc52c5
--- /dev/null
+++ b/app/components/valuation/budgets/row_component.html.erb
@@ -0,0 +1,16 @@
+<tr id="<%= dom_id(budget) %>" class="budget">
+  <td>
+    <%= budget.name %>
+  </td>
+  <td>
+    <%= budget.current_phase.name %>
+  </td>
+  <td>
+    <%= investments_count %>
+  </td>
+  <td>
+    <%= link_to t("valuation.budgets.index.evaluate"),
+                valuation_budget_budget_investments_path(budget_id: budget.id),
+                class: "button hollow expanded" %>
+  </td>
+</tr>
diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb
new file mode 100644
index 000000000..46fff464a
--- /dev/null
+++ b/app/components/valuation/budgets/row_component.rb
@@ -0,0 +1,13 @@
+class Valuation::Budgets::RowComponent < ApplicationComponent
+  attr_reader :budget
+
+  delegate :current_user, to: :helpers
+
+  def initialize(budget)
+    @budget = budget
+  end
+
+  def investments_count
+    budget.investments.by_valuator(current_user.valuator).valuation_open.count
+  end
+end
diff --git a/app/controllers/valuation/budgets_controller.rb b/app/controllers/valuation/budgets_controller.rb
index 0165fb0e2..a30ccc55e 100644
--- a/app/controllers/valuation/budgets_controller.rb
+++ b/app/controllers/valuation/budgets_controller.rb
@@ -6,8 +6,5 @@ class Valuation::BudgetsController < Valuation::BaseController
 
   def index
     @budget = current_budget
-    if @budget.present?
-      @investments = @budget.investments.by_valuator(current_user.valuator).valuation_open
-    end
   end
 end
diff --git a/app/views/valuation/budgets/index.html.erb b/app/views/valuation/budgets/index.html.erb
index fae10257b..5f2db605c 100644
--- a/app/views/valuation/budgets/index.html.erb
+++ b/app/views/valuation/budgets/index.html.erb
@@ -1,36 +1 @@
-<h2 class="inline-block"><%= t("valuation.budgets.index.title") %></h2>
-
-<% if @budget.present? %>
-  <table>
-    <thead>
-      <tr>
-        <th><%= t("valuation.budgets.index.table_name") %></th>
-        <th><%= t("valuation.budgets.index.table_phase") %></th>
-        <th><%= t("valuation.budgets.index.table_assigned_investments_valuation_open") %></th>
-        <th><%= t("valuation.budgets.index.table_actions") %></th>
-      </tr>
-    </thead>
-    <tbody>
-      <tr id="<%= dom_id(@budget) %>" class="budget">
-        <td>
-          <%= @budget.name %>
-        </td>
-        <td>
-          <%= @budget.current_phase.name %>
-        </td>
-        <td>
-          <%= @investments.count %>
-        </td>
-        <td>
-          <%= link_to t("valuation.budgets.index.evaluate"),
-                      valuation_budget_budget_investments_path(budget_id: @budget.id),
-                      class: "button hollow expanded" %>
-        </td>
-      </tr>
-    </tbody>
-  </table>
-<% else %>
-  <div class="callout primary clear">
-    <%= t("valuation.budgets.index.no_budgets") %>
-  </div>
-<% end %>
+<%= render Valuation::Budgets::IndexComponent.new(@budget) %>

From 282b8f869705bbac14a439d74d5664e18c9c3c05 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?=
 <15726+Senen@users.noreply.github.com>
Date: Wed, 4 Jan 2023 13:09:58 +0100
Subject: [PATCH 2/5] Load all the published budgets in the valuation interface

As now multiple budget can coexist at the same time it has sense to be able to
browse all the published budgets in the valuation budgets index page.
---
 .../budgets/index_component.html.erb          |  4 ++--
 .../valuation/budgets/index_component.rb      |  6 +++---
 .../valuation/budgets/row_component.rb        |  3 ++-
 .../valuation/budgets_controller.rb           |  2 +-
 app/views/valuation/budgets/index.html.erb    |  2 +-
 spec/system/valuation/budgets_spec.rb         | 19 +++++--------------
 6 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/app/components/valuation/budgets/index_component.html.erb b/app/components/valuation/budgets/index_component.html.erb
index d4ecb28d5..81e82f745 100644
--- a/app/components/valuation/budgets/index_component.html.erb
+++ b/app/components/valuation/budgets/index_component.html.erb
@@ -1,6 +1,6 @@
 <h2 class="inline-block"><%= t("valuation.budgets.index.title") %></h2>
 
-<% if budget.present? %>
+<% if budgets.any? %>
   <table>
     <thead>
       <tr>
@@ -11,7 +11,7 @@
       </tr>
     </thead>
     <tbody>
-      <%= render Valuation::Budgets::RowComponent.new(budget) %>
+      <%= render Valuation::Budgets::RowComponent.with_collection(budgets) %>
     </tbody>
   </table>
 <% else %>
diff --git a/app/components/valuation/budgets/index_component.rb b/app/components/valuation/budgets/index_component.rb
index 40138ac88..e8afdfce6 100644
--- a/app/components/valuation/budgets/index_component.rb
+++ b/app/components/valuation/budgets/index_component.rb
@@ -1,7 +1,7 @@
 class Valuation::Budgets::IndexComponent < ApplicationComponent
-  attr_reader :budget
+  attr_reader :budgets
 
-  def initialize(budget)
-    @budget = budget
+  def initialize(budgets)
+    @budgets = budgets
   end
 end
diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb
index 46fff464a..da8e3f107 100644
--- a/app/components/valuation/budgets/row_component.rb
+++ b/app/components/valuation/budgets/row_component.rb
@@ -1,9 +1,10 @@
 class Valuation::Budgets::RowComponent < ApplicationComponent
   attr_reader :budget
+  with_collection_parameter :budget
 
   delegate :current_user, to: :helpers
 
-  def initialize(budget)
+  def initialize(budget:)
     @budget = budget
   end
 
diff --git a/app/controllers/valuation/budgets_controller.rb b/app/controllers/valuation/budgets_controller.rb
index a30ccc55e..729c39112 100644
--- a/app/controllers/valuation/budgets_controller.rb
+++ b/app/controllers/valuation/budgets_controller.rb
@@ -5,6 +5,6 @@ class Valuation::BudgetsController < Valuation::BaseController
   load_and_authorize_resource
 
   def index
-    @budget = current_budget
+    @budgets = @budgets.published.order(created_at: :desc)
   end
 end
diff --git a/app/views/valuation/budgets/index.html.erb b/app/views/valuation/budgets/index.html.erb
index 5f2db605c..e435d1276 100644
--- a/app/views/valuation/budgets/index.html.erb
+++ b/app/views/valuation/budgets/index.html.erb
@@ -1 +1 @@
-<%= render Valuation::Budgets::IndexComponent.new(@budget) %>
+<%= render Valuation::Budgets::IndexComponent.new(@budgets) %>
diff --git a/spec/system/valuation/budgets_spec.rb b/spec/system/valuation/budgets_spec.rb
index c6683268d..88b6f52e2 100644
--- a/spec/system/valuation/budgets_spec.rb
+++ b/spec/system/valuation/budgets_spec.rb
@@ -7,23 +7,14 @@ describe "Valuation budgets" do
   end
 
   context "Index" do
-    scenario "Displaying budgets" do
-      budget = create(:budget)
-      visit valuation_budgets_path
-
-      expect(page).to have_content(budget.name)
-    end
-
-    scenario "Filters by phase" do
-      budget1 = create(:budget, :finished)
-      budget2 = create(:budget, :finished)
-      budget3 = create(:budget, :accepting)
+    scenario "Displays published budgets" do
+      create(:budget, name: "Sports")
+      create(:budget, name: "Draft", published: false)
 
       visit valuation_budgets_path
 
-      expect(page).not_to have_content(budget1.name)
-      expect(page).not_to have_content(budget2.name)
-      expect(page).to have_content(budget3.name)
+      expect(page).to have_content("Sports")
+      expect(page).not_to have_content("Draft")
     end
 
     scenario "With no budgets" do

From 615b2491449e355e2c652b09ab8aee71ba33f5a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?=
 <15726+Senen@users.noreply.github.com>
Date: Fri, 13 Jan 2023 13:04:53 +0100
Subject: [PATCH 3/5] Count only the investments visible and assigned to
 current valuator

It has more sense to show the count of the investments the
valuator is going to find in the investments valuation page.
---
 .../valuation/budgets/row_component.html.erb  |  2 +-
 .../valuation/budgets/row_component.rb        |  4 ++-
 .../valuation/budgets/row_component_spec.rb   | 27 +++++++++++++++++++
 spec/system/valuation/budgets_spec.rb         |  5 +---
 4 files changed, 32 insertions(+), 6 deletions(-)
 create mode 100644 spec/components/valuation/budgets/row_component_spec.rb

diff --git a/app/components/valuation/budgets/row_component.html.erb b/app/components/valuation/budgets/row_component.html.erb
index be3fc52c5..7489b50af 100644
--- a/app/components/valuation/budgets/row_component.html.erb
+++ b/app/components/valuation/budgets/row_component.html.erb
@@ -5,7 +5,7 @@
   <td>
     <%= budget.current_phase.name %>
   </td>
-  <td>
+  <td class="investments-count">
     <%= investments_count %>
   </td>
   <td>
diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb
index da8e3f107..feac0f4c2 100644
--- a/app/components/valuation/budgets/row_component.rb
+++ b/app/components/valuation/budgets/row_component.rb
@@ -9,6 +9,8 @@ class Valuation::Budgets::RowComponent < ApplicationComponent
   end
 
   def investments_count
-    budget.investments.by_valuator(current_user.valuator).valuation_open.count
+    return 0 unless budget.valuating?
+
+    budget.investments.visible_to_valuators.by_valuator(current_user.valuator).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
new file mode 100644
index 000000000..51a3f7671
--- /dev/null
+++ b/spec/components/valuation/budgets/row_component_spec.rb
@@ -0,0 +1,27 @@
+require "rails_helper"
+
+describe Valuation::Budgets::RowComponent do
+  let(:valuator) { create(:valuator) }
+
+  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)
+
+    render_inline Valuation::Budgets::RowComponent.new(budget: budget)
+
+    expect(page).to have_selector(".investments-count", text: "1")
+  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])
+
+    render_inline Valuation::Budgets::RowComponent.new(budget: budget)
+
+    expect(page).to have_selector(".investments-count", text: "0")
+  end
+end
diff --git a/spec/system/valuation/budgets_spec.rb b/spec/system/valuation/budgets_spec.rb
index 88b6f52e2..4233cd9d1 100644
--- a/spec/system/valuation/budgets_spec.rb
+++ b/spec/system/valuation/budgets_spec.rb
@@ -1,10 +1,7 @@
 require "rails_helper"
 
 describe "Valuation budgets" do
-  before do
-    valuator = create(:valuator, user: create(:user, username: "Rachel", email: "rachel@valuators.org"))
-    login_as(valuator.user)
-  end
+  before { login_as(create(:valuator).user) }
 
   context "Index" do
     scenario "Displays published budgets" do

From 0c09fd22af5007662917238bc453d10c4ee3afdf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?=
 <15726+Senen@users.noreply.github.com>
Date: Fri, 13 Jan 2023 13:51:32 +0100
Subject: [PATCH 4/5] Do not show the `Evaluate` link when there are no
 projects to evaluate for current valuator

---
 .../valuation/budgets/row_component.html.erb  | 10 ++++----
 .../valuation/budgets/row_component.rb        |  6 ++---
 .../valuation/budgets/row_component_spec.rb   | 23 +++++++++++++++++++
 spec/system/admin/budget_investments_spec.rb  |  1 +
 4 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/app/components/valuation/budgets/row_component.html.erb b/app/components/valuation/budgets/row_component.html.erb
index 7489b50af..5ba9933f8 100644
--- a/app/components/valuation/budgets/row_component.html.erb
+++ b/app/components/valuation/budgets/row_component.html.erb
@@ -6,11 +6,13 @@
     <%= budget.current_phase.name %>
   </td>
   <td class="investments-count">
-    <%= investments_count %>
+    <%= investments.count %>
   </td>
   <td>
-    <%= link_to t("valuation.budgets.index.evaluate"),
-                valuation_budget_budget_investments_path(budget_id: budget.id),
-                class: "button hollow expanded" %>
+    <% if investments.any? %>
+      <%= link_to t("valuation.budgets.index.evaluate"),
+                  valuation_budget_budget_investments_path(budget_id: budget.id),
+                  class: "button hollow expanded" %>
+    <% end %>
   </td>
 </tr>
diff --git a/app/components/valuation/budgets/row_component.rb b/app/components/valuation/budgets/row_component.rb
index feac0f4c2..4b75b196e 100644
--- a/app/components/valuation/budgets/row_component.rb
+++ b/app/components/valuation/budgets/row_component.rb
@@ -8,9 +8,9 @@ class Valuation::Budgets::RowComponent < ApplicationComponent
     @budget = budget
   end
 
-  def investments_count
-    return 0 unless budget.valuating?
+  def investments
+    return Budget::Investment.none unless budget.valuating?
 
-    budget.investments.visible_to_valuators.by_valuator(current_user.valuator).valuation_open.count
+    budget.investments.visible_to_valuators.by_valuator(current_user.valuator).valuation_open
   end
 end
diff --git a/spec/components/valuation/budgets/row_component_spec.rb b/spec/components/valuation/budgets/row_component_spec.rb
index 51a3f7671..4d0f1a7af 100644
--- a/spec/components/valuation/budgets/row_component_spec.rb
+++ b/spec/components/valuation/budgets/row_component_spec.rb
@@ -24,4 +24,27 @@ describe Valuation::Budgets::RowComponent do
 
     expect(page).to have_selector(".investments-count", text: "0")
   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]
+
+    render_inline Valuation::Budgets::RowComponent.with_collection(budgets)
+
+    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")
+  end
 end
diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb
index c0de69ff7..affd87411 100644
--- a/spec/system/admin/budget_investments_spec.rb
+++ b/spec/system/admin/budget_investments_spec.rb
@@ -1567,6 +1567,7 @@ describe "Admin budget investments", :admin do
     end
 
     scenario "Shows the correct investments to valuators" do
+      budget.update!(phase: :valuating)
       investment1.update!(visible_to_valuators: true)
       investment2.update!(visible_to_valuators: false)
 

From cdd26dd568a35c59ac7f8902df041a07f6c11036 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sen=C3=A9n=20Rodero=20Rodr=C3=ADguez?=
 <15726+Senen@users.noreply.github.com>
Date: Fri, 13 Jan 2023 15:29:37 +0100
Subject: [PATCH 5/5] Fix access restriction in valuation budget investments
 controller

Since we allow many active budgets at the same time, the
controller should now check the budget given by params.

Before this change the controller was checking the latest
published budget, ignoring the request parameter `budget_id`.
---
 .../valuation/budget_investments_controller.rb        |  4 ++--
 spec/system/valuation/budget_investments_spec.rb      | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/app/controllers/valuation/budget_investments_controller.rb b/app/controllers/valuation/budget_investments_controller.rb
index 6fe23df54..f1ff5d834 100644
--- a/app/controllers/valuation/budget_investments_controller.rb
+++ b/app/controllers/valuation/budget_investments_controller.rb
@@ -4,9 +4,9 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController
 
   feature_flag :budgets
 
+  before_action :load_budget
   before_action :restrict_access_to_assigned_items, only: [:show, :edit, :valuate]
   before_action :restrict_access, only: [:edit, :valuate]
-  before_action :load_budget
   before_action :load_investment, only: [:show, :edit, :valuate]
 
   has_orders %w[oldest], only: [:show, :edit]
@@ -110,7 +110,7 @@ class Valuation::BudgetInvestmentsController < Valuation::BaseController
     end
 
     def restrict_access
-      unless current_user.administrator? || current_budget.valuating?
+      unless current_user.administrator? || @budget.valuating?
         raise CanCan::AccessDenied, I18n.t("valuation.budget_investments.not_in_valuating_phase")
       end
     end
diff --git a/spec/system/valuation/budget_investments_spec.rb b/spec/system/valuation/budget_investments_spec.rb
index 69a5188d0..e02ea6b5d 100644
--- a/spec/system/valuation/budget_investments_spec.rb
+++ b/spec/system/valuation/budget_investments_spec.rb
@@ -501,6 +501,17 @@ describe "Valuation budget investments" do
       expect(page).to have_content("Investments can only be valuated when Budget is in valuating phase")
     end
 
+    scenario "restric access to the budget given by params when is not in valuating phase" do
+      budget.update!(phase: "publishing_prices")
+      create(:budget, :valuating)
+      investment = create(:budget_investment, budget: budget, valuators: [valuator])
+
+      login_as(valuator.user)
+      visit edit_valuation_budget_budget_investment_path(budget, investment)
+
+      expect(page).to have_content("Investments can only be valuated when Budget is in valuating phase")
+    end
+
     scenario "visible to admins regardless of not being in valuating phase" do
       budget.update!(phase: "publishing_prices")