Commit 8ee1c0ff authored by Douwe Maan's avatar Douwe Maan

Merge branch 'rs-disallow-blank-line-code' into 'master'

Disallow blank (non-null) values for a Note's `line_code` attribute

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.

See merge request !3118
parents 0dae8420 01f6db4f
...@@ -44,6 +44,7 @@ class Note < ActiveRecord::Base ...@@ -44,6 +44,7 @@ class Note < ActiveRecord::Base
delegate :name, :email, to: :author, prefix: true delegate :name, :email, to: :author, prefix: true
before_validation :set_award! before_validation :set_award!
before_validation :clear_blank_line_code!
validates :note, :project, presence: true validates :note, :project, presence: true
validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award } validates :note, uniqueness: { scope: [:author, :noteable_type, :noteable_id] }, if: ->(n) { n.is_award }
...@@ -63,7 +64,7 @@ class Note < ActiveRecord::Base ...@@ -63,7 +64,7 @@ class Note < ActiveRecord::Base
scope :nonawards, ->{ where(is_award: false) } scope :nonawards, ->{ where(is_award: false) }
scope :for_commit_id, ->(commit_id) { where(noteable_type: "Commit", commit_id: commit_id) } scope :for_commit_id, ->(commit_id) { where(noteable_type: "Commit", commit_id: commit_id) }
scope :inline, ->{ where("line_code IS NOT NULL") } 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 :system, ->{ where(system: true) }
scope :user, ->{ where(system: false) } scope :user, ->{ where(system: false) }
scope :common, ->{ where(noteable_type: ["", nil]) } scope :common, ->{ where(noteable_type: ["", nil]) }
...@@ -375,6 +376,10 @@ class Note < ActiveRecord::Base ...@@ -375,6 +376,10 @@ class Note < ActiveRecord::Base
private private
def clear_blank_line_code!
self.line_code = nil if self.line_code.blank?
end
def awards_supported? def awards_supported?
(for_issue? || for_merge_request?) && !for_diff_line? (for_issue? || for_merge_request?) && !for_diff_line?
end end
......
class DisallowBlankLineCodeOnNote < ActiveRecord::Migration
def up
execute("UPDATE notes SET line_code = NULL WHERE line_code = ''")
end
def down
# noop
end
end
...@@ -226,4 +226,12 @@ describe Note, models: true do ...@@ -226,4 +226,12 @@ describe Note, models: true do
expect(note.is_award?).to be_falsy expect(note.is_award?).to be_falsy
end end
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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment