Create system notes for MR too, improve doc + clean up code

7 files changed, 28 insertions, 32 deletions

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index bb0c1869955..ef7d047b1ad 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -75,12 +75,7 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def authorize_create_note!
-    noteable_type = note_params[:noteable_type]
-
-    return unless %w[MergeRequest Issue].include?(noteable_type)
-    return access_denied! unless can?(current_user, :create_note, project)
-
-    noteable = noteable_type.constantize.find(note_params[:noteable_id])
+    return unless noteable.lockable?
     access_denied! unless can?(current_user, :create_note, noteable)
   end
 end
diff --git a/app/models/concerns/noteable.rb b/app/models/concerns/noteable.rb
index 1c4ddabcad5..5d75b2aa6a3 100644
--- a/app/models/concerns/noteable.rb
+++ b/app/models/concerns/noteable.rb
@@ -74,4 +74,8 @@ module Noteable
   def discussions_can_be_resolved_by?(user)
     discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) }
   end
+
+  def lockable?
+    [MergeRequest, Issue].include?(self.class)
+  end
 end
diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb
index 212f4989557..f0aa16d2ecf 100644
--- a/app/policies/issuable_policy.rb
+++ b/app/policies/issuable_policy.rb
@@ -1,7 +1,8 @@
 class IssuablePolicy < BasePolicy
   delegate { @subject.project }
 
-  condition(:locked) { @subject.discussion_locked? }
+  condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? }
+
   condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) }
 
   desc "User is the assignee or author"
@@ -16,5 +17,11 @@ class IssuablePolicy < BasePolicy
     enable :update_merge_request
   end
 
-  rule { locked & ~is_project_member }.prevent :create_note
+  rule { locked & ~is_project_member }.policy do
+    prevent :create_note
+    prevent :update_note
+    prevent :admin_note
+    prevent :resolve_note
+    prevent :edit_note
+  end
 end
diff --git a/app/policies/note_policy.rb b/app/policies/note_policy.rb
index 307c514a74b..d4cb5a77e63 100644
--- a/app/policies/note_policy.rb
+++ b/app/policies/note_policy.rb
@@ -1,19 +1,17 @@
 class NotePolicy < BasePolicy
   delegate { @subject.project }
+  delegate { @subject.noteable if @subject.noteable.lockable? }
 
   condition(:is_author) { @user && @subject.author == @user }
-  condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) }
   condition(:for_merge_request, scope: :subject) { @subject.for_merge_request? }
   condition(:is_noteable_author) { @user && @subject.noteable.author_id == @user.id }
 
   condition(:editable, scope: :subject) { @subject.editable? }
-  condition(:locked) { [MergeRequest, Issue].include?(@subject.noteable.class) && @subject.noteable.discussion_locked? }
 
   rule { ~editable | anonymous }.prevent :edit_note
 
   rule { is_author | admin }.enable :edit_note
   rule { can?(:master_access) }.enable :edit_note
-  rule { locked & ~is_author & ~is_project_member }.prevent :edit_note
 
   rule { is_author }.policy do
     enable :read_note
@@ -25,10 +23,4 @@ class NotePolicy < BasePolicy
   rule { for_merge_request & is_noteable_author }.policy do
     enable :resolve_note
   end
-
-  rule { locked & ~is_project_member }.policy do
-    prevent :update_note
-    prevent :admin_note
-    prevent :resolve_note
-  end
 end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index 40793201664..157539ee05b 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -43,6 +43,10 @@ class IssuableBaseService < BaseService
     SystemNoteService.change_time_spent(issuable, issuable.project, current_user)
   end
 
+  def create_discussion_lock_note(issuable)
+    SystemNoteService.discussion_lock(issuable, current_user)
+  end
+
   def filter_params(issuable)
     ability_name = :"admin_#{issuable.to_ability_name}"
 
@@ -236,6 +240,7 @@ class IssuableBaseService < BaseService
           handle_common_system_notes(issuable, old_labels: old_labels)
         end
 
+        change_discussion_lock(issuable)
         handle_changes(
           issuable,
           old_labels: old_labels,
@@ -292,6 +297,12 @@ class IssuableBaseService < BaseService
     end
   end
 
+  def change_discussion_lock(issuable)
+    if issuable.previous_changes.include?('discussion_locked')
+      create_discussion_lock_note(issuable)
+    end
+  end
+
   def toggle_award(issuable)
     award = params.delete(:emoji_award)
     if award
diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 2a24ee85c45..b4ca3966505 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -41,10 +41,6 @@ module Issues
         create_confidentiality_note(issue)
       end
 
-      if issue.previous_changes.include?('discussion_locked')
-        create_discussion_lock_note(issue)
-      end
-
       added_labels = issue.labels - old_labels
 
       if added_labels.present?
@@ -99,9 +95,5 @@ module Issues
     def create_confidentiality_note(issue)
       SystemNoteService.change_issue_confidentiality(issue, issue.project, current_user)
     end
-
-    def create_discussion_lock_note(issue)
-      SystemNoteService.discussion_lock(issue, current_user)
-    end
   end
 end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index cec0a1b6efa..7b32e215c7f 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -592,13 +592,8 @@ module SystemNoteService
   end
 
   def discussion_lock(issuable, author)
-    if issuable.discussion_locked
-      body = 'locked this issue'
-      action = 'locked'
-    else
-      body = 'unlocked this issue'
-      action = 'unlocked'
-    end
+    action = issuable.discussion_locked? ? 'locked' : 'unlocked'
+    body = "#{action} this issue"
 
     create_note(NoteSummary.new(issuable, issuable.project, author, body, action: action))
   end