Merge branch 'master' into list-inputs

* master:
  Recover from failed run creation more gracefully.
  Use flash[:alert] not flash[:error].
  Add an alert flash to the test layout.
  [TAV-512] Fix overzealous file extension restrictions for inputs.
  Add tests for taverna credentials in env variable.
  Rework the testing of an empty log file for better testing.

README.rdoc

@@ -154,8 +154,8 @@ And everything should be found by the asset pipeline automatically.
 Make sure you have flash messages in your main layout
 (usually <tt>app/views/layouts/application.html.erb</tt>). For example:
 
-  <p class="notice"><%= notice %></p>
-  <p class="alert"><%= alert %></p>
+  <p id="notice"><%= notice %></p>
+  <p id="alert"><%= alert %></p>
 
 Taverna Player uses delayed_job to run workflows on a Taverna Server. If your
 application is not already using delayed_job then you can install the

lib/taverna_player/concerns/controllers/runs_controller.rb

@@ -126,11 +126,18 @@ def new
         # POST /runs
         def create
           @run = Run.new(params[:run])
+
+          # Set workflow, just in case the create fails and needs to redirect
+          # back to the form
+          @workflow = @run.workflow
+
           if @run.save
             flash[:notice] = "Run was successfully created."
+            respond_with(@run, :status => :created, :location => @run)
+          else
+            flash[:alert] = "Run was not successfully created."
+            respond_with(@run)
           end
-
-          respond_with(@run, :status => :created, :location => @run)
         end
 
         # PUT /runs/1
@@ -146,7 +153,7 @@ def destroy
             flash[:notice] = "Run was deleted."
             respond_with(@run)
           else
-            flash[:error] = "Run must be cancelled before deletion."
+            flash[:alert] = "Run must be cancelled before deletion."
             respond_with(@run, :nothing => true, :status => :forbidden) do |format|
               format.html { redirect_to :back }
             end

lib/taverna_player/extras/paperclip-spoofing-fix.rb

@@ -0,0 +1,25 @@
+#------------------------------------------------------------------------------
+# Copyright (c) 2014 The University of Manchester, UK.
+#
+# BSD Licenced. See LICENCE.rdoc for details.
+#
+# Taverna Player was developed in the BioVeL project, funded by the European
+# Commission 7th Framework Programme (FP7), through grant agreement
+# number 283359.
+#
+# Author: Robert Haines
+#------------------------------------------------------------------------------
+
+# We need to override the default Paperclip spoofing detector because it does
+# not allow us to have arbitrary file extensions on files.
+#
+# See https://github.com/thoughtbot/paperclip/issues/1470 and many others.
+module Paperclip
+  class MediaTypeSpoofDetector
+    alias :original_spoofed? :spoofed?
+
+    def spoofed?
+      original_spoofed? ? !(calculated_content_type == "text/plain") : false
+    end
+  end
+end

lib/taverna_player/worker.rb

@@ -246,8 +246,9 @@ def download_log(run)
       Dir.mktmpdir(run.id, Rails.root.join("tmp")) do |tmp_dir|
         tmp_file_name = File.join(tmp_dir, "log.txt")
         begin
-          run.log(tmp_file_name)
-          unless File.zero? tmp_file_name
+          # Only save the log file if it's not empty so as not to confuse
+          # Paperclip
+          unless run.log(tmp_file_name) == 0
             @run.log = File.new(tmp_file_name)
             @run.save
           end

test/dummy/app/views/layouts/application.html.erb

@@ -1,5 +1,5 @@
 <%
-  # Copyright (c) 2013 The University of Manchester, UK.
+  # Copyright (c) 2013, 2014 The University of Manchester, UK.
   #
   # BSD Licenced. See LICENCE.rdoc for details.
   #
@@ -33,6 +33,7 @@
 	</p>
 
 	<p id="notice"><%= notice %></p>
+	<p id="alert"><%= alert %></p>
 
 	<%= yield %>
 

test/fixtures/files/plain-text.bad-extension

@@ -0,0 +1 @@
+This is a plain text file with a non-standard extension!

test/functional/taverna_player/runs_controller_test.rb

@@ -161,6 +161,15 @@ class RunsControllerTest < ActionController::TestCase
         "Did not render with the correct layout"
     end
 
+    test "should fail to create run via browser" do
+      assert_no_difference("Run.count") do
+        post :create, :run => { :workflow_id => @workflow.id, :name => nil }
+      end
+
+      assert_equal "Run was not successfully created.", flash[:alert],
+        "Incorrect or missing flash notice"
+    end
+
     test "should create run via browser" do
       assert_difference("Run.count") do
         post :create, :run => { :workflow_id => @workflow.id }
@@ -244,7 +253,7 @@ class RunsControllerTest < ActionController::TestCase
           delete :destroy, :id => @run1, :use_route => :taverna_player
       end
 
-      assert_equal "Run must be cancelled before deletion.", flash[:error],
+      assert_equal "Run must be cancelled before deletion.", flash[:alert],
         "Incorrect or missing flash notice"
       assert_response :redirect, "Response was not a redirect"
       assert_redirected_to runs_path, "Did not redirect correctly"
@@ -256,7 +265,7 @@ class RunsControllerTest < ActionController::TestCase
           delete :destroy, :id => @run1, :format => :json
       end
 
-      assert_equal "Run must be cancelled before deletion.", flash[:error],
+      assert_equal "Run must be cancelled before deletion.", flash[:alert],
         "Incorrect or missing flash notice"
       assert_response :forbidden, "Response was not forbidden"
     end

test/unit/taverna_player/run_port_test.rb

@@ -478,5 +478,23 @@ class RunIoTest < ActiveSupport::TestCase
         "Incorrect filename generated"
       refute @port7.filename.ends_with?(".zip"), "Filename has 'zip' suffix"
     end
+
+    test "non-standard extensions for plain text files on input ports" do
+      file = fixture_file_upload "/files/plain-text.bad-extension"
+      assert_difference("RunPort::Input.count", 1, "Port was not created") do
+        port = RunPort::Input.create(:name => "test_port", :file => file)
+        assert port.valid?, "Port is invalid"
+        refute port.file.blank?, "File should be present"
+      end
+    end
+
+    test "non-standard extensions for plain text files on output ports" do
+      file = fixture_file_upload "/files/plain-text.bad-extension"
+      assert_difference("RunPort::Output.count", 1, "Port was not created") do
+        port = RunPort::Output.create(:name => "test_port", :file => file)
+        assert port.valid?, "Port is invalid"
+        refute port.file.blank?, "File should be present"
+      end
+    end
   end
 end

test/unit/taverna_player/worker_test.rb

@@ -37,7 +37,6 @@ class WorkerTest < ActiveSupport::TestCase
     # Stuff we can't test yet in TavernaPlayer::Worker.
     flexmock(TavernaPlayer::Worker).new_instances do |w|
       w.should_receive(:download_outputs).and_return_undefined
-      w.should_receive(:download_log).and_return_undefined
       w.should_receive(:process_outputs).and_return([])
     end
 
@@ -55,7 +54,7 @@ class WorkerTest < ActiveSupport::TestCase
     end
   end
 
-  test "server address from config" do
+  test "server address and creds from config" do
     # Stub the creation of a run on a Taverna Server so it fails.
     flexmock(T2Server::Server).new_instances do |s|
       s.should_receive(:initialize_run).once.
@@ -68,8 +67,9 @@ class WorkerTest < ActiveSupport::TestCase
       "Server address not read from config."
   end
 
-  test "server address from env" do
+  test "server address and creds from env" do
     ENV["TAVERNA_URI"] = "https://localhost:8080/taverna"
+    ENV["TAVERNA_CREDENTIALS"] = "taverna:taverna"
 
     # Stub the creation of a run on a Taverna Server so it fails.
     flexmock(T2Server::Server).new_instances do |s|
@@ -101,6 +101,7 @@ class WorkerTest < ActiveSupport::TestCase
       r.should_receive(:start_time).and_return(Time.now)
       r.should_receive(:notifications).and_return([])
       r.should_receive(:finish_time).and_return(Time.now)
+      r.should_receive(:log).once.and_return(0)
       r.should_receive(:delete).and_return_undefined
     end