Fixes #4386 - Agents cannot reopen tickets when group follow-up setting is set to "do not reopen"

app/assets/javascripts/app/controllers/ticket_zoom/sidebar_ticket.coffee

@@ -18,8 +18,6 @@ class Edit extends App.Controller
   render: (draft = {}) =>
     defaults = @ticket.attributes()
     delete defaults.article # ignore article infos
-    group = App.Group.find(defaults.group_id)
-    ticketStateType = App.TicketState.find(defaults.state_id).name
 
     taskState = @taskGet('ticket')
     handlers = @Config.get('TicketZoomFormHandler')
@@ -30,24 +28,6 @@ class Edit extends App.Controller
       # for the new ticket + eventually changed task state
       @formMeta.core_workflow = undefined
 
-    isDisabled = !@ticket.editable()
-
-    # if ticket is closed and not re-openable
-    if group.follow_up_possible == 'new_ticket' && ticketStateType is 'closed'
-      isDisabled = true
-
-    # if ticket is closed and not re-openable
-    if group.follow_up_possible == 'new_ticket_after_certain_time' && ticketStateType is 'closed' && @ticket.last_close_at
-      closed_since = (new Date - Date.parse(@ticket.last_close_at)) / (24 * 60 * 60 * 1000)
-      if closed_since >= group.reopen_time_in_days
-        isDisabled = true
-      else
-        isDisabled = false
-
-    # if ticket is shown by admin
-    if @permissionCheck('admin')
-      isDisabled = false
-
     # reset updated_at for the sidbar because we render a new state
     # it is used to compare the ticket with the rendered data later
     # and needed to prevent race conditions
@@ -61,7 +41,7 @@ class Edit extends App.Controller
       filter:         @formMeta.filter
       formMeta:       @formMeta
       params:         _.extend(defaults, draft)
-      isDisabled:     isDisabled
+      isDisabled:     @isDisabledByFollowupRules(defaults)
       taskKey:        @taskKey
       core_workflow: {
         callbacks: [@markForm]
@@ -80,6 +60,25 @@ class Edit extends App.Controller
       @render()
     )
 
+  isDisabledByFollowupRules: (attributes) =>
+    return false if @ticket.userGroupAccess('change')
+
+    group           = App.Group.find(attributes.group_id)
+    ticketStateType = App.TicketState.find(attributes.state_id).name
+    initialState    = !@ticket.editable()
+
+    return initialState if ticketStateType isnt 'closed'
+
+    switch group.follow_up_possible
+      when 'yes'
+        return initialState
+      when 'new_ticket'
+        return true
+      when 'new_ticket_after_certain_time'
+        closed_since = (new Date - Date.parse(@ticket.last_close_at)) / (24 * 60 * 60 * 1000)
+
+        return closed_since >= group.reopen_time_in_days
+
 class SidebarTicket extends App.Controller
   constructor: ->
     super

app/controllers/tickets_controller.rb

@@ -241,7 +241,6 @@ class TicketsController < ApplicationController
   def update
     ticket = Ticket.find(params[:id])
     authorize!(ticket, :follow_up?)
-    authorize!(ticket)
 
     clean_params = Ticket.association_name_to_id_convert(params)
     clean_params = Ticket.param_cleanup(clean_params, true)

app/controllers/tickets_mass_controller.rb

@@ -44,7 +44,6 @@ class TicketsMassController < ApplicationController
 
     @tickets.each do |elem|
       authorize!(elem, :follow_up?)
-      authorize!(elem, :update?)
     end
   rescue Pundit::NotAuthorizedError => e
     render json: { error: true, ticket_id: e.record.id }, status: :unprocessable_entity

app/policies/ticket_policy.rb

@@ -40,11 +40,11 @@ class TicketPolicy < ApplicationPolicy
     # This method is used to check if a follow-up is possible (mostly based on the configuration).
     # Agents are always allowed to reopen tickets, configuration does not matter.
 
-    return true if record.state.name != 'closed' # check if the ticket state is already closed
-    return true if user.permissions?('ticket.agent')
+    return update? if Ticket::StateType.lookup(id: record.state.state_type_id).name != 'closed' # check if the ticket state is already closed
+    return true if agent_update_access?
 
     # Check follow_up_possible configuration, based on the group.
-    return true if follow_up_possible?
+    return true if follow_up_possible? && update?
 
     not_authorized Exceptions::UnprocessableEntity.new __('Cannot follow-up on a closed ticket. Please create a new ticket.')
   end
@@ -62,14 +62,12 @@ class TicketPolicy < ApplicationPolicy
   def follow_up_possible?
     case record.group.follow_up_possible
     when 'yes'
-      # Easy going, just reopen the ticket.
-      return true
+      true
     when 'new_ticket_after_certain_time'
-      # Maybe we are allowed to reopen the existing ticket. Let's check.
-      return true if record.reopen_after_certain_time?
+      record.reopen_after_certain_time?
+    when 'new_ticket'
+      false
     end
-
-    false
   end
 
   def access?(access)

spec/policies/ticket_policy_spec.rb

@@ -86,20 +86,92 @@ describe TicketPolicy do
       context 'to yes' do
         let(:group) { create(:group, follow_up_possible: 'yes') }
 
-        it { is_expected.to permit_actions(%i[follow_up]) }
+        context 'when user is customer' do
+          let(:user) { record.customer }
+
+          it { is_expected.to permit_actions(%i[follow_up]) }
+        end
+
+        context 'when user has no access' do
+          it { is_expected.to forbid_actions(%i[follow_up]) }
+        end
+
+        context 'when user has change access' do
+          before do
+            user.user_groups.create! group: group, access: 'change'
+          end
+
+          it { is_expected.to permit_actions(%i[follow_up]) }
+        end
+
+        context 'when user has read access' do
+          before do
+            user.user_groups.create! group: group, access: 'read'
+          end
+
+          it { is_expected.to forbid_actions(%i[follow_up]) }
+        end
       end
 
       context 'to new_ticket' do
         let(:group) { create(:group, follow_up_possible: 'new_ticket') }
 
-        it { is_expected.to permit_actions(%i[follow_up]) }
+        context 'when user is customer' do
+          let(:user) { record.customer }
+
+          it { is_expected.to forbid_actions(%i[follow_up]) }
+        end
+
+        context 'when user has no access' do
+          it { is_expected.to forbid_actions(%i[follow_up]) }
+        end
+
+        context 'when user has change access' do
+          before do
+            user.user_groups.create! group: group, access: 'change'
+          end
+
+          it { is_expected.to permit_actions(%i[follow_up]) }
+        end
+
+        context 'when user has read access' do
+          before do
+            user.user_groups.create! group: group, access: 'read'
+          end
+
+          it { is_expected.to forbid_actions(%i[follow_up]) }
+        end
       end
 
       context 'to new_ticket_after_certain_time' do
         let(:group) { create(:group, follow_up_possible: 'new_ticket_after_certain_time', reopen_time_in_days: 2) }
 
         context 'when reopen_time_in_days is within configured time frame' do
-          it { is_expected.to permit_actions(%i[follow_up]) }
+          context 'when user is customer' do
+            let(:user) { record.customer }
+
+            it { is_expected.to permit_actions(%i[follow_up]) }
+          end
+
+          context 'when user has no access' do
+            it { is_expected.to forbid_actions(%i[follow_up]) }
+          end
+
+          context 'when user has change access' do
+            before do
+              user.user_groups.create! group: group, access: 'change'
+            end
+
+            it { is_expected.to permit_actions(%i[follow_up]) }
+          end
+
+          context 'when user has read access' do
+            before do
+              user.user_groups.create! group: group, access: 'read'
+            end
+
+            it { is_expected.to forbid_actions(%i[follow_up]) }
+          end
         end
 
         context 'when reopen_time_in_days is outside configured time frame' do
@@ -108,7 +180,31 @@ describe TicketPolicy do
             travel 3.days
           end
 
-          it { is_expected.to permit_actions(%i[follow_up]) }
+          context 'when user is customer' do
+            let(:user) { record.customer }
+
+            it { is_expected.to forbid_actions(%i[follow_up]) }
+          end
+
+          context 'when user has no access' do
+            it { is_expected.to forbid_actions(%i[follow_up]) }
+          end
+
+          context 'when user has change access' do
+            before do
+              user.user_groups.create! group: group, access: 'change'
+            end
+
+            it { is_expected.to permit_actions(%i[follow_up]) }
+          end
+
+          context 'when user has read access' do
+            before do
+              user.user_groups.create! group: group, access: 'read'
+            end
+
+            it { is_expected.to forbid_actions(%i[follow_up]) }
+          end
         end
       end
 

spec/system/ticket/zoom/sidebar_spec.rb

@@ -3,58 +3,89 @@
 require 'rails_helper'
 
 RSpec.describe 'Ticket zoom > Sidebar', authenticated_as: :user, type: :system do
-  let(:ticket) { create(:ticket, customer: user, group: group, state: Ticket::State.find_by(name: 'closed')) }
-  let(:user)   { create(:customer, organization: create(:organization)) }
-  let(:group)  { create(:group, follow_up_possible: 'yes') }
+  let(:ticket) do
+    ticket = create(:ticket, customer: customer, group: group)
 
-  context 'when login as customer' do
+    travel_to close_time do
+      ticket.update! state: Ticket::State.find_by(name: 'closed')
+    end
+
+    ticket
+  end
+
+  let(:customer)   { create(:customer, organization: create(:organization)) }
+  let(:close_time) { Time.current }
+
+  describe 're-openability of a closed ticket' do
     before do
-      travel(-4.days)
       visit "#ticket/zoom/#{ticket.id}"
-      travel_back
     end
 
-    context 'when ticket is closed and groups.follow_up_possible is "yes"' do
-      let(:group) { create(:group, follow_up_possible: 'yes') }
+    let(:state_elem) { find('.sidebar [name=state_id]') }
 
-      it 'show sidebar not as read only' do
+    shared_examples 'shows sidebar as read only' do
+      it 'shows sidebar as read only' do
         within(:active_content) do
-          expect(page).to have_css('.sidebar [name=state_id]')
-          expect(page).to have_no_css('.sidebar [name=state_id]:disabled')
+          expect(state_elem).to match_css(':disabled')
         end
       end
     end
 
-    context 'when ticket is closed and groups.follow_up_possible is "new_ticket"' do
-      let(:group) { create(:group, follow_up_possible: 'new_ticket') }
-
-      it 'show sidebar as read only' do
+    shared_examples 'shows sidebar as updatable' do
+      it 'shows sidebar as updatable' do
         within(:active_content) do
-          expect(page).to have_css('.sidebar [name=state_id]:disabled')
+          expect(state_elem).to match_css(':enabled')
         end
       end
     end
 
-    context 'when ticket is closed and groups.follow_up_possible is "new_ticket_after_certain_time" but reopen is possible' do
-      let(:group) { create(:group, follow_up_possible: 'new_ticket_after_certain_time', reopen_time_in_days: 5) }
+    shared_examples 'check roles' do |customer:, agent:, agent_customer:|
+      context 'when user is customer' do
+        let(:user) { ticket.customer }
 
-      it 'show sidebar not as read only' do
-        within(:active_content) do
-          expect(page).to have_css('.sidebar [name=state_id]')
-          expect(page).to have_no_css('.sidebar [name=state_id]:disabled')
-        end
+        include_examples customer ? 'shows sidebar as updatable' : 'shows sidebar as read only'
+      end
+
+      context 'when user is agent' do
+        let(:user) { create(:agent, groups: [ticket.group]) }
+
+        include_examples agent ? 'shows sidebar as updatable' : 'shows sidebar as read only'
+      end
+
+      context 'when user is agent-customer' do
+        let(:user)     { create(:agent_and_customer) }
+        let(:customer) { user }
+
+        include_examples agent_customer ? 'shows sidebar as updatable' : 'shows sidebar as read only'
       end
     end
 
-    context 'when ticket is closed and groups.follow_up_possible is "new_ticket_after_certain_time" reopen is not possible' do
-      let(:group) { create(:group, follow_up_possible: 'new_ticket_after_certain_time', reopen_time_in_days: 4) }
+    context 'when ticket is closed and groups.follow_up_possible is "yes"' do
+      let(:group) { create(:group, follow_up_possible: 'yes') }
 
-      it 'show sidebar as read only' do
-        within(:active_content) do
-          expect(page).to have_css('.sidebar [name=state_id]:disabled')
-        end
+      include_examples 'check roles', customer: true, agent: true, agent_customer: true
+    end
+
+    context 'when ticket is closed and groups.follow_up_possible is "new_ticket"' do
+      let(:group) { create(:group, follow_up_possible: 'new_ticket') }
+
+      include_examples 'check roles', customer: false, agent: true, agent_customer: false
+    end
+
+    context 'when ticket is closed and groups.follow_up_possible is "new_ticket_after_certain_time"' do
+      let(:group) { create(:group, follow_up_possible: 'new_ticket_after_certain_time', reopen_time_in_days: 5) }
+
+      context 'when ticket was closed within the timeframe' do
+        let(:close_time) { 3.days.ago }
+
+        include_examples 'check roles', customer: true, agent: true, agent_customer: true
+      end
+
+      context 'when ticket was closed outside of the timeframe' do
+        let(:close_time) { 1.month.ago }
+
+        include_examples 'check roles', customer: false, agent: true, agent_customer: false
       end
     end
   end
-
 end