Merge branch '21705-number-of-snippets-in-the-snippets-page-is-wrong' into 'master'

Resolve "Number of snippets in the snippets page is wrong"

See merge request gitlab-org/gitlab!23340

app/controllers/dashboard/snippets_controller.rb

@@ -7,6 +7,10 @@ class Dashboard::SnippetsController < Dashboard::ApplicationController
   skip_cross_project_access_check :index
 
   def index
+    @snippet_counts = Snippets::CountService
+      .new(current_user, author: current_user)
+      .execute
+
     @snippets = SnippetsFinder.new(current_user, author: current_user, scope: params[:scope])
       .execute
       .page(params[:page])

app/controllers/projects/snippets_controller.rb

@@ -30,6 +30,10 @@ class Projects::SnippetsController < Projects::ApplicationController
   respond_to :html
 
   def index
+    @snippet_counts = Snippets::CountService
+      .new(current_user, project: @project)
+      .execute
+
     @snippets = SnippetsFinder.new(current_user, project: @project, scope: params[:scope])
       .execute
       .page(params[:page])

app/services/snippets/count_service.rb

+# frozen_string_literal: true
+
+# Service for calculating visible Snippet counts via one query
+# for the given user or project.
+#
+# Authorisation level checks will be included, ensuring the correct
+# counts will be returned for the given user (if any).
+#
+# Basic usage:
+#
+#   user = User.find(1)
+#
+#   Snippets::CountService.new(user, author: user).execute
+#   #=> {
+#     are_public: 1,
+#     are_internal: 1,
+#     are_private: 1,
+#     all: 3
+#   }
+#
+# Counts can be scoped to a project:
+#
+#   user = User.find(1)
+#   project = Project.find(1)
+#
+#   Snippets::CountService.new(user, project: project).execute
+#   #=> {
+#     are_public: 1,
+#     are_internal: 1,
+#     are_private: 0,
+#     all: 2
+#   }
+#
+# Either a project or an author *must* be supplied.
+module Snippets
+  class CountService
+    def initialize(current_user, author: nil, project: nil)
+      if !author && !project
+        raise(
+          ArgumentError, 'Must provide either an author or a project'
+        )
+      end
+
+      @snippets_finder = SnippetsFinder.new(current_user, author: author, project: project)
+    end
+
+    def execute
+      counts = snippet_counts
+      return {} unless counts
+
+      counts.slice(
+        :are_public,
+        :are_private,
+        :are_internal,
+        :are_public_or_internal,
+        :total
+      )
+    end
+
+    private
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    def snippet_counts
+      @snippets_finder.execute
+        .reorder(nil)
+        .select("
+          count(case when snippets.visibility_level=#{Snippet::PUBLIC} and snippets.secret is FALSE then 1 else null end) as are_public,
+          count(case when snippets.visibility_level=#{Snippet::INTERNAL} then 1 else null end) as are_internal,
+          count(case when snippets.visibility_level=#{Snippet::PRIVATE} then 1 else null end) as are_private,
+          count(case when visibility_level=#{Snippet::PUBLIC} OR visibility_level=#{Snippet::INTERNAL} then 1 else null end) as are_public_or_internal,
+          count(*) as total
+        ")
+        .first
+    end
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+end

app/views/dashboard/snippets/index.html.haml

@@ -5,7 +5,7 @@
 
 = render 'dashboard/snippets_head'
 - if current_user.snippets.exists?
-  = render partial: 'snippets/snippets_scope_menu', locals: { include_private: true }
+  = render partial: 'snippets/snippets_scope_menu', locals: { include_private: true, counts: @snippet_counts }
 
 - if current_user.snippets.exists?
   = render partial: 'shared/snippets/list', locals: { link_project: true }

app/views/projects/snippets/index.html.haml

@@ -5,7 +5,7 @@
   - if current_user
     .top-area
       - include_private = @project.team.member?(current_user) || current_user.admin?
-      = render partial: 'snippets/snippets_scope_menu', locals: { subject: @project, include_private: include_private }
+      = render partial: 'snippets/snippets_scope_menu', locals: { subject: @project, include_private: include_private, counts: @snippet_counts }
 
       - if new_project_snippet_link.present?
         .nav-controls

app/views/snippets/_snippets_scope_menu.html.haml

@@ -7,25 +7,25 @@
       = _("All")
       %span.badge.badge-pill
         - if include_private
-          = subject.snippets.count
+          = counts[:total]
         - else
-          = subject.snippets.public_and_internal_only.count
+          = counts[:are_public_or_internal]
 
   - if include_private
     %li{ class: active_when(params[:scope] == "are_private") }
       = link_to subject_snippets_path(subject, scope: 'are_private') do
         = _("Private")
         %span.badge.badge-pill
-          = subject.snippets.are_private.count
+          = counts[:are_private]
 
   %li{ class: active_when(params[:scope] == "are_internal") }
     = link_to subject_snippets_path(subject, scope: 'are_internal') do
       = _("Internal")
       %span.badge.badge-pill
-        = subject.snippets.are_internal.count
+        = counts[:are_internal]
 
   %li{ class: active_when(params[:scope] == "are_public") }
     = link_to subject_snippets_path(subject, scope: 'are_public') do
       = _("Public")
       %span.badge.badge-pill
-        = subject.snippets.are_public.count
+        = counts[:are_public]

changelogs/unreleased/21705-number-of-snippets-in-the-snippets-page-is-wrong.yml

+---
+title: Fix bug with snippet counts not being scoped to current authorisation
+merge_request: 21705
+author:
+type: fixed

spec/controllers/dashboard/snippets_controller_spec.rb

@@ -17,5 +17,14 @@ describe Dashboard::SnippetsController do
         create(:personal_snippet, :public, author: user)
       end
     end
+
+    it 'fetches snippet counts via the snippet count service' do
+      service = double(:count_service, execute: {})
+      expect(Snippets::CountService)
+        .to receive(:new).with(user, author: user)
+        .and_return(service)
+
+      get :index
+    end
   end
 end

spec/controllers/projects/snippets_controller_spec.rb

@@ -27,6 +27,15 @@ describe Projects::SnippetsController do
       end
     end
 
+    it 'fetches snippet counts via the snippet count service' do
+      service = double(:count_service, execute: {})
+      expect(Snippets::CountService)
+        .to receive(:new).with(nil, project: project)
+        .and_return(service)
+
+      get :index, params: { namespace_id: project.namespace, project_id: project }
+    end
+
     context 'when the project snippet is private' do
       let!(:project_snippet) { create(:project_snippet, :private, project: project, author: user) }
 

spec/features/dashboard/snippets_spec.rb

@@ -59,6 +59,10 @@ describe 'Dashboard snippets' do
       visit dashboard_snippets_path
     end
 
+    it_behaves_like 'tabs with counts' do
+      let_it_be(:counts) { { all: '3', public: '1', private: '1', internal: '1' } }
+    end
+
     it 'contains all snippets of logged user' do
       expect(page).to have_selector('.snippet-row', count: 3)
 

spec/features/projects/snippets/user_views_snippets_spec.rb

@@ -31,6 +31,16 @@ describe 'Projects > Snippets > User views snippets' do
       it_behaves_like 'paginated snippets'
     end
 
+    context 'filtering by visibility' do
+      before do
+        visit_project_snippets
+      end
+
+      it_behaves_like 'tabs with counts' do
+        let_it_be(:counts) { { all: '1', public: '0', private: '1', internal: '0' } }
+      end
+    end
+
     it 'shows snippets' do
       visit_project_snippets
 

spec/services/snippets/count_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Snippets::CountService do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :public) }
+
+  describe '#new' do
+    it 'raises an error if no author or project' do
+      expect { described_class.new(user) }.to raise_error(ArgumentError)
+    end
+
+    it 'uses the SnippetsFinder to scope snippets by user' do
+      expect(SnippetsFinder)
+        .to receive(:new)
+        .with(user, author: user, project: nil)
+
+      described_class.new(user, author: user)
+    end
+
+    it 'allows scoping to project' do
+      expect(SnippetsFinder)
+        .to receive(:new)
+        .with(user, author: nil, project: project)
+
+      described_class.new(user, project: project)
+    end
+  end
+
+  describe '#execute' do
+    subject { described_class.new(user, author: user).execute }
+
+    it 'returns a hash of counts' do
+      expect(subject).to match({
+        are_public: 0,
+        are_internal: 0,
+        are_private: 0,
+        are_public_or_internal: 0,
+        total: 0
+      })
+    end
+
+    it 'only counts snippets the user has access to' do
+      create(:personal_snippet, :private, author: user)
+      create(:project_snippet, :private, author: user)
+      create(:project_snippet, :private, author: create(:user))
+
+      expect(subject).to match({
+        are_public: 0,
+        are_internal: 0,
+        are_private: 1,
+        are_public_or_internal: 0,
+        total: 1
+      })
+    end
+
+    it 'returns an empty hash if select returns nil' do
+      allow_next_instance_of(described_class) do |instance|
+        allow(instance).to receive(:snippet_counts).and_return(nil)
+      end
+
+      expect(subject).to match({})
+    end
+  end
+end

spec/support/shared_examples/features/snippets_shared_examples.rb

@@ -18,3 +18,35 @@ RSpec.shared_examples 'paginated snippets' do |remote: false|
     end
   end
 end
+
+RSpec.shared_examples 'tabs with counts' do
+  let(:tabs) { page.all('.snippet-scope-menu li') }
+
+  it 'shows a tab for All snippets and count' do
+    tab = tabs[0]
+
+    expect(tab.text).to include('All')
+    expect(tab.find('.badge').text).to eq(counts[:all])
+  end
+
+  it 'shows a tab for Private snippets and count' do
+    tab = tabs[1]
+
+    expect(tab.text).to include('Private')
+    expect(tab.find('.badge').text).to eq(counts[:private])
+  end
+
+  it 'shows a tab for Internal snippets and count' do
+    tab = tabs[2]
+
+    expect(tab.text).to include('Internal')
+    expect(tab.find('.badge').text).to eq(counts[:internal])
+  end
+
+  it 'shows a tab for Public snippets and count' do
+    tab = tabs[3]
+
+    expect(tab.text).to include('Public')
+    expect(tab.find('.badge').text).to eq(counts[:public])
+  end
+end