Commit b02397df authored by Rémy Coutable's avatar Rémy Coutable

Merge branch '43334-reply-by-email-did-not-pick-up-unsubscribe-quick-action' into 'master'

Resolve "Reply-by-email did not pick up /unsubscribe quick action"

Closes #43334

See merge request gitlab-org/gitlab-ce!17482
parents b3ff1235 daeeb7f8
...@@ -11,7 +11,7 @@ module NotesHelper ...@@ -11,7 +11,7 @@ module NotesHelper
end end
def note_supports_quick_actions?(note) def note_supports_quick_actions?(note)
Notes::QuickActionsService.supported?(note, current_user) Notes::QuickActionsService.supported?(note)
end end
def noteable_json(noteable) def noteable_json(noteable)
......
...@@ -9,14 +9,12 @@ module Notes ...@@ -9,14 +9,12 @@ module Notes
UPDATE_SERVICES[note.noteable_type] UPDATE_SERVICES[note.noteable_type]
end 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 end
def supported?(note) def supported?(note)
self.class.supported?(note, current_user) self.class.supported?(note)
end end
def extract_commands(note, options = {}) def extract_commands(note, options = {})
......
---
title: Fix quick actions for users who cannot update issues and merge requests
merge_request: 17482
author:
type: fixed
...@@ -59,7 +59,6 @@ feature 'Issues > User uses quick actions', :js do ...@@ -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 it 'does not create a note, and sets the due date accordingly' do
write_note("/due 2016-08-28") write_note("/due 2016-08-28")
expect(page).to have_content '/due 2016-08-28'
expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content 'Commands applied'
issue.reload issue.reload
...@@ -99,7 +98,6 @@ feature 'Issues > User uses quick actions', :js do ...@@ -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 it 'does not create a note, and sets the due date accordingly' do
write_note("/remove_due_date") write_note("/remove_due_date")
expect(page).to have_content '/remove_due_date'
expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content 'Commands applied'
issue.reload issue.reload
...@@ -147,7 +145,6 @@ feature 'Issues > User uses quick actions', :js do ...@@ -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 it 'does not create a note, and does not mark the issue as a duplicate' do
write_note("/duplicate ##{original_issue.to_reference}") 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 'Commands applied'
expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}" expect(page).not_to have_content "marked this issue as a duplicate of #{original_issue.to_reference}"
......
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).
>
...@@ -55,8 +55,8 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -55,8 +55,8 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidNoteError) expect { receiver.execute }.to raise_error(Gitlab::Email::InvalidNoteError)
end 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 context 'and current user cannot update noteable' do
it 'raises a CommandsOnlyNoteError' do it 'raises a CommandsOnlyNoteError' do
...@@ -70,13 +70,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -70,13 +70,10 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
end end
it 'does not raise an error' do 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 # One system note is created for the 'close' event
expect { receiver.execute }.to change { noteable.notes.count }.by(1) expect { receiver.execute }.to change { noteable.notes.count }.by(1)
expect(noteable.reload).to be_closed expect(noteable.reload).to be_closed
expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
end end
end end
end end
...@@ -85,15 +82,13 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -85,15 +82,13 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
context 'when the note contains quick actions' do context 'when the note contains quick actions' do
let!(:email_raw) { fixture_file("emails/commands_in_reply.eml") } 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(noteable.reload).to be_open
expect(TodoService.new.todo_exist?(noteable, user)).to be_falsy
end end
end end
...@@ -102,14 +97,14 @@ describe Gitlab::Email::Handler::CreateNoteHandler do ...@@ -102,14 +97,14 @@ describe Gitlab::Email::Handler::CreateNoteHandler do
project.add_developer(user) project.add_developer(user)
end 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 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(noteable.reload).to be_closed
expect(TodoService.new.todo_exist?(noteable, user)).to be_truthy
end end
end end
end end
......
...@@ -57,32 +57,55 @@ describe Notes::CreateService do ...@@ -57,32 +57,55 @@ describe Notes::CreateService do
end end
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
let(:note_text) { %(HELLO\n/close\n/assign @#{user.username}\nWORLD) } 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 it 'saves the note and does not alter the note text' do
expect_any_instance_of(Issues::UpdateService).to receive(:execute).and_call_original expect_any_instance_of(Issues::UpdateService).to receive(:execute).and_call_original
note = described_class.new(project, user, opts.merge(note: note_text)).execute note = described_class.new(project, user, opts.merge(note: note_text)).execute
expect(note.note).to eq "HELLO\nWORLD" expect(note.note).to eq "HELLO\nWORLD"
end
end
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') }
it 'saves the note and exectues merge command' do
note = described_class.new(project, user, params).execute
expect(note.note).to eq "HELLO\nWORLD"
end
end end
end end
describe '/merge with sha option' do context 'as a user who cannot update the target' do
let(:note_text) { %(HELLO\n/merge\nWORLD) } let(:note_text) { "HELLO\n/todo\n/assign #{user.to_reference}\nWORLD" }
let(:params) { opts.merge(note: note_text, merge_request_diff_head_sha: 'sha') } let(:note) { described_class.new(project, user, opts.merge(note: note_text)).execute }
it 'saves the note and exectues merge command' do before do
note = described_class.new(project, user, params).execute 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" expect(note.note).to eq "HELLO\nWORLD"
end end
end end
end end
describe 'personal snippet note' do context 'personal snippet note' do
subject { described_class.new(nil, user, params).execute } subject { described_class.new(nil, user, params).execute }
let(:snippet) { create(:personal_snippet) } let(:snippet) { create(:personal_snippet) }
...@@ -103,7 +126,7 @@ describe Notes::CreateService do ...@@ -103,7 +126,7 @@ describe Notes::CreateService do
end end
end end
describe 'note with emoji only' do context 'note with emoji only' do
it 'creates regular note' do it 'creates regular note' do
opts = { opts = {
note: ':smile: ', note: ':smile: ',
......
...@@ -165,31 +165,17 @@ describe Notes::QuickActionsService do ...@@ -165,31 +165,17 @@ describe Notes::QuickActionsService do
let(:note) { create(:note_on_issue, project: project) } 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 it 'returns true' do
expect(described_class.supported?(note, master)).to be_truthy expect(described_class.supported?(note)).to be_truthy
end end
end
context 'with a note on a commit' do context 'with a note on a commit' do
let(:note) { create(:note_on_commit, project: project) } let(:note) { create(:note_on_commit, project: project) }
it 'returns false' do 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 end
end end
...@@ -201,7 +187,7 @@ describe Notes::QuickActionsService do ...@@ -201,7 +187,7 @@ describe Notes::QuickActionsService do
service = described_class.new(project, master) service = described_class.new(project, master)
note = create(:note_on_issue, project: project) 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) service.supported?(note)
end end
......
...@@ -127,7 +127,6 @@ shared_examples 'issuable record that supports quick actions in its description ...@@ -127,7 +127,6 @@ shared_examples 'issuable record that supports quick actions in its description
it "does not close the #{issuable_type}" do it "does not close the #{issuable_type}" do
write_note("/close") write_note("/close")
expect(page).to have_content '/close'
expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content 'Commands applied'
expect(issuable).to be_open expect(issuable).to be_open
...@@ -165,7 +164,6 @@ shared_examples 'issuable record that supports quick actions in its description ...@@ -165,7 +164,6 @@ shared_examples 'issuable record that supports quick actions in its description
it "does not reopen the #{issuable_type}" do it "does not reopen the #{issuable_type}" do
write_note("/reopen") write_note("/reopen")
expect(page).to have_content '/reopen'
expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content 'Commands applied'
expect(issuable).to be_closed expect(issuable).to be_closed
...@@ -195,10 +193,9 @@ shared_examples 'issuable record that supports quick actions in its description ...@@ -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) visit public_send("namespace_project_#{issuable_type}_path", project.namespace, project, issuable)
end end
it "does not reopen the #{issuable_type}" do it "does not change the #{issuable_type} title" do
write_note("/title Awesome new title") write_note("/title Awesome new title")
expect(page).to have_content '/title'
expect(page).not_to have_content 'Commands applied' expect(page).not_to have_content 'Commands applied'
expect(issuable.reload.title).not_to eq 'Awesome new title' expect(issuable.reload.title).not_to eq 'Awesome new title'
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment