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

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

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

app/policies/issuable_policy.rb

 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

app/policies/note_policy.rb

 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

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

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

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

changelogs/unreleased/18608-lock-issues.yml

+---
+title: Create system notes for MR too, improve doc + clean up code
+merge_request:
+author:
+type: added

db/migrate/20161207221154_add_dicussion_locked_to_issuable.rb → db/migrate/20170815221154_add_discussion_locked_to_issuable.rb

-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)

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

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

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.

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

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?

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

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": {

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" },

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

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)

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