Merge branch...

Merge branch '208805-fix-filter-by-releases-in-groups-issue-and-merge-request-search-bar' into 'master'

Fix filter by releases at group issues and merge requests search bar

Closes #208805

See merge request gitlab-org/gitlab!26740

app/controllers/groups/releases_controller.rb

+# frozen_string_literal: true
+
+module Groups
+  class ReleasesController < Groups::ApplicationController
+    def index
+      respond_to do |format|
+        format.json do
+          render json: ReleaseSerializer.new.represent(releases)
+        end
+      end
+    end
+
+    private
+
+    def releases
+      ReleasesFinder
+        .new(@group, current_user, { include_subgroups: true })
+        .execute(preload: false)
+        .page(params[:page])
+        .per(30)
+    end
+  end
+end

app/finders/releases_finder.rb

 # frozen_string_literal: true
 
 class ReleasesFinder
-  attr_reader :project, :current_user, :params
+  include Gitlab::Utils::StrongMemoize
 
-  def initialize(project, current_user = nil, params = {})
-    @project = project
+  attr_reader :parent, :current_user, :params
+
+  def initialize(parent, current_user = nil, params = {})
+    @parent = parent
     @current_user = current_user
     @params = params
   end
 
   def execute(preload: true)
-    return Release.none unless Ability.allowed?(current_user, :read_release, project)
+    return Release.none if projects.empty?
 
-    # See https://gitlab.com/gitlab-org/gitlab/-/issues/211988
-    releases = project.releases.where.not(tag: nil) # rubocop:disable CodeReuse/ActiveRecord
+    releases = get_releases
     releases = by_tag(releases)
     releases = releases.preloaded if preload
     releases.sorted
@@ -21,6 +22,34 @@ class ReleasesFinder
 
   private
 
+  def get_releases
+    Release.where(project_id: projects).where.not(tag: nil) # rubocop: disable CodeReuse/ActiveRecord
+  end
+
+  def include_subgroups?
+    params.fetch(:include_subgroups, false)
+  end
+
+  def projects
+    strong_memoize(:projects) do
+      if parent.is_a?(Project)
+        Ability.allowed?(current_user, :read_release, parent) ? [parent] : []
+      elsif parent.is_a?(Group)
+        accessible_projects
+      end
+    end
+  end
+
+  def accessible_projects
+    projects = if include_subgroups?
+                 Project.for_group_and_its_subgroups(parent)
+               else
+                 parent.projects
+               end
+
+    projects.select { |project| Ability.allowed?(current_user, :read_release, project) }
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def by_tag(releases)
     return releases unless params[:tag].present?

app/helpers/search_helper.rb

@@ -229,6 +229,7 @@ module SearchHelper
       opts[:data]['group-id'] = @group.id
       opts[:data]['labels-endpoint'] = group_labels_path(@group)
       opts[:data]['milestones-endpoint'] = group_milestones_path(@group)
+      opts[:data]['releases-endpoint'] = group_releases_path(@group)
     else
       opts[:data]['labels-endpoint'] = dashboard_labels_path
       opts[:data]['milestones-endpoint'] = dashboard_milestones_path

app/serializers/release_entity.rb

+# frozen_string_literal: true
+
+class ReleaseEntity < Grape::Entity
+  expose :id
+  expose :tag # see https://gitlab.com/gitlab-org/gitlab/-/issues/36338
+end

app/serializers/release_serializer.rb

+# frozen_string_literal: true
+
+class ReleaseSerializer < BaseSerializer
+  entity ReleaseEntity
+end

changelogs/unreleased/208805-fix-filter-by-releases-in-groups-issue-and-merge-request-search-ba.yml

+---
+title: Fix filter by releases at group issues and merge requests search bar.
+merge_request: 26740
+author: Gilang Gumilar
+type: fixed

config/routes/group.rb

@@ -69,6 +69,8 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
       end
     end
 
+    resources :releases, only: [:index]
+
     resources :deploy_tokens, constraints: { id: /\d+/ }, only: [] do
       member do
         put :revoke

spec/controllers/groups/releases_controller_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Groups::ReleasesController do
+  let(:group) { create(:group) }
+  let!(:project)         { create(:project, :repository, :public, namespace: group) }
+  let!(:private_project) { create(:project, :repository, :private, namespace: group) }
+  let(:developer)        { create(:user) }
+  let!(:release_1)       { create(:release, project: project, tag: 'v1', released_at: Time.zone.parse('2020-02-15')) }
+  let!(:release_2)       { create(:release, project: project, tag: 'v2', released_at: Time.zone.parse('2020-02-20')) }
+  let!(:private_release_1)       { create(:release, project: private_project, tag: 'p1', released_at: Time.zone.parse('2020-03-01')) }
+  let!(:private_release_2)       { create(:release, project: private_project, tag: 'p2', released_at: Time.zone.parse('2020-03-05')) }
+
+  before do
+    private_project.add_developer(developer)
+  end
+
+  describe 'GET #index' do
+    context 'as json' do
+      let(:format) { :json }
+
+      subject { get :index, params: { group_id: group }, format: format }
+
+      context 'json_response' do
+        before do
+          subject
+        end
+
+        it 'returns an application/json content_type' do
+          expect(response.content_type).to eq 'application/json'
+        end
+
+        it 'returns OK' do
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'the user is not authorized' do
+        before do
+          subject
+        end
+
+        it 'does not return any releases' do
+          expect(json_response.map {|r| r['tag'] } ).to match_array(%w(v2 v1))
+        end
+
+        it 'returns OK' do
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'the user is authorized' do
+        it "returns all group's public and private project's releases as JSON, ordered by released_at" do
+          sign_in(developer)
+
+          subject
+
+          expect(json_response.map {|r| r['tag'] } ).to match_array(%w(p2 p1 v2 v1))
+        end
+      end
+
+      context 'N+1 queries' do
+        it 'avoids N+1 database queries' do
+          control_count = ActiveRecord::QueryRecorder.new { subject }.count
+
+          create_list(:release, 5, project: project)
+          create_list(:release, 5, project: private_project)
+
+          expect { subject }.not_to exceed_query_limit(control_count)
+        end
+      end
+    end
+  end
+end

spec/finders/releases_finder_spec.rb

@@ -3,29 +3,68 @@
 require 'spec_helper'
 
 RSpec.describe ReleasesFinder do
-  let(:user)       { create(:user) }
-  let(:project)    { create(:project, :repository) }
-  let(:params)     { {} }
+  let(:user)  { create(:user) }
+  let(:group) { create :group }
+  let(:project) { create(:project, :repository, group: group) }
+  let(:params) { {} }
+  let(:args) { {} }
   let(:repository) { project.repository }
   let(:v1_0_0)     { create(:release, project: project, tag: 'v1.0.0') }
   let(:v1_1_0)     { create(:release, project: project, tag: 'v1.1.0') }
-  let(:finder) { described_class.new(project, user, params) }
 
   before do
     v1_0_0.update_attribute(:released_at, 2.days.ago)
     v1_1_0.update_attribute(:released_at, 1.day.ago)
   end
 
-  describe '#execute' do
-    subject { finder.execute(**args) }
+  shared_examples_for 'when the user is not part of the project' do
+    it 'returns no releases' do
+      is_expected.to be_empty
+    end
+  end
+
+  # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27716
+  shared_examples_for 'when tag is nil' do
+    before do
+      v1_0_0.update_column(:tag, nil)
+    end
+
+    it 'ignores rows with a nil tag' do
+      expect(subject.size).to eq(1)
+      expect(subject).to eq([v1_1_0])
+    end
+  end
+
+  shared_examples_for 'when a tag parameter is passed' do
+    let(:params) { { tag: 'v1.0.0' } }
+
+    it 'only returns the release with the matching tag' do
+      expect(subject).to eq([v1_0_0])
+    end
+  end
+
+  shared_examples_for 'preload' do
+    it 'preloads associations' do
+      expect(Release).to receive(:preloaded).once.and_call_original
+
+      subject
+    end
+
+    context 'when preload is false' do
+      let(:args) { { preload: false } }
 
-    let(:args) { {} }
+      it 'does not preload associations' do
+        expect(Release).not_to receive(:preloaded)
 
-    context 'when the user is not part of the project' do
-      it 'returns no releases' do
-        is_expected.to be_empty
+        subject
       end
     end
+  end
+
+  describe 'when parent is a project' do
+    subject { described_class.new(project, user, params).execute(**args) }
+
+    it_behaves_like 'when the user is not part of the project'
 
     context 'when the user is a project developer' do
       before do
@@ -38,39 +77,137 @@ RSpec.describe ReleasesFinder do
         expect(subject).to eq([v1_1_0, v1_0_0])
       end
 
-      it 'preloads associations' do
-        expect(Release).to receive(:preloaded).once.and_call_original
+      it_behaves_like 'preload'
+      it_behaves_like 'when tag is nil'
+      it_behaves_like 'when a tag parameter is passed'
+    end
+  end
 
-        subject
-      end
+  describe 'when parent is a group' do
+    context 'without subgroups' do
+      let(:project2)    { create(:project, :repository, namespace: group) }
+      let!(:v6)         { create(:release, project: project2, tag: 'v6') }
 
-      context 'when preload is false' do
-        let(:args) { { preload: false } }
+      subject { described_class.new(group, user, params).execute(**args) }
 
-        it 'does not preload associations' do
-          expect(Release).not_to receive(:preloaded)
+      it_behaves_like 'when the user is not part of the project'
+
+      context 'when the user is a project developer on one sibling project' do
+        before do
+          project.add_developer(user)
+          v1_0_0.update_attribute(:released_at, 3.days.ago)
+          v1_1_0.update_attribute(:released_at, 1.day.ago)
+        end
 
-          subject
+        it 'sorts by release date' do
+          expect(subject.size).to eq(2)
+          expect(subject).to eq([v1_1_0, v1_0_0])
         end
       end
 
-      # See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/27716
-      context 'when tag is nil' do
+      context 'when the user is a project developer on all projects' do
         before do
-          v1_0_0.update_column(:tag, nil)
+          project.add_developer(user)
+          project2.add_developer(user)
+          v1_0_0.update_attribute(:released_at, 3.days.ago)
+          v6.update_attribute(:released_at, 2.days.ago)
+          v1_1_0.update_attribute(:released_at, 1.day.ago)
         end
 
-        it 'ignores rows with a nil tag' do
-          expect(subject.size).to eq(1)
-          expect(subject).to eq([v1_1_0])
+        it 'sorts by release date' do
+          expect(subject.size).to eq(3)
+          expect(subject).to eq([v1_1_0, v6, v1_0_0])
+        end
+
+        it_behaves_like 'when a tag parameter is passed'
+      end
+    end
+
+    describe 'with subgroups' do
+      let(:params) { { include_subgroups: true } }
+
+      subject { described_class.new(group, user, params).execute(**args) }
+
+      context 'with a single-level subgroup' do
+        let(:subgroup) { create :group, parent: group }
+        let(:project2) { create(:project, :repository, namespace: subgroup) }
+        let!(:v6)      { create(:release, project: project2, tag: 'v6') }
+
+        it_behaves_like 'when the user is not part of the project'
+
+        context 'when the user a project developer in the subgroup project' do
+          before do
+            project2.add_developer(user)
+          end
+
+          it 'returns only the subgroup releases' do
+            expect(subject).to match_array([v6])
+          end
+        end
+
+        context 'when the user a project developer in both projects' do
+          before do
+            project.add_developer(user)
+            project2.add_developer(user)
+            v6.update_attribute(:released_at, 2.days.ago)
+          end
+
+          it 'returns all releases' do
+            expect(subject).to match_array([v1_1_0, v1_0_0, v6])
+          end
+
+          it_behaves_like 'when a tag parameter is passed'
         end
       end
 
-      context 'when a tag parameter is passed' do
-        let(:params) { { tag: 'v1.0.0' } }
+      context 'with a multi-level subgroup' do
+        let(:subgroup) { create :group, parent: group }
+        let(:subsubgroup) { create :group, parent: subgroup }
+        let(:project2) { create(:project, :repository, namespace: subgroup) }
+        let(:project3) { create(:project, :repository, namespace: subsubgroup) }
+        let!(:v6)      { create(:release, project: project2, tag: 'v6') }
+        let!(:p3)      { create(:release, project: project3, tag: 'p3') }
+
+        before do
+          v6.update_attribute(:released_at, 2.days.ago)
+          p3.update_attribute(:released_at, 3.days.ago)
+        end
+
+        it_behaves_like 'when the user is not part of the project'
+
+        context 'when the user a project developer in the subgroup and subsubgroup project' do
+          before do
+            project2.add_developer(user)
+            project3.add_developer(user)
+          end
+
+          it 'returns only the subgroup and subsubgroup releases' do
+            expect(subject).to match_array([v6, p3])
+          end
+        end
+
+        context 'when the user a project developer in the subsubgroup project' do
+          before do
+            project3.add_developer(user)
+          end
+
+          it 'returns only the subsubgroup releases' do
+            expect(subject).to match_array([p3])
+          end
+        end
+
+        context 'when the user a project developer in all projects' do
+          before do
+            project.add_developer(user)
+            project2.add_developer(user)
+            project3.add_developer(user)
+          end
+
+          it 'returns all releases' do
+            expect(subject).to match_array([v1_1_0, v6, v1_0_0, p3])
+          end
 
-        it 'only returns the release with the matching tag' do
-          expect(subject).to eq([v1_0_0])
+          it_behaves_like 'when a tag parameter is passed'
         end
       end
     end

spec/serializers/release_serializer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ReleaseSerializer do
+  let(:user) { create(:user) }
+  let(:project) { create :project }
+
+  subject { described_class.new.represent(resource, current_user: user) }
+
+  before do
+    project.add_developer(user)
+  end
+
+  describe '#represent' do
+    context 'when a single object is being serialized' do
+      let(:resource) { create(:release, project: project) }
+
+      it 'serializes the label object' do
+        expect(subject[:tag]).to eq resource.tag
+      end
+    end
+
+    context 'when multiple objects are being serialized' do
+      let(:resource) { create_list(:release, 3) }
+
+      it 'serializes the array of releases' do
+        expect(subject.size).to eq(3)
+      end
+    end
+  end
+end