From 631b41cdaaed7c9def70e6737b82b6e101a7119b Mon Sep 17 00:00:00 2001
From: Oleksii Kriuchykhin <okriuchykhin@biosistemika.com>
Date: Thu, 19 Oct 2017 11:38:23 +0200
Subject: [PATCH] Improve error handling in file uploads
 [SCI-1609][SCI-1612][SCI-1551]

---
 .../javascripts/sitewide/drag_n_drop.js.erb   | 144 ++++++++++++------
 app/assets/stylesheets/themes/scinote.scss    |   3 +-
 app/controllers/result_assets_controller.rb   |  62 ++++----
 app/views/result_assets/_new.html.erb         |   2 +-
 4 files changed, 126 insertions(+), 85 deletions(-)

diff --git a/app/assets/javascripts/sitewide/drag_n_drop.js.erb b/app/assets/javascripts/sitewide/drag_n_drop.js.erb
index 0f1c62068..f585c3aaa 100644
--- a/app/assets/javascripts/sitewide/drag_n_drop.js.erb
+++ b/app/assets/javascripts/sitewide/drag_n_drop.js.erb
@@ -21,12 +21,13 @@
 
     // loops through a list of files and display each file in a separate panel
     function listItems() {
-      droppedFiles = droppedFiles.filter(Boolean);
+      totalSize = 0;
+      _enableSubmitButton();
       $('.panel-step-attachment-new').remove();
       _dragNdropAssetsOff();
       for(var i = 0; i < droppedFiles.length; i++) {
         $('#new-step-assets')
-          .append(_uploadedAseetPreview(droppedFiles[i], i))
+          .append(_uploadedAssetPreview(droppedFiles[i], i))
           .promise()
           .done(function() {
             _removeItemHandler(i);
@@ -42,7 +43,6 @@
       var prevEls = $('input').filter(function() {
         return this.name.match(regex);
       });
-      droppedFiles = droppedFiles.filter(Boolean);
       var fd = new FormData($(ev.target).closest('form').get(0));
       for(var i = 0; i < droppedFiles.length; i++) {
         var index = i + prevEls.length;
@@ -56,30 +56,55 @@
       return fd;
     }
 
-    function _validateFilesSize(file) {
-      var maxSize = file.size/1048576;
-      if(maxSize > <%= Constants::FILE_MAX_SIZE_MB %> && filesValid) {
-        return "<p><%= I18n.t 'general.file.size_exceeded', file_size: Constants::FILE_MAX_SIZE_MB %></p>";
+    function _disableSubmitButton() {
+      $('.step-save').prop('disabled', true);
+    }
+
+    function _enableSubmitButton() {
+      $('.step-save').prop('disabled', false);
+    }
+
+    function _filerAndCheckFiles() {
+      for(var i = 0; i < droppedFiles.length; i++) {
+        if(droppedFiles[i].isValid == false) {
+          return false;
+        }
+      }
+      return (droppedFiles.length > 0);
+    }
+
+    function _validateFilesSize(file) {
+      var fileSize = file.size;
+      totalSize += parseInt(fileSize);
+      if(fileSize > <%= Constants::FILE_MAX_SIZE_MB.megabyte %>) {
+        file.isValid = false;
+        _disableSubmitButton();
+        return "<p class='dnd-error'><%= I18n.t 'general.file.size_exceeded', file_size: Constants::FILE_MAX_SIZE_MB %></p>";
       }
-      totalSize += parseInt(maxSize);
       return '';
     }
 
     function _validateTotalSize() {
-      if(totalSize > <%= Constants::FILE_MAX_SIZE_MB %>) {
+      if(totalSize > <%= Constants::FILE_MAX_SIZE_MB.megabyte %>) {
         filesValid = false;
+        _disableSubmitButton();
         $.each($('.panel-step-attachment-new'), function() {
-          $(this)
-            .find('.panel-body')
-            .append("<p class='dnd-error'><%= I18n.t('general.file.total_size', size: Constants::FILE_MAX_SIZE_MB) %></p>");
+          if(!$(this).find('p').hasClass('dnd-total-error')) {
+            $(this)
+              .find('.panel-body')
+              .append("<p class='dnd-total-error'><%= I18n.t('general.file.total_size', size: Constants::FILE_MAX_SIZE_MB) %></p>");
+          }
         });
       } else {
-        $('.dnd-error').remove();
-        filesValid = true;
+        $('.dnd-total-error').remove();
+        if(_filerAndCheckFiles()) {
+          filesValid = true;
+          _enableSubmitButton();
+        }
       }
     }
 
-    function _uploadedAseetPreview(asset, i) {
+    function _uploadedAssetPreview(asset, i) {
       var html = '<div class="panel panel-default panel-step-attachment-new">';
       html += '<div class="panel-heading">';
       html += '<span class="glyphicon glyphicon-file"></span>';
@@ -104,10 +129,9 @@
         e.stopPropagation();
         var $el = $(this);
         var index = $el.data('item-id');
-        totalSize -= parseInt(droppedFiles[index]/1048576);
-        droppedFiles[index] = null;
-        $el.closest('.panel-step-attachment-new').remove();
-        _validateTotalSize();
+        totalSize -= parseInt(droppedFiles[index].size);
+        droppedFiles.splice(index, 1);
+        listItems();
       });
     }
 
@@ -131,11 +155,10 @@
     var totalSize = 0;
 
     function init(files) {
-      var filesPresent = droppedFiles.length;
       for(var i = 0; i < files.length; i++) {
         droppedFiles.push(files[i]);
       }
-      listItems(filesPresent);
+      listItems();
     }
 
     // return the status of files if they are ready to submit
@@ -144,18 +167,24 @@
     }
 
     // loops through a list of files and display each file in a separate panel
-    function listItems(index) {
-      _dragNdropAssetsOff();
-      for(var i = index; i < droppedFiles.length; i++) {
-          $('#new-result-assets-select')
-            .after(_uploadedAseetPreview(droppedFiles[i], i))
-            .promise()
-            .done(function() {
-              _removeItemHandler(i);
-            });
+    function listItems() {
+      totalSize = 0;
+      $('.panel-result-attachment-new').remove();
+      if(droppedFiles.length < 1) {
+        _disableSubmitButton();
+      } else {
+        _dragNdropAssetsOff();
+        for(var i = 0; i < droppedFiles.length; i++) {
+            $('#new-result-assets-select')
+              .after(_uploadedAssetPreview(droppedFiles[i], i))
+              .promise()
+              .done(function() {
+                _removeItemHandler(i);
+              });
+        }
+        _validateTotalSize();
+        dragNdropAssetsInit('results');
       }
-      _validateTotalSize();
-      dragNdropAssetsInit('results');
     }
 
     // appent the files to the form before submit
@@ -181,31 +210,51 @@
       return fd;
     }
 
+    function _disableSubmitButton() {
+      $('.save-result').prop('disabled', true);
+    }
+
+    function _enableSubmitButton() {
+      $('.save-result').prop('disabled', false);
+    }
+
     function _filerAndCheckFiles() {
-      droppedFiles = droppedFiles.filter(Boolean);
+      for(var i = 0; i < droppedFiles.length; i++) {
+        if(droppedFiles[i].isValid == false) {
+          return false;
+        }
+      }
       return (droppedFiles.length > 0);
     }
 
     function _validateFilesSize(file) {
-      var maxSize = file.size/1048576;
-      if(maxSize > <%= Constants::FILE_MAX_SIZE_MB %> && isValid) {
-        return "<p><%= I18n.t 'general.file.size_exceeded', file_size: Constants::FILE_MAX_SIZE_MB %></p>";
+      var fileSize = file.size;
+      totalSize += parseInt(fileSize);
+      if(fileSize > <%= Constants::FILE_MAX_SIZE_MB.megabyte %>) {
+        file.isValid = false;
+        _disableSubmitButton();
+        return "<p class='dnd-error'><%= I18n.t 'general.file.size_exceeded', file_size: Constants::FILE_MAX_SIZE_MB %></p>";
       }
-      totalSize += parseInt(maxSize);
       return '';
     }
 
     function _validateTotalSize() {
-      if(totalSize > <%= Constants::FILE_MAX_SIZE_MB %>) {
+      if(totalSize > <%= Constants::FILE_MAX_SIZE_MB.megabyte %>) {
         isValid = false;
+        _disableSubmitButton();
         $.each($('.panel-result-attachment-new'), function() {
-          $(this)
-            .find('.panel-body')
-            .append("<p class='dnd-error'><%= I18n.t('general.file.total_size', size: Constants::FILE_MAX_SIZE_MB) %></p>");
+          if(!$(this).find('p').hasClass('dnd-total-error')) {
+            $(this)
+              .find('.panel-body')
+              .append("<p class='dnd-total-error'><%= I18n.t('general.file.total_size', size: Constants::FILE_MAX_SIZE_MB) %></p>");
+            }
         });
       } else {
-        $('.dnd-error').remove();
-        isValid = true;
+        $('.dnd-total-error').remove();
+        if(_filerAndCheckFiles()) {
+          isValid = true;
+          _enableSubmitButton();
+        }
       }
     }
 
@@ -220,7 +269,7 @@
       }
     }
 
-    function _uploadedAseetPreview(asset, i) {
+    function _uploadedAssetPreview(asset, i) {
       var html = '<div class="panel panel-default panel-result-attachment-new">';
       html += '<div class="panel-heading">';
       html += '<span class="glyphicon glyphicon-file"></span>';
@@ -249,10 +298,9 @@
         e.stopPropagation();
         var $el = $(this);
         var index = $el.data('item-id');
-        totalSize -= parseInt(droppedFiles[index]/1048576);
-        droppedFiles[index] = null;
-        $el.closest('.panel-result-attachment-new').remove();
-        _validateTotalSize();
+        totalSize -= parseInt(droppedFiles[index].size);
+        droppedFiles.splice(index, 1);
+        listItems();
       });
     }
 
diff --git a/app/assets/stylesheets/themes/scinote.scss b/app/assets/stylesheets/themes/scinote.scss
index e5a41a988..91a296e07 100644
--- a/app/assets/stylesheets/themes/scinote.scss
+++ b/app/assets/stylesheets/themes/scinote.scss
@@ -1288,7 +1288,8 @@ ul.content-module-activities {
   padding-top: 30px;
 }
 
-.dnd-error {
+.dnd-error,
+.dnd-total-error {
   color: $color-milano-red;
 }
 
diff --git a/app/controllers/result_assets_controller.rb b/app/controllers/result_assets_controller.rb
index faeb069e2..e966bd731 100644
--- a/app/controllers/result_assets_controller.rb
+++ b/app/controllers/result_assets_controller.rb
@@ -76,23 +76,23 @@ class ResultAssetsController < ApplicationController
 
     @result.last_modified_by = current_user
     @result.assign_attributes(update_params)
-    success_flash = t("result_assets.update.success_flash",
-            module: @my_module.name)
+    success_flash = t('result_assets.update.success_flash',
+                      module: @my_module.name)
 
     if @result.archived_changed?(from: false, to: true)
       if previous_asset.locked?
         respond_to do |format|
-          format.html {
+          format.html do
             flash[:error] = t('result_assets.archive.error_flash')
             redirect_to results_my_module_path(@my_module)
             return
-          }
+          end
         end
       end
 
       saved = @result.archive(current_user)
-      success_flash = t("result_assets.archive.success_flash",
-            module: @my_module.name)
+      success_flash = t('result_assets.archive.success_flash',
+                        module: @my_module.name)
       if saved
         Activity.create(
           type_of: :archive_result,
@@ -126,7 +126,7 @@ class ResultAssetsController < ApplicationController
       # Asset (file) and/or name has been changed
       saved = @result.save
 
-      if saved then
+      if saved
         # Release team's space taken due to
         # previous asset being removed
         team = @result.my_module.experiment.project.team
@@ -134,9 +134,7 @@ class ResultAssetsController < ApplicationController
         team.save
 
         # Post process new file if neccesary
-        if @result.asset.present?
-          @result.asset.post_process_file(team)
-        end
+        @result.asset.post_process_file(team) if @result.asset.present?
 
         Activity.create(
           type_of: :edit_result,
@@ -144,32 +142,33 @@ class ResultAssetsController < ApplicationController
           project: @my_module.experiment.project,
           experiment: @my_module.experiment,
           my_module: @my_module,
-          message: t(
-            "activities.edit_asset_result",
-            user: current_user.full_name,
-            result: @result.name
-          )
+          message: t('activities.edit_asset_result',
+                     user: current_user.full_name,
+                     result: @result.name)
         )
       end
     end
 
     respond_to do |format|
       if saved
-        format.html {
+        format.html do
           flash[:success] = success_flash
           redirect_to results_my_module_path(@my_module)
-        }
-        format.json {
+        end
+        format.json do
           render json: {
-            html: render_to_string({
-              partial: "my_modules/result.html.erb", locals: {result: @result}
-            })
+            html: render_to_string(
+              partial: 'my_modules/result.html.erb', locals: { result: @result }
+            )
           }, status: :ok
-        }
+        end
       else
-        format.json {
-          render json: @result.errors, status: :bad_request
-        }
+        format.json do
+          render json: {
+            status: :error,
+            errors: @result.errors
+          }, status: :bad_request
+        end
       end
     end
   end
@@ -189,22 +188,15 @@ class ResultAssetsController < ApplicationController
 
   def load_vars_nested
     @my_module = MyModule.find_by_id(params[:my_module_id])
-
-    unless @my_module
-      render_404
-    end
+    render_404 unless @my_module
   end
 
   def check_create_permissions
-    unless can_create_result_asset_in_module(@my_module)
-      render_403
-    end
+    render_403 unless can_create_result_asset_in_module(@my_module)
   end
 
   def check_edit_permissions
-    unless can_edit_result_asset_in_module(@my_module)
-      render_403
-    end
+    render_403 unless can_edit_result_asset_in_module(@my_module)
   end
 
   def check_archive_permissions
diff --git a/app/views/result_assets/_new.html.erb b/app/views/result_assets/_new.html.erb
index fe28d123d..275e40cd9 100644
--- a/app/views/result_assets/_new.html.erb
+++ b/app/views/result_assets/_new.html.erb
@@ -11,6 +11,7 @@
   <br />
     <button type="button"
             class="btn btn-primary save-result"
+            disabled="true"
             data-href="<%= my_module_result_assets_path(format: :json) %>"
             onClick="DragNDropResults.processResult(this)"><%=t 'result_assets.new.create' %></button>
     <button type="button"
@@ -18,5 +19,4 @@
             onClick="DragNDropResults.destroyAll()">
       <%= t("general.cancel")%>
     </button>
-  <%# end %>
 </div>