Fixes #2853 - Deletion of notes impossible if internal and communicat…

…ion = true

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'
 

app/policies/ticket/article_policy.rb

@@ -25,7 +25,7 @@ def destroy?
     # 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

spec/requests/ticket/article_spec.rb

@@ -480,52 +480,142 @@
     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)
+
+      # 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!
+
+      output
+    end
 
-    let!(:article) { create(:ticket_article, sender_name: 'Agent', type_name: 'note', updated_by_id: agent_user.id, created_by_id: agent_user.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
 
-    context 'by Admin user' do
-      before do
-        authenticated_as(admin_user)
-      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
 
-      it 'always succeeds' do
-        expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.to change { Ticket::Article.exists?(id: article.id) }
+    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
-        end
+        include_examples now ? 'succeeds' : 'fails'
 
-        it 'succeeds' do
-          expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.to change { Ticket::Article.exists?(id: article.id) }
-        end
-      end
+        context '8 minutes later' do
+          before { article && travel(8.minutes) }
 
-      context '10+ minutes after creation' do
-        before do
-          authenticated_as(agent_user)
-          travel 11.minutes
+          include_examples later ? 'succeeds' : 'fails'
         end
 
-        it 'fails' do
-          expect { delete "/api/v1/ticket_articles/#{article.id}", params: {}, as: :json }.not_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 '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
+
+    end
   end
 end

spec/requests/ticket_spec.rb

@@ -879,6 +879,18 @@
       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)

spec/support/capybara/common_actions.rb

@@ -212,6 +212,19 @@ def modal_ready(timeout: 4)
   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|

spec/support/capybara/selectors.rb

@@ -8,6 +8,10 @@
   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