From 9176de949a972cee2176b43f9c4b7f5f4a074e0c Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 18:37:03 +0100 Subject: [PATCH 1/6] Refactor concept of current budget When there was only one budget this implementation worked fine Nowadays there can be multiple budgets, and therefore the definition of the current_budget has changed. It is no longer a budget that has not finished, but rather, the last budget created that is not in the initial drafting phase. Budgets in the drafting phase are not considered the current_budget, but rather a budget that is still being prepared and that soon will become the current_budget --- app/models/budget.rb | 8 +++----- spec/models/budget_spec.rb | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/models/budget.rb b/app/models/budget.rb index af1149d66..55c4de841 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -31,7 +31,9 @@ class Budget < ActiveRecord::Base scope :reviewing_ballots, -> { where(phase: "reviewing_ballots") } scope :finished, -> { where(phase: "finished") } - scope :current, -> { where.not(phase: "finished") } + def self.current + where.not(phase: "drafting").last + end def description send("description_#{phase}").try(:html_safe) @@ -93,10 +95,6 @@ class Budget < ActiveRecord::Base balloting_process? || finished? end - def current? - !finished? - end - def heading_price(heading) heading_ids.include?(heading.id) ? heading.price : -1 end diff --git a/spec/models/budget_spec.rb b/spec/models/budget_spec.rb index 70c292962..9bca092ea 100644 --- a/spec/models/budget_spec.rb +++ b/spec/models/budget_spec.rb @@ -98,6 +98,31 @@ describe Budget do end end + describe "#current" do + + it "returns nil if there is only one budget and it is still in drafting phase" do + budget = create(:budget, phase: "drafting") + + expect(Budget.current).to eq(nil) + end + + it "returns the budget if there is only one and not in drafting phase" do + budget = create(:budget, phase: "accepting") + + expect(Budget.current).to eq(budget) + end + + it "returns the last budget created that is not in drafting phase" do + old_budget = create(:budget, phase: "finished", created_at: 2.years.ago) + previous_budget = create(:budget, phase: "accepting", created_at: 1.year.ago) + current_budget = create(:budget, phase: "accepting", created_at: 1.month.ago) + next_budget = create(:budget, phase: "drafting", created_at: 1.week.ago) + + expect(Budget.current).to eq(current_budget) + end + + end + describe "heading_price" do let(:budget) { create(:budget) } let(:group) { create(:budget_group, budget: budget) } From be554a629c00ae2cb9943bf6cb148d317b2a70d7 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 18:37:32 +0100 Subject: [PATCH 2/6] Make current_budget accessible in controller and views --- app/controllers/application_controller.rb | 5 +++++ .../application_controller_spec.rb | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 spec/controllers/application_controller_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 495a25314..f03a05239 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -25,6 +25,7 @@ class ApplicationController < ActionController::Base layout :set_layout respond_to :html + helper_method :current_budget private @@ -120,4 +121,8 @@ class ApplicationController < ActionController::Base params[:filter] ||= "selected" end end + + def current_budget + Budget.current + end end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb new file mode 100644 index 000000000..f11068b65 --- /dev/null +++ b/spec/controllers/application_controller_spec.rb @@ -0,0 +1,19 @@ +require 'rails_helper' + +describe ApplicationController do + + describe "#current_budget" do + + it "returns the last budget that is not in draft phase" do + old_budget = create(:budget, phase: "finished", created_at: 2.years.ago) + previous_budget = create(:budget, phase: "accepting", created_at: 1.year.ago) + current_budget = create(:budget, phase: "accepting", created_at: 1.month.ago) + next_budget = create(:budget, phase: "drafting", created_at: 1.week.ago) + + budget = subject.instance_eval{ current_budget } + expect(budget).to eq(current_budget) + end + + end + +end \ No newline at end of file From 349780922d2dc1d132ac24fe2762645dddb82f54 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 20:23:46 +0100 Subject: [PATCH 3/6] Add Budget.open scope Before Budget.current could return multiple budgets, now there can only be a single current_budget. Adding the concept of open, which better reflects what the admin sees in this page: A tab for open budgets and a tab for finished budgets --- app/controllers/admin/budgets_controller.rb | 2 +- app/models/budget.rb | 1 + spec/models/budget_spec.rb | 12 ++++++++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/budgets_controller.rb b/app/controllers/admin/budgets_controller.rb index d2f8706c0..7dbd66d7b 100644 --- a/app/controllers/admin/budgets_controller.rb +++ b/app/controllers/admin/budgets_controller.rb @@ -2,7 +2,7 @@ class Admin::BudgetsController < Admin::BaseController include FeatureFlags feature_flag :budgets - has_filters %w{current finished}, only: :index + has_filters %w{open finished}, only: :index load_and_authorize_resource diff --git a/app/models/budget.rb b/app/models/budget.rb index 55c4de841..608599217 100644 --- a/app/models/budget.rb +++ b/app/models/budget.rb @@ -30,6 +30,7 @@ class Budget < ActiveRecord::Base scope :balloting, -> { where(phase: "balloting") } scope :reviewing_ballots, -> { where(phase: "reviewing_ballots") } scope :finished, -> { where(phase: "finished") } + scope :open, -> { where.not(phase: "finished") } def self.current where.not(phase: "drafting").last diff --git a/spec/models/budget_spec.rb b/spec/models/budget_spec.rb index 9bca092ea..f8ec2d5a5 100644 --- a/spec/models/budget_spec.rb +++ b/spec/models/budget_spec.rb @@ -123,6 +123,18 @@ describe Budget do end + describe "#open" do + + it "returns all budgets that are not in the finished phase" do + phases = Budget::PHASES - ["finished"] + phases.each do |phase| + budget = create(:budget, phase: phase) + expect(Budget.open).to include(budget) + end + end + + end + describe "heading_price" do let(:budget) { create(:budget) } let(:group) { create(:budget_group, budget: budget) } From 01ef4390535f65792c5cc636e8e69f4f214cb105 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 20:26:57 +0100 Subject: [PATCH 4/6] Display only current budget to Managers printing investments In the specs, some investment were missing a heading_id, thus creating another unexpected budget By explicitly setting the heading_id we can control better which budgets are created in each test --- app/controllers/management/budgets_controller.rb | 2 +- .../management/budgets/print_investments.html.erb | 10 ++++------ spec/features/management/budget_investments_spec.rb | 7 +++++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/app/controllers/management/budgets_controller.rb b/app/controllers/management/budgets_controller.rb index dd7259c99..470d1f9fd 100644 --- a/app/controllers/management/budgets_controller.rb +++ b/app/controllers/management/budgets_controller.rb @@ -19,7 +19,7 @@ class Management::BudgetsController < Management::BaseController end def print_investments - @budgets = Budget.current.order(created_at: :desc).page(params[:page]) + @budget = Budget.current end private diff --git a/app/views/management/budgets/print_investments.html.erb b/app/views/management/budgets/print_investments.html.erb index bc115ea3d..d5be7c1ff 100644 --- a/app/views/management/budgets/print_investments.html.erb +++ b/app/views/management/budgets/print_investments.html.erb @@ -1,12 +1,10 @@ -<% @budgets.each do |budget| %> - - - + + + -<% end %>
<%= budget.name %><%= budget.translated_phase %>
<%= @budget.name %><%= @budget.translated_phase %> <%= link_to t("management.budgets.print_investments"), - print_management_budget_investments_path(budget) %> + print_management_budget_investments_path(@budget) %>
diff --git a/spec/features/management/budget_investments_spec.rb b/spec/features/management/budget_investments_spec.rb index f1ced7334..6132c40cd 100644 --- a/spec/features/management/budget_investments_spec.rb +++ b/spec/features/management/budget_investments_spec.rb @@ -259,9 +259,10 @@ feature 'Budget Investments' do context "Printing" do scenario 'Printing budget investments' do - 16.times { create(:budget_investment, budget: @budget) } + 16.times { create(:budget_investment, budget: @budget, heading: @heading) } click_link "Print Budget Investments" + expect(page).to have_content(@budget.name) within "#budget_#{@budget.id}" do click_link "Print Budget Investments" @@ -273,15 +274,17 @@ feature 'Budget Investments' do scenario "Filtering budget investments by heading to be printed", :js do district_9 = create(:budget_heading, group: @group, name: "District Nine") + another_heading = create(:budget_heading, group: @group) low_investment = create(:budget_investment, budget: @budget, title: 'Nuke district 9', heading: district_9, cached_votes_up: 1) mid_investment = create(:budget_investment, budget: @budget, title: 'Change district 9', heading: district_9, cached_votes_up: 10) top_investment = create(:budget_investment, budget: @budget, title: 'Destroy district 9', heading: district_9, cached_votes_up: 100) - unvoted_investment = create(:budget_investment, budget: @budget, title: 'Add new districts to the city') + unvoted_investment = create(:budget_investment, budget: @budget, heading: another_heading, title: 'Add new districts to the city') user = create(:user, :level_two) login_managed_user(user) click_link "Print Budget Investments" + expect(page).to have_content(@budget.name) within "#budget_#{@budget.id}" do click_link "Print Budget Investments" From 5086314bee20ec0a61065c498c7fe8b3855a2d52 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 20:29:49 +0100 Subject: [PATCH 5/6] Display only current budget for Valuators Before we could have multiple current budgets, as we now only have one current_budget, some specs broke. As there is no need to display multiple budgets to Valuators, only the current budget is necessary, we can remove arrays and assume that only a single budget, the current budget, is displayed to Valuators --- .../valuation/budgets_controller.rb | 8 ++-- app/views/valuation/budgets/index.html.erb | 38 ++++++++----------- spec/features/valuation/budgets_spec.rb | 15 +++----- 3 files changed, 26 insertions(+), 35 deletions(-) diff --git a/app/controllers/valuation/budgets_controller.rb b/app/controllers/valuation/budgets_controller.rb index 744b0d4bf..cf99f95e3 100644 --- a/app/controllers/valuation/budgets_controller.rb +++ b/app/controllers/valuation/budgets_controller.rb @@ -5,10 +5,10 @@ class Valuation::BudgetsController < Valuation::BaseController load_and_authorize_resource def index - @budgets = @budgets.current.order(created_at: :desc).page(params[:page]) - @investments_with_valuation_open = {} - @budgets.each do |b| - @investments_with_valuation_open[b.id] = b.investments + @budget = Budget.current + if @budget.present? + @investments_with_valuation_open = {} + @investments_with_valuation_open = @budget.investments .by_valuator(current_user.valuator.try(:id)) .valuation_open .count diff --git a/app/views/valuation/budgets/index.html.erb b/app/views/valuation/budgets/index.html.erb index 6d65f9ee6..f1fd302b6 100644 --- a/app/views/valuation/budgets/index.html.erb +++ b/app/views/valuation/budgets/index.html.erb @@ -1,7 +1,5 @@

<%= t("valuation.budgets.index.title") %>

-

<%= page_entries_info @budgets %>

- @@ -12,25 +10,21 @@ - <% @budgets.each do |budget| %> - - - - - - - <% end %> + + + + + +
- <%= budget.name %> - - <%= t("budgets.phase.#{budget.phase}") %> - - <%= @investments_with_valuation_open[budget.id] %> - - <%= link_to t("valuation.budgets.index.evaluate"), - valuation_budget_budget_investments_path(budget_id: budget.id), - class: "button hollow expanded" %> -
+ <%= @budget.name %> + + <%= t("budgets.phase.#{@budget.phase}") %> + + <%= @investments_with_valuation_open %> + + <%= link_to t("valuation.budgets.index.evaluate"), + valuation_budget_budget_investments_path(budget_id: @budget.id), + class: "button hollow expanded" %> +
- -<%= paginate @budgets %> diff --git a/spec/features/valuation/budgets_spec.rb b/spec/features/valuation/budgets_spec.rb index ec3b780ab..4fd821c7e 100644 --- a/spec/features/valuation/budgets_spec.rb +++ b/spec/features/valuation/budgets_spec.rb @@ -24,18 +24,15 @@ feature 'Valuation budgets' do end scenario 'Filters by phase' do - budget1 = create(:budget) - budget2 = create(:budget, :accepting) - budget3 = create(:budget, :selecting) - budget4 = create(:budget, :balloting) - budget5 = create(:budget, :finished) + budget1 = create(:budget, :finished) + budget2 = create(:budget, :finished) + budget3 = create(:budget, :accepting) visit valuation_budgets_path - expect(page).to have_content(budget1.name) - expect(page).to have_content(budget2.name) + + expect(page).to_not have_content(budget1.name) + expect(page).to_not have_content(budget2.name) expect(page).to have_content(budget3.name) - expect(page).to have_content(budget4.name) - expect(page).not_to have_content(budget5.name) end end From 34e0c23bb37bbd0c7b9b17d26a71d6adec9802e2 Mon Sep 17 00:00:00 2001 From: rgarcia Date: Mon, 15 Jan 2018 20:34:30 +0100 Subject: [PATCH 6/6] Fix valuators authorization spec This spec used to pass, because even though there were no budgets, as Budget.current returned an array, it gracefully handled situations without budgets Now we assume that there can only be a single current budget, and so calling any method of budget will raise an exception unless there is a current budget present Valuators should not access this page when there is no budget present, however it might be wise to create an issue to cover this case, just in case --- spec/features/valuation_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/features/valuation_spec.rb b/spec/features/valuation_spec.rb index c004f0773..1cb95d23d 100644 --- a/spec/features/valuation_spec.rb +++ b/spec/features/valuation_spec.rb @@ -66,6 +66,8 @@ feature 'Valuation' do scenario 'Access as a valuator is authorized' do create(:valuator, user: user) + create(:budget) + login_as(user) visit root_path @@ -78,6 +80,8 @@ feature 'Valuation' do scenario 'Access as an administrator is authorized' do create(:administrator, user: user) + create(:budget) + login_as(user) visit root_path @@ -90,6 +94,8 @@ feature 'Valuation' do scenario "Valuation access links" do create(:valuator, user: user) + create(:budget) + login_as(user) visit root_path @@ -100,6 +106,8 @@ feature 'Valuation' do scenario 'Valuation dashboard' do create(:valuator, user: user) + create(:budget) + login_as(user) visit root_path