folder_controller optimizations stopped sending filename field

data.filename was not being populated, so the filename field
of the multipart request was empty, causing rack to just throw
it away. NOTE: rack throws away fields if you provide filename=""
with content-disposition file.

fixes #5154

Change-Id: If6f0a3a818aaa2f97ee4aded4bea80063209b12a
Reviewed-on: https://gerrit.instructure.com/4840
Tested-by: Hudson <hudson@instructure.com>
Tested-by: Selenium <selenium@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>

app/controllers/folders_controller.rb

@@ -35,7 +35,7 @@ class FoldersController < ApplicationController
                 else
                   @folder.visible_file_attachments
                 end
-        files_options = {:permissions => {:user => @current_user}, :methods => [:currently_locked, :mime_class, :readable_size, :scribdable?], :only => [:id, :comments, :content_type, :context_id, :context_type, :display_name, :folder_id, :position, :media_entry_id, :scribd_doc]}
+        files_options = {:permissions => {:user => @current_user}, :methods => [:currently_locked, :mime_class, :readable_size, :scribdable?], :only => [:id, :comments, :content_type, :context_id, :context_type, :display_name, :folder_id, :position, :media_entry_id, :scribd_doc, :filename]}
         folders_options = {:permissions => {:user => @current_user}, :methods => [:currently_locked, :mime_class], :only => [:id, :context_id, :context_type, :lock_at, :last_lock_at, :last_unlock_at, :name, :parent_folder_id, :position, :unlock_at]}
         res = {
           :actual_folder => @folder.as_json(folders_options),

app/models/attachment.rb

@@ -462,11 +462,17 @@ class Attachment < ActiveRecord::Base
   def self.file_store_config
     # Return existing value, even if nil, as long as it's defined
     @file_store_config ||= YAML.load_file(RAILS_ROOT + "/config/file_store.yml")[RAILS_ENV] rescue nil
-    @file_store_config ||= {'path_prefix' => 'tmp/files', 'storage' => 'local'}
+    @file_store_config ||= {'storage' => 'local'}
+    @file_store_config['path_prefix'] ||= @file_store_config['path'] || 'tmp/files'
+    if RAILS_ENV == "test"
+      file_storage_test_override = Setting.get("file_storage_test_override", nil)
+      return @file_store_config.merge({"storage" => file_storage_test_override}) if file_storage_test_override
+    end
+    return @file_store_config
   end
   
   def self.s3_storage?
-    (RAILS_ENV == "test" && Setting.get("file_storage_test_override", nil) || file_store_config['storage'] rescue nil) == 's3' && s3_config
+    (file_store_config['storage'] rescue nil) == 's3' && s3_config
   end
   
   def self.local_storage?
@@ -487,7 +493,7 @@ class Attachment < ActiveRecord::Base
   # too, it cares about local vs s3 storage.
   if local_storage?
     has_attachment(
-        :path_prefix => (file_store_config['path_prefix'] || 'tmp/files'),
+        :path_prefix => file_store_config['path_prefix'],
         :thumbnails => { :thumb => '200x50' }, 
         :thumbnail_class => 'Thumbnail'
     )

lib/multipart.rb

@@ -45,7 +45,7 @@ module Multipart
     BOUNDARY = AutoHandle.generate('canvas-rules', 15)
     HEADER = {"Content-type" => "multipart/form-data, boundary=" + BOUNDARY + " "}
 
-    def prepare_query (params)
+    def prepare_query (params, field_priority=[])
       fp = []
       creds = params.delete :basic_auth
       if creds
@@ -54,13 +54,22 @@ module Multipart
         puts creds[:password]
         puts Base64.encode64("#{creds[:username]}:#{creds[:password]}")
       end
-      params.each {|k,v|
-        if v.respond_to?(:read)
-          fp.push(FileParam.new(k, v.path, v.read))
+      def file_param(k, v)
+        file_data = v.read rescue nil
+        if file_data
+          FileParam.new(k, v.path, file_data)
         else
-          fp.push(Param.new(k,v))
+          Param.new(k,v)
         end
-      }
+      end
+      completed_fields = {}
+      field_priority.each do |k|
+        if params.has_key?(k) && !completed_fields.has_key?(k)
+          fp.push(file_param(k, params[k]))
+          completed_fields[k] = true
+        end
+      end
+      params.each {|k,v| fp.push(file_param(k, v)) unless completed_fields.has_key?(k) }
       query = fp.collect {|p| "--" + BOUNDARY + "\r\n" + p.to_multipart }.join("") + "--" + BOUNDARY + "--"
       return query, HEADER
     end

lib/ssl_common.rb

@@ -20,7 +20,7 @@ require 'net/https'
 
 class SSLCommon
   SSL_CA_PATH = "/etc/ssl/certs/"
-  
+
   class << self
     def get_http_conn(host, port, ssl)
       http = Net::HTTP.new(host, port)
@@ -29,24 +29,34 @@ class SSLCommon
         http.ca_path = SSL_CA_PATH
         http.verify_mode = OpenSSL::SSL::VERIFY_PEER
       end
-      http 
+      http
     end
-    
-    def post_form(url, form_data)
+
+    def raw_post(url, payload, headers = {}, form_data = nil)
       url = URI.parse(url)
       http = self.get_http_conn(url.host, url.port, url.scheme.downcase == 'https')
-      req = Net::HTTP::Post.new(url.path)
-      req.form_data = form_data
-      http.start {|http| http.request(req) }
+      req = Net::HTTP::Post.new(url.request_uri, headers)
+      req.form_data = form_data if form_data
+      http.start {|http| http.request(req, payload) }
     end
-    
-    def post_data(url, data, content_type)
+
+    def get(url, headers={})
       url = URI.parse(url)
       http = self.get_http_conn(url.host, url.port, url.scheme.downcase == 'https')
-      req = Net::HTTP::Post.new(url.path)
-      req['Content-Type'] = content_type
-      http.start {|http| http.request(req, data) }
+      http.get(url.request_uri, headers)
+    end
+
+    def post_form(url, form_data, headers={})
+      self.raw_post(url, nil, headers, form_data)
+    end
+
+    def post_multipart_form(url, form_data, headers={}, field_priority=[])
+      payload, mp_headers = Multipart::MultipartPost.new.prepare_query(form_data, field_priority)
+      self.raw_post(url, payload, mp_headers.merge(headers))
+    end
+
+    def post_data(url, data, content_type, headers={})
+      self.raw_post(url, data, {"Content-Type" => content_type}.merge(headers))
     end
   end
 end
-

public/javascripts/full_files.js

@@ -1672,7 +1672,7 @@ var fileStructureData = [];
       var url = $item.find(".download_url").attr('href');
       var display_name = $item.find(".name:first").text();
       var $dialog = $("#edit_content_dialog");
-      $dialog.data('update_url', $item.find(".rename_item_link").attr('href')).data('content_type', data.content_type || '').data('filename', data.filename || '');
+      $dialog.data('update_url', $item.find(".rename_item_link").attr('href')).data('content_type', data.content_type || '').data('filename', data.filename || 'file_to_update');
       $dialog.find(".display_name").text(display_name);
       $dialog.find(".loading_message").text("Loading File Contents...").show().end()
         .find(".content").hide();

spec/fixtures/files/html-editing-test.html

@@ -0,0 +1,3 @@
+<html>
+test12
+</html>

spec/selenium/files_sel.rb

@@ -1,13 +1,114 @@
 require File.expand_path(File.dirname(__FILE__) + '/common')
 
-describe "files view" do
-  it_should_behave_like "in-process server selenium tests"
+shared_examples_for "files selenium tests" do
+  it_should_behave_like "forked server selenium tests"
+
+  def fixture_file_path(file)
+    path = ActionController::TestCase.respond_to?(:fixture_path) ? ActionController::TestCase.send(:fixture_path) : nil
+    return "#{path}#{file}"
+  end
+
+  def fixture_file_upload(file, mimetype)
+    ActionController::TestUploadedFile.new(fixture_file_path(file), mimetype)
+  end
+
+  def login(username, password)
+    resp, body = SSLCommon.get "#{app_host}/login"
+    resp.code.should == "200"
+    @cookie = resp.response['set-cookie']
+    resp, body = SSLCommon.post_form("#{app_host}/login", {
+        "pseudonym_session[unique_id]" => username,
+        "pseudonym_session[password]" => password,
+        "redirect_to_ssl" => "0",
+        "pseudonym_session[remember_me]" => "0" },
+        { "Cookie" => @cookie })
+    resp.code.should == "302"
+    @cookie = resp.response['set-cookie']
+    login_as username, password
+  end
+
+  def add_file(fixture, course, name)
+    resp, body = SSLCommon.get "#{app_host}/courses/#{course.id}/files",
+        "Cookie" => @cookie
+    resp.code.should == "200"
+    body.should =~ /<div id="ajax_authenticity_token">([^<]*)<\/div>/
+    authenticity_token = $1
+    resp, body = SSLCommon.post_form("#{app_host}/files/pending", {
+        "attachment[folder_id]" => course.folders.active.first.id,
+        "attachment[filename]" => name,
+        "attachment[context_code]" => "Course_#{course.id}",
+        "authenticity_token" => authenticity_token,
+        "no_redirect" => true}, { "Cookie" => @cookie })
+    resp.code.should == "200"
+    data = ActiveSupport::JSON.decode(body)
+    data["upload_url"] = data["proxied_upload_url"] || data["upload_url"]
+    data["upload_url"] = "#{app_host}#{data["upload_url"]}" if data["upload_url"] =~ /^\//
+    data["success_url"] = "#{app_host}#{data["success_url"]}" if data["success_url"] =~ /^\//
+    data["upload_params"]["file"] = fixture
+    resp, body = SSLCommon.post_multipart_form(data["upload_url"], data["upload_params"], { "Cookie" => @cookie }, ["bucket", "key", "acl"])
+    resp.code.should =~ /^20/
+    if body =~ /<PostResponse>/
+      resp, body = SSLCommon.get data["success_url"]
+      resp.code.should == "200"
+    end
+  end
 
   it "should show students link to download zip of folder" do
-    course_with_student_logged_in
+    user_with_pseudonym :username => "nobody3@example.com",
+                        :password => "asdfasdf3"
+    course_with_student_logged_in :user => @user
+    login_as "nobody3@example.com", "asdfasdf3"
     get "/courses/#{@course.id}/files"
     link = keep_trying_until { driver.find_element(:css, "div.links a.download_zip_link") }
     link.should be_displayed
     link.attribute('href').should match(%r"/courses/#{@course.id}/folders/\d+/download")
   end
+
+  it "should allow you to edit html files" do
+    user_with_pseudonym :username => "nobody2@example.com",
+                        :password => "asdfasdf2"
+    course_with_teacher_logged_in :user => @user
+    login "nobody2@example.com", "asdfasdf2"
+    add_file(fixture_file_upload('files/html-editing-test.html', 'text/html'),
+        @course, "html-editing-test.html")
+    get "/courses/#{@course.id}/files"
+    link = keep_trying_until { driver.find_element(:css, "li.editable_folder_item div.header a.download_url") }
+    link.should be_displayed
+    link.text.should == "html-editing-test.html"
+    current_content = File.read(fixture_file_path("files/html-editing-test.html"))
+    4.times do
+      get "/courses/#{@course.id}/files"
+      new_content = "<html>#{ActiveSupport::SecureRandom.hex(10)}</html>"
+      link = keep_trying_until { driver.find_element(:css, "li.editable_folder_item div.header a.edit_item_content_link") }
+      link.should be_displayed
+      link.text.should == "edit content"
+      link.click
+      keep_trying_until { driver.find_element(:css, "#edit_content_dialog").displayed?}
+      keep_trying_until { driver.execute_script("return $('#edit_content_textarea')[0].value;") == current_content }
+      driver.execute_script("$('#edit_content_textarea')[0].value = '#{new_content}';")
+      current_content = new_content
+      driver.find_element(:css, "#edit_content_dialog button.save_button").click
+      keep_trying_until { !driver.find_element(:css, "#edit_content_dialog").displayed?}
+    end
+  end
+end
+
+describe "files Windows-Firefox-Local-Tests" do
+  it_should_behave_like "files selenium tests"
+  prepend_before(:each) {
+    Setting.set("file_storage_test_override", "local")
+  }
+  prepend_before(:all) {
+    Setting.set("file_storage_test_override", "local")
+  }
+end
+
+describe "files Windows-Firefox-S3-Tests" do
+  it_should_behave_like "files selenium tests"
+  prepend_before(:each) {
+    Setting.set("file_storage_test_override", "s3")
+  }
+  prepend_before(:all) {
+    Setting.set("file_storage_test_override", "s3")
+  }
 end