Add latest changes from gitlab-org/gitlab@master

.gitlab/CODEOWNERS

@@ -3,7 +3,7 @@
 *.rake @gitlab-org/maintainers/rails-backend
 
 # Technical writing team are the default reviewers for everything in `doc/`
-/doc/ @axil @marcia @eread @mikelewis
+/doc/ @gl-docsteam
 
 # Frontend maintainers should see everything in `app/assets/`
 app/assets/ @gitlab-org/maintainers/frontend

app/helpers/blob_helper.rb

@@ -141,11 +141,7 @@ module BlobHelper
     if @build && @entry
       raw_project_job_artifacts_url(@project, @build, path: @entry.path, **kwargs)
     elsif @snippet
-      if @snippet.project_id
-        raw_project_snippet_url(@project, @snippet, **kwargs)
-      else
-        raw_snippet_url(@snippet, **kwargs)
-      end
+      reliable_raw_snippet_url(@snippet)
     elsif @blob
       project_raw_url(@project, @id, **kwargs)
     end

app/helpers/snippets_helper.rb

@@ -11,22 +11,40 @@ module SnippetsHelper
     end
   end
 
-  def reliable_snippet_path(snippet, opts = nil)
+  def reliable_snippet_path(snippet, opts = {})
+    reliable_snippet_url(snippet, opts.merge(only_path: true))
+  end
+
+  def reliable_raw_snippet_path(snippet, opts = {})
+    reliable_raw_snippet_url(snippet, opts.merge(only_path: true))
+  end
+
+  def reliable_snippet_url(snippet, opts = {})
     if snippet.project_id?
-      project_snippet_path(snippet.project, snippet, opts)
+      project_snippet_url(snippet.project, snippet, nil, opts)
     else
-      snippet_path(snippet, opts)
+      snippet_url(snippet, nil, opts)
     end
   end
 
-  def download_snippet_path(snippet)
-    if snippet.project_id
-      raw_project_snippet_path(@project, snippet, inline: false)
+  def reliable_raw_snippet_url(snippet, opts = {})
+    if snippet.project_id?
+      raw_project_snippet_url(snippet.project, snippet, nil, opts)
     else
-      raw_snippet_path(snippet, inline: false)
+      raw_snippet_url(snippet, nil, opts)
     end
   end
 
+  def download_raw_snippet_button(snippet)
+    link_to(icon('download'),
+            reliable_raw_snippet_path(snippet, inline: false),
+            target: '_blank',
+            rel: 'noopener noreferrer',
+            class: "btn btn-sm has-tooltip",
+            title: 'Download',
+            data: { container: 'body' })
+  end
+
   # Return the path of a snippets index for a user or for a project
   #
   # @returns String, path to snippet index
@@ -114,30 +132,45 @@ module SnippetsHelper
     { snippet_object: snippet, snippet_chunks: snippet_chunks }
   end
 
-  def snippet_embed
-    "<script src=\"#{url_for(only_path: false, overwrite_params: nil)}.js\"></script>"
+  def snippet_embed_tag(snippet)
+    content_tag(:script, nil, src: reliable_snippet_url(snippet, format: :js, only_path: false))
   end
 
-  def embedded_snippet_raw_button
-    blob = @snippet.blob
-    return if blob.empty? || blob.binary? || blob.stored_externally?
+  def snippet_badge(snippet)
+    return unless attrs = snippet_badge_attributes(snippet)
 
-    snippet_raw_url = if @snippet.is_a?(PersonalSnippet)
-                        raw_snippet_url(@snippet)
-                      else
-                        raw_project_snippet_url(@snippet.project, @snippet)
+    css_class, text = attrs
+    tag.span(class: ['badge', 'badge-gray']) do
+      concat(tag.i(class: ['fa', css_class]))
+      concat(' ')
+      concat(text)
+    end
   end
 
-    link_to external_snippet_icon('doc-code'), snippet_raw_url, class: 'btn', target: '_blank', rel: 'noopener noreferrer', title: 'Open raw'
+  def snippet_badge_attributes(snippet)
+    if snippet.private?
+      ['fa-lock', _('private')]
+    end
   end
 
-  def embedded_snippet_download_button
-    download_url = if @snippet.is_a?(PersonalSnippet)
-                     raw_snippet_url(@snippet, inline: false)
-                   else
-                     raw_project_snippet_url(@snippet.project, @snippet, inline: false)
+  def embedded_raw_snippet_button
+    blob = @snippet.blob
+    return if blob.empty? || blob.binary? || blob.stored_externally?
+
+    link_to(external_snippet_icon('doc-code'),
+            reliable_raw_snippet_url(@snippet),
+            class: 'btn',
+            target: '_blank',
+            rel: 'noopener noreferrer',
+            title: 'Open raw')
   end
 
-    link_to external_snippet_icon('download'), download_url, class: 'btn', target: '_blank', title: 'Download', rel: 'noopener noreferrer'
+  def embedded_snippet_download_button
+    link_to(external_snippet_icon('download'),
+            reliable_raw_snippet_url(@snippet, inline: false),
+            class: 'btn',
+            target: '_blank',
+            title: 'Download',
+            rel: 'noopener noreferrer')
   end
 end

app/models/group.rb

@@ -263,8 +263,8 @@ class Group < Namespace
     members_with_parents.maintainers.exists?(user_id: user)
   end
 
-  def has_container_repositories?
-    container_repositories.exists?
+  def has_container_repository_including_subgroups?
+    ::ContainerRepository.for_group_and_its_subgroups(self).exists?
   end
 
   # @deprecated

app/services/groups/transfer_service.rb

@@ -75,7 +75,7 @@ module Groups
     # rubocop: enable CodeReuse/ActiveRecord
 
     def group_projects_contain_registry_images?
-      @group.has_container_repositories?
+      @group.has_container_repository_including_subgroups?
     end
 
     def update_group_attributes

app/services/groups/update_service.rb

@@ -43,8 +43,9 @@ module Groups
     def renaming_group_with_container_registry_images?
       new_path = params[:path]
 
-      new_path && new_path != group.path &&
-        group.has_container_repositories?
+      new_path &&
+        new_path != group.path &&
+        group.has_container_repository_including_subgroups?
     end
 
     def container_images_error

app/views/search/results/_snippet_blob.html.haml

 - snippet_blob = chunk_snippet(snippet_blob, @search_term)
 - snippet = snippet_blob[:snippet_object]
 - snippet_chunks = snippet_blob[:snippet_chunks]
+- snippet_path = reliable_snippet_path(snippet)
 
 .search-result-row
   %span
@@ -11,7 +12,6 @@
       = snippet.author_name
     %span.light= time_ago_with_tooltip(snippet.created_at)
   %h4.snippet-title
-  - snippet_path = reliable_snippet_path(snippet)
   .file-holder
     .js-file-title.file-title
       = link_to snippet_path do

app/views/search/results/_snippet_title.html.haml

@@ -2,10 +2,7 @@
   %h4.snippet-title.term
     = link_to reliable_snippet_path(snippet_title) do
       = truncate(snippet_title.title, length: 60)
-      - if snippet_title.private?
-        %span.badge.badge-gray
-          %i.fa.fa-lock
-          = _("private")
+      = snippet_badge(snippet_title)
     %span.cgray.monospace.tiny.float-right.term
       = snippet_title.file_name
 

app/views/shared/snippets/_blob.html.haml

@@ -8,7 +8,6 @@
     .btn-group{ role: "group" }<
       = copy_blob_source_button(blob)
       = open_raw_blob_button(blob)
-
-      = link_to icon('download'), download_snippet_path(@snippet), target: '_blank', class: "btn btn-sm has-tooltip", title: 'Download', data: { container: 'body' }
+      = download_raw_snippet_button(@snippet)
 
 = render 'projects/blob/content', blob: blob

app/views/shared/snippets/_embed.html.haml

@@ -17,7 +17,7 @@
 
     .file-actions.d-none.d-sm-block
       .btn-group{ role: "group" }<
-        = embedded_snippet_raw_button
+        = embedded_raw_snippet_button
 
         = embedded_snippet_download_button
   %article.file-holder.snippet-file-content

app/views/shared/snippets/_header.html.haml

@@ -44,7 +44,7 @@
             %li
               %button.js-share-btn.btn.btn-transparent{ type: 'button' }
                 %strong.embed-toggle-list-item= _("Share")
-        %input.js-snippet-url-area.snippet-embed-input.form-control{ type: "text", autocomplete: 'off', value: snippet_embed }
+        %input.js-snippet-url-area.snippet-embed-input.form-control{ type: "text", autocomplete: 'off', value: snippet_embed_tag(@snippet) }
         .input-group-append
           = clipboard_button(title: _('Copy'), class: 'js-clipboard-btn snippet-clipboard-btn btn btn-default', target: '.js-snippet-url-area')
     .clearfix

spec/finders/container_repositories_finder_spec.rb

@@ -8,7 +8,7 @@ describe ContainerRepositoriesFinder do
 
   let(:group) { create(:group) }
   let(:project) { create(:project, group: group) }
-  let(:project_repository) { create(:container_repository, project: project) }
+  let!(:project_repository) { create(:container_repository, project: project) }
 
   before do
     group.add_reporter(reporter)

spec/helpers/snippets_helper_spec.rb

@@ -3,33 +3,217 @@
 require 'spec_helper'
 
 describe SnippetsHelper do
+  include Gitlab::Routing
   include IconsHelper
 
-  describe '#embedded_snippet_raw_button' do
-    it 'gives view raw button of embedded snippets for project snippets' do
-      @snippet = create(:project_snippet, :public)
+  let_it_be(:public_personal_snippet) { create(:personal_snippet, :public) }
+  let_it_be(:public_project_snippet) { create(:project_snippet, :public) }
+
+  describe '#reliable_snippet_path' do
+    subject { reliable_snippet_path(snippet) }
+
+    context 'personal snippets' do
+      let(:snippet) { public_personal_snippet }
+
+      context 'public' do
+        it 'returns a full path' do
+          expect(subject).to eq("/snippets/#{snippet.id}")
+        end
+      end
+    end
+
+    context 'project snippets' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns a full path' do
+        expect(subject).to eq("/#{snippet.project.full_path}/snippets/#{snippet.id}")
+      end
+    end
+  end
+
+  describe '#reliable_snippet_url' do
+    subject { reliable_snippet_url(snippet) }
+
+    context 'personal snippets' do
+      let(:snippet) { public_personal_snippet }
+
+      context 'public' do
+        it 'returns a full url' do
+          expect(subject).to eq("http://test.host/snippets/#{snippet.id}")
+        end
+      end
+    end
+
+    context 'project snippets' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns a full url' do
+        expect(subject).to eq("http://test.host/#{snippet.project.full_path}/snippets/#{snippet.id}")
+      end
+    end
+  end
+
+  describe '#reliable_raw_snippet_path' do
+    subject { reliable_raw_snippet_path(snippet) }
 
-      expect(embedded_snippet_raw_button.to_s).to eq("<a class=\"btn\" target=\"_blank\" rel=\"noopener noreferrer\" title=\"Open raw\" href=\"#{raw_project_snippet_url(@snippet.project, @snippet)}\">#{external_snippet_icon('doc-code')}</a>")
+    context 'personal snippets' do
+      let(:snippet) { public_personal_snippet }
+
+      context 'public' do
+        it 'returns a full path' do
+          expect(subject).to eq("/snippets/#{snippet.id}/raw")
+        end
+      end
+    end
+
+    context 'project snippets' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns a full path' do
+        expect(subject).to eq("/#{snippet.project.full_path}/snippets/#{snippet.id}/raw")
+      end
+    end
+  end
+
+  describe '#reliable_raw_snippet_url' do
+    subject { reliable_raw_snippet_url(snippet) }
+
+    context 'personal snippets' do
+      let(:snippet) { public_personal_snippet }
+
+      context 'public' do
+        it 'returns a full url' do
+          expect(subject).to eq("http://test.host/snippets/#{snippet.id}/raw")
         end
+      end
+    end
+
+    context 'project snippets' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns a full url' do
+        expect(subject).to eq("http://test.host/#{snippet.project.full_path}/snippets/#{snippet.id}/raw")
+      end
+    end
+  end
+
+  describe '#embedded_raw_snippet_button' do
+    subject { embedded_raw_snippet_button.to_s }
 
-    it 'gives view raw button of embedded snippets for personal snippets' do
+    it 'returns view raw button of embedded snippets for personal snippets' do
       @snippet = create(:personal_snippet, :public)
 
-      expect(embedded_snippet_raw_button.to_s).to eq("<a class=\"btn\" target=\"_blank\" rel=\"noopener noreferrer\" title=\"Open raw\" href=\"#{raw_snippet_url(@snippet)}\">#{external_snippet_icon('doc-code')}</a>")
+      expect(subject).to eq(download_link("http://test.host/snippets/#{@snippet.id}/raw"))
+    end
+
+    it 'returns view raw button of embedded snippets for project snippets' do
+      @snippet = create(:project_snippet, :public)
+
+      expect(subject).to eq(download_link("http://test.host/#{@snippet.project.path_with_namespace}/snippets/#{@snippet.id}/raw"))
+    end
+
+    def download_link(url)
+      "<a class=\"btn\" target=\"_blank\" rel=\"noopener noreferrer\" title=\"Open raw\" href=\"#{url}\">#{external_snippet_icon('doc-code')}</a>"
     end
   end
 
   describe '#embedded_snippet_download_button' do
-    it 'gives download button of embedded snippets for project snippets' do
+    subject { embedded_snippet_download_button }
+
+    it 'returns download button of embedded snippets for personal snippets' do
+      @snippet = create(:personal_snippet, :public)
+
+      expect(subject).to eq(download_link("http://test.host/snippets/#{@snippet.id}/raw"))
+    end
+
+    it 'returns download button of embedded snippets for project snippets' do
       @snippet = create(:project_snippet, :public)
 
-      expect(embedded_snippet_download_button.to_s).to eq("<a class=\"btn\" target=\"_blank\" title=\"Download\" rel=\"noopener noreferrer\" href=\"#{raw_project_snippet_url(@snippet.project, @snippet, inline: false)}\">#{external_snippet_icon('download')}</a>")
+      expect(subject).to eq(download_link("http://test.host/#{@snippet.project.path_with_namespace}/snippets/#{@snippet.id}/raw"))
     end
 
-    it 'gives download button of embedded snippets for personal snippets' do
-      @snippet = create(:personal_snippet, :public)
+    def download_link(url)
+      "<a class=\"btn\" target=\"_blank\" title=\"Download\" rel=\"noopener noreferrer\" href=\"#{url}?inline=false\">#{external_snippet_icon('download')}</a>"
+    end
+  end
+
+  describe '#snippet_embed_tag' do
+    subject { snippet_embed_tag(snippet) }
+
+    context 'personal snippets' do
+      let(:snippet) { public_personal_snippet }
+
+      context 'public' do
+        it 'returns a script tag with the snippet full url' do
+          expect(subject).to eq(script_embed("http://test.host/snippets/#{snippet.id}"))
+        end
+      end
+    end
+
+    context 'project snippets' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns a script tag with the snippet full url' do
+        expect(subject).to eq(script_embed("http://test.host/#{snippet.project.path_with_namespace}/snippets/#{snippet.id}"))
+      end
+    end
+
+    def script_embed(url)
+      "<script src=\"#{url}.js\"></script>"
+    end
+  end
+
+  describe '#download_raw_snippet_button' do
+    subject { download_raw_snippet_button(snippet) }
 
-      expect(embedded_snippet_download_button.to_s).to eq("<a class=\"btn\" target=\"_blank\" title=\"Download\" rel=\"noopener noreferrer\" href=\"#{raw_snippet_url(@snippet, inline: false)}\">#{external_snippet_icon('download')}</a>")
+    context 'with personal snippet' do
+      let(:snippet) { public_personal_snippet }
+
+      it 'returns the download button' do
+        expect(subject).to eq(download_link("/snippets/#{snippet.id}/raw"))
+      end
+    end
+
+    context 'with project snippet' do
+      let(:snippet) { public_project_snippet }
+
+      it 'returns the download button' do
+        expect(subject).to eq(download_link("/#{snippet.project.path_with_namespace}/snippets/#{snippet.id}/raw"))
+      end
+    end
+
+    def download_link(url)
+      "<a target=\"_blank\" rel=\"noopener noreferrer\" class=\"btn btn-sm has-tooltip\" title=\"Download\" data-container=\"body\" href=\"#{url}?inline=false\"><i aria-hidden=\"true\" data-hidden=\"true\" class=\"fa fa-download\"></i></a>"
+    end
+  end
+
+  describe '#snippet_badge' do
+    let(:snippet) { build(:personal_snippet, visibility) }
+
+    subject { snippet_badge(snippet) }
+
+    context 'when snippet is private' do
+      let(:visibility) { :private }
+
+      it 'returns the snippet badge' do
+        expect(subject).to eq "<span class=\"badge badge-gray\"><i class=\"fa fa-lock\"></i> private</span>"
+      end
+    end
+
+    context 'when snippet is public' do
+      let(:visibility) { :public }
+
+      it 'does not return anything' do
+        expect(subject).to be_nil
+      end
+    end
+
+    context 'when snippet is internal' do
+      let(:visibility) { :internal }
+
+      it 'does not return anything' do
+        expect(subject).to be_nil
+      end
     end
   end
 end

spec/models/personal_snippet_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe PersonalSnippet do
+  describe '#embeddable?' do
+    [
+      { snippet: :public,   embeddable: true },
+      { snippet: :internal, embeddable: false },
+      { snippet: :private,  embeddable: false }
+    ].each do |combination|
+      it 'returns true when snippet is public' do
+        snippet = build(:personal_snippet, combination[:snippet])
+
+        expect(snippet.embeddable?).to eq(combination[:embeddable])
+      end
+    end
+  end
+end

spec/models/project_snippet_spec.rb

@@ -10,4 +10,25 @@ describe ProjectSnippet do
   describe "Validation" do
     it { is_expected.to validate_presence_of(:project) }
   end
+
+  describe '#embeddable?' do
+    [
+      { project: :public,   snippet: :public,   embeddable: true },
+      { project: :internal, snippet: :public,   embeddable: false },
+      { project: :private,  snippet: :public,   embeddable: false },
+      { project: :public,   snippet: :internal, embeddable: false },
+      { project: :internal, snippet: :internal, embeddable: false },
+      { project: :private,  snippet: :internal, embeddable: false },
+      { project: :public,   snippet: :private,  embeddable: false },
+      { project: :internal, snippet: :private,  embeddable: false },
+      { project: :private,  snippet: :private,  embeddable: false }
+    ].each do |combination|
+      it 'only returns true when both project and snippet are public' do
+        project = create(:project, combination[:project])
+        snippet = build(:project_snippet, combination[:snippet], project: project)
+
+        expect(snippet.embeddable?).to eq(combination[:embeddable])
+      end
+    end
+  end
 end

spec/models/snippet_spec.rb

@@ -451,41 +451,4 @@ describe Snippet do
       expect(blob.data).to eq(snippet.content)
     end
   end
-
-  describe '#embeddable?' do
-    context 'project snippet' do
-      [
-        { project: :public,   snippet: :public,   embeddable: true },
-        { project: :internal, snippet: :public,   embeddable: false },
-        { project: :private,  snippet: :public,   embeddable: false },
-        { project: :public,   snippet: :internal, embeddable: false },
-        { project: :internal, snippet: :internal, embeddable: false },
-        { project: :private,  snippet: :internal, embeddable: false },
-        { project: :public,   snippet: :private,  embeddable: false },
-        { project: :internal, snippet: :private,  embeddable: false },
-        { project: :private,  snippet: :private,  embeddable: false }
-      ].each do |combination|
-        it 'only returns true when both project and snippet are public' do
-          project = create(:project, combination[:project])
-          snippet = create(:project_snippet, combination[:snippet], project: project)
-
-          expect(snippet.embeddable?).to eq(combination[:embeddable])
-        end
-      end
-    end
-
-    context 'personal snippet' do
-      [
-        { snippet: :public,   embeddable: true },
-        { snippet: :internal, embeddable: false },
-        { snippet: :private,  embeddable: false }
-      ].each do |combination|
-        it 'only returns true when snippet is public' do
-          snippet = create(:personal_snippet, combination[:snippet])
-
-          expect(snippet.embeddable?).to eq(combination[:embeddable])
-        end
-      end
-    end
-  end
 end

spec/services/groups/transfer_service_spec.rb

@@ -427,21 +427,35 @@ describe Groups::TransferService do
       end
     end
 
-    context 'when a project in group has container images' do
+    context 'when a project has container images' do
       let(:group) { create(:group, :public, :nested) }
-      let!(:project) { create(:project, :repository, :public, namespace: group) }
+      let!(:container_repository) { create(:container_repository, project: project) }
+
+      subject { transfer_service.execute(new_parent_group) }
 
       before do
-        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
-        create(:container_repository, project: project, name: :image)
-        create(:group_member, :owner, group: new_parent_group, user: user)
+        group.add_owner(user)
+        new_parent_group.add_owner(user)
       end
 
-      it 'does not allow group to be transferred' do
-        transfer_service.execute(new_parent_group)
+      context 'within group' do
+        let(:project) { create(:project, :repository, :public, namespace: group) }
 
+        it 'does not transfer' do
+          expect(subject).to be false
           expect(transfer_service.error).to match(/Docker images in their Container Registry/)
         end
       end
+
+      context 'within subgroup' do
+        let(:subgroup) { create(:group, parent: group) }
+        let(:project) { create(:project, :repository, :public, namespace: subgroup) }
+
+        it 'does not transfer' do
+          expect(subject).to be false
+          expect(transfer_service.error).to match(/Docker images in their Container Registry/)
+        end
+      end
+    end
   end
 end

spec/services/groups/update_service_spec.rb

@@ -32,6 +32,43 @@ describe Groups::UpdateService do
 
           expect(service.execute).to be_falsey
         end
+
+        context 'when a project has container images' do
+          let(:params) { { path: SecureRandom.hex } }
+          let!(:container_repository) { create(:container_repository, project: project) }
+
+          subject { described_class.new(public_group, user, params).execute }
+
+          context 'within group' do
+            let(:project) { create(:project, group: public_group) }
+
+            context 'with path updates' do
+              it 'does not allow the update' do
+                expect(subject).to be false
+                expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
+              end
+            end
+
+            context 'with name updates' do
+              let(:params) { { name: 'new-name' } }
+
+              it 'allows the update' do
+                expect(subject).to be true
+                expect(public_group.reload.name).to eq('new-name')
+              end
+            end
+          end
+
+          context 'within subgroup' do
+            let(:subgroup) { create(:group, parent: public_group) }
+            let(:project) { create(:project, group: subgroup) }
+
+            it 'does not allow path updates' do
+              expect(subject).to be false
+              expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
+            end
+          end
+        end
       end
 
       context "internal group with internal project" do
@@ -148,30 +185,6 @@ describe Groups::UpdateService do
     end
   end
 
-  context 'projects in group have container images' do
-    let(:service) { described_class.new(public_group, user, path: SecureRandom.hex) }
-    let(:project) { create(:project, :internal, group: public_group) }
-
-    before do
-      stub_container_registry_tags(repository: /image/, tags: %w[rc1])
-      create(:container_repository, project: project, name: :image)
-    end
-
-    it 'does not allow path to be changed' do
-      result = described_class.new(public_group, user, path: 'new-path').execute
-
-      expect(result).to eq false
-      expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
-    end
-
-    it 'allows other settings to be changed' do
-      result = described_class.new(public_group, user, name: 'new-name').execute
-
-      expect(result).to eq true
-      expect(public_group.reload.name).to eq('new-name')
-    end
-  end
-
   context 'for a subgroup' do
     let(:subgroup) { create(:group, :private, parent: private_group) }