Refactoring: Created generic UploadCache class, controller and routes to handle attachment upload.

app/assets/javascripts/app/controllers/_ui_element/richtext.coffee

@@ -38,8 +38,7 @@ class App.UiElement.richtext
         # delete attachment from storage
         App.Ajax.request(
           type:        'DELETE'
-          url:         "#{App.Config.get('api_path')}/ticket_attachment_upload"
+          url:         "#{App.Config.get('api_path')}/upload_caches/#{@form_id}/items/#{id}"
-          data:        JSON.stringify(id: id),
           processData: false
         )
 
@@ -57,59 +56,58 @@ class App.UiElement.richtext
       @attachmentsHolder     = item.find('.attachments')
       @cancelContainer       = item.find('.js-cancel')
 
-      u = => html5Upload.initialize(
+      upload_initialize_callback = =>
-        uploadUrl:              App.Config.get('api_path') + '/ticket_attachment_upload'
+        form_id = item.closest('form').find('[name=form_id]').val()
-        dropContainer:          item.closest('form').get(0)
+        html5Upload.initialize(
-        cancelContainer:        @cancelContainer
+          uploadUrl:              "#{App.Config.get('api_path')}/upload_caches/#{form_id}"
-        inputField:             item.find('input').get(0)
+          dropContainer:          item.closest('form').get(0)
-        maxSimultaneousUploads: 1,
+          cancelContainer:        @cancelContainer
-        key:                    'File'
+          inputField:             item.find('input').get(0)
-        data:
+          maxSimultaneousUploads: 1,
-          form_id: item.closest('form').find('[name=form_id]').val()
+          key:                    'File'
-        onFileAdded: (file) =>
+          onFileAdded: (file) =>
 
-          file.on(
+            file.on(
-            onStart: =>
+              onStart: =>
-              @attachmentPlaceholder.addClass('hide')
+                @attachmentPlaceholder.addClass('hide')
-              @attachmentUpload.removeClass('hide')
+                @attachmentUpload.removeClass('hide')
-              @cancelContainer.removeClass('hide')
+                @cancelContainer.removeClass('hide')
-              item.find('[contenteditable]').trigger('fileUploadStart')
+                item.find('[contenteditable]').trigger('fileUploadStart')
-              App.Log.debug 'UiElement.richtext', 'upload start'
+                App.Log.debug 'UiElement.richtext', 'upload start'
 
-            onAborted: =>
+              onAborted: =>
-              @attachmentPlaceholder.removeClass('hide')
+                @attachmentPlaceholder.removeClass('hide')
-              @attachmentUpload.addClass('hide')
+                @attachmentUpload.addClass('hide')
-              item.find('input').val('')
+                item.find('input').val('')
-              item.find('[contenteditable]').trigger('fileUploadStop', ['aborted'])
+                item.find('[contenteditable]').trigger('fileUploadStop', ['aborted'])
 
-            # Called after received response from the server
+              # Called after received response from the server
-            onCompleted: (response) =>
+              onCompleted: (response) =>
-              response = JSON.parse(response)
+                response = JSON.parse(response)
 
-              @attachmentPlaceholder.removeClass('hide')
+                @attachmentPlaceholder.removeClass('hide')
-              @attachmentUpload.addClass('hide')
+                @attachmentUpload.addClass('hide')
 
-              # reset progress bar
+                # reset progress bar
-              @progressBar.width(parseInt(0) + '%')
+                @progressBar.width(parseInt(0) + '%')
-              @progressText.text('')
+                @progressText.text('')
 
-              renderFile(response.data)
+                renderFile(response.data)
-              item.find('input').val('')
+                item.find('input').val('')
-              item.find('[contenteditable]').trigger('fileUploadStop', ['completed'])
+                item.find('[contenteditable]').trigger('fileUploadStop', ['completed'])
-              App.Log.debug 'UiElement.richtext', 'upload complete', response.data
+                App.Log.debug 'UiElement.richtext', 'upload complete', response.data
 
-            # Called during upload progress, first parameter
+              # Called during upload progress, first parameter
-            # is decimal value from 0 to 100.
+              # is decimal value from 0 to 100.
-            onProgress: (progress, fileSize, uploadedBytes) =>
+              onProgress: (progress, fileSize, uploadedBytes) =>
-              @progressBar.width(parseInt(progress) + '%')
+                @progressBar.width(parseInt(progress) + '%')
-              @progressText.text(parseInt(progress))
+                @progressText.text(parseInt(progress))
-              # hide cancel on 90%
+                # hide cancel on 90%
-              if parseInt(progress) >= 90
+                if parseInt(progress) >= 90
-                @cancelContainer.addClass('hide')
+                  @cancelContainer.addClass('hide')
-              App.Log.debug 'UiElement.richtext', 'uploadProgress ', parseInt(progress)
+                App.Log.debug 'UiElement.richtext', 'uploadProgress ', parseInt(progress)
-
+            )
-          )
+        )
-      )
+      App.Delay.set(upload_initialize_callback, 100, undefined, 'form_upload')
-      App.Delay.set(u, 100, undefined, 'form_upload')
 
     item

app/assets/javascripts/app/controllers/_ui_element/textarea.coffee

@@ -22,11 +22,10 @@ class App.UiElement.textarea
 
         # only add upload item if html element exists
         if $('#' + fileUploaderId )[0]
+          form_id = item.closest('form').find('[name=form_id]').val()
           $('#' + fileUploaderId ).fineUploader(
             request:
-              endpoint: App.Config.get('api_path') + '/ticket_attachment_upload'
+              endpoint: "#{App.Config.get('api_path')}/upload_caches/#{form_id}"
-              params:
-                form_id: item.closest('form').find('[name=form_id]').val()
             text:
               uploadButton: App.Utils.icon('paperclip')
             template: '<div class="qq-uploader">' +

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

@@ -958,8 +958,7 @@ class App.TicketZoom extends App.Controller
     # reset/delete uploaded attachments
     App.Ajax.request(
       type:  'DELETE'
-      url:   App.Config.get('api_path') + '/ticket_attachment_upload'
+      url:   "#{App.Config.get('api_path')}/upload_caches/#{@form_id}"
-      data:  JSON.stringify(form_id: @form_id)
       processData: false
     )
 

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

@@ -172,13 +172,11 @@ class App.TicketZoomArticleNew extends App.Controller
     })
 
     html5Upload.initialize(
-      uploadUrl:       App.Config.get('api_path') + '/ticket_attachment_upload'
+      uploadUrl:       "#{App.Config.get('api_path')}/upload_caches/#{@form_id}"
       dropContainer:   @$('.article-add').get(0)
       cancelContainer: @cancelContainer
       inputField:      @$('.article-attachment input').get(0)
       key:             'File'
-      data:
-        form_id: @form_id
       maxSimultaneousUploads: 1
       onFileAdded:            (file) =>
 
@@ -599,9 +597,8 @@ class App.TicketZoomArticleNew extends App.Controller
 
       # delete attachment from storage
       App.Ajax.request(
-        type:  'DELETE'
+        type:        'DELETE'
-        url:   App.Config.get('api_path') + '/ticket_attachment_upload'
+        url:         "#{App.Config.get('api_path')}/upload_caches/#{@form_id}/items/#{id}"
-        data:  JSON.stringify(id: id)
         processData: false
       )
 

app/controllers/concerns/creates_ticket_articles.rb

@@ -55,10 +55,7 @@ module CreatesTicketArticles
 
     # find attachments in upload cache
     if form_id
-      article.attachments = Store.list(
+      article.attachments = UploadCache.new(form_id).attachments
-        object: 'UploadCache',
-        o_id:   form_id,
-      )
     end
 
     # set subtype of present
@@ -130,11 +127,8 @@ module CreatesTicketArticles
       .first { |taskbar| taskbar.persisted_form_id == form_id }
       &.update!(state: {})
 
-    # remove attachments from upload cache
+    # remove temporary attachment cache
-    Store.remove(
+    UploadCache.new(form_id).destroy
-      object: 'UploadCache',
-      o_id:   form_id,
-    )
 
     article
   end

app/controllers/ticket_articles_controller.rb

@@ -154,66 +154,6 @@ class TicketArticlesController < ApplicationController
     raise Exceptions::NotAuthorized, 'Not authorized (admin permission required)!'
   end
 
-  # DELETE /ticket_attachment_upload
-  def ticket_attachment_upload_delete
-
-    if params[:id].present?
-      Store.remove_item(params[:id])
-      render json: {
-        success: true,
-      }
-      return
-    end
-
-    if params[:form_id].present?
-      Store.remove(
-        object: 'UploadCache',
-        o_id:   params[:form_id],
-      )
-      render json: {
-        success: true,
-      }
-      return
-    end
-
-    render json: { message: 'No such id or form_id!' }, status: :unprocessable_entity
-  end
-
-  # POST /ticket_attachment_upload
-  def ticket_attachment_upload_add
-
-    # store file
-    file = params[:File]
-    content_type = file.content_type
-    if !content_type || content_type == 'application/octet-stream'
-      content_type = if MIME::Types.type_for(file.original_filename).first
-                       MIME::Types.type_for(file.original_filename).first.content_type
-                     else
-                       'application/octet-stream'
-                     end
-    end
-    headers_store = {
-      'Content-Type' => content_type
-    }
-    store = Store.add(
-      object:      'UploadCache',
-      o_id:        params[:form_id],
-      data:        file.read,
-      filename:    file.original_filename,
-      preferences: headers_store
-    )
-
-    # return result
-    render json: {
-      success: true,
-      data:    {
-        id:       store.id,
-        filename: file.original_filename,
-        size:     store.size,
-      }
-    }
-  end
-
   # POST /ticket_attachment_upload_clone_by_article
   def ticket_attachment_upload_clone_by_article
     article = Ticket::Article.find(params[:article_id])

app/controllers/upload_caches_controller.rb

@@ -0,0 +1,58 @@
+# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
+
+class UploadCachesController < ApplicationController
+  prepend_before_action :authentication_check
+
+  # POST /upload_caches/1
+  def update
+    file = params[:File]
+    content_type = file.content_type
+    if !content_type || content_type == 'application/octet-stream'
+      mime_type    = MIME::Types.type_for(file.original_filename).first
+      content_type = mime_type&.content_type || 'application/octet-stream'
+    end
+    headers_store = {
+      'Content-Type' => content_type
+    }
+
+    store = cache.add(
+      filename:    file.original_filename,
+      data:        file.read,
+      preferences: headers_store
+    )
+
+    # return result
+    render json: {
+      success: true,
+      data:    {
+        id:       store.id, # TODO: rename?
+        filename: file.original_filename,
+        size:     store.size,
+      }
+    }
+  end
+
+  # DELETE /upload_caches/1
+  def destroy
+    cache.destroy
+
+    render json: {
+      success: true,
+    }
+  end
+
+  # DELETE /upload_caches/1/items/1
+  def remove_item
+    cache.remove_item(params[:store_id])
+
+    render json: {
+      success: true,
+    }
+  end
+
+  private
+
+  def cache
+    UploadCache.new(params[:id])
+  end
+end

app/models/taskbar.rb

@@ -54,7 +54,7 @@ class Taskbar < ApplicationModel
   def attachments
     return [] if persisted_form_id.blank?
 
-    Store.list(object: 'UploadCache', o_id: persisted_form_id)
+    UploadCache.new(persisted_form_id).attachments
   end
 
   def add_attachments_to_attributes(attributes)

config/routes/ticket.rb

@@ -42,8 +42,6 @@ Zammad::Application.routes.draw do
   match api_path + '/ticket_articles/:id',                           to: 'ticket_articles#update',          via: :put
   match api_path + '/ticket_articles/:id',                           to: 'ticket_articles#destroy',     via: :delete
   match api_path + '/ticket_attachment/:ticket_id/:article_id/:id',  to: 'ticket_articles#attachment',      via: :get
-  match api_path + '/ticket_attachment_upload',                      to: 'ticket_articles#ticket_attachment_upload_add', via: :post
-  match api_path + '/ticket_attachment_upload',                      to: 'ticket_articles#ticket_attachment_upload_delete', via: :delete
   match api_path + '/ticket_attachment_upload_clone_by_article/:article_id', to: 'ticket_articles#ticket_attachment_upload_clone_by_article', via: :post
   match api_path + '/ticket_article_plain/:id',                      to: 'ticket_articles#article_plain',   via: :get
 

config/routes/upload_cache.rb

@@ -0,0 +1,9 @@
+Zammad::Application.routes.draw do
+  api_path = Rails.configuration.api_path
+
+  # upload cache
+  match api_path + '/upload_caches/:id',                 to: 'upload_caches#update', via: :post
+  match api_path + '/upload_caches/:id',                 to: 'upload_caches#destroy', via: :delete
+  match api_path + '/upload_caches/:id/items/:store_id', to: 'upload_caches#remove_item', via: :delete
+
+end

lib/upload_cache.rb

@@ -0,0 +1,98 @@
+# A wrapper class around Store that handles temporary attachment uploads
+# and provides an interface for those.
+class UploadCache
+
+  attr_reader :id
+
+  # Initializes a UploadCache for a given form_id.
+  #
+  # @example
+  #   cache = UploadCache.new(form_id)
+  #
+  # @return [UploadCache]
+  def initialize(id)
+    # conversion to Integer is required for proper Store#o_id comparsion
+    @id = id.to_i
+  end
+
+  # Adds a Store item with the given attributes for the form_id.
+  #
+  # @see Store#add
+  #
+  # @example
+  #   cache = UploadCache.new(form_id)
+  #   store = cache.add(
+  #     filename:    file.original_filename,
+  #     data:        file.read,
+  #     preferences: {
+  #       'Content-Type' => 'application/octet-stream'
+  #     }
+  #   )
+  #
+  # @return [Store] the created Store item
+  def add(args)
+    Store.add(
+      args.merge(
+        object: store_object,
+        o_id:   id,
+      )
+    )
+  end
+
+  # Provides all Store items associated to the form_id.
+  #
+  # @see Store#list
+  #
+  # @example
+  #   attachments = UploadCache.new(form_id).attachments
+  #
+  # @return [Array<Store>] an enumerator of Store items
+  def attachments
+    Store.list(
+      object: store_object,
+      o_id:   id,
+    )
+  end
+
+  # Removes all Store items associated to the form_id.
+  #
+  # @see Store#remove
+  #
+  # @example
+  #   UploadCache.new(form_id).destroy
+  #
+  def destroy
+    Store.remove(
+      object: store_object,
+      o_id:   id,
+    )
+  end
+
+  # Removes all Store items associated to the form_id.
+  #
+  # @see Store#remove
+  #
+  # @example
+  #   UploadCache.new(form_id).remove_item(store_id)
+  #
+  # @raise [Exceptions::UnprocessableEntity] in cases where a Store item should get deleted that is not an UploadCache item
+  #
+  def remove_item(store_id = nil)
+    store = Store.find(store_id)
+    if store.o_id != id || store.store_object_id != store_object_id
+      raise Exceptions::UnprocessableEntity, "Attempt to delete Store item #{store_id} that is not bound to UploadCache object"
+    end
+
+    Store.remove_item(store_id)
+  end
+
+  private
+
+  def store_object
+    self.class.name
+  end
+
+  def store_object_id
+    Store::Object.lookup(name: store_object).id
+  end
+end

spec/fixtures/upload/hello_world.txt

@@ -0,0 +1 @@
+Hello World!

spec/lib/upload_cache_spec.rb

@@ -0,0 +1,114 @@
+require 'rails_helper'
+
+RSpec.describe UploadCache do
+
+  let(:subject) { described_class.new(1337) }
+
+  # required for adding items to the Store
+  before { UserInfo.current_user_id = 1 }
+
+  describe '#initialize' do
+
+    it 'converts given (form_)id to an Integer' do
+      expect(described_class.new('1337').id).to eq(1337)
+    end
+  end
+
+  describe '#add' do
+
+    it 'adds a Store item' do
+      expect do
+        subject.add(
+          data:        'content_file3_normally_should_be_an_image',
+          filename:    'some_file3.jpg',
+          preferences: {
+            'Content-Type'        => 'image/jpeg',
+            'Mime-Type'           => 'image/jpeg',
+            'Content-Disposition' => 'attached',
+          },
+        )
+      end.to change { Store.count }.by(1)
+    end
+  end
+
+  describe '#attachments' do
+
+    before do
+      subject.add(
+        data:        'hello world',
+        filename:    'some.txt',
+        preferences: {
+          'Content-Type' => 'text/plain',
+        },
+      )
+    end
+
+    it 'returns all Store items' do
+      attachments = subject.attachments
+
+      expect(attachments.count).to be(1)
+      expect(attachments).to include(Store.last)
+    end
+  end
+
+  describe '#destroy' do
+
+    before do
+      subject.add(
+        data:        'hello world',
+        filename:    'some.txt',
+        preferences: {
+          'Content-Type' => 'text/plain',
+        },
+      )
+
+      subject.add(
+        data:        'hello other world',
+        filename:    'another_some.txt',
+        preferences: {
+          'Content-Type' => 'text/plain',
+        },
+      )
+    end
+
+    it 'removes all added Store items' do
+      expect { subject.destroy }.to change { Store.count }.by(-2)
+    end
+  end
+
+  describe '#remove_item' do
+
+    before do
+      subject.add(
+        data:        'hello world',
+        filename:    'some.txt',
+        preferences: {
+          'Content-Type' => 'text/plain',
+        },
+      )
+    end
+
+    it 'removes the Store item matching the given ID' do
+      expect { subject.remove_item(Store.last.id) }.to change { Store.count }.by(-1)
+    end
+
+    it 'prevents removage of non UploadCache Store items' do
+
+      item = Store.add(
+        object:      'Ticket',
+        o_id:        1,
+        data:        "Can't touch this",
+        filename:    'keep.txt',
+        preferences: {
+          'Content-Type' => 'text/plain',
+        },
+      )
+
+      expect { subject.remove_item(item.id) }.to raise_error(Exceptions::UnprocessableEntity)
+    end
+
+    it 'fails for non existing UploadCache Store items' do
+      expect { subject.remove_item(1337) }.to raise_error(ActiveRecord::RecordNotFound)
+    end
+  end
+end

spec/requests/upload_cache_spec.rb

@@ -0,0 +1,82 @@
+require 'rails_helper'
+
+RSpec.describe 'UploadCache', type: :request do
+
+  let(:user)    { create(:customer_user) }
+  let(:form_id) { 1337 }
+  let(:upload_cache) { UploadCache.new(form_id) }
+
+  # required for adding items to the Store
+  before { UserInfo.current_user_id = 1 }
+
+  before { authenticated_as(user) }
+
+  describe '/upload_caches/:id' do
+
+    context 'for POST requests' do
+
+      it 'adds items to UploadCache' do
+        params = {
+          File: fixture_file_upload('upload/hello_world.txt', 'text/plain')
+        }
+        post "/api/v1/upload_caches/#{form_id}", params: params
+
+        expect(response).to have_http_status(200)
+      end
+
+      it 'detects Content-Type for binary uploads' do
+        params = {
+          File: fixture_file_upload('upload/hello_world.txt', 'application/octet-stream')
+        }
+        post "/api/v1/upload_caches/#{form_id}", params: params
+
+        expect(Store.last.preferences['Content-Type']).to eq('text/plain')
+      end
+    end
+
+    context 'for DELETE requests' do
+
+      before do
+        2.times do |iteration|
+          upload_cache.add(
+            data:        "Can't touch this #{iteration}",
+            filename:    'keep.txt',
+            preferences: {
+              'Content-Type' => 'text/plain',
+            },
+          )
+        end
+      end
+
+      it 'removes all form_id UploadCache items' do
+        expect do
+          delete "/api/v1/upload_caches/#{form_id}", as: :json
+        end.to change { upload_cache.attachments }.to([])
+      end
+    end
+  end
+
+  describe '/upload_caches/:id/items/:store_id' do
+
+    context 'for DELETE requests' do
+
+      before do
+        upload_cache.add(
+          data:        "Can't touch this",
+          filename:    'keep.txt',
+          preferences: {
+            'Content-Type' => 'text/plain',
+          },
+        )
+      end
+
+      it 'removes a UploadCache item by given store id' do
+
+        store_id = upload_cache.attachments.first.id
+        delete "/api/v1/upload_caches/#{form_id}/items/#{store_id}", as: :json
+
+        expect(Store.exists?(store_id)).to be(false)
+      end
+    end
+  end
+end