diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-03-12 14:01:06 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-03-12 14:01:06 +0000 |
commit | 8ee1c0ffe538f1b0a4eecf859c3e79724ffae5ef (patch) | |
tree | c5cee2ec002cbfad3f70dbc4594581dbdce3f096 | |
parent | 0dae84200529dd97a0a3db3e7d56b7c33108fbe5 (diff) | |
parent | 01f6db4f643380d143772958b22d3f318b1db3a7 (diff) | |
download | gitlab-ce-8ee1c0ffe538f1b0a4eecf859c3e79724ffae5ef.tar.gz |
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
-rw-r--r-- | app/models/note.rb | 7 | ||||
-rw-r--r-- | db/migrate/20160307221555_disallow_blank_line_code_on_note.rb | 9 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 8 |
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 |