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

22 files changed, 121 insertions, 54 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
diff --git a/changelogs/unreleased/18608-lock-issues.yml b/changelogs/unreleased/18608-lock-issues.yml
new file mode 100644
index 00000000000..6c0ae7cebf5
--- /dev/null
+++ b/changelogs/unreleased/18608-lock-issues.yml
@@ -0,0 +1,5 @@
+---
+title: Create system notes for MR too, improve doc + clean up code
+merge_request:
+author:
+type: added
diff --git a/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb
index bb60ac2a410..5bd777c53a0 100644
--- a/db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb
+++ b/db/migrate/20170815221154_add_discussion_locked_to_issuable.rb
@@ -1,8 +1,5 @@
-class AddDicussionLockedToIssuable < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
+class AddDiscussionLockedToIssuable < ActiveRecord::Migration
   DOWNTIME = false
-  disable_ddl_transaction!
 
   def up
     add_column(:merge_requests, :discussion_locked, :boolean)
diff --git a/db/schema.rb b/db/schema.rb
index 16f38f7b60b..6cdf929b1b6 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -660,7 +660,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do
     t.integer "cached_markdown_version"
     t.datetime "last_edited_at"
     t.integer "last_edited_by_id"
-    t.boolean "discussion_locked", default: false, null: false
+    t.boolean "discussion_locked"
   end
 
   add_index "issues", ["assignee_id"], name: "index_issues_on_assignee_id", using: :btree
@@ -883,7 +883,7 @@ ActiveRecord::Schema.define(version: 20170905112933) do
     t.integer "head_pipeline_id"
     t.boolean "ref_fetched"
     t.string "merge_jid"
-    t.boolean "discussion_locked", default: false, null: false
+    t.boolean "discussion_locked"
   end
 
   add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
diff --git a/doc/api/issues.md b/doc/api/issues.md
index 61e42345153..479f8754bcc 100644
--- a/doc/api/issues.md
+++ b/doc/api/issues.md
@@ -515,7 +515,7 @@ PUT /projects/:id/issues/:issue_iid
 | `state_event`  | string  | no       | The state event of an issue. Set `close` to close the issue and `reopen` to reopen it                      |
 | `updated_at`   | string  | no       | Date time string, ISO 8601 formatted, e.g. `2016-03-11T03:45:40Z` (requires admin or project owner rights) |
 | `due_date`     | string  | no       | Date time string in the format YEAR-MONTH-DAY, e.g. `2016-03-11`                                           |
-| `discussion_locked` | boolean | no  | Updates an issue to lock or unlock its discussion |
+| `discussion_locked` | boolean | no  | Flag indicating if the issue's discussion is locked. If the discussion is locked only project members can add or edit comments. |
 
 
 ```bash
diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md
index e5d1ebb9cfb..64daed7c326 100644
--- a/doc/api/merge_requests.md
+++ b/doc/api/merge_requests.md
@@ -504,7 +504,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid
 | `labels`               | string  | no       | Labels for MR as a comma-separated list                                         |
 | `milestone_id`         | integer | no       | The ID of a milestone                                                           |
 | `remove_source_branch` | boolean | no       | Flag indicating if a merge request should remove the source branch when merging |
-| `discussion_locked` | boolean | no | Flag indicating if the merge request's discussion is locked |
+| `discussion_locked`    | boolean | no       | Flag indicating if the merge request's discussion is locked. If the discussion is locked only project members can add, edit or resolve comments. |
 
 Must include at least one non-required attribute from above.
 
diff --git a/lib/api/issues.rb b/lib/api/issues.rb
index 88b592083db..0df41dcc903 100644
--- a/lib/api/issues.rb
+++ b/lib/api/issues.rb
@@ -48,7 +48,7 @@ module API
         optional :labels, type: String, desc: 'Comma-separated list of label names'
         optional :due_date, type: String, desc: 'Date string in the format YEAR-MONTH-DAY'
         optional :confidential, type: Boolean, desc: 'Boolean parameter if the issue should be confidential'
-        optional :discussion_locked, type: Boolean, desc: "Boolean parameter if the issue's discussion should be locked"
+        optional :discussion_locked, type: Boolean, desc: " Boolean parameter indicating if the issue's discussion is locked"
       end
 
       params :issue_params do
diff --git a/lib/api/notes.rb b/lib/api/notes.rb
index b3db366d875..0b9ab4eeb05 100644
--- a/lib/api/notes.rb
+++ b/lib/api/notes.rb
@@ -71,8 +71,6 @@ module API
         post ":id/#{noteables_str}/:noteable_id/notes" do
           noteable = find_project_noteable(noteables_str, params[:noteable_id])
 
-          authorize! :create_note, user_project
-
           opts = {
             note: params[:body],
             noteable_type: noteables_str.classify,
@@ -80,15 +78,12 @@ module API
           }
 
           if can?(current_user, noteable_read_ability_name(noteable), noteable)
+            authorize! :create_note, noteable
+
             if params[:created_at] && (current_user.admin? || user_project.owner == current_user)
               opts[:created_at] = params[:created_at]
             end
 
-            noteable_type = opts[:noteable_type].to_s
-            noteable = Issue.find(opts[:noteable_id]) if noteable_type == 'Issue'
-            noteable = MergeRequest.find(opts[:noteable_id]) if noteable_type == 'MergeRequest'
-            authorize! :create_note, noteable if noteable
-
             note = ::Notes::CreateService.new(user_project, current_user, opts).execute
 
             if note.valid?
diff --git a/spec/controllers/projects/notes_controller_spec.rb b/spec/controllers/projects/notes_controller_spec.rb
index 10edad462c1..6a6430dfc13 100644
--- a/spec/controllers/projects/notes_controller_spec.rb
+++ b/spec/controllers/projects/notes_controller_spec.rb
@@ -239,6 +239,15 @@ describe Projects::NotesController do
         merge_request.update_attribute(:discussion_locked, true)
       end
 
+      context 'when a noteable is not found' do
+        it 'returns 404 status' do
+          request_params[:note][:noteable_id] = 9999
+          post :create, request_params.merge(format: :json)
+
+          expect(response).to have_http_status(404)
+        end
+      end
+
       context 'when a user is a team member' do
         it 'returns 302 status for html' do
           post :create, request_params
diff --git a/spec/fixtures/api/schemas/public_api/v4/issues.json b/spec/fixtures/api/schemas/public_api/v4/issues.json
index 8c854a43fc6..b7d6dbff031 100644
--- a/spec/fixtures/api/schemas/public_api/v4/issues.json
+++ b/spec/fixtures/api/schemas/public_api/v4/issues.json
@@ -9,7 +9,7 @@
       "title": { "type": "string" },
       "description": { "type": ["string", "null"] },
       "state": { "type": "string" },
-      "discussion_locked": { "type": "boolean" },
+      "discussion_locked": { "type": ["boolean", "null"] },
       "created_at": { "type": "date" },
       "updated_at": { "type": "date" },
       "labels": {
diff --git a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
index 3b42333bb10..5828be5255b 100644
--- a/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
+++ b/spec/fixtures/api/schemas/public_api/v4/merge_requests.json
@@ -72,7 +72,7 @@
       "user_notes_count": { "type": "integer" },
       "should_remove_source_branch": { "type": ["boolean", "null"] },
       "force_remove_source_branch": { "type": ["boolean", "null"] },
-      "discussion_locked": { "type": "boolean" },
+      "discussion_locked": { "type": ["boolean", "null"] },
       "web_url": { "type": "uri" },
       "time_stats": {
         "time_estimate": { "type": "integer" },
diff --git a/spec/policies/issuable_policy_spec.rb b/spec/policies/issuable_policy_spec.rb
index 9b399d764ea..2cf669e8191 100644
--- a/spec/policies/issuable_policy_spec.rb
+++ b/spec/policies/issuable_policy_spec.rb
@@ -16,7 +16,7 @@ describe IssuablePolicy, models: true do
 
       context 'when the user is a project member' do
         before do
-          project.team << [user, :guest]
+          project.add_guest(user)
         end
 
         it 'can create a note' do
diff --git a/spec/policies/note_policy_spec.rb b/spec/policies/note_policy_spec.rb
index 70a99ed0198..58d36a2c84e 100644
--- a/spec/policies/note_policy_spec.rb
+++ b/spec/policies/note_policy_spec.rb
@@ -5,9 +5,15 @@ describe NotePolicy, mdoels: true do
     let(:user) { create(:user) }
     let(:project) { create(:project, :public) }
     let(:issue) { create(:issue, project: project) }
-    let(:note) { create(:note, noteable: issue, author: user, project: project) }
 
-    let(:policies) { described_class.new(user, note) }
+    def policies(noteable = nil)
+      return @policies if @policies
+
+      noteable ||= issue
+      note = create(:note, noteable: noteable, author: user, project: project)
+
+      @policies = described_class.new(user, note)
+    end
 
     context 'when the project is public' do
       context 'when the note author is not a project member' do
@@ -19,6 +25,17 @@ describe NotePolicy, mdoels: true do
         end
       end
 
+      context 'when the noteable is a snippet' do
+        it 'can edit note' do
+          policies = policies(create(:project_snippet, project: project))
+
+          expect(policies).to be_allowed(:update_note)
+          expect(policies).to be_allowed(:admin_note)
+          expect(policies).to be_allowed(:resolve_note)
+          expect(policies).to be_allowed(:read_note)
+        end
+      end
+
       context 'when a discussion is locked' do
         before do
           issue.update_attribute(:discussion_locked, true)
@@ -29,7 +46,7 @@ describe NotePolicy, mdoels: true do
             project.add_developer(user)
           end
 
-          it 'can eddit a note' do
+          it 'can edit a note' do
             expect(policies).to be_allowed(:update_note)
             expect(policies).to be_allowed(:admin_note)
             expect(policies).to be_allowed(:resolve_note)
diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb
index f5882c0c74a..fb440fa551c 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -302,6 +302,40 @@ describe API::Notes do
         expect(private_issue.notes.reload).to be_empty
       end
     end
+
+    context 'when the merge request discussion is locked' do
+      before do
+        merge_request.update_attribute(:discussion_locked, true)
+      end
+
+      context 'when a user is a team member' do
+        subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", user), body: 'Hi!' }
+
+        it 'returns 200 status' do
+          subject
+
+          expect(response).to have_http_status(201)
+        end
+
+        it 'creates a new note' do
+          expect { subject }.to change { Note.count }.by(1)
+        end
+      end
+
+      context 'when a user is not a team member' do
+        subject { post api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/notes", private_user), body: 'Hi!' }
+
+        it 'returns 403 status' do
+          subject
+
+          expect(response).to have_http_status(403)
+        end
+
+        it 'does not create a new note' do
+          expect { subject }.not_to change { Note.count }
+        end
+      end
+    end
   end
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb
index 0ed4c2152bb..bcce827fe71 100644
--- a/spec/services/issues/update_service_spec.rb
+++ b/spec/services/issues/update_service_spec.rb
@@ -144,6 +144,13 @@ describe Issues::UpdateService, :mailer do
           expect(note).not_to be_nil
           expect(note.note).to eq 'changed title from **{-Old-} title** to **{+New+} title**'
         end
+
+        it 'creates system note about discussion lock' do
+          note = find_note('locked this issue')
+
+          expect(note).not_to be_nil
+          expect(note.note).to eq 'locked this issue'
+        end
       end
     end
 
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index a07b7ac2218..b11a1b31f32 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -125,6 +125,13 @@ describe MergeRequests::UpdateService, :mailer do
         expect(note.note).to eq 'changed target branch from `master` to `target`'
       end
 
+      it 'creates system note about discussion lock' do
+        note = find_note('locked this issue')
+
+        expect(note).not_to be_nil
+        expect(note.note).to eq 'locked this issue'
+      end
+
       context 'when not including source branch removal options' do
         before do
           opts.delete(:force_remove_source_branch)