Ensure proper access level to productivity analytics

Productivity analytics page should be visible regardless of license

changelogs/unreleased/33876-ensure-proper-access-level-check-on-pa.yml

+---
+title: Allow to view productivity analytics page without a license
+merge_request: 33876
+author:
+type: fixed

ee/app/controllers/analytics/application_controller.rb

@@ -21,8 +21,7 @@ class Analytics::ApplicationController < ApplicationController
 
   def check_feature_availability!(feature)
     return render_403 unless ::License.feature_available?(feature)
-    return unless @group
-    return render_403 unless @group.root_ancestor.feature_available?(feature)
+    return render_403 unless @group && @group.root_ancestor.feature_available?(feature)
   end
 
   def load_group

ee/app/controllers/analytics/productivity_analytics_controller.rb

@@ -9,7 +9,8 @@ class Analytics::ProductivityAnalyticsController < Analytics::ApplicationControl
   before_action :load_project
   before_action -> {
     check_feature_availability!(:productivity_analytics)
-  }
+  }, if: -> { request.format.json? }
+
   before_action -> {
     authorize_view_by_action!(:view_productivity_analytics)
   }

ee/spec/controllers/analytics/productivity_analytics_controller_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 describe Analytics::ProductivityAnalyticsController do
   let(:current_user) { create(:user) }
+  let(:group) { create :group }
 
   before do
     sign_in(current_user) if current_user
@@ -12,8 +13,6 @@ describe Analytics::ProductivityAnalyticsController do
   end
 
   describe 'usage counter' do
-    let(:group) { create :group }
-
     before do
       group.add_owner(current_user)
     end
@@ -29,7 +28,7 @@ describe Analytics::ProductivityAnalyticsController do
     it "doesn't increment the usage counter when JSON request is sent" do
       expect(Gitlab::UsageDataCounters::ProductivityAnalyticsCounter).not_to receive(:count).with(:views)
 
-      get :show, format: :json
+      get :show, format: :json, params: { group_id: group }
 
       expect(response).to be_successful
     end
@@ -38,14 +37,6 @@ describe Analytics::ProductivityAnalyticsController do
   describe 'GET show' do
     subject { get :show }
 
-    it 'checks for premium license' do
-      stub_licensed_features(productivity_analytics: false)
-
-      subject
-
-      expect(response).to have_gitlab_http_status(403)
-    end
-
     it 'authorizes for ability to view analytics' do
       expect(Ability).to receive(:allowed?).with(current_user, :view_productivity_analytics, :global).and_return(false)
 
@@ -54,7 +45,9 @@ describe Analytics::ProductivityAnalyticsController do
       expect(response).to have_gitlab_http_status(403)
     end
 
-    it 'renders show template' do
+    it 'renders show template regardless of license' do
+      stub_licensed_features(productivity_analytics: false)
+
       subject
 
       expect(response).to be_successful
@@ -86,75 +79,102 @@ describe Analytics::ProductivityAnalyticsController do
               .and_return(analytics_mock)
     end
 
+    it 'checks for premium license' do
+      stub_licensed_features(productivity_analytics: false)
+
+      subject
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+
+    context 'without group_id specified' do
+      it 'returns 403' do
+        subject
+
+        expect(response).to have_gitlab_http_status(403)
+      end
+    end
+
     context 'with non-existing group_id' do
       let(:params) { { group_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
 
       it 'renders 404' do
         subject
+
         expect(response).to have_gitlab_http_status(404)
       end
     end
 
     context 'with non-existing project_id' do
-      let(:group) { create :group }
-      let(:params) { { group_id: group.full_path, project_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
+      let(:params) { { group_id: group, project_id: 'SOMETHING_THAT_DOES_NOT_EXIST' } }
 
       it 'renders 404' do
         subject
+
         expect(response).to have_gitlab_http_status(404)
       end
     end
 
-    context 'for list of MRs' do
-      let!(:merge_request ) { create :merge_request, :merged}
-
-      let(:serializer_mock) { instance_double('BaseSerializer') }
+    context 'with group specified' do
+      let(:params) { { group_id: group } }
 
       before do
-        allow(BaseSerializer).to receive(:new).with(current_user: current_user).and_return(serializer_mock)
-        allow(analytics_mock).to receive(:merge_requests_extended).and_return(MergeRequest.all)
-        allow(serializer_mock).to receive(:represent)
-                                    .with(merge_request, {}, ProductivityAnalyticsMergeRequestEntity)
-                                    .and_return('mr_representation')
+        group.add_owner(current_user)
       end
 
-      it 'serializes whatever analytics returns with ProductivityAnalyticsMergeRequestEntity' do
-        subject
+      context 'for list of MRs' do
+        let!(:merge_request ) { create :merge_request, :merged}
 
-        expect(response.body).to eq '["mr_representation"]'
-      end
+        let(:serializer_mock) { instance_double('BaseSerializer') }
 
-      it 'sets pagination headers' do
-        subject
+        before do
+          allow(BaseSerializer).to receive(:new).with(current_user: current_user).and_return(serializer_mock)
+          allow(analytics_mock).to receive(:merge_requests_extended).and_return(MergeRequest.all)
+          allow(serializer_mock).to receive(:represent)
+                                      .with(merge_request, {}, ProductivityAnalyticsMergeRequestEntity)
+                                      .and_return('mr_representation')
+        end
 
-        expect(response.headers['X-Per-Page']).to eq '20'
-        expect(response.headers['X-Page']).to eq '1'
-        expect(response.headers['X-Next-Page']).to eq ''
-        expect(response.headers['X-Prev-Page']).to eq ''
-        expect(response.headers['X-Total']).to eq '1'
-        expect(response.headers['X-Total-Pages']).to eq '1'
+        it 'serializes whatever analytics returns with ProductivityAnalyticsMergeRequestEntity' do
+          subject
+
+          expect(response.body).to eq '["mr_representation"]'
+        end
+
+        it 'sets pagination headers' do
+          subject
+
+          expect(response.headers['X-Per-Page']).to eq '20'
+          expect(response.headers['X-Page']).to eq '1'
+          expect(response.headers['X-Next-Page']).to eq ''
+          expect(response.headers['X-Prev-Page']).to eq ''
+          expect(response.headers['X-Total']).to eq '1'
+          expect(response.headers['X-Total-Pages']).to eq '1'
+        end
       end
-    end
 
-    context 'for scatterplot charts' do
-      let(:params) { { chart_type: 'scatterplot', metric_type: 'commits_count' } }
-      it 'renders whatever analytics returns for scatterplot' do
-        allow(analytics_mock).to receive(:scatterplot_data).with(type: 'commits_count').and_return('scatterplot_data')
+      context 'for scatterplot charts' do
+        let(:params) { super().merge({ chart_type: 'scatterplot', metric_type: 'commits_count' }) }
 
-        subject
+        it 'renders whatever analytics returns for scatterplot' do
+          allow(analytics_mock).to receive(:scatterplot_data).with(type: 'commits_count').and_return('scatterplot_data')
 
-        expect(response.body).to eq 'scatterplot_data'
+          subject
+
+          expect(response.body).to eq 'scatterplot_data'
+        end
       end
-    end
 
-    context 'for histogram charts' do
-      let(:params) { { chart_type: 'histogram', metric_type: 'commits_count' } }
-      it 'renders whatever analytics returns for histogram' do
-        allow(analytics_mock).to receive(:histogram_data).with(type: 'commits_count').and_return('histogram_data')
+      context 'for histogram charts' do
+        let(:params) { super().merge({ chart_type: 'histogram', metric_type: 'commits_count' }) }
 
-        subject
+        it 'renders whatever analytics returns for histogram' do
+          allow(analytics_mock).to receive(:histogram_data).with(type: 'commits_count').and_return('histogram_data')
+
+          subject
 
-        expect(response.body).to eq 'histogram_data'
+          expect(response.body).to eq 'histogram_data'
+        end
       end
     end
   end