Refactor controller method into separate finder

Add specs

Move compliance dashboard feature to ultumate

Add changelog entry

Add specs and regenerate string file

Fix lint problem

Add cr remarks

Add code review remarks

Rename controller

ee/app/controllers/groups/security/compliance_dashboard_controller.rb

-# frozen_string_literal: true
-class Groups::Security::ComplianceDashboardController < Groups::ApplicationController
-  include Gitlab::IssuableMetadata
-
-  layout 'group'
-
-  before_action :authorize_compliance_dashboard!
-
-  def show
-    preload_for_collection = [:target_project, source_project: :route, target_project: :namespace, approvals: :user]
-    finder_options = {
-      scope: :all,
-      state: :merged,
-      sort: :by_merge_date,
-      include_subgroups: true,
-      attempt_group_search_optimizations: true
-    }
-    finder_options[:group_id] = @group.id
-
-    @merge_requests = MergeRequestsFinder.new(current_user, finder_options).execute
-      .select('DISTINCT ON (merge_requests.target_project_id) merge_requests.*')
-      .preload(preload_for_collection)
-
-    @merge_requests = @merge_requests.order('merge_request_metrics.merged_at').page(params[:page])
-  end
-
-  private
-
-  def authorize_compliance_dashboard!
-    render_403 unless group.feature_available?(:group_level_compliance_dashboard) &&
-      can?(current_user, :read_group_compliance_dashboard, group)
-  end
-end

ee/app/controllers/groups/security/compliance_dashboards_controller.rb

+# frozen_string_literal: true
+class Groups::Security::ComplianceDashboardsController < Groups::ApplicationController
+  layout 'group'
+
+  before_action :authorize_compliance_dashboard!
+
+  def show
+    preload_for_collection = [:target_project, :metrics, :approved_by_users, source_project: :route, target_project: :namespace]
+
+    @merge_requests = MergeRequestsComplianceFinder.new(current_user, { group_id: @group.id })
+      .execute
+      .preload(preload_for_collection) # rubocop: disable CodeReuse/ActiveRecord
+      .page(params[:page])
+  end
+
+  private
+
+  def authorize_compliance_dashboard!
+    render_404 unless group.feature_available?(:group_level_compliance_dashboard) &&
+      can?(current_user, :read_group_compliance_dashboard, group)
+  end
+end

ee/app/finders/merge_requests_compliance_finder.rb

+# frozen_string_literal: true
+
+# Finders::MergeRequest class
+#
+# Used to filter MergeRequests collections for compliance dashboard
+#
+# Arguments:
+#   current_user - which user use
+#   params:
+#     group_id: integer
+#
+class MergeRequestsComplianceFinder < MergeRequestsFinder
+  def execute
+    sql = super
+      .select('DISTINCT ON (merge_requests.target_project_id) merge_requests.*, merge_request_metrics.merged_at')
+      .to_sql
+
+    # rubocop: disable CodeReuse/ActiveRecord
+    MergeRequest
+      .from([Arel.sql("(#{sql}) AS #{MergeRequest.table_name}")])
+      .order('merged_at DESC')
+    # rubocop: enable CodeReuse/ActiveRecord
+  end
+
+  private
+
+  def params
+    finder_options = {
+      scope: :all,
+      state: :merged,
+      sort: :by_merged_at,
+      include_subgroups: true,
+      attempt_group_search_optimizations: true
+    }
+    super.merge(finder_options)
+  end
+end

ee/app/models/ee/merge_request.rb

@@ -53,7 +53,6 @@ module EE
         end
       end
 
-<<<<<<< HEAD
       scope :order_review_time_desc, -> do
         joins(:metrics).reorder(::Gitlab::Database.nulls_last_order('merge_request_metrics.first_comment_at'))
       end
@@ -63,9 +62,6 @@ module EE
           :author, :approved_by_users, :metrics,
           latest_merge_request_diff: :merge_request_diff_files, target_project: :namespace, milestone: :project)
       end
-=======
-      scope :by_merge_date, -> { joins(:metrics).order("merge_requests.target_project_id, merge_request_metrics.merged_at DESC") }
->>>>>>> Add new scope to gather data properly
     end
 
     class_methods do

ee/app/models/license.rb

@@ -102,7 +102,6 @@ class License < ApplicationRecord
     type_of_work_analytics
     unprotection_restrictions
     ci_project_subscriptions
-    group_level_compliance_dashboard
   ]
   EEP_FEATURES.freeze
 
@@ -114,6 +113,7 @@ class License < ApplicationRecord
     dependency_scanning
     epics
     group_ip_restriction
+    group_level_compliance_dashboard
     incident_management
     insights
     license_management

ee/app/views/groups/security/compliance_dashboard/_approvers.html.haml → ee/app/views/groups/security/compliance_dashboards/_approvers.html.haml

-- max_render = 2
-- approvers_rendering_overflow = merge_request.approved_by_users.size > max_render
-- render_count = approvers_rendering_overflow ? max_render - 1 : max_render
-- more_approvers_count = merge_request.approved_by_users.size - render_count
+- presentable_approvers_limit = 0
+- approvers_over_presentable_limit = merge_request.approved_by_users.size - presentable_approvers_limit
+- project = merge_request.project
 
 = _('Approved by: ')
-- merge_request.approved_by_users.take(render_count).each do |approver| # rubocop: disable CodeReuse/ActiveRecord
+- merge_request.approved_by_users.take(presentable_approvers_limit).each do |approver| # rubocop: disable CodeReuse/ActiveRecord
   = link_to_member(project, approver, name: true, title: "Approved by  :name")
-
-- if more_approvers_count.positive?
-  %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{more_approvers_count} more approvers", qa_selector: 'avatar_counter' } } +#{more_approvers_count}
+- if approvers_over_presentable_limit.positive?
+  %span{ class: 'avatar-counter has-tooltip', data: { container: 'body', placement: 'bottom', 'line-type' => 'old', 'original-title' => "+#{approvers_over_presentable_limit} more approvers", qa_selector: 'avatar_counter' } }
+    = "+ #{approvers_over_presentable_limit}"

ee/app/views/groups/security/compliance_dashboard/_merge_request.html.haml → ee/app/views/groups/security/compliance_dashboards/_merge_request.html.haml

-%li{ id: dom_id(merge_request), class: mr_css_classes(merge_request), data: { labels: merge_request.label_ids, id: merge_request.id } }
+%li{ id: dom_id(merge_request), class: mr_css_classes(merge_request), data: { id: merge_request.id } }
   .issuable-info-container
     .issuable-main-info
       .merge-request-title.title
@@ -13,6 +13,6 @@
       %ul.controls.d-flex.align-items-end
         - if merge_request.approved_by_users.any?
           %li.d-flex
-            = render 'approvers', project: merge_request.project, merge_request: merge_request
+            = render 'approvers', merge_request: merge_request
       %span
         = _('merged %{time_ago}').html_safe % { time_ago: time_ago_with_tooltip(merge_request.merged_at, placement: 'bottom', html_class: 'merge_request_updated_ago') }

ee/app/views/groups/security/compliance_dashboard/_merge_requests.html.haml → ee/app/views/groups/security/compliance_dashboards/_merge_requests.html.haml

-- if @merge_requests.to_a.any?
+- if @merge_requests.present?
   .card.card-small.card-without-border
     %ul.content-list.mr-list.issuable-list
       = render partial: 'merge_request', collection: @merge_requests
 
-  = paginate @merge_requests, theme: "gitlab"
+  = paginate_without_count @merge_requests
 
 - else
   = render 'empty_state'

ee/app/views/layouts/nav/ee/_security_link.html.haml

-- if @group.feature_available?(:group_level_compliance_dashboard)
-  = nav_link(path: 'groups/security/compliance_dashboard#show') do
-    = link_to group_security_compliance_dashboard_path(@group), data: { qa_selector: 'security_dashboard_link' } do
-      .nav-icon-container
-        = sprite_icon('shield')
-      %span.nav-item-name
-        = _('Security & Compliance')
-    %ul.sidebar-sub-level-items
-      - if @group.feature_available?(:security_dashboard)
-        = nav_link(path: 'groups/security/dashboard#show') do
-          = link_to group_security_dashboard_path(@group), title: _('Security') do
-            %span= _('Security')
+- return unless @group.feature_available?(:group_level_compliance_dashboard) || @group.feature_available?(:security_dashboard)
+
+- main_path = @group.feature_available?(:group_level_compliance_dashboard) ? 'groups/security/compliance_dashboards#show' : 'groups/security/dashboard#show'
+= nav_link(path: main_path) do
+  = link_to group_security_compliance_dashboard_path(@group), data: { qa_selector: 'security_dashboard_link' } do
+    .nav-icon-container
+      = sprite_icon('shield')
+    %span.nav-item-name
+      = _('Security & Compliance')
+  %ul.sidebar-sub-level-items
+    - if @group.feature_available?(:security_dashboard)
+      = nav_link(path: 'groups/security/dashboard#show') do
+        = link_to group_security_dashboard_path(@group), title: _('Security') do
+          %span= _('Security')
+
+    - if @group.feature_available?(:group_level_compliance_dashboard)
       = nav_link(path: 'groups/security/compliance_dashboard#show') do
-        = link_to group_security_compliance_dashboard_path(@group), title: _('Security') do
+        = link_to group_security_compliance_dashboard_path(@group), title: _('Compliance') do
           %span= _('Compliance')

ee/changelogs/unreleased/36524-group-level-compliance-dashboard-mvc.yml

+---
+title: Add Group-level compliance dashboard MVC
+merge_request: 20844
+author:
+type: added

ee/config/routes/group.rb

@@ -113,7 +113,7 @@ constraints(::Constraints::GroupUrlConstrainer.new) do
 
     namespace :security do
       resource :dashboard, only: [:show], controller: :dashboard
-      resource :compliance_dashboard, only: [:show], controller: :compliance_dashboard
+      resource :compliance_dashboard, only: [:show]
       resources :vulnerable_projects, only: [:index]
 
       resources :vulnerability_findings, only: [:index] do

ee/spec/controllers/groups/security/compliance_dashboard_controller_spec.rb → ee/spec/controllers/groups/security/compliance_dashboards_controller_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe Groups::Security::ComplianceDashboardController do
+describe Groups::Security::ComplianceDashboardsController do
   let(:user) { create(:user) }
   let(:group) { create(:group) }
 
@@ -19,6 +19,8 @@ describe Groups::Security::ComplianceDashboardController do
       end
 
       context 'and user is allowed to access group compliance dashboard' do
+        render_views
+
         before do
           group.add_owner(user)
         end
@@ -26,62 +28,36 @@ describe Groups::Security::ComplianceDashboardController do
         it { is_expected.to have_gitlab_http_status(200) }
 
         context 'when there are no merge requests' do
-          render_views
-
           it 'renders empty state' do
             subject
             expect(response.body).to have_css("div.empty-state")
           end
         end
 
-        context 'when there are merge requests from projects in group' do
+        context 'when there are merge requests' do
           let(:project) { create(:project, namespace: group) }
-          let(:project_2) { create(:project, namespace: group) }
 
           let(:mr_1) { create(:merge_request, source_project: project, state: :merged) }
-          let(:mr_2) { create(:merge_request, source_project: project_2, state: :merged) }
-          let(:mr_3) { create(:merge_request, source_project: project, source_branch: 'A', state: :merged) }
-          let(:mr_4) { create(:merge_request, source_project: project_2, source_branch: 'A', state: :merged) }
+          let(:mr_2) { create(:merge_request, source_project: project, source_branch: 'A', state: :merged) }
 
           before do
             mr_1.metrics.update!(merged_at: 20.minutes.ago)
-            mr_2.metrics.update!(merged_at: 40.minutes.ago)
-            mr_3.metrics.update!(merged_at: 30.minutes.ago)
-            mr_4.metrics.update!(merged_at: 50.minutes.ago)
           end
 
-          it 'shows only most recent Merge Request from each project' do
+          it 'renders merge request' do
             subject
-            expect(assigns(:merge_requests)).to contain_exactly(mr_1, mr_2)
-          end
-
-          context 'when there are merge requests from projects in group and subgroups' do
-            let(:subgroup) { create(:group, parent: group) }
-            let(:sub_project) { create(:project, namespace: subgroup) }
-
-            let(:mr_5) { create(:merge_request, source_project: sub_project, state: :merged) }
-            let(:mr_6) { create(:merge_request, source_project: sub_project, state: :merged) }
-
-            before do
-              mr_5.metrics.update!(merged_at: 10.minutes.ago)
-              mr_6.metrics.update!(merged_at: 30.minutes.ago)
-            end
-
-            xit 'shows only most recent Merge Request from each project' do
-              subject
-              expect(assigns(:merge_requests)).to eq([mr_5, mr_1, mr_2])
-            end
+            expect(response.body).to have_css(".merge-request-title.title")
           end
         end
       end
 
       context 'when user is not allowed to access group compliance dashboard' do
-        it { is_expected.to have_gitlab_http_status(403) }
+        it { is_expected.to have_gitlab_http_status(404) }
       end
     end
 
-    context 'when security compliance feature is disabled' do
-      it { is_expected.to have_gitlab_http_status(403) }
+    context 'when compliance dashboard feature is disabled' do
+      it { is_expected.to have_gitlab_http_status(404) }
     end
   end
 end

ee/spec/finders/merge_requests_compliance_finder_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequestsComplianceFinder do
+  subject { described_class.new(current_user, search_params) }
+
+  let(:current_user) { create(:admin) }
+  let(:search_params) { { group_id: group.id } }
+  let(:group) { create(:group) }
+  let(:project) { create(:project, namespace: group) }
+  let(:project_2) { create(:project, namespace: group) }
+
+  let(:mr_1) { create(:merge_request, source_project: project, state: :merged) }
+  let(:mr_2) { create(:merge_request, source_project: project_2, state: :merged) }
+  let(:mr_3) { create(:merge_request, source_project: project, source_branch: 'A', state: :merged) }
+  let(:mr_4) { create(:merge_request, source_project: project_2, source_branch: 'A', state: :merged) }
+
+  before do
+    mr_1.metrics.update!(merged_at: 20.minutes.ago)
+    mr_2.metrics.update!(merged_at: 40.minutes.ago)
+    mr_3.metrics.update!(merged_at: 30.minutes.ago)
+    mr_4.metrics.update!(merged_at: 50.minutes.ago)
+  end
+
+  context 'when there are merge requests from projects in group' do
+    it 'shows only most recent Merge Request from each project' do
+      expect(subject.execute).to contain_exactly(mr_1, mr_2)
+    end
+
+    it 'shows as many Merge Requests as they are projects with MR in group' do
+      expect(subject.execute.size).to eq(group.projects.size)
+    end
+
+    context 'when there are merge requests from projects in group and subgroups' do
+      let(:subgroup) { create(:group, parent: group) }
+      let(:sub_project) { create(:project, namespace: subgroup) }
+
+      let(:mr_5) { create(:merge_request, source_project: sub_project, state: :merged) }
+      let(:mr_6) { create(:merge_request, source_project: sub_project, state: :merged) }
+
+      before do
+        mr_5.metrics.update!(merged_at: 10.minutes.ago)
+        mr_6.metrics.update!(merged_at: 30.minutes.ago)
+      end
+
+      it 'shows only most recent Merge Request from each project' do
+        expect(subject.execute).to contain_exactly(mr_1, mr_2, mr_5)
+      end
+
+      it 'shows Merge Requests from most recent to least recent' do
+        expect(subject.execute).to eq([mr_5, mr_1, mr_2])
+      end
+    end
+  end
+end

ee/spec/views/layouts/nav/sidebar/_group.html.haml_spec.rb

@@ -70,30 +70,47 @@ describe 'layouts/nav/sidebar/_group' do
   end
 
   describe 'security dashboard tab' do
+    let(:group) { create(:group, plan: :gold_plan) }
+
     before do
-      stub_licensed_features(security_dashboard: true)
       enable_namespace_license_check!
 
       create(:gitlab_subscription, hosted_plan: group.plan, namespace: group)
     end
 
     context 'when security dashboard feature is enabled' do
-      let(:group) { create(:group, plan: :gold_plan) }
+      before do
+        stub_licensed_features(security_dashboard: true)
+      end
 
       it 'is visible' do
         render
 
+        expect(rendered).to have_link 'Security & Compliance'
         expect(rendered).to have_link 'Security'
       end
     end
 
+    context 'when compliance dashboard feature is enabled' do
+      before do
+        stub_licensed_features(group_level_compliance_dashboard: true)
+      end
+
+      it 'is visible' do
+        render
+
+        expect(rendered).to have_link 'Security & Compliance'
+        expect(rendered).to have_link 'Compliance'
+      end
+    end
+
     context 'when security dashboard feature is disabled' do
       let(:group) { create(:group, plan: :bronze_plan) }
 
       it 'is not visible' do
         render
 
-        expect(rendered).not_to have_link 'Security'
+        expect(rendered).not_to have_link 'Security & Compliance'
       end
     end
   end

locale/gitlab.pot

@@ -2095,6 +2095,9 @@ msgstr ""
 msgid "Approve the current merge request."
 msgstr ""
 
+msgid "Approved by: "
+msgstr ""
+
 msgid "Approved the current merge request."
 msgstr ""
 
@@ -4825,6 +4828,12 @@ msgstr ""
 msgid "Complete"
 msgstr ""
 
+msgid "Compliance"
+msgstr ""
+
+msgid "Compliance Dashboard"
+msgstr ""
+
 msgid "Confidence: %{confidence}"
 msgstr ""
 
@@ -22400,6 +22409,9 @@ msgid_plural "merge requests"
 msgstr[0] ""
 msgstr[1] ""
 
+msgid "merged %{time_ago}"
+msgstr ""
+
 msgid "milestone should belong either to a project or a group."
 msgstr ""