Fix quick actions for users who cannot update issues and MRs

There are several quick actions now that don't need this access - /todo and
/unsubscribe for instance - but when these were first added, there
weren't. Quick actions are now responsible for checking their own permissions.

app/helpers/notes_helper.rb

@@ -11,7 +11,7 @@ module NotesHelper
   end
 
   def note_supports_quick_actions?(note)
-    Notes::QuickActionsService.supported?(note, current_user)
+    Notes::QuickActionsService.supported?(note)
   end
 
   def noteable_json(noteable)

app/services/notes/quick_actions_service.rb

@@ -9,14 +9,12 @@ module Notes
       UPDATE_SERVICES[note.noteable_type]
     end
 
-    def self.supported?(note, current_user)
+    def self.supported?(note)
-      noteable_update_service(note) &&
+      !!noteable_update_service(note)
-        current_user &&
-        current_user.can?(:"update_#{note.to_ability_name}", note.noteable)
     end
 
     def supported?(note)
-      self.class.supported?(note, current_user)
+      self.class.supported?(note)
     end
 
     def extract_commands(note, options = {})

changelogs/unreleased/43334-reply-by-email-did-not-pick-up-unsubscribe-quick-action.yml

@@ -0,0 +1,5 @@
+---
+title: Fix quick actions for users who cannot update issues and merge requests
+merge_request: 17482
+author:
+type: fixed

spec/features/issues/user_uses_slash_commands_spec.rb

@@ -59,7 +59,6 @@ feature 'Issues > User uses quick actions', :js do
         it 'does not create a note, and sets the due date accordingly' do
           write_note("/due 2016-08-28")
 
-          expect(page).to have_content '/due 2016-08-28'
           expect(page).not_to have_content 'Commands applied'
 
           issue.reload
@@ -99,7 +98,6 @@ feature 'Issues > User uses quick actions', :js do
         it 'does not create a note, and sets the due date accordingly' do
           write_note("/remove_due_date")
 
-          expect(page).to have_content '/remove_due_date'
           expect(page).not_to have_content 'Commands applied'
 
           issue.reload
@@ -147,7 +145,6 @@ feature 'Issues > User uses quick actions', :js do
         it 'does not create a note, and does not mark the issue as a duplicate' do
           write_note("/duplicate ##{original_issue.to_reference}")
 
-          expect(page).to have_content "/duplicate ##{original_issue.to_reference}"
           expect(page).not_to have_content 'Commands applied'
           expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}"
 

spec/fixtures/emails/update_commands_only_reply.eml

@@ -0,0 +1,38 @@
+Return-Path: <jake@adventuretime.ooo>
+Received: from iceking.adventuretime.ooo ([unix socket]) by iceking (Cyrus v2.2.13-Debian-2.2.13-19+squeeze3) with LMTPA; Thu, 13 Jun 2013 17:03:50 -0400
+Received: from mail-ie0-x234.google.com (mail-ie0-x234.google.com [IPv6:2607:f8b0:4001:c03::234]) by iceking.adventuretime.ooo (8.14.3/8.14.3/Debian-9.4) with ESMTP id r5DL3nFJ016967 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for <reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 17:03:50 -0400
+Received: by mail-ie0-f180.google.com with SMTP id f4so21977375iea.25 for <reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo>; Thu, 13 Jun 2013 14:03:48 -0700
+Received: by 10.0.0.1 with HTTP; Thu, 13 Jun 2013 14:03:48 -0700
+Date: Thu, 13 Jun 2013 17:03:48 -0400
+From: Jake the Dog <jake@adventuretime.ooo>
+To: reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo
+Message-ID: <CADkmRc+rNGAGGbV2iE5p918UVy4UyJqVcXRO2=otppgzduJSg@mail.gmail.com>
+In-Reply-To: <issue_1@localhost>
+References: <issue_1@localhost> <reply-59d8df8370b7e95c5a49fbf86aeb2c93@localhost>
+Subject: re: [Discourse Meta] eviltrout posted in 'Adventure Time Sux'
+Mime-Version: 1.0
+Content-Type: text/plain;
+ charset=ISO-8859-1
+Content-Transfer-Encoding: 7bit
+X-Sieve: CMU Sieve 2.2
+X-Received: by 10.0.0.1 with SMTP id n7mr11234144ipb.85.1371157428600; Thu,
+ 13 Jun 2013 14:03:48 -0700 (PDT)
+X-Scanned-By: MIMEDefang 2.69 on IPv6:2001:470:1d:165::1
+
+/close
+
+On Sun, Jun 9, 2013 at 1:39 PM, eviltrout via Discourse Meta
+<reply+59d8df8370b7e95c5a49fbf86aeb2c93@appmail.adventuretime.ooo> wrote:
+>
+>
+>
+> eviltrout posted in 'Adventure Time Sux' on Discourse Meta:
+>
+> ---
+> hey guys everyone knows adventure time sucks!
+>
+> ---
+> Please visit this link to respond: http://localhost:3000/t/adventure-time-sux/1234/3
+>
+> To unsubscribe from these emails, visit your [user preferences](http://localhost:3000/user_preferences).
+>

spec/lib/gitlab/email/handler/create_note_handler_spec.rb

@@ -55,8 +55,8 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
       expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidNoteError)
     end
 
-    context 'because the note was commands only' do
+    context 'because the note was update commands only' do
-      let!(:email_raw) { fixture_file("emails/commands_only_reply.eml") }
+      let!(:email_raw) { fixture_file("emails/update_commands_only_reply.eml") }
 
       context 'and current user cannot update noteable' do
         it 'raises a CommandsOnlyNoteError' do
@@ -70,13 +70,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
         end
 
         it 'does not raise an error' do
-          expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
-
           # One system note is created for the 'close' event
           expect { receiver.execute }.to change { noteable.notes.count }.by(1)
 
           expect(noteable.reload).to be_closed
-          expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
         end
       end
     end
@@ -85,15 +82,13 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
   context 'when the note contains quick actions' do
     let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") }
 
-    context 'and current user cannot update noteable' do
+    context 'and current user cannot update the noteable' do
-      it 'post a note and does not update the noteable' do
+      it 'only executes the commands that the user can perform' do
-        expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
+        expect { receiver.execute }
-
+          .to change { noteable.notes.user.count }.by(1)
-        # One system note is created for the new note
+          .and change { user.todos_pending_count }.from(0).to(1)
-        expect { receiver.execute }.to change { noteable.notes.count }.by(1)
 
         expect(noteable.reload).to be_open
-        expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
       end
     end
 
@@ -102,14 +97,14 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
         project.add_developer(user)
       end
 
-      it 'post a note and updates the noteable' do
+      it 'posts a note and updates the noteable' do
         expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
 
-        # One system note is created for the new note, one for the 'close' event
+        expect { receiver.execute }
-        expect { receiver.execute }.to change { noteable.notes.count }.by(2)
+          .to change { noteable.notes.user.count }.by(1)
+          .and change { user.todos_pending_count }.from(0).to(1)
 
         expect(noteable.reload).to be_closed
-        expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
       end
     end
   end

spec/services/notes/create_service_spec.rb

@@ -57,8 +57,9 @@ describe Notes::CreateService do
       end
     end
 
-    describe 'note with commands' do
+    context 'note with commands' do
-      describe '/close, /label, /assign & /milestone' do
+      context 'as a user who can update the target' do
+        context '/close, /label, /assign & /milestone' do
           let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) }
 
           it 'saves the note and does not alter the note text' do
@@ -70,7 +71,7 @@ describe Notes::CreateService do
           end
         end
 
-      describe '/merge with sha option' do
+        context '/merge with sha option' do
           let(:note_text) { %(HELLO\n/merge\nWORLD) }
           let(:params) { opts.merge(note: note_text, merge_request_diff_head_sha: 'sha') }
 
@@ -82,7 +83,29 @@ describe Notes::CreateService do
         end
       end
 
-    describe 'personal snippet note' do
+      context 'as a user who cannot update the target' do
+        let(:note_text) { "HELLO\n/todo\n/assign #{user.to_reference}\nWORLD" }
+        let(:note) { described_class.new(project, user, opts.merge(note: note_text)).execute }
+
+        before do
+          project.team.find_member(user.id).update!(access_level: Gitlab::Access::GUEST)
+        end
+
+        it 'applies commands the user can execute' do
+          expect { note }.to change { user.todos_pending_count }.from(0).to(1)
+        end
+
+        it 'does not apply commands the user cannot execute' do
+          expect { note }.not_to change { issue.assignees }
+        end
+
+        it 'saves the note' do
+          expect(note.note).to eq "HELLO\nWORLD"
+        end
+      end
+    end
+
+    context 'personal snippet note' do
       subject { described_class.new(nil, user, params).execute }
 
       let(:snippet) { create(:personal_snippet) }
@@ -103,7 +126,7 @@ describe Notes::CreateService do
       end
     end
 
-    describe 'note with emoji only' do
+    context 'note with emoji only' do
       it 'creates regular note' do
         opts = {
           note: ':smile: ',

spec/services/notes/quick_actions_service_spec.rb

@@ -165,31 +165,17 @@ describe Notes::QuickActionsService do
 
     let(:note) { create(:note_on_issue, project: project) }
 
-    context 'with no current_user' do
+    context 'with a note on an issue' do
-      it 'returns false' do
-        expect(described_class.supported?(note, nil)).to be_falsy
-      end
-    end
-
-    context 'when current_user cannot update the noteable' do
-      it 'returns false' do
-        user = create(:user)
-
-        expect(described_class.supported?(note, user)).to be_falsy
-      end
-    end
-
-    context 'when current_user can update the noteable' do
       it 'returns true' do
-        expect(described_class.supported?(note, master)).to be_truthy
+        expect(described_class.supported?(note)).to be_truthy
+      end
     end
 
     context 'with a note on a commit' do
       let(:note) { create(:note_on_commit, project: project) }
 
       it 'returns false' do
-          expect(described_class.supported?(note, nil)).to be_falsy
+        expect(described_class.supported?(note)).to be_falsy
-        end
       end
     end
   end
@@ -201,7 +187,7 @@ describe Notes::QuickActionsService do
       service = described_class.new(project, master)
       note = create(:note_on_issue, project: project)
 
-      expect(described_class).to receive(:supported?).with(note, master)
+      expect(described_class).to receive(:supported?).with(note)
 
       service.supported?(note)
     end

spec/support/features/issuable_slash_commands_shared_examples.rb

@@ -127,7 +127,6 @@ shared_examples 'issuable record that supports quick actions in its description
         it "does not close the #{issuable_type}" do
           write_note("/close")
 
-          expect(page).to have_content '/close'
           expect(page).not_to have_content 'Commands applied'
 
           expect(issuable).to be_open
@@ -165,7 +164,6 @@ shared_examples 'issuable record that supports quick actions in its description
         it "does not reopen the #{issuable_type}" do
           write_note("/reopen")
 
-          expect(page).to have_content '/reopen'
           expect(page).not_to have_content 'Commands applied'
 
           expect(issuable).to be_closed
@@ -195,10 +193,9 @@ shared_examples 'issuable record that supports quick actions in its description
           visit public_send("namespace_project_#{issuable_type}_path", project.namespace, project, issuable)
         end
 
-        it "does not reopen the #{issuable_type}" do
+        it "does not change the #{issuable_type} title" do
           write_note("/title Awesome new title")
 
-          expect(page).to have_content '/title'
           expect(page).not_to have_content 'Commands applied'
 
           expect(issuable.reload.title).not_to eq 'Awesome new title'