Commit 50839384 authored by Miguel Rincon's avatar Miguel Rincon Committed by Phil Hughes

Only inject gon variables and perform redirects for HTML

Instead of excluding XHRs for these actions, we only want to perform
them when we're serving an HTML page. If we're serving an image or an
Atom feed, they are mostly useless:

1. Gon variables can't be used by an image.
2. Redirects won't be seen if an image is embedded in another page.
parent 9c0c5d01
......@@ -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"
......
......@@ -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
......
......@@ -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
......
# 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
......
---
title: Add edit button to metrics dashboard
merge_request: 19279
author:
type: added
......@@ -6022,6 +6022,9 @@ msgstr ""
msgid "Edit comment"
msgstr ""
msgid "Edit dashboard"
msgstr ""
msgid "Edit description"
msgstr ""
......
......@@ -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
......
......@@ -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
......
......@@ -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);
......
......@@ -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',
},
];
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment