Merge branch '34779-editing-metric-dashboard-using-yml-file' into 'master'

Add "Edit Dashboard" to metrics dashboard using the dashboard's project_blob_path

Closes #35230

See merge request gitlab-org/gitlab!19279

app/assets/javascripts/monitoring/components/dashboard.vue

@@ -187,8 +187,11 @@ export default {
     firstDashboard() {
       return this.allDashboards[0] || {};
     },
+    selectedDashboard() {
+      return this.allDashboards.find(d => d.path === this.currentDashboard) || this.firstDashboard;
+    },
     selectedDashboardText() {
-      return this.currentDashboard || this.firstDashboard.display_name;
+      return this.selectedDashboard.display_name;
     },
     showRearrangePanelsBtn() {
       return !this.showEmptyState && this.rearrangePanelsAvailable;
@@ -199,6 +202,14 @@ export default {
     alertWidgetAvailable() {
       return IS_EE && this.prometheusAlertsAvailable && this.alertsEndpoint;
     },
+    hasHeaderButtons() {
+      return (
+        this.addingMetricsAvailable ||
+        this.showRearrangePanelsBtn ||
+        this.selectedDashboard.can_edit ||
+        this.externalDashboardUrl.length
+      );
+    },
   },
   created() {
     this.setEndpoints({
@@ -390,7 +401,7 @@ export default {
         </template>
 
         <gl-form-group
-          v-if="addingMetricsAvailable || showRearrangePanelsBtn || externalDashboardUrl.length"
+          v-if="hasHeaderButtons"
           label-for="prometheus-graphs-dropdown-buttons"
           class="dropdown-buttons col-md d-md-flex col-lg d-lg-flex align-items-end"
         >
@@ -437,6 +448,14 @@ export default {
               </div>
             </gl-modal>
 
+            <gl-button
+              v-if="selectedDashboard.can_edit"
+              class="mt-1 js-edit-link"
+              :href="selectedDashboard.project_blob_path"
+            >
+              {{ __('Edit dashboard') }}
+            </gl-button>
+
             <gl-button
               v-if="externalDashboardUrl.length"
               class="mt-1 js-external-dashboard-link"

app/controllers/application_controller.rb

@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
   before_action :authenticate_user!, except: [:route_not_found]
   before_action :enforce_terms!, if: :should_enforce_terms?
   before_action :validate_user_service_ticket!
-  before_action :check_password_expiration
+  before_action :check_password_expiration, if: :html_request?
   before_action :ldap_security_check
   before_action :sentry_context
   before_action :default_headers
-  before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
+  before_action :add_gon_variables, if: :html_request?
   before_action :configure_permitted_parameters, if: :devise_controller?
   before_action :require_email, unless: :devise_controller?
   before_action :active_user_check, unless: :devise_controller?
@@ -455,8 +455,8 @@ class ApplicationController < ActionController::Base
     response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
   end
 
-  def peek_request?
-    request.path.start_with?('/-/peek')
+  def html_request?
+    request.format.html?
   end
 
   def json_request?
@@ -466,7 +466,7 @@ class ApplicationController < ActionController::Base
   def should_enforce_terms?
     return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
 
-    !(peek_request? || devise_controller?)
+    html_request? && !devise_controller?
   end
 
   def set_usage_stats_consent_flag

app/controllers/concerns/confirm_email_warning.rb

@@ -4,15 +4,18 @@ module ConfirmEmailWarning
   extend ActiveSupport::Concern
 
   included do
-    before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
+    before_action :set_confirm_warning, if: :show_confirm_warning?
   end
 
   protected
 
+  def show_confirm_warning?
+    html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
+  end
+
   def set_confirm_warning
     return unless current_user
     return if current_user.confirmed?
-    return if peek_request? || json_request? || !request.get?
 
     email = current_user.unconfirmed_email || current_user.email
 

app/controllers/concerns/uploads_actions.rb

 # frozen_string_literal: true
 
 module UploadsActions
+  extend ActiveSupport::Concern
   include Gitlab::Utils::StrongMemoize
   include SendFileUpload
 
   UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
 
+  included do
+    prepend_before_action :set_request_format_from_path_extension
+  end
+
   def create
     uploader = UploadService.new(model, params[:file], uploader_class).execute
 
@@ -64,6 +69,18 @@ module UploadsActions
 
   private
 
+  # From ActionDispatch::Http::MimeNegotiation. We have an initializer that
+  # monkey-patches this method out (so that repository paths don't guess a
+  # format based on extension), but we do want this behaviour when serving
+  # uploads.
+  def set_request_format_from_path_extension
+    path = request.headers['action_dispatch.original_path'] || request.headers['PATH_INFO']
+
+    if match = path&.match(/\.(\w+)\z/)
+      request.format = match.captures.first
+    end
+  end
+
   def uploader_class
     raise NotImplementedError
   end

changelogs/unreleased/34779-editing-metric-dashboard-using-yml-file.yml

+---
+title: Add edit button to metrics dashboard
+merge_request: 19279
+author:
+type: added

locale/gitlab.pot

@@ -6022,6 +6022,9 @@ msgstr ""
 msgid "Edit comment"
 msgstr ""
 
+msgid "Edit dashboard"
+msgstr ""
+
 msgid "Edit description"
 msgstr ""
 

spec/controllers/application_controller_spec.rb

@@ -90,14 +90,6 @@ describe ApplicationController do
       let(:format) { :html }
 
       it_behaves_like 'setting gon variables'
-
-      context 'for peek requests' do
-        before do
-          request.path = '/-/peek'
-        end
-
-        it_behaves_like 'not setting gon variables'
-      end
     end
 
     context 'with json format' do
@@ -105,6 +97,12 @@ describe ApplicationController do
 
       it_behaves_like 'not setting gon variables'
     end
+
+    context 'with atom format' do
+      let(:format) { :atom }
+
+      it_behaves_like 'not setting gon variables'
+    end
   end
 
   describe 'session expiration' do

spec/controllers/uploads_controller_spec.rb

@@ -228,10 +228,10 @@ describe UploadsController do
             user.block
           end
 
-          it "redirects to the sign in page" do
+          it "responds with status 401" do
             get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
 
-            expect(response).to redirect_to(new_user_session_path)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
 
@@ -320,10 +320,10 @@ describe UploadsController do
         end
 
         context "when not signed in" do
-          it "redirects to the sign in page" do
+          it "responds with status 401" do
             get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
 
-            expect(response).to redirect_to(new_user_session_path)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
 
@@ -343,10 +343,10 @@ describe UploadsController do
                 project.add_maintainer(user)
               end
 
-              it "redirects to the sign in page" do
+              it "responds with status 401" do
                 get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
 
-                expect(response).to redirect_to(new_user_session_path)
+                expect(response).to have_gitlab_http_status(401)
               end
             end
 
@@ -439,10 +439,10 @@ describe UploadsController do
                 user.block
               end
 
-              it "redirects to the sign in page" do
+              it "responds with status 401" do
                 get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
 
-                expect(response).to redirect_to(new_user_session_path)
+                expect(response).to have_gitlab_http_status(401)
               end
             end
 
@@ -526,10 +526,10 @@ describe UploadsController do
         end
 
         context "when not signed in" do
-          it "redirects to the sign in page" do
+          it "responds with status 401" do
             get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
 
-            expect(response).to redirect_to(new_user_session_path)
+            expect(response).to have_gitlab_http_status(401)
           end
         end
 
@@ -549,10 +549,10 @@ describe UploadsController do
                 project.add_maintainer(user)
               end
 
-              it "redirects to the sign in page" do
+              it "responds with status 401" do
                 get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
 
-                expect(response).to redirect_to(new_user_session_path)
+                expect(response).to have_gitlab_http_status(401)
               end
             end
 

spec/javascripts/monitoring/components/dashboard_spec.js

@@ -623,6 +623,49 @@ describe('Dashboard', () => {
     });
   });
 
+  describe('dashboard edit link', () => {
+    let wrapper;
+    const findEditLink = () => wrapper.find('.js-edit-link');
+
+    beforeEach(done => {
+      mock.onGet(mockApiEndpoint).reply(200, metricsGroupsAPIResponse);
+
+      wrapper = shallowMount(DashboardComponent, {
+        localVue,
+        sync: false,
+        attachToDocument: true,
+        propsData: { ...propsData, hasMetrics: true },
+        store,
+      });
+
+      wrapper.vm.$store.commit(
+        `monitoringDashboard/${types.SET_ALL_DASHBOARDS}`,
+        dashboardGitResponse,
+      );
+      wrapper.vm.$nextTick(done);
+    });
+
+    afterEach(() => {
+      wrapper.destroy();
+    });
+
+    it('is not present for the default dashboard', () => {
+      expect(findEditLink().exists()).toBe(false);
+    });
+
+    it('is present for a custom dashboard, and links to its edit_path', done => {
+      const dashboard = dashboardGitResponse[1]; // non-default dashboard
+      const currentDashboard = dashboard.path;
+
+      wrapper.setProps({ currentDashboard });
+      wrapper.vm.$nextTick(() => {
+        expect(findEditLink().exists()).toBe(true);
+        expect(findEditLink().attributes('href')).toBe(dashboard.project_blob_path);
+        done();
+      });
+    });
+  });
+
   describe('external dashboard link', () => {
     beforeEach(() => {
       mock.onGet(mockApiEndpoint).reply(200, metricsGroupsAPIResponse);

spec/javascripts/monitoring/mock_data.js

@@ -931,14 +931,25 @@ export const metricsDashboardResponse = {
 
 export const dashboardGitResponse = [
   {
-    path: 'config/prometheus/common_metrics.yml',
-    display_name: 'Common Metrics',
     default: true,
+    display_name: 'Default',
+    can_edit: false,
+    project_blob_path: null,
+    path: 'config/prometheus/common_metrics.yml',
   },
   {
-    path: '.gitlab/dashboards/super.yml',
+    default: false,
     display_name: 'Custom Dashboard 1',
+    can_edit: true,
+    project_blob_path: `${mockProjectPath}/blob/master/dashboards/.gitlab/dashboards/dashboard_1.yml`,
+    path: '.gitlab/dashboards/dashboard_1.yml',
+  },
+  {
     default: false,
+    display_name: 'Custom Dashboard 2',
+    can_edit: true,
+    project_blob_path: `${mockProjectPath}/blob/master/dashboards/.gitlab/dashboards/dashboard_2.yml`,
+    path: '.gitlab/dashboards/dashboard_2.yml',
   },
 ];
 

spec/requests/user_avatar_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Loading a user avatar' do
+  let(:user) { create(:user, :with_avatar) }
+
+  context 'when logged in' do
+    # The exact query count will vary depending on the 2FA settings of the
+    # instance, group, and user. Removing those extra 2FA queries in this case
+    # may not be a good idea, so we just set up the ideal case.
+    before do
+      stub_application_setting(require_two_factor_authentication: true)
+
+      login_as(create(:user, :two_factor))
+    end
+
+    # One each for: current user, avatar user, and upload record
+    it 'only performs three SQL queries' do
+      get user.avatar_url # Skip queries on first application load
+
+      expect(response).to have_gitlab_http_status(200)
+      expect { get user.avatar_url }.not_to exceed_query_limit(3)
+    end
+  end
+
+  context 'when logged out' do
+    # One each for avatar user and upload record
+    it 'only performs two SQL queries' do
+      get user.avatar_url # Skip queries on first application load
+
+      expect(response).to have_gitlab_http_status(200)
+      expect { get user.avatar_url }.not_to exceed_query_limit(2)
+    end
+  end
+end