Merge pull request #1313 from ChrisBr/refactor_save_file

[webui] Refactor package save_file

src/api/app/controllers/webui/package_controller.rb

@@ -3,6 +3,7 @@
 
 class Webui::PackageController < Webui::WebuiController
 
+  require_dependency 'opensuse/validator'
   include Webui::HasComments
   include ParsePackageDiff
   include Webui::WebuiHelper
@@ -634,94 +635,64 @@ def add_file
     set_file_details
   end
 
-  def valid_file_name? name
-    name.present? && name =~ %r{^[^\/]+$}
-  end
-
   def save_file
     authorize @package, :update?
 
     file = params[:file]
     file_url = params[:file_url]
     filename = params[:filename]
 
-    if file.present? # we are getting an uploaded file
-      filename = file.original_filename if filename.blank?
-
-      unless valid_file_name?(filename)
-        if request.xhr?
-          render(json: { error: "'#{filename}' is not a valid filename." }, status: 400)
-        else
-          redirect_to(:back, :error => "'#{filename}' is not a valid filename.")
-        end
-        return
-      end
+    errors = []
 
-      begin
+    begin
+      if file.present?
+        # We are getting an uploaded file
+        filename = file.original_filename if filename.blank?
         @package.save_file(file: file, filename: filename, :comment => params[:comment])
-      rescue Timeout::Error => e
-        if request.xhr?
-          render(json: { error: 'Timeout when saving file. Please try again.'}, status: 400)
-        else
-          redirect_to(:back, :error => 'Timeout when saving file. Please try again.')
+      elsif file_url.present?
+        # we have a remote file URI, so we have to download and save it
+        services = @package.services
+
+        # detects automatically git://, src.rpm formats
+        services.addDownloadURL(file_url, filename)
+
+        unless services.save
+          errors << "Failed to add file from URL '#{file_url}'"
         end
-        return
-      rescue Suse::ValidationError,
-             ActiveXML::Transport::Error => e
-        if request.xhr?
-          render(json: { error: e.summary }, status: 400)
+      else
+        # No file is provided so we just create an empty new file (touch)
+        if filename.present?
+          @package.save_file(filename: filename)
         else
-          redirect_to(:back, :error => e.summary)
+          errors << 'No file or URI given'
         end
-        return
       end
-    # update package timestamp and reindex sources
-    elsif file_url.present? # we have a remote file uri
-      return unless add_file_url(file_url, filename)
-    else
-      return unless add_file_filename(filename)
-    end
-    if request.xhr?
-      render(json: { status: 'ok' })
-    else
-      redirect_to({:action => :show, :project => @project, :package => @package}, :success => "The file #{filename} has been added.")
+
+    rescue APIException, StandardError => exception
+      errors << exception.message
     end
-  end
 
-  def add_file_filename(filename)
-    if filename.blank?
-      flash[:error] = 'No file or URI given.'
-      redirect_back_or_to action: :add_file, project: params[:project], package: params[:package]
-      return false
-    else
-      unless valid_file_name?(filename)
-        flash[:error] = "'#{filename}' is not a valid filename."
-        redirect_back_or_to action: :add_file, project: params[:project], package: params[:package]
-        return false
+    if errors.empty?
+      message = "The file '#{filename}' has been successfully saved."
+      # We have to check if it's an AJAX request or not
+      if request.xhr?
+        flash.now[:success] = message
+        render layout: false, partial: 'layouts/webui/flash', object: flash
+      else
+        redirect_to({ action: :show, project: @project, package: @package }, success: message)
       end
-      begin
-        @package.save_file filename: filename
-      rescue ActiveXML::Transport::Error => e
-        flash[:error] = e.summary
-        redirect_back_or_to action: :add_file, project: params[:project], package: params[:package]
-        return false
+      return
+    else
+      message = "Error while creating '#{filename}' file: #{errors.compact.join("\n")}."
+      # We have to check if it's an AJAX request or not
+      if request.xhr?
+        flash.now[:error] = message
+        render layout: false, status: 400, partial: 'layouts/webui/flash', object: flash
+      else
+        redirect_to(:back, error: message)
       end
+      return
     end
-    true
-  end
-
-  def add_file_url(file_url, filename = nil)
-    @services = Service.find(project: @project, package: @package.name)
-    unless @services
-      @services = Service.new(project: @project, package: @package.name)
-    end
-    @services.addDownloadURL(file_url, filename) # detects automatically git://, src.rpm formats
-    unless @services.save
-      flash[:error] = "Failed to add file from URL '#{file_url}'"
-      redirect_back_or_to action: :add_file, project: params[:project], package: params[:package]
-      return false
-    end
-    true
   end
 
   def remove_file

src/api/app/models/package.rb

@@ -1194,6 +1194,12 @@ def commit( rev = nil )
   end
 
   def self.verify_file!(pkg, name, content)
+    raise IllegalFileName, "'#{name}' is not a valid filename" if (!name.present? || !(name =~ %r{^[^\/]+$}))
+
+    # file is an ActionDispatch::Http::UploadedFile and Suse::Validator.validate
+    # will call to_s therefore we have to read the content first
+    content = File.open(content.path).read if content.kind_of?(ActionDispatch::Http::UploadedFile)
+
     # schema validation, if possible
     %w{aggregate constraints link service patchinfo channel}.each do |schema|
       if name == '_' + schema
@@ -1242,9 +1248,10 @@ def self.verify_file!(pkg, name, content)
   def save_file(opt = {})
     content = '' # touch an empty file first
     content = opt[:file] if opt[:file]
+
     logger.debug "storing file: filename: #{opt[:filename]}, comment: #{opt[:comment]}"
 
-    Package.verify_file!(self, opt[:filename], opt[:file])
+    Package.verify_file!(self, opt[:filename], content)
     unless User.current.can_modify_package?(self)
       raise PutFileNoPermission, "Insufficient permissions to store file in package #{self.name}, project #{self.project.name}"
     end

src/api/app/views/webui/package/add_file.html.erb

@@ -13,19 +13,39 @@
     <b>Filename (taken from uploaded file if empty):</b><br/>
     <%= text_field_tag('filename', '', :size => 80) %><br/>
     <b>Upload from</b> <%= select_tag('file_type', options_for_select([['local file', 'local'], ['remote URL', 'remote']])) %> :<br/>
-    <%= javascript_tag do %>
-      $('#file_type').change(function() {
-        if ($('#file_type option:selected').attr('value') == 'local') {
-          $('#file').show();
-          $('#file_url').hide();
-        } else {
-          $('#file').hide();
-          $('#file_url').show();
-        }
-      });
-    <% end %>
     <%= file_field_tag('file') %>
     <%= text_field_tag('file_url', '', :size => 80, :class => "hidden") %>
   </p>
-  <p><%= submit_tag %></p>
+  <p><%= submit_tag 'Save changes ', id: 'submit_button', disabled: true %></p>
+<% end %>
+
+<%= javascript_tag do %>
+  $('#file_type').change(function() {
+    if ($('#file_type option:selected').attr('value') == 'local') {
+      $('#file').show();
+      $('#file_url').hide();
+    } else {
+      $('#file').hide();
+      $('#file_url').show();
+    }
+  });
+
+  $('#filename, #file_url').keyup(function(){
+    changeButton();
+  });
+
+  $('#file').change(function(){
+    changeButton();
+  });
+
+  function changeButton(){
+    var filename = $('#filename').val().length;
+    var file = $('#file').val().length;
+    var file_url = $('#file_url').val().length;
+
+    if(filename != 0 || file != 0 || file_url != 0)
+      $('#submit_button').attr('disabled', false);
+    else
+      $('#submit_button').attr('disabled',true);
+  }
 <% end %>

src/api/lib/opensuse/validator.rb

@@ -91,12 +91,12 @@ def validate(opt, content)
         end
 
         if content.nil?
-          raise "illegal option; need content for #{schema_base_filename}"
+          raise "illegal option; need content for #{schema_file}"
         end
         content = content.to_s
         if content.empty?
           logger.debug "no content, skipping validation for #{schema_file}"
-          raise ValidationError, "Document is empty, not allowed for #{schema_base_filename}"
+          raise ValidationError, "Document is empty, not allowed for #{schema_file}"
         end
 
         begin

src/api/test/functional/webui/package_controller_test.rb

@@ -345,10 +345,6 @@ def test_submit_package
     page.wont_have_field('supersede_request_ids[]')
   end
 
-  def test_save_file
-    skip("modify some file, store and check for errors. Esp. validation errors on special files like _link or .product files")
-  end
-
   def test_remove_file
     use_js