diff options
| -rw-r--r-- | app/controllers/application_controller.rb | 3 | ||||
| -rw-r--r-- | app/controllers/sessions_controller.rb | 9 | ||||
| -rw-r--r-- | app/helpers/broadcast_messages_helper.rb | 10 | ||||
| -rw-r--r-- | app/services/labels/transfer_service.rb | 6 | ||||
| -rw-r--r-- | changelogs/unreleased/210051-optimize-or-remove-ldap_users-counter.yml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/fix-duplicate-labels-when-moving-projects.yml | 5 | ||||
| -rw-r--r-- | db/migrate/20200313123934_add_index_on_user_id_type_source_type_ldap_and_created_at_to_members.rb | 18 | ||||
| -rw-r--r-- | db/schema.rb | 3 | ||||
| -rw-r--r-- | lib/banzai/filter/broadcast_message_placeholders_filter.rb | 57 | ||||
| -rw-r--r-- | lib/banzai/pipeline/post_process_pipeline.rb | 3 | ||||
| -rw-r--r-- | locale/gitlab.pot | 33 | ||||
| -rw-r--r-- | spec/controllers/sessions_controller_spec.rb | 24 | ||||
| -rw-r--r-- | spec/features/broadcast_messages_spec.rb | 11 | ||||
| -rw-r--r-- | spec/helpers/broadcast_messages_helper_spec.rb | 3 | ||||
| -rw-r--r-- | spec/lib/banzai/filter/broadcast_message_placeholders_filter_spec.rb | 89 | ||||
| -rw-r--r-- | spec/services/labels/transfer_service_spec.rb | 104 |
16 files changed, 330 insertions, 53 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3c03c387dba..c5c586ea489 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -35,8 +35,9 @@ class ApplicationController < ActionController::Base before_action :check_impersonation_availability before_action :required_signup_info + prepend_around_action :set_current_context + around_action :sessionless_bypass_admin_mode!, if: :sessionless_user? - around_action :set_current_context around_action :set_locale around_action :set_session_storage around_action :set_current_admin diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 2d1c1eeeea0..2c87c3c890f 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -26,8 +26,6 @@ class SessionsController < Devise::SessionsController before_action :load_recaptcha before_action :frontend_tracking_data, only: [:new] - around_action :set_current_context - after_action :log_failed_login, if: :action_new_and_failed_login? helper_method :captcha_enabled?, :captcha_on_login_required? @@ -307,13 +305,6 @@ class SessionsController < Devise::SessionsController # We want tracking data pushed to the frontend when the user is _in_ the control group frontend_experimentation_tracking_data(:signup_flow, 'start') unless experiment_enabled?(:signup_flow) end - - def set_current_context(&block) - Gitlab::ApplicationContext.with_context( - user: -> { current_user }, - caller_id: "#{self.class.name}##{action_name}", - &block) - end end SessionsController.prepend_if_ee('EE::SessionsController') diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 7638710a7c2..73c68dd9e18 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -47,7 +47,15 @@ module BroadcastMessagesHelper end def render_broadcast_message(broadcast_message) - Banzai.render_field(broadcast_message, :message).html_safe + if Feature.enabled?(:broadcast_message_placeholders) + Banzai.render_and_post_process(broadcast_message.message, { + current_user: current_user, + skip_project_check: true, + broadcast_message_placeholders: true + }).html_safe + else + Banzai.render_field(broadcast_message, :message).html_safe + end end def broadcast_type_options diff --git a/app/services/labels/transfer_service.rb b/app/services/labels/transfer_service.rb index 91984403db3..e6f9cf35fcb 100644 --- a/app/services/labels/transfer_service.rb +++ b/app/services/labels/transfer_service.rb @@ -49,7 +49,7 @@ module Labels Label.joins(:issues) .where( issues: { project_id: project.id }, - labels: { type: 'GroupLabel', group_id: old_group.id } + labels: { type: 'GroupLabel', group_id: old_group.self_and_ancestors } ) end # rubocop: enable CodeReuse/ActiveRecord @@ -59,14 +59,14 @@ module Labels Label.joins(:merge_requests) .where( merge_requests: { target_project_id: project.id }, - labels: { type: 'GroupLabel', group_id: old_group.id } + labels: { type: 'GroupLabel', group_id: old_group.self_and_ancestors } ) end # rubocop: enable CodeReuse/ActiveRecord def find_or_create_label!(label) params = label.attributes.slice('title', 'description', 'color') - new_label = FindOrCreateService.new(current_user, project, params).execute + new_label = FindOrCreateService.new(current_user, project, params.merge(include_ancestor_groups: true)).execute new_label.id end diff --git a/changelogs/unreleased/210051-optimize-or-remove-ldap_users-counter.yml b/changelogs/unreleased/210051-optimize-or-remove-ldap_users-counter.yml new file mode 100644 index 00000000000..5f9a93c8d8d --- /dev/null +++ b/changelogs/unreleased/210051-optimize-or-remove-ldap_users-counter.yml @@ -0,0 +1,5 @@ +--- +title: Optimize members counters query performance in usage data +merge_request: 27197 +author: +type: performance diff --git a/changelogs/unreleased/fix-duplicate-labels-when-moving-projects.yml b/changelogs/unreleased/fix-duplicate-labels-when-moving-projects.yml new file mode 100644 index 00000000000..a5827b08f33 --- /dev/null +++ b/changelogs/unreleased/fix-duplicate-labels-when-moving-projects.yml @@ -0,0 +1,5 @@ +--- +title: Fix duplicate labels when moving projects within the same ancestor group +merge_request: 27261 +author: +type: fixed diff --git a/db/migrate/20200313123934_add_index_on_user_id_type_source_type_ldap_and_created_at_to_members.rb b/db/migrate/20200313123934_add_index_on_user_id_type_source_type_ldap_and_created_at_to_members.rb new file mode 100644 index 00000000000..0215d102529 --- /dev/null +++ b/db/migrate/20200313123934_add_index_on_user_id_type_source_type_ldap_and_created_at_to_members.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class AddIndexOnUserIdTypeSourceTypeLdapAndCreatedAtToMembers < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + INDEX_NAME = 'index_members_on_user_id_created_at' + + disable_ddl_transaction! + + def up + add_concurrent_index :members, [:user_id, :created_at], where: "ldap = TRUE AND type = 'GroupMember' AND source_type = 'Namespace'", name: INDEX_NAME + end + + def down + remove_concurrent_index :members, INDEX_NAME + end +end diff --git a/db/schema.rb b/db/schema.rb index 5e6dfd05436..55c99cb1027 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_03_12_163407) do +ActiveRecord::Schema.define(version: 2020_03_13_123934) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -2442,6 +2442,7 @@ ActiveRecord::Schema.define(version: 2020_03_12_163407) do t.index ["invite_token"], name: "index_members_on_invite_token", unique: true t.index ["requested_at"], name: "index_members_on_requested_at" t.index ["source_id", "source_type"], name: "index_members_on_source_id_and_source_type" + t.index ["user_id", "created_at"], name: "index_members_on_user_id_created_at", where: "((ldap = true) AND ((type)::text = 'GroupMember'::text) AND ((source_type)::text = 'Namespace'::text))" t.index ["user_id"], name: "index_members_on_user_id" end diff --git a/lib/banzai/filter/broadcast_message_placeholders_filter.rb b/lib/banzai/filter/broadcast_message_placeholders_filter.rb new file mode 100644 index 00000000000..5b5e2f643c5 --- /dev/null +++ b/lib/banzai/filter/broadcast_message_placeholders_filter.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +module Banzai + module Filter + # Replaces placeholders for broadcast messages with data from the current + # user or the instance. + class BroadcastMessagePlaceholdersFilter < HTML::Pipeline::Filter + def call + return doc unless context[:broadcast_message_placeholders] + + doc.traverse { |node| replace_placeholders(node) } + end + + private + + def replace_placeholders(node) + if node.text? && !node.content.empty? + node.content = replace_content(node.content) + elsif href = link_href(node) + href.value = replace_content(href.value, url_safe_encoding: true) + end + + node + end + + def link_href(node) + node.element? && + node.name == 'a' && + node.attribute_nodes.find { |a| a.name == "href" } + end + + def replace_content(content, url_safe_encoding: false) + placeholders.each do |placeholder, method| + regex = Regexp.new("{{#{placeholder}}}|#{CGI.escape("{{#{placeholder}}}")}") + value = url_safe_encoding ? CGI.escape(method.call.to_s) : method.call.to_s + content.gsub!(regex, value) + end + + content + end + + def placeholders + { + "email" => -> { current_user.try(:email) }, + "name" => -> { current_user.try(:name) }, + "user_id" => -> { current_user.try(:id) }, + "username" => -> { current_user.try(:username) }, + "instance_id" => -> { Gitlab::CurrentSettings.try(:uuid) } + } + end + + def current_user + context[:current_user] + end + end + end +end diff --git a/lib/banzai/pipeline/post_process_pipeline.rb b/lib/banzai/pipeline/post_process_pipeline.rb index 5e02d972614..8236b702147 100644 --- a/lib/banzai/pipeline/post_process_pipeline.rb +++ b/lib/banzai/pipeline/post_process_pipeline.rb @@ -8,7 +8,8 @@ module Banzai def self.filters @filters ||= FilterArray[ *internal_link_filters, - Filter::AbsoluteLinkFilter + Filter::AbsoluteLinkFilter, + Filter::BroadcastMessagePlaceholdersFilter ] end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0a6266635fb..d0c2f4b2906 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -217,6 +217,9 @@ msgstr "" msgid "%{authorsName}'s thread" msgstr "" +msgid "%{buy_now_link_start}Buy now!%{link_end}" +msgstr "" + msgid "%{commit_author_link} authored %{commit_timeago}" msgstr "" @@ -2412,6 +2415,12 @@ msgstr "" msgid "Ascending" msgstr "" +msgid "Ask an admin to upload a new license to ensure uninterrupted service." +msgstr "" + +msgid "Ask an admin to upload a new license to restore service." +msgstr "" + msgid "Ask your group maintainer to set up a group Runner." msgstr "" @@ -16153,6 +16162,12 @@ msgstr "" msgid "Pushes" msgstr "" +msgid "Pushing code and creation of issues and merge requests has been disabled." +msgstr "" + +msgid "Pushing code and creation of issues and merge requests will be disabled on %{disabled_on}." +msgstr "" + msgid "PushoverService|%{user_name} deleted branch \"%{ref}\"." msgstr "" @@ -21573,6 +21588,12 @@ msgstr "" msgid "Upload a certificate for your domain with all intermediates" msgstr "" +msgid "Upload a new license in the admin area to ensure uninterrupted service." +msgstr "" + +msgid "Upload a new license in the admin area to restore service." +msgstr "" + msgid "Upload a private key for your certificate" msgstr "" @@ -23302,9 +23323,15 @@ msgstr "" msgid "Your issues will be imported in the background. Once finished, you'll get a confirmation email." msgstr "" +msgid "Your license expired on %{expires_at}." +msgstr "" + msgid "Your license is valid from" msgstr "" +msgid "Your license will expire in %{remaining_days}." +msgstr "" + msgid "Your message here" msgstr "" @@ -23335,6 +23362,12 @@ msgstr "" msgid "Your request for access has been queued for review." msgstr "" +msgid "Your trial license expired on %{expires_at}." +msgstr "" + +msgid "Your trial license will expire in %{remaining_days}." +msgstr "" + msgid "Zoom meeting added" msgstr "" diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index a677e17ab0c..f3e2ea50913 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -497,13 +497,13 @@ describe SessionsController do end describe '#set_current_context' do + let_it_be(:user) { create(:user) } + before do set_devise_mapping(context: @request) end context 'when signed in' do - let_it_be(:user) { create(:user) } - before do sign_in(user) end @@ -535,5 +535,25 @@ describe SessionsController do get :new end end + + context 'when the user becomes locked' do + before do + user.update!(failed_attempts: User.maximum_attempts.pred) + end + + it 'sets the caller_id in the context' do + allow_any_instance_of(User).to receive(:lock_access!).and_wrap_original do |m, *args| + expect(Labkit::Context.current.to_h) + .to include('meta.caller_id' => 'SessionsController#create') + expect(Labkit::Context.current.to_h) + .not_to include('meta.user') + + m.call(*args) + end + + post(:create, + params: { user: { login: user.username, password: user.password.succ } }) + end + end end end diff --git a/spec/features/broadcast_messages_spec.rb b/spec/features/broadcast_messages_spec.rb index d7c84b8085b..809e53ed7f7 100644 --- a/spec/features/broadcast_messages_spec.rb +++ b/spec/features/broadcast_messages_spec.rb @@ -57,4 +57,15 @@ describe 'Broadcast Messages' do it_behaves_like 'a dismissable Broadcast Messages' end + + it 'renders broadcast message with placeholders' do + create(:broadcast_message, broadcast_type: 'notification', message: 'Hi {{name}}') + + user = create(:user) + sign_in(user) + + visit root_path + + expect(page).to have_content "Hi #{user.name}" + end end diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index 69ff5f16ec1..58cc03a9446 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/spec/helpers/broadcast_messages_helper_spec.rb @@ -27,10 +27,11 @@ describe BroadcastMessagesHelper do end describe 'broadcast_message' do + let_it_be(:user) { create(:user) } let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') } before do - allow(helper).to receive(:current_user).and_return(create(:user)) + allow(helper).to receive(:current_user).and_return(user) end it 'returns nil when no current message' do diff --git a/spec/lib/banzai/filter/broadcast_message_placeholders_filter_spec.rb b/spec/lib/banzai/filter/broadcast_message_placeholders_filter_spec.rb new file mode 100644 index 00000000000..1a90abc12d9 --- /dev/null +++ b/spec/lib/banzai/filter/broadcast_message_placeholders_filter_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Banzai::Filter::BroadcastMessagePlaceholdersFilter do + include FilterSpecHelper + + subject { filter(text, current_user: user, broadcast_message_placeholders: true).to_html } + + describe 'when current user is set' do + let_it_be(:user) { create(:user, email: "helloworld@example.com", name: "GitLab Tanunki :)") } + + context 'replaces placeholder in text' do + let(:text) { 'Email: {{email}}' } + + it { expect(subject).to eq("Email: #{user.email}") } + end + + context 'replaces placeholder when they are in a link' do + let(:text) { '<a href="http://example.com?email={{email}}"">link</a>' } + + it { expect(subject).to eq("<a href=\"http://example.com?email=helloworld%40example.com\">link</a>") } + end + + context 'replaces placeholder when they are in an escaped link' do + let(:text) { '<a href="http://example.com?name=%7B%7Bname%7D%7D">link</a>' } + + it { expect(subject).to eq("<a href=\"http://example.com?name=GitLab+Tanunki+%3A%29\">link</a>") } + end + + context 'works with empty text' do + let(:text) {" "} + + it { expect(subject).to eq(" ") } + end + + context 'replaces multiple placeholders in a given text' do + let(:text) { "{{email}} {{name}}" } + + it { expect(subject).to eq("#{user.email} #{user.name}") } + end + + context 'available placeholders' do + context 'replaces the email of the user' do + let(:text) { "{{email}}"} + + it { expect(subject).to eq(user.email) } + end + + context 'replaces the name of the user' do + let(:text) { "{{name}}"} + + it { expect(subject).to eq(user.name) } + end + + context 'replaces the ID of the user' do + let(:text) { "{{user_id}}" } + + it { expect(subject).to eq(user.id.to_s) } + end + + context 'replaces the username of the user' do + let(:text) { "{{username}}" } + + it { expect(subject).to eq(user.username) } + end + + context 'replaces the instance_id' do + before do + stub_application_setting(uuid: '123') + end + + let(:text) { "{{instance_id}}" } + + it { expect(subject).to eq(Gitlab::CurrentSettings.uuid) } + end + end + end + + describe 'when there is no current user set' do + let(:user) { nil } + + context 'replaces placeholder with empty string' do + let(:text) { "Email: {{email}}" } + + it { expect(subject).to eq("Email: ") } + end + end +end diff --git a/spec/services/labels/transfer_service_spec.rb b/spec/services/labels/transfer_service_spec.rb index e29c6aeef5b..a2a9c8dddf2 100644 --- a/spec/services/labels/transfer_service_spec.rb +++ b/spec/services/labels/transfer_service_spec.rb @@ -4,65 +4,101 @@ require 'spec_helper' describe Labels::TransferService do describe '#execute' do - let(:user) { create(:admin) } - let(:group_1) { create(:group) } - let(:group_2) { create(:group) } - let(:group_3) { create(:group) } - let(:project_1) { create(:project, namespace: group_2) } - let(:project_2) { create(:project, namespace: group_3) } - let(:project_3) { create(:project, namespace: group_1) } - - let(:group_label_1) { create(:group_label, group: group_1, name: 'Group Label 1') } - let(:group_label_2) { create(:group_label, group: group_1, name: 'Group Label 2') } - let(:group_label_3) { create(:group_label, group: group_1, name: 'Group Label 3') } - let(:group_label_4) { create(:group_label, group: group_2, name: 'Group Label 4') } - let(:group_label_5) { create(:group_label, group: group_3, name: 'Group Label 5') } - let(:project_label_1) { create(:label, project: project_1, name: 'Project Label 1') } - - subject(:service) { described_class.new(user, group_1, project_1) } - - before do - create(:labeled_issue, project: project_1, labels: [group_label_1]) - create(:labeled_issue, project: project_1, labels: [group_label_4]) - create(:labeled_issue, project: project_1, labels: [project_label_1]) - create(:labeled_issue, project: project_2, labels: [group_label_5]) - create(:labeled_issue, project: project_3, labels: [group_label_1]) - create(:labeled_merge_request, source_project: project_1, labels: [group_label_1, group_label_2]) - create(:labeled_merge_request, source_project: project_2, labels: [group_label_5]) + let_it_be(:user) { create(:admin) } + + let_it_be(:old_group_ancestor) { create(:group) } + let_it_be(:old_group) { create(:group, parent: old_group_ancestor) } + + let_it_be(:new_group) { create(:group) } + + let_it_be(:project) { create(:project, :repository, group: new_group) } + + subject(:service) { described_class.new(user, old_group, project) } + + it 'recreates missing group labels at project level and assigns them to the issuables' do + old_group_label_1 = create(:group_label, group: old_group) + old_group_label_2 = create(:group_label, group: old_group) + + labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label_1]) + labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_label_2]) + + expect { service.execute }.to change(project.labels, :count).by(2) + expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_1.title)) + expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_label_2.title)) end - it 'recreates the missing group labels at project level' do - expect { service.execute }.to change(project_1.labels, :count).by(2) + it 'recreates missing ancestor group labels at project level and assigns them to the issuables' do + old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor) + old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor) + + labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1]) + labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2]) + + expect { service.execute }.to change(project.labels, :count).by(2) + expect(labeled_issue.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_1.title)) + expect(labeled_merge_request.reload.labels).to contain_exactly(project.labels.find_by_title(old_group_ancestor_label_2.title)) end it 'recreates label priorities related to the missing group labels' do - create(:label_priority, project: project_1, label: group_label_1, priority: 1) + old_group_label = create(:group_label, group: old_group) + create(:labeled_issue, project: project, labels: [old_group_label]) + create(:label_priority, project: project, label: old_group_label, priority: 1) service.execute - new_project_label = project_1.labels.find_by(title: group_label_1.title) - expect(new_project_label.id).not_to eq group_label_1.id + new_project_label = project.labels.find_by(title: old_group_label.title) + expect(new_project_label.id).not_to eq old_group_label.id expect(new_project_label.priorities).not_to be_empty end it 'does not recreate missing group labels that are not applied to issues or merge requests' do + old_group_label = create(:group_label, group: old_group) + service.execute - expect(project_1.labels.where(title: group_label_3.title)).to be_empty + expect(project.labels.where(title: old_group_label.title)).to be_empty end it 'does not recreate missing group labels that already exist in the project group' do + old_group_label = create(:group_label, group: old_group) + labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label]) + + new_group_label = create(:group_label, group: new_group, title: old_group_label.title) + service.execute - expect(project_1.labels.where(title: group_label_4.title)).to be_empty + expect(project.labels.where(title: old_group_label.title)).to be_empty + expect(labeled_issue.reload.labels).to contain_exactly(new_group_label) end it 'updates only label links in the given project' do + old_group_label = create(:group_label, group: old_group) + other_project = create(:project, group: old_group) + + labeled_issue = create(:labeled_issue, project: project, labels: [old_group_label]) + other_project_labeled_issue = create(:labeled_issue, project: other_project, labels: [old_group_label]) + service.execute - targets = LabelLink.where(label_id: group_label_1.id).map(&:target) + expect(labeled_issue.reload.labels).not_to include(old_group_label) + expect(other_project_labeled_issue.reload.labels).to contain_exactly(old_group_label) + end + + context 'when moving within the same ancestor group' do + let(:other_subgroup) { create(:group, parent: old_group_ancestor) } + let(:project) { create(:project, :repository, group: other_subgroup) } + + it 'does not recreate ancestor group labels' do + old_group_ancestor_label_1 = create(:group_label, group: old_group_ancestor) + old_group_ancestor_label_2 = create(:group_label, group: old_group_ancestor) + + labeled_issue = create(:labeled_issue, project: project, labels: [old_group_ancestor_label_1]) + labeled_merge_request = create(:labeled_merge_request, source_project: project, labels: [old_group_ancestor_label_2]) - expect(targets).to eq(project_3.issues) + expect { service.execute }.not_to change(project.labels, :count) + expect(labeled_issue.reload.labels).to contain_exactly(old_group_ancestor_label_1) + expect(labeled_merge_request.reload.labels).to contain_exactly(old_group_ancestor_label_2) + end end end end |