From 49808195288dd94f23ea42391325699361e51659 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 25 Feb 2019 12:18:24 +0100 Subject: [PATCH 1/2] Fix valuation tags being overwritten When params[:budget_investment][:valuation_tag_list] was not present, which is the case when updating an investment using the "mark as visible to valuators" checkbox, we were removing all valuation tags. Using a virtual attribute to assign the tags only if the parameter is present simplifies the code in the controller and avoids the issue. --- .../admin/budget_investments_controller.rb | 6 ------ app/models/budget/investment.rb | 4 ++++ spec/features/admin/budget_investments_spec.rb | 16 ++++++++++++++++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/budget_investments_controller.rb b/app/controllers/admin/budget_investments_controller.rb index 71c92aee9..7bf3a4bb4 100644 --- a/app/controllers/admin/budget_investments_controller.rb +++ b/app/controllers/admin/budget_investments_controller.rb @@ -38,7 +38,6 @@ class Admin::BudgetInvestmentsController < Admin::BaseController end def update - set_valuation_tags if @investment.update(budget_investment_params) redirect_to admin_budget_budget_investment_path(@budget, @investment, @@ -118,11 +117,6 @@ class Admin::BudgetInvestmentsController < Admin::BaseController @ballot = @budget.balloting? ? query.first_or_create : query.first_or_initialize end - def set_valuation_tags - @investment.set_tag_list_on(:valuation, budget_investment_params[:valuation_tag_list]) - params[:budget_investment] = params[:budget_investment].except(:valuation_tag_list) - end - def parse_valuation_filters if params[:valuator_or_group_id] model, id = params[:valuator_or_group_id].split("_") diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index a4d3be921..a040e48e9 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -349,6 +349,10 @@ class Budget self.valuator_groups.collect(&:name).compact.join(', ').presence end + def valuation_tag_list=(tags) + set_tag_list_on(:valuation, tags) + end + def self.with_milestone_status_id(status_id) joins(:milestones).includes(:milestones).select do |investment| investment.milestone_status_id == status_id.to_i diff --git a/spec/features/admin/budget_investments_spec.rb b/spec/features/admin/budget_investments_spec.rb index 4f7e56c3c..fc9d51ce1 100644 --- a/spec/features/admin/budget_investments_spec.rb +++ b/spec/features/admin/budget_investments_spec.rb @@ -1376,6 +1376,22 @@ feature "Admin budget investments" do expect(valuating_checkbox).not_to be_checked end end + + scenario "Keeps the valuation tags", :js do + investment1.set_tag_list_on(:valuation, %w[Possimpible Truthiness]) + investment1.save + + visit admin_budget_budget_investments_path(budget) + + within("#budget_investment_#{investment1.id}") do + check "budget_investment_visible_to_valuators" + end + + visit edit_admin_budget_budget_investment_path(budget, investment1) + + expect(page).to have_content "Possimpible" + expect(page).to have_content "Truthiness" + end end context "Selecting csv" do From c5c56ad969e34dd128333018bb65e54f13d905ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 25 Feb 2019 12:26:01 +0100 Subject: [PATCH 2/2] Use a virtual attribute to get valuation tags It was strange to set the valuation tags using `valuation_tag_list=` but then accessing the valuation tags using `tag_list_on(:valuation)`. --- app/models/budget/investment.rb | 4 ++++ app/views/admin/budget_investments/edit.html.erb | 2 +- spec/models/budget/investment_spec.rb | 8 ++++++++ 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/budget/investment.rb b/app/models/budget/investment.rb index a040e48e9..97c187966 100644 --- a/app/models/budget/investment.rb +++ b/app/models/budget/investment.rb @@ -349,6 +349,10 @@ class Budget self.valuator_groups.collect(&:name).compact.join(', ').presence end + def valuation_tag_list + tag_list_on(:valuation) + end + def valuation_tag_list=(tags) set_tag_list_on(:valuation, tags) end diff --git a/app/views/admin/budget_investments/edit.html.erb b/app/views/admin/budget_investments/edit.html.erb index fc56f88df..5c98f633b 100644 --- a/app/views/admin/budget_investments/edit.html.erb +++ b/app/views/admin/budget_investments/edit.html.erb @@ -57,7 +57,7 @@ <% end %> <%= f.text_field :valuation_tag_list, - value: @investment.tag_list_on(:valuation).sort.join(','), + value: @investment.valuation_tag_list.sort.join(','), label: false, placeholder: t("admin.budget_investments.edit.tags_placeholder"), class: 'js-tag-list' %> diff --git a/spec/models/budget/investment_spec.rb b/spec/models/budget/investment_spec.rb index c012ff5ca..7c2b0a980 100644 --- a/spec/models/budget/investment_spec.rb +++ b/spec/models/budget/investment_spec.rb @@ -642,6 +642,14 @@ describe Budget::Investment do results = described_class.search("Latin") expect(results.first).to eq(investment) end + + it "gets and sets valuation tags through virtual attributes" do + investment = create(:budget_investment) + + investment.valuation_tag_list = %w[Code Test Refactor] + + expect(investment.valuation_tag_list).to match_array(%w[Code Test Refactor]) + end end end