From e4a6a3944efa436cf7fb10e1181aa2f2277c6aa5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Zrim=C5=A1ek?= Date: Wed, 17 Aug 2016 09:44:53 +0200 Subject: [PATCH] Added deleting of files on S3 server when editing/removing a file. Some refactoring. --- app/assets/javascripts/application.js | 7 ++-- app/assets/javascripts/direct-upload.js | 24 ++++++------- app/assets/javascripts/my_modules/results.js | 24 ++----------- app/assets/javascripts/protocols/steps.js | 16 ++------- .../javascripts/results/result_assets.js | 8 +++-- .../javascripts/sitewide/draw_components.js | 12 ++++--- .../javascripts/users/registrations/edit.js | 12 ++----- app/controllers/assets_controller.rb | 36 ++++++------------- app/controllers/result_assets_controller.rb | 9 +---- app/controllers/steps_controller.rb | 33 +++++++++-------- app/models/asset.rb | 17 ++++++++- app/models/step.rb | 2 +- app/models/step_asset.rb | 2 +- 13 files changed, 83 insertions(+), 119 deletions(-) diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index a7c1df68a..17b81c6aa 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -116,8 +116,11 @@ function animateSpinner(el, start, options) { } /* - * Prevents user from accidentally leaving page when - * server is busy and notifies him with a message. + * Prevents user from accidentally leaving page when server is busy + * and notifies him with a message. + * + * NOTE: Don't prevent event propagation (ev.stopPropagation()), or + * else all events occurring when alert is up will be ignored. */ function preventLeavingPage(prevent, msg) { busy = prevent; diff --git a/app/assets/javascripts/direct-upload.js b/app/assets/javascripts/direct-upload.js index 20a540f47..8d848fef7 100644 --- a/app/assets/javascripts/direct-upload.js +++ b/app/assets/javascripts/direct-upload.js @@ -149,12 +149,17 @@ * TODO On S3 server upload error the other files that were already * asynchronously uploaded remain on the server, but should be deleted - this * should generally not happen, because all files should fail to upload in - * such case (connection error). + * such cases (like S3 server connection error), except if user cancels the + * upload while still in progress. But this can be abused! One way to solve + * it is to make request to our server for each file seperatelly, and not for + * all together as it is now, despite being less efficient. To make it + * bulletproof, post requests should be issued on server-side. */ - exports.directUpload = function (ev, form, signUrl, cb) { - form.clearFormErrors(); - form.removeBlankFileForms(); - $fileInputs = $(form).find("input[type=file]"); + exports.directUpload = function (ev, signUrl) { + $form = $(ev.target.form); + $form.clearFormErrors(); + $form.removeBlankFileForms(); + $fileInputs = $form.find("input[type=file]"); var signRequests = []; if ($fileInputs.length) { @@ -162,7 +167,6 @@ animateSpinner(); preventLeavingPage(true, I18n.t("general.file.uploading")); ev.preventDefault(); - ev.stopPropagation(); // Validates files and if OK gets upload post requests _.each($fileInputs, function (fileInput) { @@ -178,7 +182,6 @@ // After successful file validation and posts fetching if (signRequests.length) { var fileRequests = []; - var fileIds = []; if (signRequests.length == 1) { arguments = [arguments]; @@ -188,7 +191,6 @@ var jqXHR = responseData[2]; var data = JSON.parse(jqXHR.responseText); processPosts(ev, $fileInput, data.posts, fileRequests); - fileIds.push(data.asset_id); }); $.when.apply($, fileRequests).always(function () { @@ -196,11 +198,7 @@ preventLeavingPage(false); }).then(function () { // After successful posts processing and file uploading - $.each($fileInputs, function (fileIdx, fileInput) { - // Use file input to pass file id on submit - cb(fileInput, fileIds[fileIdx]); - }); - $(ev.target.form).submit(); + $form.submit(); }); } }, function () { diff --git a/app/assets/javascripts/my_modules/results.js b/app/assets/javascripts/my_modules/results.js index 284207f2f..ada319ac6 100644 --- a/app/assets/javascripts/my_modules/results.js +++ b/app/assets/javascripts/my_modules/results.js @@ -143,7 +143,6 @@ function applyCollapseLinkCallBack() { // Toggle collapse button collapseIcon.toggleClass("glyphicon-collapse-up", !collapsed); collapseIcon.toggleClass("glyphicon-collapse-down", collapsed); - }); } @@ -323,7 +322,8 @@ function processResult(ev, resultTypeEnum, forS3) { if(nameValid && filesValid) { if(forS3) { // Redirects file uploading to S3 - startFileUpload(ev, ev.target); + var url = '/asset_signature.json'; + directUpload(ev, url); } else { // Local file uploading animateSpinner(); @@ -347,26 +347,6 @@ function processResult(ev, resultTypeEnum, forS3) { } } -// S3 direct uploading -function startFileUpload(ev, btn) { - var $form = $(btn.form); - var $editFileInput = $form.find("input[name='result[asset_attributes][id]']").get(0); - var url = '/asset_signature.json'; - - directUpload(ev, $form, url, function (fileInput, fileId) { - if ($editFileInput) { - // edit mode - input field has to be removed - $editFileInput.value = fileId; - $(fileInput).remove(); - } else { - // create mode - fileInput.type = "hidden"; - fileInput.name = fileInput.name.replace("[file]", "[id]"); - fileInput.value = fileId; - } - }); -} - // This checks if the ctarget param exist in the // rendered url and opens the comment tab $(document).ready(function(){ diff --git a/app/assets/javascripts/protocols/steps.js b/app/assets/javascripts/protocols/steps.js index e301963f9..8fd05f4ef 100644 --- a/app/assets/javascripts/protocols/steps.js +++ b/app/assets/javascripts/protocols/steps.js @@ -576,7 +576,6 @@ $("[data-action='new-step']").on("ajax:success", function(e, data) { // experience is improved function processStep(ev, editMode, forS3) { var $form = $(ev.target.form); - $form.clearFormErrors(); $form.removeBlankExcelTables(editMode); $form.removeBlankFileForms(); @@ -591,7 +590,8 @@ function processStep(ev, editMode, forS3) { if (filesValid && checklistsValid && nameValid) { if (forS3) { // Redirects file uploading to S3 - startFileUpload(ev, ev.target); + var url = '/asset_signature.json'; + directUpload(ev, url); } else { // Local file uploading animateSpinner(); @@ -647,15 +647,3 @@ function renderTable(table) { $(table).find(".ht_master .wtHolder").css("height", "100%"); } } - -// S3 direct uploading -function startFileUpload(ev, btn) { - var $form = $(btn.form); - var url = '/asset_signature.json'; - - directUpload(ev, $form, url, function (fileInput, fileId) { - fileInput.type = "hidden"; - fileInput.name = fileInput.name.replace("[file]", "[id]"); - fileInput.value = fileId; - }); -} diff --git a/app/assets/javascripts/results/result_assets.js b/app/assets/javascripts/results/result_assets.js index 74b6bdb00..73aade08e 100644 --- a/app/assets/javascripts/results/result_assets.js +++ b/app/assets/javascripts/results/result_assets.js @@ -54,15 +54,17 @@ function formAjaxResultAsset($form) { $form .on("ajax:success", function(e, data) { $form.after(data.html); - var newResult = $form.next(); - initFormSubmitLinks(newResult); + var $newResult = $form.next(); + initFormSubmitLinks($newResult); $(this).remove(); applyEditResultAssetCallback(); applyCollapseLinkCallBack(); toggleResultEditButtons(true); initResultCommentTabAjax(); - expandResult(newResult); + expandResult($newResult); + $imgs = $newResult.find("img"); + reloadImages($imgs); }) .on("ajax:error", function(e, data) { $form.renderFormErrors("result", data.errors, true, e); diff --git a/app/assets/javascripts/sitewide/draw_components.js b/app/assets/javascripts/sitewide/draw_components.js index a9db3b320..520795cca 100644 --- a/app/assets/javascripts/sitewide/draw_components.js +++ b/app/assets/javascripts/sitewide/draw_components.js @@ -3,9 +3,11 @@ * force browser to reload/update cached image * (useful for AJAX calls). */ -function reloadImage(img) { - var src = $(img).attr("src"); - src = src.split("?", 1); - src += "?timestamp=" + new Date().getTime(); - $(img).attr("src", src); +function reloadImages(imgs) { + _.each(imgs, function (img) { + var src = $(img).attr("src"); + src = src.split("?", 1); + src += "?timestamp=" + new Date().getTime(); + $(img).attr("src", src); + }); } diff --git a/app/assets/javascripts/users/registrations/edit.js b/app/assets/javascripts/users/registrations/edit.js index cb2986009..2459d059b 100644 --- a/app/assets/javascripts/users/registrations/edit.js +++ b/app/assets/javascripts/users/registrations/edit.js @@ -71,19 +71,11 @@ function processFile(ev, forS3) { if(filesValidator(ev, $fileInput, FileTypeEnum.AVATAR)) { if(forS3) { // Redirects file uploading to S3 - startFileUpload(ev, ev.target); + var url = "/avatar_signature.json"; + directUpload(ev, url); } else { // Local file uploading animateSpinner(); } } } - -// S3 direct uploading -function startFileUpload(ev, btn) { - var $form = $(btn.form); - var url = "/avatar_signature.json"; - - directUpload(ev, $form, url, function () { - }); -} diff --git a/app/controllers/assets_controller.rb b/app/controllers/assets_controller.rb index 1739aece6..891823b84 100644 --- a/app/controllers/assets_controller.rb +++ b/app/controllers/assets_controller.rb @@ -8,33 +8,20 @@ class AssetsController < ApplicationController respond_to do |format| format.json { - if asset_params[:asset_id] - asset = Asset.find_by_id asset_params[:asset_id] - asset.file.destroy - asset.file_empty asset_params[:file].original_filename, asset_params[:file].size() - validationAsset = asset - else - # We can't verify file content (spoofing) of an empty - # file, so we use dummy validationAsset instead - asset = Asset.new_empty asset_params[:file].original_filename, asset_params[:file].size() - validationAsset = Asset.new(asset_params) - end - - # We need to validate again so that asset's - # after_validation gets triggered - if validationAsset.valid? - asset.save! - - posts = generate_upload_posts asset - render json: { - asset_id: asset.id, - posts: posts - } - else + asset = Asset.new(asset_params) + if asset.errors.any? + # We need to validate, although 'new' already does it, so that + # asset's after_validation gets triggered, which modifies errors + asset.valid? render json: { status: 'error', - errors: validationAsset.errors + errors: asset.errors } , status: :bad_request + else + posts = generate_upload_posts asset + render json: { + posts: posts + } end } end @@ -156,7 +143,6 @@ class AssetsController < ApplicationController def asset_params params.permit( - :asset_id, :file ) end diff --git a/app/controllers/result_assets_controller.rb b/app/controllers/result_assets_controller.rb index 362559976..80adafce1 100644 --- a/app/controllers/result_assets_controller.rb +++ b/app/controllers/result_assets_controller.rb @@ -32,14 +32,7 @@ class ResultAssetsController < ApplicationController end def create - asset_attrs = result_params[:asset_attributes] - - if asset_attrs and asset_attrs[:id] - @asset = Asset.find_by_id asset_attrs[:id] - else - @asset = Asset.new(result_params[:asset_attributes]) - end - + @asset = Asset.new(result_params[:asset_attributes]) @asset.created_by = current_user @asset.last_modified_by = current_user @result = Result.new( diff --git a/app/controllers/steps_controller.rb b/app/controllers/steps_controller.rb index ec1e8b744..56f25f0ac 100644 --- a/app/controllers/steps_controller.rb +++ b/app/controllers/steps_controller.rb @@ -30,21 +30,21 @@ class StepsController < ApplicationController def create if @direct_upload + new_assets = [] step_data = step_params.except(:assets_attributes) step_assets = step_params.slice(:assets_attributes) @step = Step.new(step_data) - if step_assets.size > 0 - step_assets[:assets_attributes].each do |i, data| - # Ignore destroy requests on create - if data[:_destroy].nil? and data[:id].present? - asset = Asset.find_by_id(data[:id]) - asset.created_by = current_user - asset.last_modified_by = current_user - @step.assets << asset - end + step_assets[:assets_attributes].each do |i, data| + # Ignore destroy requests on create + if data[:_destroy].nil? + asset = Asset.new(data) + asset.created_by = current_user + asset.last_modified_by = current_user + new_assets << asset end end + @step.assets << new_assets else @step = Step.new(step_params) end @@ -98,6 +98,13 @@ class StepsController < ApplicationController })}, status: :ok } else + # On error, delete the newly added files from S3, as they were + # uploaded on client-side (in case of client-side hacking of + # asset's signature response) + new_assets.each do |asset| + asset.destroy + end + format.json { render json: { html: render_to_string({ @@ -155,8 +162,7 @@ class StepsController < ApplicationController if step_assets.include? :assets_attributes step_assets[:assets_attributes].each do |i, data| - asset = Asset.find_by_id(data[:id]) - + asset = Asset.new(data) unless @step.assets.include? asset or not asset asset.created_by = current_user asset.last_modified_by = current_user @@ -532,12 +538,11 @@ class StepsController < ApplicationController attr_params = update_params[key] for pos, attrs in params[key] do - assetExists = Asset.exists?(attrs[:id]) if attrs[:_destroy] == "1" attr_params[pos] = {id: attrs[:id], _destroy: "1"} - params[key].delete(pos) if assetExists + params[key].delete(pos) elsif has_destroy_params(params[key][pos]) - attr_params[pos] = {id: attrs[:id]} if assetExists + attr_params[pos] = {id: attrs[:id]} extract_destroy_params(params[key][pos], attr_params[pos]) end end diff --git a/app/models/asset.rb b/app/models/asset.rb index 1492d46f6..654a669e7 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -208,6 +208,21 @@ class Asset < ActiveRecord::Base end end + def destroy + # Delete files from S3 (when updating an existing file, paperclip does + # this automatically, so this is not needed in such cases) + key = self.file.path[1..-1] + S3_BUCKET.object(key).delete + if (self.file_content_type =~ /^image\//) == 0 + self.file.options[:styles].each do |style, option| + key = self.file.path(style)[1..-1] + S3_BUCKET.object(key).delete + end + end + + self.delete + end + # If organization is provided, its space_taken # is updated as well def update_estimated_size(org = nil) @@ -292,7 +307,7 @@ class Asset < ActiveRecord::Base def step_or_result # We must allow both step and result to be blank because of GUI - # (eventhough it's not really a "valid" asset) + # (even though it's not really a "valid" asset) if step.present? && result.present? errors.add(:base, "Asset can only be result or step, not both.") end diff --git a/app/models/step.rb b/app/models/step.rb index 51b83b498..37c75e8b7 100644 --- a/app/models/step.rb +++ b/app/models/step.rb @@ -114,7 +114,7 @@ class Step < ActiveRecord::Base def cascade_after_destroy Comment.destroy(@c_ids) @c_ids = nil - Asset.destroy(@a_ids) + # Assets already deleted by here @a_ids = nil Table.destroy(@t_ids) @t_ids = nil diff --git a/app/models/step_asset.rb b/app/models/step_asset.rb index 084b87610..d8256588c 100644 --- a/app/models/step_asset.rb +++ b/app/models/step_asset.rb @@ -2,5 +2,5 @@ class StepAsset < ActiveRecord::Base validates :step, :asset, presence: true belongs_to :step, inverse_of: :step_assets - belongs_to :asset, inverse_of: :step_asset + belongs_to :asset, inverse_of: :step_asset, :dependent => :destroy end