From 222e8c0dd3e787762191c6b378fd5959ec482dd2 Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Mon, 13 Apr 2020 21:24:03 +0300 Subject: [PATCH] Fixes #2853 - Deletion of notes impossible if internal and communication = true --- .../ticket_zoom/article_action/delete.coffee | 41 +++-- app/policies/ticket/article_policy.rb | 2 +- spec/requests/ticket/article_spec.rb | 152 ++++++++++++++---- spec/requests/ticket_spec.rb | 12 ++ spec/support/capybara/common_actions.rb | 13 ++ spec/support/capybara/selectors.rb | 4 + spec/system/ticket/zoom_spec.rb | 141 ++++++++++++++++ 7 files changed, 319 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee index 7d4e5437f..5a93af661 100644 --- a/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee +++ b/app/assets/javascripts/app/controllers/ticket_zoom/article_action/delete.coffee @@ -1,18 +1,8 @@ class Delete @action: (actions, ticket, article, ui) -> - return actions if ui.permissionCheck('ticket.customer') + status = @isDeletable(actions, ticket, article, ui) - return actions if article.type.name isnt 'note' - - return actions if App.User.current()?.id != article.created_by_id - - return actions if !ui.permissionCheck('ticket.agent') - - # return if article is older then 10 minutes - created_at = Date.parse(article.created_at) - time_to_show = 600000 - (Date.parse(new Date()) - created_at) - - return actions if time_to_show <= 0 + return actions if !status.isDeletable actions.push { name: 'delete' @@ -21,11 +11,34 @@ class Delete href: '#' } - # rerender ations in 10 minutes again to hide delete action of article - ui.delay(ui.render, time_to_show, 'actions-rerender') + # rerender actions if ability to delete expires + if status.timeout + ui.delay(ui.render, status.timeout, 'actions-rerender') actions + @isDeletable: (actions, ticket, article, ui) -> + return { isDeletable: true } if ui.permissionCheck('admin') + + return { isDeletable: false } if !@deletableForAgent(actions, ticket, article, ui) + + timeout = @deletableTimeout(actions, ticket, article, ui) + + return { isDeletable: false } if timeout <= 0 + + { isDeletable: true, timeout: timeout } + + @deletableTimeout: (actions, ticket, article, ui) -> + created_at = Date.parse(article.created_at) + 600000 - (Date.parse(new Date()) - created_at) + + @deletableForAgent: (actions, ticket, article, ui) -> + return false if !ui.permissionCheck('ticket.agent') + return false if article.created_by_id != App.User.current()?.id + return false if article.type.communication and !article.internal + + true + @perform: (articleContainer, type, ticket, article, ui) -> return true if type isnt 'delete' diff --git a/app/policies/ticket/article_policy.rb b/app/policies/ticket/article_policy.rb index d80c195e9..fa9fa5ff2 100644 --- a/app/policies/ticket/article_policy.rb +++ b/app/policies/ticket/article_policy.rb @@ -25,7 +25,7 @@ class Ticket::ArticlePolicy < ApplicationPolicy # which were created by themselves within the last 10 minutes return missing_admin_permission if !user.permissions?('ticket.agent') return missing_admin_permission if record.created_by_id != user.id - return missing_admin_permission if record.type.communication? + return missing_admin_permission if record.type.communication? && !record.internal? return too_old_to_undo if record.created_at <= 10.minutes.ago true diff --git a/spec/requests/ticket/article_spec.rb b/spec/requests/ticket/article_spec.rb index 584f11ca3..8e78469ee 100644 --- a/spec/requests/ticket/article_spec.rb +++ b/spec/requests/ticket/article_spec.rb @@ -480,52 +480,142 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO end end - describe 'DELETE /api/v1/ticket_articles/:id' do + describe 'DELETE /api/v1/ticket_articles/:id', authenticated_as: -> { user } do + let(:ticket) do + output = create(:ticket) - let!(:article) { create(:ticket_article, sender_name: 'Agent', type_name: 'note', updated_by_id: agent_user.id, created_by_id: agent_user.id ) } + # make group ticket was created in available to current user + role = user.roles.first + map = role.group_ids_access_map + map[output.group.id] = :full + role.group_ids_access_map = map + role.save! - context 'by Admin user' do - before do - authenticated_as(admin_user) - end + output + end - it 'always succeeds' do - expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.to change { Ticket::Article.exists?(id: article.id) } + let(:article_communication) do + create(:ticket_article, + sender_name: 'Agent', type_name: 'email', ticket: ticket, + updated_by_id: agent_user.id, created_by_id: agent_user.id ) + end + + let(:article_note) do + create(:ticket_article, + sender_name: 'Agent', internal: true, type_name: 'note', ticket: ticket, + updated_by_id: agent_user.id, created_by_id: agent_user.id ) + end + + let(:article_note_customer) do + create(:ticket_article, + sender_name: 'Customer', internal: false, type_name: 'note', ticket: ticket, + updated_by_id: customer_user.id, created_by_id: customer_user.id ) + end + + let(:article_note_communication) do + create(:ticket_article_type, name: 'note_communication', communication: true) + + create(:ticket_article, + sender_name: 'Agent', internal: true, type_name: 'note_communication', ticket: ticket, + updated_by_id: agent_user.id, created_by_id: agent_user.id ) + end + + def delete_article_via_rest(article) + delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json + end + + shared_examples 'succeeds' do + it 'succeeds' do + expect { delete_article_via_rest(article) }.to change { Ticket::Article.exists?(id: article.id) } end end - context 'by Agent user' do - before do - # this is needed, role needs full rights for the new group - # so that agent can delete the article - group_ids_access_map = Group.all.pluck(:id).each_with_object({}) { |group_id, result| result[group_id] = 'full'.freeze } - role = Role.find_by(name: 'Agent') - role.group_ids_access_map = group_ids_access_map - role.save! + shared_examples 'fails' do + it 'fails' do + expect { delete_article_via_rest(article) }.not_to change { Ticket::Article.exists?(id: article.id) } end + end - context 'within 10 minutes of creation' do - before do + shared_examples 'deleting' do |item:, now:, later:, much_later:| + context "deleting #{item}" do + let(:article) { send(item) } - authenticated_as(agent_user) - travel 8.minutes + include_examples now ? 'succeeds' : 'fails' + + context '8 minutes later' do + before { article && travel(8.minutes) } + + include_examples later ? 'succeeds' : 'fails' end - it 'succeeds' do - expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.to change { Ticket::Article.exists?(id: article.id) } + context '11 minutes later' do + before { article && travel(11.minutes) } + + include_examples much_later ? 'succeeds' : 'fails' end end + end - context '10+ minutes after creation' do - before do - authenticated_as(agent_user) - travel 11.minutes - end + context 'as admin' do + let(:user) { admin_user } + + include_examples 'deleting', + item: 'article_communication', + now: true, later: true, much_later: true + + include_examples 'deleting', + item: 'article_note', + now: true, later: true, much_later: true + + include_examples 'deleting', + item: 'article_note_customer', + now: true, later: true, much_later: true + + include_examples 'deleting', + item: 'article_note_communication', + now: true, later: true, much_later: true + end + + context 'as agent' do + let(:user) { agent_user } + + include_examples 'deleting', + item: 'article_communication', + now: false, later: false, much_later: false + + include_examples 'deleting', + item: 'article_note', + now: true, later: true, much_later: false + + include_examples 'deleting', + item: 'article_note_customer', + now: false, later: false, much_later: false + + include_examples 'deleting', + item: 'article_note_communication', + now: true, later: true, much_later: false + + end + + context 'as customer' do + let(:user) { customer_user } + + include_examples 'deleting', + item: 'article_communication', + now: false, later: false, much_later: false + + include_examples 'deleting', + item: 'article_note', + now: false, later: false, much_later: false + + include_examples 'deleting', + item: 'article_note_customer', + now: false, later: false, much_later: false + + include_examples 'deleting', + item: 'article_note_communication', + now: false, later: false, much_later: false - it 'fails' do - expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.not_to change { Ticket::Article.exists?(id: article.id) } - end - end end end end diff --git a/spec/requests/ticket_spec.rb b/spec/requests/ticket_spec.rb index 8b4628cb7..e2cc32b51 100644 --- a/spec/requests/ticket_spec.rb +++ b/spec/requests/ticket_spec.rb @@ -879,6 +879,18 @@ RSpec.describe 'Ticket', type: :request do expect(json_response['sender_id']).to eq(Ticket::Article::Sender.lookup(name: 'Agent').id) expect(json_response['type_id']).to eq(Ticket::Article::Type.lookup(name: 'email').id) + params = { + from: 'something which should not be changed on server side', + ticket_id: ticket.id, + subject: 'some subject', + body: 'some body', + type: 'email', + internal: false, + } + post '/api/v1/ticket_articles', params: params, as: :json + expect(response).to have_http_status(:created) + expect(json_response['internal']).to eq(false) + delete "/api/v1/ticket_articles/#{json_response['id']}", params: {}, as: :json expect(response).to have_http_status(:unauthorized) expect(json_response).to be_a_kind_of(Hash) diff --git a/spec/support/capybara/common_actions.rb b/spec/support/capybara/common_actions.rb index 934fae1d1..e4ae54aac 100644 --- a/spec/support/capybara/common_actions.rb +++ b/spec/support/capybara/common_actions.rb @@ -212,6 +212,19 @@ module CommonActions def modal_disappear(timeout: 4) wait(timeout).until_disappears { find('.modal', wait: 0) } end + + # Executes action inside of modal. Makes sure modal has opened and closes + # + # @param timeout [Integer] seconds to wait + def in_modal(timeout: 4) + modal_ready(timeout: timeout) + + within '.modal' do + yield + end + + modal_disappear(timeout: timeout) + end end RSpec.configure do |config| diff --git a/spec/support/capybara/selectors.rb b/spec/support/capybara/selectors.rb index cc55290b0..ba4dab2d5 100644 --- a/spec/support/capybara/selectors.rb +++ b/spec/support/capybara/selectors.rb @@ -8,6 +8,10 @@ Capybara.add_selector(:active_content) do css { |content_class| ['.content.active', content_class].compact.join(' ') } end +Capybara.add_selector(:active_ticket_article) do + css { |article| ['.content.active', "#article-#{article.id}" ].compact.join(' ') } +end + Capybara.add_selector(:manage) do css { 'a[href="#manage"]' } end diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index fb8cedf86..443870166 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/spec/system/ticket/zoom_spec.rb @@ -206,4 +206,145 @@ RSpec.describe 'Ticket zoom', type: :system do end end end + + describe 'delete article', authenticated: -> { user } do + let(:admin_user) { User.find_by! email: 'master@example.com' } + let(:agent_user) { create :agent, password: 'test', groups: [Group.first] } + let(:customer_user) { create :customer, password: 'test' } + let(:ticket) { create :ticket, group: agent_user.groups.first, customer: customer_user } + let(:article) { send(item) } + + def article_communication + create_ticket_article(sender_name: 'Agent', internal: false, type_name: 'email', updated_by: customer_user) + end + + def article_note + create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note', updated_by: agent_user) + end + + def article_note_customer + create_ticket_article(sender_name: 'Customer', internal: false, type_name: 'note', updated_by: customer_user) + end + + def article_note_communication + create(:ticket_article_type, name: 'note_communication', communication: true) + + create_ticket_article(sender_name: 'Agent', internal: true, type_name: 'note_communication', updated_by: agent_user) + end + + def create_ticket_article(sender_name:, internal:, type_name:, updated_by:) + create(:ticket_article, + sender_name: sender_name, internal: internal, type_name: type_name, ticket: ticket, + body: "to be deleted #{offset} #{item}", + updated_by_id: updated_by.id, created_by_id: updated_by.id, + created_at: offset.ago, updated_at: offset.ago) + end + + context 'going through full stack' do + context 'as admin' do + let(:user) { admin_user } + let(:item) { 'article_communication' } + let(:offset) { 0.minutes } + + it 'succeeds' do + refresh # make sure user roles are loaded + + ensure_websocket do + visit "ticket/zoom/#{ticket.id}" + end + + within :active_ticket_article, article, wait: 15 do + click '.js-ArticleAction[data-type=delete]' + end + + in_modal do + click '.js-submit' + end + + wait.until_disappears { find :active_ticket_article, article, wait: false } + end + end + end + + context 'verifying permissions matrix' do + shared_examples 'according to permission matrix' do |item:, expects_visible:, offset:, description:| + context "looking at #{description} #{item}" do + let(:item) { item } + let!(:article) { send(item) } + + let(:offset) { offset } + let(:matcher) { expects_visible ? :have_css : :have_no_css } + + it expects_visible ? 'delete button is visible' : 'delete button is not visible' do + refresh # make sure user roles are loaded + + visit "ticket/zoom/#{ticket.id}" + + within :active_ticket_article, article, wait: 15 do + expect(page).to send(matcher, '.js-ArticleAction[data-type=delete]', wait: 0) + end + end + end + end + + shared_examples 'deleting ticket article' do |item:, now:, later:, much_later:| + include_examples 'according to permission matrix', item: item, expects_visible: now, offset: 0.minutes, description: 'just created' + include_examples 'according to permission matrix', item: item, expects_visible: later, offset: 6.minutes, description: 'few minutes old' + include_examples 'according to permission matrix', item: item, expects_visible: much_later, offset: 11.minutes, description: 'very old' + end + + context 'as admin' do + let(:user) { admin_user } + + include_examples 'deleting ticket article', + item: 'article_communication', + now: true, later: true, much_later: true + + include_examples 'deleting ticket article', + item: 'article_note', + now: true, later: true, much_later: true + + include_examples 'deleting ticket article', + item: 'article_note_customer', + now: true, later: true, much_later: true + + include_examples 'deleting ticket article', + item: 'article_note_communication', + now: true, later: true, much_later: true + end + + context 'as agent' do + let(:user) { agent_user } + + include_examples 'deleting ticket article', + item: 'article_communication', + now: false, later: false, much_later: false + + include_examples 'deleting ticket article', + item: 'article_note', + now: true, later: true, much_later: false + + include_examples 'deleting ticket article', + item: 'article_note_customer', + now: false, later: false, much_later: false + + include_examples 'deleting ticket article', + item: 'article_note_communication', + now: true, later: true, much_later: false + end + + context 'as customer' do + let(:user) { customer_user } + + include_examples 'deleting ticket article', + item: 'article_communication', + now: false, later: false, much_later: false + + include_examples 'deleting ticket article', + item: 'article_note_customer', + now: false, later: false, much_later: false + + end + end + end end