Add all the tests and fix stuffs along the way:

It turns out they are different:

    builds.success.latest.first

and

    builds.latest.success.first

If we put success first, that latest would also filter via success,
and which is what we want here.

4 files changed, 150 insertions, 5 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index 7d6ba80b965..25a1c2ca7e1 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -63,7 +63,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
     if params[:ref]
       builds = project.builds_for(params[:build_name], params[:ref])
 
-      builds.latest.success.first
+      builds.success.latest.first
     end
   end
 
diff --git a/config/routes.rb b/config/routes.rb
index 10b497d05a0..32b00652bb3 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -736,7 +736,7 @@ Rails.application.routes.draw do
         resources :artifacts, only: [] do
           collection do
             get :search, path: ':ref/:build_name/*path', format: false,
-                         constraints: { ref: %r{.+(?=/)} } # ref could have /
+                         constraints: { ref: /.+/ } # ref could have /
           end
         end
 
diff --git a/spec/requests/projects/artifacts_controller_spec.rb b/spec/requests/projects/artifacts_controller_spec.rb
new file mode 100644
index 00000000000..b11eb1aedcc
--- /dev/null
+++ b/spec/requests/projects/artifacts_controller_spec.rb
@@ -0,0 +1,145 @@
+require 'spec_helper'
+
+describe Projects::ArtifactsController do
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+  let(:pipeline) do
+    create(:ci_pipeline, project: project, sha: project.commit('fix').sha)
+  end
+  let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
+
+  before do
+    login_as(user)
+    project.team << [user, :developer]
+  end
+
+  describe 'GET /:project/artifacts/:ref/:build_name/browse' do
+    context '404' do
+      it 'has no such ref' do
+        path = search_namespace_project_artifacts_path(
+          project.namespace,
+          project,
+          'TAIL',
+          build.name,
+          'browse')
+
+        get path
+
+        expect(response.status).to eq(404)
+      end
+
+      it 'has no such build' do
+        get search_namespace_project_artifacts_path(
+          project.namespace,
+          project,
+          pipeline.sha,
+          'NOBUILD',
+          'browse')
+
+        expect(response.status).to eq(404)
+      end
+
+      it 'has no path' do
+        get search_namespace_project_artifacts_path(
+          project.namespace,
+          project,
+          pipeline.sha,
+          build.name,
+          '')
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context '302' do
+      def path_from_sha
+        search_namespace_project_artifacts_path(
+          project.namespace,
+          project,
+          pipeline.sha,
+          build.name,
+          'browse')
+      end
+
+      shared_examples 'redirect to the build' do
+        it 'redirects' do
+          path = browse_namespace_project_build_artifacts_path(
+            project.namespace,
+            project,
+            build)
+
+          expect(response).to redirect_to(path)
+        end
+      end
+
+      context 'with sha' do
+        before do
+          get path_from_sha
+        end
+
+        it_behaves_like 'redirect to the build'
+      end
+
+      context 'with regular branch' do
+        before do
+          pipeline.update(sha: project.commit('master').sha)
+        end
+
+        before do
+          get search_namespace_project_artifacts_path(
+            project.namespace,
+            project,
+            'master',
+            build.name,
+            'browse')
+        end
+
+        it_behaves_like 'redirect to the build'
+      end
+
+      context 'with branch name containing slash' do
+        before do
+          pipeline.update(sha: project.commit('improve/awesome').sha)
+        end
+
+        before do
+          get search_namespace_project_artifacts_path(
+            project.namespace,
+            project,
+            'improve/awesome',
+            build.name,
+            'browse')
+        end
+
+        it_behaves_like 'redirect to the build'
+      end
+
+      context 'with latest build' do
+        before do
+          3.times do # creating some old builds
+            create(:ci_build, :success, :artifacts, pipeline: pipeline)
+          end
+        end
+
+        before do
+          get path_from_sha
+        end
+
+        it_behaves_like 'redirect to the build'
+      end
+
+      context 'with success build' do
+        before do
+          build # make sure build was old, but still the latest success one
+          create(:ci_build, :pending, :artifacts, pipeline: pipeline)
+        end
+
+        before do
+          get path_from_sha
+        end
+
+        it_behaves_like 'redirect to the build'
+      end
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 606da1b7605..62097de2768 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -28,9 +28,9 @@ RSpec.configure do |config|
   config.verbose_retry = true
   config.display_try_failure_messages = true
 
-  config.include Devise::TestHelpers, type: :controller
-  config.include LoginHelpers,        type: :feature
-  config.include LoginHelpers,        type: :request
+  config.include Devise::TestHelpers,   type: :controller
+  config.include Warden::Test::Helpers, type: :request
+  config.include LoginHelpers,          type: :feature
   config.include StubConfiguration
   config.include EmailHelpers
   config.include RelativeUrl,         type: feature