Merge pull request #3330 from consul/backport-fix_valuation_tags

Fix valuation tags being overwritten

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("_")

app/models/budget/investment.rb

@@ -349,6 +349,14 @@ 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
+
     def self.with_milestone_status_id(status_id)
       joins(:milestones).includes(:milestones).select do |investment|
         investment.milestone_status_id == status_id.to_i

app/views/admin/budget_investments/edit.html.erb

@@ -57,7 +57,7 @@
         <% end %>
       </div>
       <%= 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' %>

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

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