Strip attributes for Milestone and Issuable. #3428

7 files changed, 62 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 0e1e1a3671d..39a57231d9c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.3.0 (unreleased)
   - Fix: Assignee selector is empty when 'Unassigned' is selected (Jose Corcuera)
   - Fix 500 error when update group member permission
   - Fix: Raw private snippets access workflow
+  - Trim leading and trailing whitespace of milestone and issueable titles (Jose Corcuera)
 
 v 8.2.1
   - Forcefully update builds that didn't want to update with state machine
diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 5250a0f5e67..ae474cf8d68 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -158,12 +158,10 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def issue_params
-    permitted = params.require(:issue).permit(
+    params.require(:issue).permit(
       :title, :assignee_id, :position, :description,
       :milestone_id, :state_event, :task_num, label_ids: []
     )
-    params[:issue][:title].strip! if params[:issue][:title]
-    permitted
   end
 
   def bulk_update_params
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 6378a1f56b0..3f47f2ddb2c 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -276,13 +276,11 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def merge_request_params
-    permitted = params.require(:merge_request).permit(
+    params.require(:merge_request).permit(
       :title, :assignee_id, :source_project_id, :source_branch,
       :target_project_id, :target_branch, :milestone_id,
       :state_event, :description, :task_num, label_ids: []
     )
-    params[:merge_request][:title].strip! if params[:merge_request][:title]
-    permitted
   end
 
   # Make sure merge requests created before 8.0
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index 68138688aab..badeadfa418 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -8,6 +8,7 @@ module Issuable
   extend ActiveSupport::Concern
   include Participable
   include Mentionable
+  include StripAttribute
 
   included do
     belongs_to :author, class_name: "User"
@@ -51,6 +52,7 @@ module Issuable
 
     attr_mentionable :title, :description
     participant :author, :assignee, :notes_with_associations
+    strip_attributes :title
   end
 
   module ClassMethods
diff --git a/app/models/concerns/strip_attribute.rb b/app/models/concerns/strip_attribute.rb
new file mode 100644
index 00000000000..8806ebe897a
--- /dev/null
+++ b/app/models/concerns/strip_attribute.rb
@@ -0,0 +1,34 @@
+# == Strip Attribute module
+#
+# Contains functionality to clean attributes before validation
+#
+# Usage:
+#
+#     class Milestone < ActiveRecord::Base
+#       strip_attributes :title
+#     end
+#
+#
+module StripAttribute
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    def strip_attributes(*attrs)
+      strip_attrs.concat(attrs)
+    end
+
+    def strip_attrs
+      @strip_attrs ||= []
+    end
+  end
+
+  included do
+    before_validation :strip_attributes
+  end
+
+  def strip_attributes
+    self.class.strip_attrs.each do |attr|
+      self[attr].strip! if self[attr] && self[attr].respond_to?(:strip!)
+    end
+  end
+end
diff --git a/app/models/milestone.rb b/app/models/milestone.rb
index 2ff16e2825c..c2642b75b8a 100644
--- a/app/models/milestone.rb
+++ b/app/models/milestone.rb
@@ -22,6 +22,7 @@ class Milestone < ActiveRecord::Base
 
   include InternalId
   include Sortable
+  include StripAttribute
 
   belongs_to :project
   has_many :issues
@@ -35,6 +36,8 @@ class Milestone < ActiveRecord::Base
   validates :title, presence: true
   validates :project, presence: true
 
+  strip_attributes :title
+
   state_machine :state, initial: :active do
     event :close do
       transition active: :closed
diff --git a/spec/models/concerns/strip_attribute_spec.rb b/spec/models/concerns/strip_attribute_spec.rb
new file mode 100644
index 00000000000..6445e29c3ef
--- /dev/null
+++ b/spec/models/concerns/strip_attribute_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+describe Milestone, "StripAttribute" do
+  let(:milestone) { create(:milestone) }
+
+  describe ".strip_attributes" do
+    it { expect(Milestone).to respond_to(:strip_attributes) }
+    it { expect(Milestone.strip_attrs).to include(:title) }
+  end
+
+  describe "#strip_attributes" do
+    before do
+      milestone.title = '    8.3   '
+      milestone.valid?
+    end
+
+    it { expect(milestone.title).to eq('8.3') }
+  end
+
+end