diff options
author | Douwe Maan <douwe@selenight.nl> | 2017-04-06 18:05:57 +0300 |
---|---|---|
committer | Douwe Maan <douwe@selenight.nl> | 2017-04-06 18:51:45 +0300 |
commit | cc656a11992483911cefe035d579096581cfde57 (patch) | |
tree | 5af4a41d9ae36b3900fdfa0b312b125995c8818c /app | |
parent | 64c1735c22871d94e0bda8b9f5aece2a29739236 (diff) |
Refactor resolvability checks based on type
Diffstat (limited to 'app')
-rw-r--r-- | app/models/concerns/issuable.rb | 11 | ||||
-rw-r--r-- | app/models/concerns/noteable.rb | 25 | ||||
-rw-r--r-- | app/models/concerns/resolvable_discussion.rb | 7 | ||||
-rw-r--r-- | app/models/concerns/resolvable_note.rb | 12 | ||||
-rw-r--r-- | app/models/diff_discussion.rb | 6 | ||||
-rw-r--r-- | app/models/diff_note.rb | 2 | ||||
-rw-r--r-- | app/models/discussion.rb | 6 | ||||
-rw-r--r-- | app/models/discussion_note.rb | 6 | ||||
-rw-r--r-- | app/models/individual_note_discussion.rb | 6 | ||||
-rw-r--r-- | app/models/legacy_diff_discussion.rb | 7 | ||||
-rw-r--r-- | app/models/legacy_diff_note.rb | 3 | ||||
-rw-r--r-- | app/models/note.rb | 9 | ||||
-rw-r--r-- | app/models/out_of_context_discussion.rb | 8 | ||||
-rw-r--r-- | app/services/issues/build_service.rb | 8 | ||||
-rw-r--r-- | app/views/projects/notes/_comment_button.html.haml | 28 | ||||
-rw-r--r-- | app/views/projects/notes/_comment_type_button.html.haml | 28 | ||||
-rw-r--r-- | app/views/projects/notes/_form.html.haml | 2 |
17 files changed, 107 insertions, 67 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index b4dded7e27e..3d2258d5e3e 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -292,17 +292,6 @@ module Issuable self.class.to_ability_name end - # Convert this Issuable class name to a format usable by notifications. - # - # Examples: - # - # issuable.class # => MergeRequest - # issuable.human_class_name # => "merge request" - - def human_class_name - @human_class_name ||= self.class.name.titleize.downcase - end - # Returns a Hash of attributes to be used for Twitter card metadata def card_attributes { diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb index 631df4757a4..772ff6a6d2f 100644 --- a/app/models/concerns/noteable.rb +++ b/app/models/concerns/noteable.rb @@ -1,4 +1,29 @@ module Noteable + # Names of all implementers of `Noteable` that support resolvable notes. + RESOLVABLE_TYPES = %w(MergeRequest).freeze + + def base_class_name + self.class.base_class.name + end + + # Convert this Noteable class name to a format usable by notifications. + # + # Examples: + # + # noteable.class # => MergeRequest + # noteable.human_class_name # => "merge request" + def human_class_name + @human_class_name ||= base_class_name.titleize.downcase + end + + def supports_resolvable_notes? + RESOLVABLE_TYPES.include?(base_class_name) + end + + def supports_discussions? + DiscussionNote::NOTEABLE_TYPES.include?(base_class_name) + end + def discussion_notes notes end diff --git a/app/models/concerns/resolvable_discussion.rb b/app/models/concerns/resolvable_discussion.rb index 5cb51196a94..dd979e7bb17 100644 --- a/app/models/concerns/resolvable_discussion.rb +++ b/app/models/concerns/resolvable_discussion.rb @@ -20,6 +20,8 @@ module ResolvableDiscussion :last_note ) + delegate :potentially_resolvable?, to: :first_note + delegate :resolved_at, :resolved_by, @@ -27,11 +29,6 @@ module ResolvableDiscussion allow_nil: true end - # Keep this method in sync with the `potentially_resolvable` scope on `ResolvableNote` - def potentially_resolvable? - for_merge_request? - end - def resolvable? return @resolvable if @resolvable.present? diff --git a/app/models/concerns/resolvable_note.rb b/app/models/concerns/resolvable_note.rb index 2c70678429a..05eb6f86704 100644 --- a/app/models/concerns/resolvable_note.rb +++ b/app/models/concerns/resolvable_note.rb @@ -1,6 +1,7 @@ module ResolvableNote extend ActiveSupport::Concern + # Names of all subclasses of `Note` that can be resolvable. RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze included do @@ -8,10 +9,8 @@ module ResolvableNote validates :resolved_by, presence: true, if: :resolved? - # Keep this scope in sync with the logic in `#potentially_resolvable?` in subclasses of `Discussion` that are resolvable. - # `RESOLVABLE_TYPES` should include names of all subclasses that are resolvable (where the method can return true), and - # the scope should also match the criteria `ResolvableDiscussion#potentially_resolvable?` puts on resolvability. - scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: 'MergeRequest') } + # Keep this scope in sync with `#potentially_resolvable?` + scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) } # Keep this scope in sync with `#resolvable?` scope :resolvable, -> { potentially_resolvable.user } @@ -31,7 +30,10 @@ module ResolvableNote end end - delegate :potentially_resolvable?, to: :to_discussion + # Keep this method in sync with the `potentially_resolvable` scope + def potentially_resolvable? + RESOLVABLE_TYPES.include?(self.class.name) && noteable.supports_resolvable_notes? + end # Keep this method in sync with the `resolvable` scope def resolvable? diff --git a/app/models/diff_discussion.rb b/app/models/diff_discussion.rb index 22912b30c6a..d9b7e484e0f 100644 --- a/app/models/diff_discussion.rb +++ b/app/models/diff_discussion.rb @@ -1,7 +1,13 @@ # A discussion on merge request or commit diffs consisting of `DiffNote` notes. +# +# A discussion of this type can be resolvable. class DiffDiscussion < Discussion include DiscussionOnDiff + def self.note_class + DiffNote + end + delegate :position, :original_position, diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 9185a5a0ad8..1523244f8a8 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -1,4 +1,6 @@ # A note on merge request or commit diffs +# +# A note of this type can be resolvable. class DiffNote < Note include NoteOnDiff diff --git a/app/models/discussion.rb b/app/models/discussion.rb index 8268a140403..2c0e6379e6a 100644 --- a/app/models/discussion.rb +++ b/app/models/discussion.rb @@ -1,4 +1,6 @@ # A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes. +# +# A discussion of this type can be resolvable. class Discussion include ResolvableDiscussion @@ -54,6 +56,10 @@ class Discussion nil end + def self.note_class + DiscussionNote + end + def initialize(notes, noteable = nil) @notes = notes @noteable = noteable diff --git a/app/models/discussion_note.rb b/app/models/discussion_note.rb index 337fbc8435c..e660b024083 100644 --- a/app/models/discussion_note.rb +++ b/app/models/discussion_note.rb @@ -1,7 +1,9 @@ -# A note in a non-diff discussion on an issue, merge request, commit, or snippet +# A note in a non-diff discussion on an issue, merge request, commit, or snippet. +# +# A note of this type can be resolvable. class DiscussionNote < Note + # Names of all implementers of `Noteable` that support discussions. NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze - RESOLVABLE_TYPES = %w(MergeRequest).freeze validates :noteable_type, inclusion: { in: NOTEABLE_TYPES } diff --git a/app/models/individual_note_discussion.rb b/app/models/individual_note_discussion.rb index 42318fa3d6e..c3f21c55240 100644 --- a/app/models/individual_note_discussion.rb +++ b/app/models/individual_note_discussion.rb @@ -1,8 +1,10 @@ # A discussion to wrap a single `Note` note on the root of an issue, merge request, # commit, or snippet, that is not displayed as a discussion. +# +# A discussion of this type is never resolvable. class IndividualNoteDiscussion < Discussion - def potentially_resolvable? - false + def self.note_class + Note end def individual_note? diff --git a/app/models/legacy_diff_discussion.rb b/app/models/legacy_diff_discussion.rb index 6515762c92d..cb2651a03f8 100644 --- a/app/models/legacy_diff_discussion.rb +++ b/app/models/legacy_diff_discussion.rb @@ -1,6 +1,9 @@ # A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes. +# # All new diff discussions are of the type `DiffDiscussion`, but any diff discussions created # before the introduction of the new implementation still use `LegacyDiffDiscussion`. +# +# A discussion of this type is never resolvable. class LegacyDiffDiscussion < Discussion include DiscussionOnDiff @@ -8,8 +11,8 @@ class LegacyDiffDiscussion < Discussion true end - def potentially_resolvable? - false + def self.note_class + LegacyDiffNote end def collapsed? diff --git a/app/models/legacy_diff_note.rb b/app/models/legacy_diff_note.rb index b9fddb2ea79..9a77557ebcd 100644 --- a/app/models/legacy_diff_note.rb +++ b/app/models/legacy_diff_note.rb @@ -1,6 +1,9 @@ # A note on merge request or commit diffs, using the legacy implementation. +# # All new diff notes are of the type `DiffNote`, but any diff notes created # before the introduction of the new implementation still use `LegacyDiffNote`. +# +# A note of this type is never resolvable. class LegacyDiffNote < Note include NoteOnDiff diff --git a/app/models/note.rb b/app/models/note.rb index cd4996bdbae..401e3d7bcbc 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -1,3 +1,6 @@ +# A note on the root of an issue, merge request, commit, or snippet. +# +# A note of this type is never resolvable. class Note < ActiveRecord::Base extend ActiveModel::Naming include Gitlab::CurrentSettings @@ -225,11 +228,7 @@ class Note < ActiveRecord::Base end def can_be_discussion_note? - DiscussionNote::NOTEABLE_TYPES.include?(self.noteable_type) && !part_of_discussion? - end - - def can_be_resolvable? - DiscussionNote::RESOLVABLE_TYPES.include?(self.noteable_type) + self.noteable.supports_discussions? && !part_of_discussion? end def discussion_class(noteable = nil) diff --git a/app/models/out_of_context_discussion.rb b/app/models/out_of_context_discussion.rb index 31c6d5cff8b..85794630f70 100644 --- a/app/models/out_of_context_discussion.rb +++ b/app/models/out_of_context_discussion.rb @@ -2,6 +2,8 @@ # contains that commit, they are displayed as if they were a discussion. # # This represents one of those discussions, consisting of `Note` notes. +# +# A discussion of this type is never resolvable. class OutOfContextDiscussion < Discussion # Returns an array of discussion ID components def self.build_discussion_id(note) @@ -13,8 +15,8 @@ class OutOfContextDiscussion < Discussion def self.override_discussion_id(note) discussion_id(note) end - - def potentially_resolvable? - false + + def self.note_class + Note end end diff --git a/app/services/issues/build_service.rb b/app/services/issues/build_service.rb index 7b5858a8ccb..3a4f7b159f1 100644 --- a/app/services/issues/build_service.rb +++ b/app/services/issues/build_service.rb @@ -36,12 +36,14 @@ module Issues def item_for_discussion(discussion) first_note_to_resolve = discussion.first_note_to_resolve || discussion.first_note - other_note_count = discussion.notes.size - 1 - note_url = Gitlab::UrlBuilder.build(first_note_to_resolve) is_very_first_note = first_note_to_resolve == discussion.first_note action = is_very_first_note ? "started" : "commented on" - + + note_url = Gitlab::UrlBuilder.build(first_note_to_resolve) + + other_note_count = discussion.notes.size - 1 + discussion_info = "- [ ] #{first_note_to_resolve.author.to_reference} #{action} a [discussion](#{note_url}): " discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0 diff --git a/app/views/projects/notes/_comment_button.html.haml b/app/views/projects/notes/_comment_button.html.haml new file mode 100644 index 00000000000..008363263b3 --- /dev/null +++ b/app/views/projects/notes/_comment_button.html.haml @@ -0,0 +1,28 @@ +- noteable_name = @note.noteable.human_class_name + +.pull-left.btn-group.append-right-10.comment-type-dropdown.js-comment-type-dropdown + %input.btn.btn-nr.btn-create.comment-btn.js-comment-button.js-comment-submit-button{ type: 'submit', value: 'Comment' } + + - if @note.can_be_discussion_note? + = button_tag type: 'button', class: 'btn btn-nr dropdown-toggle comment-btn js-note-new-discussion js-disable-on-submit', data: { 'dropdown-trigger' => '#resolvable-comment-menu' }, 'aria-label' => 'Open comment type dropdown' do + = icon('caret-down') + + %ul#resolvable-comment-menu.dropdown-menu{ data: { dropdown: true } } + %li#comment.droplab-item-selected{ data: { value: '', 'button-text' => 'Comment', 'secondary-button-text' => "Comment & close #{noteable_name}" } } + = icon('check') + .description + %strong Comment + %p + Add a general comment to this #{noteable_name}. + + %li.divider + + %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_name}" } } + = icon('check') + .description + %strong Start discussion + %p + = succeed '.' do + Discuss a specific suggestion or question + - if @note.noteable.supports_resolvable_notes? + that needs to be resolved diff --git a/app/views/projects/notes/_comment_type_button.html.haml b/app/views/projects/notes/_comment_type_button.html.haml deleted file mode 100644 index 1444a9d1b65..00000000000 --- a/app/views/projects/notes/_comment_type_button.html.haml +++ /dev/null @@ -1,28 +0,0 @@ -- noteable_type = @note.noteable_type - -.pull-left.btn-group.append-right-10.comment-type-dropdown.js-comment-type-dropdown - %input.btn.btn-nr.btn-create.comment-btn.js-comment-button.js-comment-submit-button{ type: 'submit', value: 'Comment' } - - if @note.can_be_discussion_note? - = button_tag type: 'button', class: 'btn btn-nr dropdown-toggle comment-btn js-note-new-discussion js-disable-on-submit', data: { 'dropdown-trigger' => '#resolvable-comment-menu' }, 'aria-label' => 'Open comment type dropdown' do - = icon('caret-down') - %ul#resolvable-comment-menu.dropdown-menu{ data: { dropdown: true } } - %li#comment.droplab-item-selected{ data: { value: '', 'button-text' => 'Comment', 'secondary-button-text' => "Comment & close #{noteable_type.titleize.downcase}" } } - = icon('check') - .description - %strong Comment - %p= "Add a general comment to this #{noteable_type.titleize.downcase}." - %li.divider - - if @note.can_be_resolvable? - %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_type.titleize.downcase}" } } - = icon('check') - .description - %strong Start discussion - %p - Discuss a specific suggestion or question that needs to be resolved. - - else - %li#discussion{ data: { value: 'DiscussionNote', 'button-text' => 'Start discussion', 'secondary-button-text' => "Start discussion & close #{noteable_type.titleize.downcase}"} } - = icon('check') - .description - %strong Start discussion - %p - Discuss a specific suggestion or question. diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml index 057c6801edf..0d835a9e949 100644 --- a/app/views/projects/notes/_form.html.haml +++ b/app/views/projects/notes/_form.html.haml @@ -28,7 +28,7 @@ .error-alert .note-form-actions.clearfix - = render partial: 'projects/notes/comment_type_button', locals: { show_close_button: true } + = render partial: 'projects/notes/comment_button' = yield(:note_actions) |