From c826ecc3e76917d7b77701551e25425da9274b2e Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 20 Aug 2018 16:48:08 +0200 Subject: [PATCH] Port changes for CODEOWNERS to CE This ports the changes for the CODEOWNERS feature to CE: - It adds the CODEOWNERS file. - It adds the mention of the `with-codeowners` branch in TestEnv --- .gitlab/CODEOWNERS | 15 ++ app/models/concerns/case_sensitivity.rb | 47 +++- app/models/user.rb | 5 +- app/views/projects/blob/_blob.html.haml | 1 + lib/gitlab/user_extractor.rb | 53 +++++ spec/lib/gitlab/user_extractor_spec.rb | 58 +++++ spec/models/concerns/case_sensitivity_spec.rb | 212 ++++-------------- spec/models/user_spec.rb | 17 ++ spec/support/helpers/test_env.rb | 3 +- 9 files changed, 222 insertions(+), 189 deletions(-) create mode 100644 .gitlab/CODEOWNERS create mode 100644 lib/gitlab/user_extractor.rb create mode 100644 spec/lib/gitlab/user_extractor_spec.rb diff --git a/.gitlab/CODEOWNERS b/.gitlab/CODEOWNERS new file mode 100644 index 00000000000..5b6e5a719fa --- /dev/null +++ b/.gitlab/CODEOWNERS @@ -0,0 +1,15 @@ +# Backend Maintainers are the default for all ruby files +*.rb @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern +*.rake @ayufan @DouweM @dzaporozhets @grzesiek @nick.thomas @rspeicher @rymai @smcgivern + +# Technical writing team are the default reviewers for everything in `doc/` +/doc/ @axil @marcia + +# Frontend maintainers should see everything in `app/assets/` +app/assets/ @annabeldunstone @ClemMakesApps @fatihacet @filipa @iamphill @mikegreiling @timzallmann + +# Someone from the database team should review changes in `db/` +db/ @abrandl @NikolayS + +# Feature specific owners +/ee/lib/gitlab/code_owners/ @reprazent diff --git a/app/models/concerns/case_sensitivity.rb b/app/models/concerns/case_sensitivity.rb index 6e80365ee5b..c93b6589ee7 100644 --- a/app/models/concerns/case_sensitivity.rb +++ b/app/models/concerns/case_sensitivity.rb @@ -9,23 +9,46 @@ module CaseSensitivity # # Unlike other ActiveRecord methods this method only operates on a Hash. def iwhere(params) - criteria = self - cast_lower = Gitlab::Database.postgresql? + criteria = self params.each do |key, value| - column = ActiveRecord::Base.connection.quote_table_name(key) - - condition = - if cast_lower - "LOWER(#{column}) = LOWER(:value)" - else - "#{column} = :value" - end - - criteria = criteria.where(condition, value: value) + criteria = case value + when Array + criteria.where(value_in(key, value)) + else + criteria.where(value_equal(key, value)) + end end criteria end + + private + + def value_equal(column, value) + lower_value = lower_value(value) + + lower_column(arel_table[column]).eq(lower_value).to_sql + end + + def value_in(column, values) + lower_values = values.map do |value| + lower_value(value) + end + + lower_column(arel_table[column]).in(lower_values).to_sql + end + + def lower_value(value) + return value if Gitlab::Database.mysql? + + Arel::Nodes::NamedFunction.new('LOWER', [Arel::Nodes.build_quoted(value)]) + end + + def lower_column(column) + return column if Gitlab::Database.mysql? + + column.lower + end end end diff --git a/app/models/user.rb b/app/models/user.rb index f21ca1c569f..0fcc952b5cd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -257,6 +257,7 @@ class User < ActiveRecord::Base scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'DESC')) } scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('current_sign_in_at', 'ASC')) } scope :confirmed, -> { where.not(confirmed_at: nil) } + scope :by_username, -> (usernames) { iwhere(username: usernames) } # Limits the users to those that have TODOs, optionally in the given state. # @@ -444,11 +445,11 @@ class User < ActiveRecord::Base end def find_by_username(username) - iwhere(username: username).take + by_username(username).take end def find_by_username!(username) - iwhere(username: username).take! + by_username(username).take! end def find_by_personal_access_token(token_string) diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index a4b1b496b69..cf273aab108 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -5,6 +5,7 @@ %ul.blob-commit-info = render 'projects/commits/commit', commit: @last_commit, project: @project, ref: @ref + = render_if_exists 'projects/blob/owners', blob: blob = render "projects/blob/auxiliary_viewer", blob: blob #blob-content-holder.blob-content-holder diff --git a/lib/gitlab/user_extractor.rb b/lib/gitlab/user_extractor.rb new file mode 100644 index 00000000000..3ede0a5b5e6 --- /dev/null +++ b/lib/gitlab/user_extractor.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +# This class extracts all users found in a piece of text by the username or the +# email adress + +module Gitlab + class UserExtractor + # Not using `Devise.email_regexp` to filter out any chars that an email + # does not end with and not pinning the email to a start of end of a string. + EMAIL_REGEXP = /(?([^@\s]+@[^@\s]+(? 'bar')).to eq(criteria) - end - end - - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('"foo"') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('"bar"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("bar") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('"foo"."bar"') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('"foo"."baz"') - - expect(model).to receive(:where) - .with(%q{LOWER("foo"."bar") = LOWER(:value)}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{LOWER("foo"."baz") = LOWER(:value)}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') - - expect(got).to eq(final) - end + let(:model) do + Class.new(ActiveRecord::Base) do + include CaseSensitivity + self.table_name = 'namespaces' end end - describe 'using MySQL' do - before do - allow(Gitlab::Database).to receive(:postgresql?).and_return(false) - allow(Gitlab::Database).to receive(:mysql?).and_return(true) + let!(:model_1) { model.create(path: 'mOdEl-1', name: 'mOdEl 1') } + let!(:model_2) { model.create(path: 'mOdEl-2', name: 'mOdEl 2') } + + it 'finds a single instance by a single attribute regardless of case' do + expect(model.iwhere(path: 'MODEL-1')).to contain_exactly(model_1) + end + + it 'finds multiple instances by a single attribute regardless of case' do + expect(model.iwhere(path: %w(MODEL-1 model-2))).to contain_exactly(model_1, model_2) + end + + it 'finds instances by multiple attributes' do + expect(model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1')) + .to contain_exactly(model_1) + end + + # Using `mysql` & `postgresql` metadata-tags here because both adapters build + # the query slightly differently + context 'for MySQL', :mysql do + it 'builds a simple query' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT `namespaces`.* FROM `namespaces` WHERE (`namespaces`.`path` IN ('MODEL-1', 'model-2')) AND (`namespaces`.`name` = 'model 1') + QRY + + expect(query).to eq(expected_query) end + end - describe 'with a single column/value pair' do - it 'returns the criteria for a column and a value' do - criteria = double(:criteria) + context 'for PostgreSQL', :postgresql do + it 'builds a query using LOWER' do + query = model.iwhere(path: %w(MODEL-1 model-2), name: 'model 1').to_sql + expected_query = <<~QRY.strip + SELECT \"namespaces\".* FROM \"namespaces\" WHERE (LOWER(\"namespaces\".\"path\") IN (LOWER('MODEL-1'), LOWER('model-2'))) AND (LOWER(\"namespaces\".\"name\") = LOWER('model 1')) + QRY - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(criteria) - - expect(model.iwhere(foo: 'bar')).to eq(criteria) - end - - it 'returns the criteria for a column with a table, and a value' do - criteria = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') - - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(criteria) - - expect(model.iwhere('foo.bar'.to_sym => 'bar')) - .to eq(criteria) - end - end - - describe 'with multiple column/value pairs' do - it 'returns the criteria for a column and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:foo) - .and_return('`foo`') - - expect(connection).to receive(:quote_table_name) - .with(:bar) - .and_return('`bar`') - - expect(model).to receive(:where) - .with(%q{`foo` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`bar` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere(foo: 'bar', bar: 'baz') - - expect(got).to eq(final) - end - - it 'returns the criteria for a column with a table, and a value' do - initial = double(:criteria) - final = double(:criteria) - - expect(connection).to receive(:quote_table_name) - .with(:'foo.bar') - .and_return('`foo`.`bar`') - - expect(connection).to receive(:quote_table_name) - .with(:'foo.baz') - .and_return('`foo`.`baz`') - - expect(model).to receive(:where) - .with(%q{`foo`.`bar` = :value}, value: 'bar') - .and_return(initial) - - expect(initial).to receive(:where) - .with(%q{`foo`.`baz` = :value}, value: 'baz') - .and_return(final) - - got = model.iwhere('foo.bar'.to_sym => 'bar', - 'foo.baz'.to_sym => 'baz') - - expect(got).to eq(final) - end + expect(query).to eq(expected_query) end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index fd99acb3bb2..2a7aff39240 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -405,6 +405,23 @@ describe User do end end end + + describe '.by_username' do + it 'finds users regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + user2 = create(:user, username: 'UPPERCASE') + + expect(described_class.by_username(%w(CAMELCASED uppercase))) + .to contain_exactly(user, user2) + end + + it 'finds a single user regardless of the case passed' do + user = create(:user, username: 'CaMeLcAsEd') + + expect(described_class.by_username('CAMELCASED')) + .to contain_exactly(user) + end + end end describe "Respond to" do diff --git a/spec/support/helpers/test_env.rb b/spec/support/helpers/test_env.rb index 21103771d1f..3f8e3ae5190 100644 --- a/spec/support/helpers/test_env.rb +++ b/spec/support/helpers/test_env.rb @@ -52,7 +52,8 @@ module TestEnv 'add_images_and_changes' => '010d106', 'update-gitlab-shell-v-6-0-1' => '2f61d70', 'update-gitlab-shell-v-6-0-3' => 'de78448', - '2-mb-file' => 'bf12d25' + '2-mb-file' => 'bf12d25', + 'with-codeowners' => '219560e' }.freeze # gitlab-test-fork is a fork of gitlab-fork, but we don't necessarily