Improve readability of artifacts browser `Entry` related code

8 files changed, 101 insertions, 77 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index 00daac0cb30..dff0732bdfe 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -18,17 +18,17 @@ class Projects::ArtifactsController < Projects::ApplicationController
     return render_404 unless build.artifacts?
 
     directory = params[:path] ? "#{params[:path]}/" : ''
-    @path = build.artifacts_metadata_path(directory)
+    @entry = build.artifacts_metadata_entry(directory)
 
-    return render_404 unless @path.exists?
+    return render_404 unless @entry.exists?
   end
 
   def file
-    file_path = build.artifacts_metadata_path(params[:path])
+    entry = build.artifacts_metadata_entry(params[:path])
 
-    if file_path.exists?
+    if entry.exists?
       render json: { archive: build.artifacts_file.path,
-                     path: Base64.encode64(file_path.path) }
+                     entry: Base64.encode64(entry.path) }
     else
       render json: {}, status: 404
     end
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index fef667f865e..6cc26abce66 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -347,8 +347,8 @@ module Ci
       artifacts? && artifacts_metadata.exists?
     end
 
-    def artifacts_metadata_path(path)
-      Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_path
+    def artifacts_metadata_entry(path)
+      Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_entry
     end
 
     private
diff --git a/app/views/projects/artifacts/browse.html.haml b/app/views/projects/artifacts/browse.html.haml
index fc161be2ffc..1add7ef6bfb 100644
--- a/app/views/projects/artifacts/browse.html.haml
+++ b/app/views/projects/artifacts/browse.html.haml
@@ -1,11 +1,11 @@
-- page_title "#{@build.name} (##{@build.id})", 'Build artifacts'
-- header_title project_title(@project, "Build artifacts", namespace_project_build_path(@project.namespace, @project, @build))
+- page_title 'Artifacts', "#{@build.name} (##{@build.id})", 'Builds'
+= render 'projects/builds/header_title'
 
 #tree-holder.tree-holder
   .gray-content-block.top-block.clearfix
     .pull-right
       = link_to download_namespace_project_build_artifacts_path(@project.namespace, @project, @build),
-        class: 'btn btn-default', method: :get do
+        class: 'btn btn-default' do
         = icon('download')
         Download artifacts archive
 
@@ -17,8 +17,8 @@
           %th Name
           %th Size
           %th Download
-      = render partial: 'tree_directory', collection: @path.directories!, as: :directory
-      = render partial: 'tree_file', collection: @path.files, as: :file
+      = render partial: 'tree_directory', collection: @entry.directories(parent: true), as: :directory
+      = render partial: 'tree_file', collection: @entry.files, as: :file
 
-- if @path.children.empty?
+- if @entry.empty?
   .center Empty
diff --git a/features/steps/project/builds.rb b/features/steps/project/builds.rb
index 5702496ac84..28395281077 100644
--- a/features/steps/project/builds.rb
+++ b/features/steps/project/builds.rb
@@ -84,6 +84,6 @@ class Spinach::Features::ProjectBuilds < Spinach::FeatureSteps
     # this will be accelerated by Workhorse
     response_json = JSON.parse(page.body, symbolize_names: true)
     expect(response_json[:archive]).to end_with('build_artifacts.zip')
-    expect(response_json[:path]).to eq Base64.encode64('ci_artifacts.txt')
+    expect(response_json[:entry]).to eq Base64.encode64('ci_artifacts.txt')
   end
 end
diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb
index e9ec8f1302c..bfdfc9a1d7d 100644
--- a/lib/gitlab/ci/build/artifacts/metadata.rb
+++ b/lib/gitlab/ci/build/artifacts/metadata.rb
@@ -34,8 +34,9 @@ module Gitlab
             end
           end
 
-          def to_path
-            Path.new(@path, *match!)
+          def to_entry
+            entires, metadata = match!
+            Entry.new(@path, entires, metadata)
           end
 
           private
diff --git a/lib/gitlab/ci/build/artifacts/metadata/path.rb b/lib/gitlab/ci/build/artifacts/metadata/entry.rb
index 6896aa936d5..2fb6c327729 100644
--- a/lib/gitlab/ci/build/artifacts/metadata/path.rb
+++ b/lib/gitlab/ci/build/artifacts/metadata/entry.rb
@@ -2,7 +2,7 @@ module Gitlab
   module Ci::Build::Artifacts
     class Metadata
       ##
-      # Class that represents a simplified path to a file or
+      # Class that represents an entry (path and metadata) to a file or
       # directory in GitLab CI Build Artifacts binary file / archive
       #
       # This is IO-operations safe class, that does similar job to
@@ -10,13 +10,13 @@ module Gitlab
       #
       # This class is working only with UTF-8 encoded paths.
       #
-      class Path
-        attr_reader :path, :universe
+      class Entry
+        attr_reader :path, :entires
         attr_accessor :name
 
-        def initialize(path, universe, metadata = [])
+        def initialize(path, entires, metadata = [])
           @path = path.force_encoding('UTF-8')
-          @universe = universe
+          @entires = entires
           @metadata = metadata
 
           if path.include?("\0")
@@ -46,11 +46,11 @@ module Gitlab
         end
 
         def basename
-          (directory? && !blank_node?) ? name + ::File::SEPARATOR : name
+          (directory? && !blank_node?) ? name + '/' : name
         end
 
         def name
-          @name || @path.split(::File::SEPARATOR).last.to_s
+          @name || @path.split('/').last.to_s
         end
 
         def children
@@ -58,20 +58,17 @@ module Gitlab
           return @children if @children
 
           child_pattern = %r{^#{Regexp.escape(@path)}[^/]+/?$}
-          @children = select { |entry| entry =~ child_pattern }
+          @children = select_entires { |entry| entry =~ child_pattern }
         end
 
-        def directories
+        def directories(opts = {})
           return [] unless directory?
-          children.select(&:directory?)
-        end
-
-        def directories!
-          return directories unless has_parent?
+          dirs = children.select(&:directory?)
+          return dirs unless has_parent? && opts[:parent]
 
           dotted_parent = parent
           dotted_parent.name = '..'
-          directories.prepend(dotted_parent)
+          dirs.prepend(dotted_parent)
         end
 
         def files
@@ -80,7 +77,7 @@ module Gitlab
         end
 
         def metadata
-          @index ||= @universe.index(@path)
+          @index ||= @entires.index(@path)
           @metadata[@index] || {}
         end
 
@@ -88,12 +85,16 @@ module Gitlab
           @path.count('/') + (file? ? 1 : 0)
         end
 
+        def blank_node?
+          @path.empty? # "" is considered to be './'
+        end
+
         def exists?
-          blank_node? || @universe.include?(@path)
+          blank_node? || @entires.include?(@path)
         end
 
-        def blank_node?
-          @path.empty? # "" is considered to be './'
+        def empty?
+          children.empty?
         end
 
         def to_s
@@ -101,7 +102,7 @@ module Gitlab
         end
 
         def ==(other)
-          @path == other.path && @universe == other.universe
+          @path == other.path && @entires == other.entires
         end
 
         def inspect
@@ -111,11 +112,11 @@ module Gitlab
         private
 
         def new(path)
-          self.class.new(path, @universe, @metadata)
+          self.class.new(path, @entires, @metadata)
         end
 
-        def select
-          selected = @universe.select { |entry| yield entry }
+        def select_entires
+          selected = @entires.select { |entry| yield entry }
           selected.map { |path| new(path) }
         end
       end
diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb
index 3b254c3ce2f..a728227f87c 100644
--- a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb
+++ b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
-describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
-  let(:universe) do
+describe Gitlab::Ci::Build::Artifacts::Metadata::Entry do
+  let(:entries) do
     ['path/',
      'path/dir_1/',
      'path/dir_1/file_1',
@@ -21,7 +21,7 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
   end
 
   def string_path(string_path)
-    described_class.new(string_path, universe)
+    described_class.new(string_path, entries)
   end
 
   describe '/file/with/absolute_path', path: '/file/with/absolute_path' do
@@ -79,21 +79,38 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
     end
 
     describe '#directories' do
-      subject { |example| path(example).directories }
+      context 'without options' do
+        subject { |example| path(example).directories }
 
-      it { is_expected.to all(be_directory) }
-      it { is_expected.to all(be_an_instance_of described_class) }
-      it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') }
-    end
+        it { is_expected.to all(be_directory) }
+        it { is_expected.to all(be_an_instance_of described_class) }
+        it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') }
+      end
 
-    describe '#directories!' do
-      subject { |example| path(example).directories! }
+      context 'with option parent: true' do
+        subject { |example| path(example).directories(parent: true) }
 
-      it { is_expected.to all(be_directory) }
-      it { is_expected.to all(be_an_instance_of described_class) }
-      it do
-        is_expected.to contain_exactly string_path('path/dir_1/subdir/'),
-                                       string_path('path/')
+        it { is_expected.to all(be_directory) }
+        it { is_expected.to all(be_an_instance_of described_class) }
+        it do
+          is_expected.to contain_exactly string_path('path/dir_1/subdir/'),
+                                         string_path('path/')
+        end
+      end
+
+      describe '#nodes' do
+        subject { |example| path(example).nodes }
+        it { is_expected.to eq 2 }
+      end
+
+      describe '#exists?' do
+        subject { |example| path(example).exists? }
+        it { is_expected.to be true }
+      end
+
+      describe '#empty?' do
+        subject { |example| path(example).empty? }
+        it { is_expected.to be false }
       end
     end
   end
@@ -106,20 +123,37 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
       subject { |example| path(example).children }
       it { expect(subject.count).to eq 3 }
     end
+
+  end
+
+  describe 'path/dir_1/subdir/subfile', path: 'path/dir_1/subdir/subfile' do
+    describe '#nodes' do
+      subject { |example| path(example).nodes }
+      it { is_expected.to eq 4 }
+    end
   end
 
-  describe '#nodes', path: 'test' do
-    subject { |example| path(example).nodes }
-    it { is_expected.to eq 1 }
+  describe 'non-existent/', path: 'non-existent/' do
+    describe '#empty?' do
+      subject { |example| path(example).empty? }
+      it { is_expected.to be true }
+    end
+
+    describe '#exists?' do
+      subject { |example| path(example).exists? }
+      it { is_expected.to be false }
+    end
   end
 
-  describe '#nodes', path: 'test/' do
-    subject { |example| path(example).nodes }
-    it { is_expected.to eq 1 }
+  describe 'another_directory/', path: 'another_directory/' do
+    describe '#empty?' do
+      subject { |example| path(example).empty? }
+      it { is_expected.to be true }
+    end
   end
 
   describe '#metadata' do
-    let(:universe) do
+    let(:entries) do
       ['path/', 'path/file1', 'path/file2']
     end
 
@@ -128,21 +162,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
     end
 
     subject do
-      described_class.new('path/file1', universe, metadata).metadata[:name]
+      described_class.new('path/file1', entries, metadata).metadata[:name]
     end
 
     it { is_expected.to eq '/path/file1' }
   end
-
-  describe '#exists?', path: 'another_file' do
-    subject { |example| path(example).exists? }
-    it { is_expected.to be true }
-  end
-
-  describe '#exists?', path: './non_existent/' do
-    let(:universe) { ['./something'] }
-    subject { |example| path(example).exists? }
-
-    it { is_expected.to be false }
-  end
 end
diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
index 5c1a94974e8..42fbe40c890 100644
--- a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
+++ b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb
@@ -51,9 +51,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
       end
     end
 
-    describe '#to_path' do
-      subject { metadata('').to_path }
-      it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Path) }
+    describe '#to_entry' do
+      subject { metadata('').to_entry }
+      it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) }
     end
 
     describe '#full_version' do