diff options
author | Nick Thomas <nick@gitlab.com> | 2017-10-05 18:03:17 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2017-10-05 18:03:17 +0300 |
commit | ce87f42dc928377abdd1dd445598afb8150d4521 (patch) | |
tree | 875d7173db44d05c67b6de45f57359bb550ec62e | |
parent | cd60a02ad94088ae1444ac9a8b355b4d05fc30a3 (diff) | |
parent | d1dd115356a06fd20e8b9293cd645bb064b7c56a (diff) |
Merge branch '34102-online-view-of-artifacts-fe' into 'master'
Add external link for online artifacts
Closes #34102
See merge request gitlab-org/gitlab-ce!14399
-rw-r--r-- | app/assets/javascripts/build_artifacts.js | 24 | ||||
-rw-r--r-- | app/assets/javascripts/lib/utils/url_utility.js | 20 | ||||
-rw-r--r-- | app/assets/stylesheets/pages/tree.scss | 8 | ||||
-rw-r--r-- | app/controllers/projects/artifacts_controller.rb | 18 | ||||
-rw-r--r-- | app/models/ci/artifact_blob.rb | 26 | ||||
-rw-r--r-- | app/models/concerns/routable.rb | 4 | ||||
-rw-r--r-- | app/views/projects/artifacts/_tree_file.html.haml | 15 | ||||
-rw-r--r-- | changelogs/unreleased/34102-online-view-of-artifacts-fe.yml | 5 | ||||
-rw-r--r-- | config/gitlab.yml.example | 1 | ||||
-rw-r--r-- | config/initializers/1_settings.rb | 19 | ||||
-rw-r--r-- | doc/user/project/pipelines/img/job_artifacts_browser.png | bin | 3771 -> 3944 bytes | |||
-rw-r--r-- | doc/user/project/pipelines/job_artifacts.md | 8 | ||||
-rw-r--r-- | spec/controllers/projects/artifacts_controller_spec.rb | 70 | ||||
-rw-r--r-- | spec/features/projects/artifacts/browse_spec.rb | 50 | ||||
-rw-r--r-- | spec/models/ci/artifact_blob_spec.rb | 50 |
15 files changed, 277 insertions, 41 deletions
diff --git a/app/assets/javascripts/build_artifacts.js b/app/assets/javascripts/build_artifacts.js index bd479700fd3..19388f1f9ae 100644 --- a/app/assets/javascripts/build_artifacts.js +++ b/app/assets/javascripts/build_artifacts.js @@ -1,9 +1,12 @@ /* eslint-disable func-names, space-before-function-paren, wrap-iife, prefer-arrow-callback, no-unused-vars, no-return-assign, max-len */ +import { visitUrl } from './lib/utils/url_utility'; +import { convertPermissionToBoolean } from './lib/utils/common_utils'; window.BuildArtifacts = (function() { function BuildArtifacts() { this.disablePropagation(); this.setupEntryClick(); + this.setupTooltips(); } BuildArtifacts.prototype.disablePropagation = function() { @@ -17,9 +20,28 @@ window.BuildArtifacts = (function() { BuildArtifacts.prototype.setupEntryClick = function() { return $('.tree-holder').on('click', 'tr[data-link]', function(e) { - return window.location = this.dataset.link; + visitUrl(this.dataset.link, convertPermissionToBoolean(this.dataset.externalLink)); }); }; + BuildArtifacts.prototype.setupTooltips = function() { + $('.js-artifact-tree-tooltip').tooltip({ + placement: 'bottom', + // Stop the tooltip from hiding when we stop hovering the element directly + // We handle all the showing/hiding below + trigger: 'manual', + }); + + // We want the tooltip to show if you hover anywhere on the row + // But be placed below and in the middle of the file name + $('.js-artifact-tree-row') + .on('mouseenter', (e) => { + $(e.currentTarget).find('.js-artifact-tree-tooltip').tooltip('show'); + }) + .on('mouseleave', (e) => { + $(e.currentTarget).find('.js-artifact-tree-tooltip').tooltip('hide'); + }); + }; + return BuildArtifacts; })(); diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index 3328ff9cc23..78c7a094127 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -1,4 +1,5 @@ /* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-param-reassign, no-cond-assign, one-var, one-var-declaration-per-line, no-void, guard-for-in, no-restricted-syntax, prefer-template, quotes, max-len */ + var base; var w = window; if (w.gl == null) { @@ -86,6 +87,21 @@ w.gl.utils.getLocationHash = function(url) { w.gl.utils.refreshCurrentPage = () => gl.utils.visitUrl(document.location.href); -w.gl.utils.visitUrl = (url) => { - document.location.href = url; +// eslint-disable-next-line import/prefer-default-export +export function visitUrl(url, external = false) { + if (external) { + // Simulate `target="blank" rel="noopener noreferrer"` + // See https://mathiasbynens.github.io/rel-noopener/ + const otherWindow = window.open(); + otherWindow.opener = null; + otherWindow.location = url; + } else { + document.location.href = url; + } +} + +window.gl = window.gl || {}; +window.gl.utils = { + ...(window.gl.utils || {}), + visitUrl, }; diff --git a/app/assets/stylesheets/pages/tree.scss b/app/assets/stylesheets/pages/tree.scss index 224eee90a3f..e2f6e511c86 100644 --- a/app/assets/stylesheets/pages/tree.scss +++ b/app/assets/stylesheets/pages/tree.scss @@ -169,6 +169,14 @@ } } + .tree-item-file-external-link { + margin-right: 4px; + + span { + text-decoration: inherit; + } + } + .tree_commit { max-width: 320px; diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index eb010923466..0837451cc49 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -29,13 +29,17 @@ class Projects::ArtifactsController < Projects::ApplicationController blob = @entry.blob conditionally_expand_blob(blob) - respond_to do |format| - format.html do - render 'file' - end - - format.json do - render_blob_json(blob) + if blob.external_link?(build) + redirect_to blob.external_url(@project, build) + else + respond_to do |format| + format.html do + render 'file' + end + + format.json do + render_blob_json(blob) + end end end end diff --git a/app/models/ci/artifact_blob.rb b/app/models/ci/artifact_blob.rb index b35febc9ac5..8b66531ec7b 100644 --- a/app/models/ci/artifact_blob.rb +++ b/app/models/ci/artifact_blob.rb @@ -2,6 +2,8 @@ module Ci class ArtifactBlob include BlobLike + EXTENTIONS_SERVED_BY_PAGES = %w[.html .htm .txt .json].freeze + attr_reader :entry def initialize(entry) @@ -17,6 +19,7 @@ module Ci def size entry.metadata[:size] end + alias_method :external_size, :size def data "Build artifact #{path}" @@ -30,6 +33,27 @@ module Ci :build_artifact end - alias_method :external_size, :size + def external_url(project, job) + return unless external_link?(job) + + components = project.full_path_components + components << "-/jobs/#{job.id}/artifacts/file/#{path}" + artifact_path = components[1..-1].join('/') + + "#{pages_config.protocol}://#{components[0]}.#{pages_config.host}/#{artifact_path}" + end + + def external_link?(job) + pages_config.enabled && + pages_config.artifacts_server && + EXTENTIONS_SERVED_BY_PAGES.include?(File.extname(name)) && + job.project.public? + end + + private + + def pages_config + Gitlab.config.pages + end end end diff --git a/app/models/concerns/routable.rb b/app/models/concerns/routable.rb index f5048d17d80..12e93be2104 100644 --- a/app/models/concerns/routable.rb +++ b/app/models/concerns/routable.rb @@ -106,6 +106,10 @@ module Routable RequestStore[full_path_key] ||= uncached_full_path end + def full_path_components + full_path.split('/') + end + def expires_full_path_cache RequestStore.delete(full_path_key) if RequestStore.active? @full_path = nil diff --git a/app/views/projects/artifacts/_tree_file.html.haml b/app/views/projects/artifacts/_tree_file.html.haml index 8edb9be049a..a97ddb3c377 100644 --- a/app/views/projects/artifacts/_tree_file.html.haml +++ b/app/views/projects/artifacts/_tree_file.html.haml @@ -1,10 +1,17 @@ +- blob = file.blob - path_to_file = file_project_job_artifacts_path(@project, @build, path: file.path) +- external_link = blob.external_link?(@build) -%tr.tree-item{ 'data-link' => path_to_file } - - blob = file.blob +%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) - = link_to path_to_file do - %span.str-truncated= blob.name + - 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 + = icon('external-link', class: 'js-artifact-tree-external-icon') + - else + = link_to path_to_file do + %span.str-truncated= blob.name %td = number_to_human_size(blob.size, precision: 2) diff --git a/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml b/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml new file mode 100644 index 00000000000..ce83b140eb6 --- /dev/null +++ b/changelogs/unreleased/34102-online-view-of-artifacts-fe.yml @@ -0,0 +1,5 @@ +--- +title: Add online view of HTML artifacts for public projects +merge_request: 14399 +author: +type: added diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 88771c5f5bb..1069c7be5f0 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -164,6 +164,7 @@ production: &base host: example.com port: 80 # Set to 443 if you serve the pages with HTTPS https: false # Set to true if you serve the pages with HTTPS + artifacts_server: true # external_http: ["1.1.1.1:80", "[2001::1]:80"] # If defined, enables custom domain support in GitLab Pages # external_https: ["1.1.1.1:443", "[2001::1]:443"] # If defined, enables custom domain and certificate support in GitLab Pages diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index a23b3208dab..a4b7c1a3919 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -316,15 +316,16 @@ Settings.registry['path'] = Settings.absolute(Settings.registry['path # Pages # Settings['pages'] ||= Settingslogic.new({}) -Settings.pages['enabled'] = false if Settings.pages['enabled'].nil? -Settings.pages['path'] = Settings.absolute(Settings.pages['path'] || File.join(Settings.shared['path'], "pages")) -Settings.pages['https'] = false if Settings.pages['https'].nil? -Settings.pages['host'] ||= "example.com" -Settings.pages['port'] ||= Settings.pages.https ? 443 : 80 -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['enabled'] = false if Settings.pages['enabled'].nil? +Settings.pages['path'] = Settings.absolute(Settings.pages['path'] || File.join(Settings.shared['path'], "pages")) +Settings.pages['https'] = false if Settings.pages['https'].nil? +Settings.pages['host'] ||= "example.com" +Settings.pages['port'] ||= Settings.pages.https ? 443 : 80 +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'] if Settings.pages['artifacts_server'].nil? # # Git LFS diff --git a/doc/user/project/pipelines/img/job_artifacts_browser.png b/doc/user/project/pipelines/img/job_artifacts_browser.png Binary files differindex 145fe156bbb..d3d8de5ac60 100644 --- a/doc/user/project/pipelines/img/job_artifacts_browser.png +++ b/doc/user/project/pipelines/img/job_artifacts_browser.png diff --git a/doc/user/project/pipelines/job_artifacts.md b/doc/user/project/pipelines/job_artifacts.md index 4e93e680fd2..9ef6f9185c9 100644 --- a/doc/user/project/pipelines/job_artifacts.md +++ b/doc/user/project/pipelines/job_artifacts.md @@ -50,6 +50,10 @@ For more examples on artifacts, follow the [artifacts reference in With GitLab 9.2, PDFs, images, videos and other formats can be previewed directly in the job artifacts browser without the need to download them. +>**Note:** +With [GitLab 10.1][ce-14399], HTML files in a public project can be previewed +directly in a new tab without the need to download them. + After a job finishes, if you visit the job's specific page, there are three buttons. You can download the artifacts archive or browse its contents, whereas the **Keep** button appears only if you have set an [expiry date] to the @@ -64,7 +68,8 @@ archive. If your artifacts contained directories, then you are also able to browse inside them. Below you can see how browsing looks like. In this case we have browsed inside -the archive and at this point there is one directory and one HTML file. +the archive and at this point there is one directory, a couple files, and +one HTML file that you can view directly online (opens in a new tab). ![Job artifacts browser](img/job_artifacts_browser.png) @@ -158,3 +163,4 @@ information in the UI. [expiry date]: ../../../ci/yaml/README.md#artifacts-expire_in +[ce-14399]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/14399 diff --git a/spec/controllers/projects/artifacts_controller_spec.rb b/spec/controllers/projects/artifacts_controller_spec.rb index caa63e7bd22..d0992719171 100644 --- a/spec/controllers/projects/artifacts_controller_spec.rb +++ b/spec/controllers/projects/artifacts_controller_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper' describe Projects::ArtifactsController do - let(:user) { create(:user) } - let(:project) { create(:project, :repository) } + set(:user) { create(:user) } + set(:project) { create(:project, :repository, :public) } let(:pipeline) do create(:ci_pipeline, @@ -15,7 +15,7 @@ describe Projects::ArtifactsController do let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } before do - project.team << [user, :developer] + project.add_developer(user) sign_in(user) end @@ -47,19 +47,67 @@ describe Projects::ArtifactsController do end describe 'GET file' 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' + before do + allow(Gitlab.config.pages).to receive(:enabled).and_return(true) + end - expect(response).to render_template('projects/artifacts/file') + 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 + + 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 + + context 'when the project is private' do + let(:private_project) { create(:project, :repository, :private) } + let(:pipeline) { create(:ci_pipeline, project: private_project) } + let(:job) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } + + before do + private_project.add_developer(user) + + allow(Gitlab.config.pages).to receive(:artifacts_server).and_return(true) + end + + it 'does not redirect the request' do + get :file, namespace_id: private_project.namespace, project_id: private_project, job_id: job, path: 'ci_artifacts.txt' + + expect(response).to have_http_status(:ok) + expect(response).to render_template('projects/artifacts/file') end end end diff --git a/spec/features/projects/artifacts/browse_spec.rb b/spec/features/projects/artifacts/browse_spec.rb index 42b47cb3301..cb69aff8d5f 100644 --- a/spec/features/projects/artifacts/browse_spec.rb +++ b/spec/features/projects/artifacts/browse_spec.rb @@ -4,16 +4,15 @@ feature 'Browse artifact', :js do let(:project) { create(:project, :public) } let(:pipeline) { create(:ci_empty_pipeline, project: project) } let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) } + let(:browse_url) do + browse_path('other_artifacts_0.1.2') + end def browse_path(path) browse_project_job_artifacts_path(project, job, path) end context 'when visiting old URL' do - let(:browse_url) do - browse_path('other_artifacts_0.1.2') - end - before do visit browse_url.sub('/-/jobs', '/builds') end @@ -22,4 +21,47 @@ feature 'Browse artifact', :js do expect(page.current_path).to eq(browse_url) end end + + context 'when browsing a directory with an text file' do + let(:txt_entry) { job.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') } + + 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 project is public' do + it "shows external link icon and styles" do + visit browse_url + + link = first('.tree-item-file-external-link') + + expect(page).to have_link('doc_sample.txt', href: file_project_job_artifacts_path(project, job, path: txt_entry.blob.path)) + expect(link[:target]).to eq('_blank') + expect(link[:rel]).to include('noopener') + expect(link[:rel]).to include('noreferrer') + expect(page).to have_selector('.js-artifact-tree-external-icon') + end + end + + context 'when the project is private' do + let!(:private_project) { create(:project, :private) } + let(:pipeline) { create(:ci_empty_pipeline, project: private_project) } + let(:job) { create(:ci_build, :artifacts, pipeline: pipeline) } + let(:user) { create(:user) } + + before do + private_project.add_developer(user) + + sign_in(user) + end + + it 'shows internal link styles' do + visit browse_project_job_artifacts_path(private_project, job, 'other_artifacts_0.1.2') + + expect(page).to have_link('doc_sample.txt') + expect(page).not_to have_selector('.js-artifact-tree-external-icon') + end + end + end end diff --git a/spec/models/ci/artifact_blob_spec.rb b/spec/models/ci/artifact_blob_spec.rb index a10a8af5303..d5ba088af53 100644 --- a/spec/models/ci/artifact_blob_spec.rb +++ b/spec/models/ci/artifact_blob_spec.rb @@ -1,7 +1,8 @@ require 'spec_helper' describe Ci::ArtifactBlob do - let(:build) { create(:ci_build, :artifacts) } + set(:project) { create(:project, :public) } + set(:build) { create(:ci_build, :artifacts, project: project) } let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/another-subdirectory/banana_sample.gif') } subject { described_class.new(entry) } @@ -41,4 +42,51 @@ describe Ci::ArtifactBlob do expect(subject.external_storage).to eq(:build_artifact) end end + + 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) + end + + context '.gif extension' do + it 'returns nil' do + expect(subject.external_url(build.project, build)).to be_nil + end + end + + context 'txt extensions' do + let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') } + + it 'returns a URL' do + url = subject.external_url(build.project, build) + + expect(url).not_to be_nil + expect(url).to start_with("http") + expect(url).to match Gitlab.config.pages.host + expect(url).to end_with(entry.path) + end + end + end + + describe '#external_link?' 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 'gif extensions' do + it 'returns false' do + expect(subject.external_link?(build)).to be false + end + end + + context 'txt extensions' do + let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/doc_sample.txt') } + + it 'returns true' do + expect(subject.external_link?(build)).to be true + end + end + end end |