summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <rspeicher@gmail.com>2016-03-11 17:46:50 -0500
committerRobert Speicher <rspeicher@gmail.com>2016-03-11 17:46:50 -0500
commit01f6db4f643380d143772958b22d3f318b1db3a7 (patch)
tree26fd4477e1ffc4149bece7d72687779de94d68a0
parent70bf6dc702b6354c3a00d0b81e7d7c10be25ffb8 (diff)
downloadgitlab-ce-rs-disallow-blank-line-code.tar.gz
Disallow blank (non-null) values for a Note's `line_code` attributers-disallow-blank-line-code
It's unclear how these blank values got added, but GitLab.com had a few: ``` irb(main):002:0> Note.where("line_code IS NOT NULL AND line_code = ''").count => 439 ``` We've added a migration to convert any existing records to use a NULL value when blank, and updated Note to set blank values to nil before validation.
-rw-r--r--app/models/note.rb7
-rw-r--r--db/migrate/20160307221555_disallow_blank_line_code_on_note.rb9
-rw-r--r--spec/models/note_spec.rb8
3 files changed, 23 insertions, 1 deletions
diff --git a/app/models/note.rb b/app/models/note.rb
index 8b0610ff77e..2e084b5c80c 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -44,6 +44,7 @@ class Note < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true
before_validation :set_award!
+ before_validation :clear_blank_line_code!
validates :note, :project, presence: true
validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award }
@@ -63,7 +64,7 @@ class Note < ActiveRecord::Base
scope :nonawards, ->{ where(is_award: false) }
scope :for_commit_id, ->(commit_id) { where(noteable_type: "Commit", commit_id: commit_id) }
scope :inline, ->{ where("line_code IS NOT NULL") }
- scope :not_inline, ->{ where(line_code: [nil, '']) }
+ scope :not_inline, ->{ where(line_code: nil) }
scope :system, ->{ where(system: true) }
scope :user, ->{ where(system: false) }
scope :common, ->{ where(noteable_type: ["", nil]) }
@@ -375,6 +376,10 @@ class Note < ActiveRecord::Base
private
+ def clear_blank_line_code!
+ self.line_code = nil if self.line_code.blank?
+ end
+
def awards_supported?
(for_issue? || for_merge_request?) && !for_diff_line?
end
diff --git a/db/migrate/20160307221555_disallow_blank_line_code_on_note.rb b/db/migrate/20160307221555_disallow_blank_line_code_on_note.rb
new file mode 100644
index 00000000000..49e787d9a9a
--- /dev/null
+++ b/db/migrate/20160307221555_disallow_blank_line_code_on_note.rb
@@ -0,0 +1,9 @@
+class DisallowBlankLineCodeOnNote < ActiveRecord::Migration
+ def up
+ execute("UPDATE notes SET line_code = NULL WHERE line_code = ''")
+ end
+
+ def down
+ # noop
+ end
+end
diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb
index cd620ea5440..b854de1d3d5 100644
--- a/spec/models/note_spec.rb
+++ b/spec/models/note_spec.rb
@@ -226,4 +226,12 @@ describe Note, models: true do
expect(note.is_award?).to be_falsy
end
end
+
+ describe 'clear_blank_line_code!' do
+ it 'clears a blank line code before validation' do
+ note = build(:note, line_code: ' ')
+
+ expect { note.valid? }.to change(note, :line_code).to(nil)
+ end
+ end
end