From 6f72ac308b67faaa43b10acc359b306b3a836d04 Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <git@zjvandeweg.nl>
Date: Wed, 4 Oct 2017 21:01:27 +0200
Subject: Incorporate feedback

---
 app/views/projects/artifacts/_tree_file.html.haml  |  5 +--
 config/initializers/1_settings.rb                  |  2 +-
 .../projects/artifacts_controller_spec.rb          | 42 +++++++++++++++++-----
 spec/models/ci/artifact_blob_spec.rb               |  2 +-
 4 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/app/views/projects/artifacts/_tree_file.html.haml b/app/views/projects/artifacts/_tree_file.html.haml
index 49dfe2d7c16..473b9debb3a 100644
--- a/app/views/projects/artifacts/_tree_file.html.haml
+++ b/app/views/projects/artifacts/_tree_file.html.haml
@@ -1,10 +1,11 @@
 - blob = file.blob
 - path_to_file = file_project_job_artifacts_path(@project, @build, path: file.path)
+- external_link = blob.external_link? && @project.public?
 
-%tr.tree-item.js-artifact-tree-row{ data: { link: path_to_file, external_link: "#{blob.external_link?}" } }
+%tr.tree-item.js-artifact-tree-row{ data: { link: path_to_file, external_link: "#{external_link}" } }
   %td.tree-item-file-name
     = tree_icon('file', blob.mode, blob.name)
-    - if blob.external_link?
+    - if external_link
       = link_to path_to_file, class: 'tree-item-file-external-link js-artifact-tree-tooltip',
           target: '_blank', rel: 'noopener noreferrer', title: _('Opens in a new window') do
         %span.str-truncated>= blob.name
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 4cbc801cc21..412d77284aa 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -373,7 +373,7 @@ Settings.pages['protocol']          ||= Settings.pages.https ? "https" : "http"
 Settings.pages['url']               ||= Settings.__send__(:build_pages_url)
 Settings.pages['external_http']     ||= false unless Settings.pages['external_http'].present?
 Settings.pages['external_https']    ||= false unless Settings.pages['external_https'].present?
-Settings.pages['artifacts_server']  ||= Settings.pages['enabled']
+Settings.pages['artifacts_server']  ||= Settings.pages['enabled'] if Settings.pages['artifacts_server'].nil?
 
 #
 # Geo
diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb
index 5517d983280..64688be5e07 100644
--- a/spec/controllers/projects/artifacts_controller_spec.rb
+++ b/spec/controllers/projects/artifacts_controller_spec.rb
@@ -49,22 +49,46 @@ describe Projects::ArtifactsController do
   describe 'GET file' do
     before do
       allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
-      allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
     end
 
-    context 'when the file exists' do
-      it 'renders the file view' do
-        get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
+    context 'when the file is served by GitLab Pages' do
+      before do
+        allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
+      end
+
+      context 'when the file exists' do
+        it 'renders the file view' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
+
+          expect(response).to have_http_status(302)
+        end
+      end
 
-        expect(response).to have_http_status(302)
+      context 'when the file does not exist' do
+        it 'responds Not Found' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+
+          expect(response).to be_not_found
+        end
       end
     end
 
-    context 'when the file does not exist' do
-      it 'responds Not Found' do
-        get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+    context 'when the file is served through Rails' do
+      context 'when the file exists' do
+        it 'renders the file view' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'ci_artifacts.txt'
 
-        expect(response).to be_not_found
+          expect(response).to have_http_status(:ok)
+          expect(response).to render_template('projects/artifacts/file')
+        end
+      end
+
+      context 'when the file does not exist' do
+        it 'responds Not Found' do
+          get :file, namespace_id: project.namespace, project_id: project, job_id: job, path: 'unknown'
+
+          expect(response).to be_not_found
+        end
       end
     end
   end
diff --git a/spec/models/ci/artifact_blob_spec.rb b/spec/models/ci/artifact_blob_spec.rb
index 3ea11a3bd54..81500fce6f9 100644
--- a/spec/models/ci/artifact_blob_spec.rb
+++ b/spec/models/ci/artifact_blob_spec.rb
@@ -42,7 +42,7 @@ describe Ci::ArtifactBlob do
     end
   end
 
-  describe '#url' do
+  describe '#external_url' do
     before do
       allow(Gitlab.config.pages).to receive(:enabled).and_return(true)
       allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true)
-- 
cgit v1.2.1