From 7bb7e7a438d1bb4a969d878d06c705f74cd11701 Mon Sep 17 00:00:00 2001
From: Mantas <mantas@idev.lt>
Date: Thu, 3 May 2018 17:09:33 +0300
Subject: [PATCH] Fixes #1123 Attachments get doubled when suspending a draft

---
 .../controllers/agent_ticket_create.coffee    |  2 +
 .../app/controllers/ticket_zoom.coffee        | 12 +++-
 .../ticket_zoom/article_new.coffee            |  4 ++
 .../javascripts/app/models/taskbar.coffee     |  2 +-
 .../concerns/creates_ticket_articles.rb       |  6 ++
 app/models/store.rb                           |  4 ++
 app/models/taskbar.rb                         | 32 ++++++++-
 app/models/ticket/article.rb                  | 27 ++------
 script/build/test_slice_tests.sh              | 12 ++++
 ...te_attachment_missing_after_reload_test.rb | 60 +++++++++++++++++
 ...ket_update_with_attachment_refresh_test.rb | 66 +++++++++++++++++++
 11 files changed, 203 insertions(+), 24 deletions(-)
 create mode 100644 test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
 create mode 100644 test/browser/agent_ticket_update_with_attachment_refresh_test.rb

diff --git a/app/assets/javascripts/app/controllers/agent_ticket_create.coffee b/app/assets/javascripts/app/controllers/agent_ticket_create.coffee
index fc202802d..153b1bc4f 100644
--- a/app/assets/javascripts/app/controllers/agent_ticket_create.coffee
+++ b/app/assets/javascripts/app/controllers/agent_ticket_create.coffee
@@ -256,6 +256,8 @@ class App.TicketCreate extends App.Controller
       params = template.options
     else if App.TaskManager.get(@taskKey) && !_.isEmpty(App.TaskManager.get(@taskKey).state)
       params = App.TaskManager.get(@taskKey).state
+      params.attachments = App.TaskManager.get(@taskKey).attachments
+
       if !_.isEmpty(params['form_id'])
         @formId = params['form_id']
 
diff --git a/app/assets/javascripts/app/controllers/ticket_zoom.coffee b/app/assets/javascripts/app/controllers/ticket_zoom.coffee
index cc9dd7fe0..df80f1048 100644
--- a/app/assets/javascripts/app/controllers/ticket_zoom.coffee
+++ b/app/assets/javascripts/app/controllers/ticket_zoom.coffee
@@ -439,7 +439,7 @@ class App.TicketZoom extends App.Controller
       #if @shown
       #  @attributeBar.start()
 
-      @form_id = App.ControllerForm.formId()
+      @form_id = @taskGet('article').form_id || App.ControllerForm.formId()
 
       @articleNew = new App.TicketZoomArticleNew(
         ticket:    @ticket
@@ -666,7 +666,6 @@ class App.TicketZoom extends App.Controller
     else
       delete currentParams.article.attachments
 
-    # remove not needed attributes
     delete currentParams.article.form_id
 
     if @permissionCheck('ticket.customer')
@@ -966,6 +965,10 @@ class App.TicketZoom extends App.Controller
   taskGet: (area) =>
     return {} if !App.TaskManager.get(@taskKey)
     @localTaskData = App.TaskManager.get(@taskKey).state || {}
+
+    if _.isObject(@localTaskData.article) && _.isArray(App.TaskManager.get(@taskKey).attachments)
+      @localTaskData.article['attachments'] = App.TaskManager.get(@taskKey).attachments
+
     if area
       if !@localTaskData[area]
         @localTaskData[area] = {}
@@ -980,10 +983,15 @@ class App.TicketZoom extends App.Controller
 
   taskUpdateAll: (data) =>
     @localTaskData = data
+    @localTaskData.article['form_id'] = @form_id
     App.TaskManager.update(@taskKey, { 'state': @localTaskData })
 
   # reset task state
   taskReset: =>
+    @form_id = App.ControllerForm.formId()
+    @articleNew.form_id = @form_id
+    @articleNew.render()
+
     @localTaskData =
       ticket:  {}
       article: {}
diff --git a/app/assets/javascripts/app/controllers/ticket_zoom/article_new.coffee b/app/assets/javascripts/app/controllers/ticket_zoom/article_new.coffee
index a8138a5d3..d02f7db5a 100644
--- a/app/assets/javascripts/app/controllers/ticket_zoom/article_new.coffee
+++ b/app/assets/javascripts/app/controllers/ticket_zoom/article_new.coffee
@@ -47,6 +47,10 @@ class App.TicketZoomArticleNew extends App.Controller
     if @defaults.body or @isIE10()
       @openTextarea(null, true)
 
+    if _.isArray(@defaults.attachments)
+      for attachment in @defaults.attachments
+        @renderAttachment(attachment)
+
     # set article type and expand text area
     @bind('ui::ticket::setArticleType', (data) =>
       return if data.ticket.id.toString() isnt @ticket_id.toString()
diff --git a/app/assets/javascripts/app/models/taskbar.coffee b/app/assets/javascripts/app/models/taskbar.coffee
index 6dfb7d9f0..142c1d163 100644
--- a/app/assets/javascripts/app/models/taskbar.coffee
+++ b/app/assets/javascripts/app/models/taskbar.coffee
@@ -1,5 +1,5 @@
 class App.Taskbar extends App.Model
-  @configure 'Taskbar', 'key', 'client_id', 'callback', 'state', 'params', 'prio', 'notify', 'active', 'updated_at'
+  @configure 'Taskbar', 'key', 'client_id', 'callback', 'state', 'params', 'prio', 'notify', 'active', 'attachments', 'updated_at'
 #  @extend Spine.Model.Local
   @extend Spine.Model.Ajax
   @url: @apiPath + '/taskbar'
diff --git a/app/controllers/concerns/creates_ticket_articles.rb b/app/controllers/concerns/creates_ticket_articles.rb
index 6d5ca281b..a048cbe81 100644
--- a/app/controllers/concerns/creates_ticket_articles.rb
+++ b/app/controllers/concerns/creates_ticket_articles.rb
@@ -119,6 +119,12 @@ module CreatesTicketArticles
 
     return article if form_id.blank?
 
+    # clear in-progress state from taskbar
+    Taskbar
+      .where(user_id: current_user.id)
+      .first { |taskbar| taskbar.persisted_form_id == form_id }
+      &.update!(state: {})
+
     # remove attachments from upload cache
     Store.remove(
       object: 'UploadCache',
diff --git a/app/models/store.rb b/app/models/store.rb
index 41ddb9b17..516f06588 100644
--- a/app/models/store.rb
+++ b/app/models/store.rb
@@ -192,6 +192,10 @@ returns
     path
   end
 
+  def attributes_for_display
+    slice :id, :filename, :size, :preferences
+  end
+
   def provider
     file = Store::File.find_by(id: store_file_id)
     if !file
diff --git a/app/models/taskbar.rb b/app/models/taskbar.rb
index ba4c64fe8..b96f2d441 100644
--- a/app/models/taskbar.rb
+++ b/app/models/taskbar.rb
@@ -16,8 +16,9 @@ class Taskbar < ApplicationModel
     return false if state.blank?
     state.each_value do |value|
       if value.is_a? Hash
-        value.each_value do |value1|
+        value.each do |key1, value1|
           next if value1.blank?
+          next if key1 == 'form_id'
           return true
         end
       else
@@ -28,8 +29,37 @@ class Taskbar < ApplicationModel
     false
   end
 
+  def attributes_with_association_names
+    add_attachments_to_attributes(super)
+  end
+
+  def attributes_with_association_ids
+    add_attachments_to_attributes(super)
+  end
+
+  def as_json(options = {})
+    add_attachments_to_attributes(super)
+  end
+
+  # form_id is saved directly in a new ticket, but inside of the article when updating an existing ticket
+  def persisted_form_id
+    state&.dig(:form_id) || state&.dig(:article, :form_id)
+  end
+
   private
 
+  def attachments
+    return [] if persisted_form_id.blank?
+
+    Store.list(object: 'UploadCache', o_id: persisted_form_id)
+  end
+
+  def add_attachments_to_attributes(attributes)
+    attributes.tap do |result|
+      result['attachments'] = attachments.map(&:attributes_for_display)
+    end
+  end
+
   def update_last_contact
     return true if local_update
     return true if changes.blank?
diff --git a/app/models/ticket/article.rb b/app/models/ticket/article.rb
index a6442bc1d..4e5a14494 100644
--- a/app/models/ticket/article.rb
+++ b/app/models/ticket/article.rb
@@ -238,16 +238,7 @@ returns
 
   def attributes_with_association_names
     attributes = super
-    attributes['attachments'] = []
-    attachments.each do |attachment|
-      item = {
-        id: attachment['id'],
-        filename: attachment['filename'],
-        size: attachment['size'],
-        preferences: attachment['preferences'],
-      }
-      attributes['attachments'].push item
-    end
+    add_attachments_to_attributes(attributes)
     Ticket::Article.insert_urls(attributes)
   end
 
@@ -266,16 +257,7 @@ returns
 
   def attributes_with_association_ids
     attributes = super
-    attributes['attachments'] = []
-    attachments.each do |attachment|
-      item = {
-        id: attachment['id'],
-        filename: attachment['filename'],
-        size: attachment['size'],
-        preferences: attachment['preferences'],
-      }
-      attributes['attachments'].push item
-    end
+    add_attachments_to_attributes(attributes)
     if attributes['body'] && attributes['content_type'] =~ %r{text/html}i
       attributes['body'] = HtmlSanitizer.dynamic_image_size(attributes['body'])
     end
@@ -284,6 +266,11 @@ returns
 
   private
 
+  def add_attachments_to_attributes(attributes)
+    attributes['attachments'] = attachments.map(&:attributes_for_display)
+    attributes
+  end
+
   # strip not wanted chars
   def check_subject
     return true if subject.blank?
diff --git a/script/build/test_slice_tests.sh b/script/build/test_slice_tests.sh
index 5f7f178dd..ecdf63c91 100755
--- a/script/build/test_slice_tests.sh
+++ b/script/build/test_slice_tests.sh
@@ -27,6 +27,7 @@ if [ "$LEVEL" == '1' ]; then
   rm test/browser/agent_ticket_attachment_test.rb
   rm test/browser/agent_ticket_auto_assignment_test.rb
   rm test/browser/agent_ticket_create_available_types_test.rb
+  rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   rm test/browser/agent_ticket_create_default_type_test.rb
   rm test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -49,6 +50,7 @@ if [ "$LEVEL" == '1' ]; then
   rm test/browser/agent_ticket_update3_test.rb
   rm test/browser/agent_ticket_update4_test.rb
   rm test/browser/agent_ticket_update5_test.rb
+  rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   rm test/browser/agent_ticket_update_and_reload_test.rb
   rm test/browser/agent_user_manage_test.rb
   rm test/browser/agent_user_profile_test.rb
@@ -102,6 +104,7 @@ elif [ "$LEVEL" == '2' ]; then
   rm test/browser/agent_ticket_attachment_test.rb
   rm test/browser/agent_ticket_auto_assignment_test.rb
   rm test/browser/agent_ticket_create_available_types_test.rb
+  rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   rm test/browser/agent_ticket_create_default_type_test.rb
   rm test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -124,6 +127,7 @@ elif [ "$LEVEL" == '2' ]; then
   # test/browser/agent_ticket_update3_test.rb
   # test/browser/agent_ticket_update4_test.rb
   # rm test/browser/agent_ticket_update5_test.rb
+  # rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   # test/browser/agent_ticket_update_and_reload_test.rb
   rm test/browser/agent_user_manage_test.rb
   rm test/browser/agent_user_profile_test.rb
@@ -177,6 +181,7 @@ elif [ "$LEVEL" == '3' ]; then
   # test/browser/agent_ticket_attachment_test.rb
   # test/browser/agent_ticket_auto_assignment_test.rb
   # rm test/browser/agent_ticket_create_available_types_test.rb
+  # rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   #rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   # rm test/browser/agent_ticket_create_default_type_test.rb
   # test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -199,6 +204,7 @@ elif [ "$LEVEL" == '3' ]; then
   rm test/browser/agent_ticket_update3_test.rb
   rm test/browser/agent_ticket_update4_test.rb
   rm test/browser/agent_ticket_update5_test.rb
+  rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   rm test/browser/agent_ticket_update_and_reload_test.rb
   rm test/browser/agent_user_manage_test.rb
   rm test/browser/agent_user_profile_test.rb
@@ -252,6 +258,7 @@ elif [ "$LEVEL" == '4' ]; then
   rm test/browser/agent_ticket_attachment_test.rb
   rm test/browser/agent_ticket_auto_assignment_test.rb
   rm test/browser/agent_ticket_create_available_types_test.rb
+  rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   rm test/browser/agent_ticket_create_default_type_test.rb
   rm test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -274,6 +281,7 @@ elif [ "$LEVEL" == '4' ]; then
   rm test/browser/agent_ticket_update3_test.rb
   rm test/browser/agent_ticket_update4_test.rb
   rm test/browser/agent_ticket_update5_test.rb
+  rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   rm test/browser/agent_ticket_update_and_reload_test.rb
   rm test/browser/agent_user_manage_test.rb
   rm test/browser/agent_user_profile_test.rb
@@ -326,6 +334,7 @@ elif [ "$LEVEL" == '5' ]; then
   rm test/browser/agent_ticket_attachment_test.rb
   rm test/browser/agent_ticket_auto_assignment_test.rb
   rm test/browser/agent_ticket_create_available_types_test.rb
+  rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   rm test/browser/agent_ticket_create_default_type_test.rb
   rm test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -348,6 +357,7 @@ elif [ "$LEVEL" == '5' ]; then
   rm test/browser/agent_ticket_update3_test.rb
   rm test/browser/agent_ticket_update4_test.rb
   rm test/browser/agent_ticket_update5_test.rb
+  rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   rm test/browser/agent_ticket_update_and_reload_test.rb
   # test/browser/agent_user_manage_test.rb
   # test/browser/agent_user_profile_test.rb
@@ -403,6 +413,7 @@ elif [ "$LEVEL" == '6' ]; then
   rm test/browser/agent_ticket_attachment_test.rb
   rm test/browser/agent_ticket_auto_assignment_test.rb
   rm test/browser/agent_ticket_create_available_types_test.rb
+  rm test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
   rm test/browser/agent_ticket_create_cc_tokenizer_test.rb
   rm test/browser/agent_ticket_create_default_type_test.rb
   rm test/browser/agent_ticket_create_reset_customer_selection_test.rb
@@ -425,6 +436,7 @@ elif [ "$LEVEL" == '6' ]; then
   rm test/browser/agent_ticket_update3_test.rb
   rm test/browser/agent_ticket_update4_test.rb
   rm test/browser/agent_ticket_update5_test.rb
+  rm test/browser/agent_ticket_update_with_attachment_refresh_test.rb
   rm test/browser/agent_ticket_update_and_reload_test.rb
   rm test/browser/agent_user_manage_test.rb
   rm test/browser/agent_user_profile_test.rb
diff --git a/test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb b/test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
new file mode 100644
index 000000000..874fcbe67
--- /dev/null
+++ b/test/browser/agent_ticket_create_attachment_missing_after_reload_test.rb
@@ -0,0 +1,60 @@
+require 'browser_test_helper'
+
+# https://github.com/zammad/zammad/issues/1123
+# Make sure attachment is shown after reloading a new in-progress ticket
+
+class AgentticketCreateAttachmentMissingAfterReloadTest < TestCase
+  def test_attachments
+    @browser = browser_instance
+    login(
+      username: 'agent1@example.com',
+      password: 'test',
+      url: browser_url,
+    )
+    tasks_close_all()
+
+    #
+    # attachment checks - new ticket
+    #
+
+    # create new ticket with no attachment, attachment check should pop up
+    ticket_create(
+      data: {
+        customer: 'nico',
+        group: 'Users',
+        title: 'test 6 - ticket 1',
+        body: 'test 6 - ticket 1 body',
+      },
+      do_not_submit: true,
+    )
+    sleep 1
+
+    # since selenium webdriver with firefox is not able to upload files, skipp here
+    # https://github.com/w3c/webdriver/issues/1230
+    return if browser == 'firefox'
+
+    # add attachment, attachment check should quiet
+    file_upload(
+      css:   '.content.active .attachmentPlaceholder-inputHolder input',
+      files: ['test/data/upload/upload1.txt'],
+    )
+
+    sleep 2
+
+    # check if attachment is shown
+    match(
+      css: '.content.active .newTicket .attachments .attachment:nth-child(1) .attachment-name',
+      value: 'upload1.txt'
+    )
+
+    @browser.navigate.refresh
+
+    sleep 1
+
+    # check if attachment is shown
+    match(
+      css: '.content.active .newTicket .attachments .attachment:nth-child(1) .attachment-name',
+      value: 'upload1.txt'
+    )
+  end
+end
diff --git a/test/browser/agent_ticket_update_with_attachment_refresh_test.rb b/test/browser/agent_ticket_update_with_attachment_refresh_test.rb
new file mode 100644
index 000000000..af4f0ab1d
--- /dev/null
+++ b/test/browser/agent_ticket_update_with_attachment_refresh_test.rb
@@ -0,0 +1,66 @@
+require 'browser_test_helper'
+
+# https://github.com/zammad/zammad/issues/1123
+# Make sure attachment is shown after reloading an in-progress ticket update
+
+class AgentTicketUpdateWithAttachmentRefreshTest < TestCase
+  def test_attachment_after_refresh
+    @browser = browser_instance
+    login(
+      username: 'agent1@example.com',
+      password: 'test',
+      url: browser_url,
+    )
+
+    #
+    # attachment checks - existing ticket
+    #
+
+    # create new ticket with no attachment, attachment check should pop up
+    ticket_create(
+      data: {
+        customer: 'nico',
+        group: 'Users',
+        title: 'test 6 - ticket 1',
+        body: 'test 6 - ticket 1 body',
+      },
+    )
+    sleep 1
+
+    # fill body
+    ticket_update(
+      data: {
+        body: 'keep me',
+      },
+      do_not_submit: true,
+    )
+
+    # since selenium webdriver with firefox is not able to upload files, skipp here
+    # https://github.com/w3c/webdriver/issues/1230
+    return if browser == 'firefox'
+
+    # add attachment, attachment check should quiet
+    file_upload(
+      css:   '.content.active .attachmentPlaceholder-inputHolder input',
+      files: ['test/data/upload/upload1.txt'],
+    )
+
+    sleep 2
+
+    # check if attachment is shown
+    match(
+      css: '.content.active .ticketZoom .attachments .attachment:nth-child(1) .attachment-name',
+      value: 'upload1.txt'
+    )
+
+    @browser.navigate.refresh
+
+    sleep 1
+
+    # check if attachment is shown
+    match(
+      css: '.content.active .ticketZoom .attachments .attachment:nth-child(1) .attachment-name',
+      value: 'upload1.txt'
+    )
+  end
+end