diff --git a/app/finders/projects_finder.rb b/app/finders/projects_finder.rb index 2c3611875a2..23648e0f494 100644 --- a/app/finders/projects_finder.rb +++ b/app/finders/projects_finder.rb @@ -142,8 +142,8 @@ class ProjectsFinder < UnionFinder # rubocop: disable CodeReuse/ActiveRecord def by_ids(items) items = items.where(id: project_ids_relation) if project_ids_relation - items = items.where('id > ?', params[:id_after]) if params[:id_after] - items = items.where('id < ?', params[:id_before]) if params[:id_before] + items = items.where('projects.id > ?', params[:id_after]) if params[:id_after] + items = items.where('projects.id < ?', params[:id_before]) if params[:id_before] items end # rubocop: enable CodeReuse/ActiveRecord diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb index 406a1fac05c..cda27890d6b 100644 --- a/app/graphql/resolvers/merge_requests_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -4,11 +4,11 @@ module Resolvers class MergeRequestsResolver < BaseResolver argument :iid, GraphQL::STRING_TYPE, required: false, - description: 'The IID of the merge request, e.g., "1"' + description: 'IID of the merge request, for example `1`' argument :iids, [GraphQL::STRING_TYPE], required: false, - description: 'The list of IIDs of issues, e.g., [1, 2]' + description: 'Array of IIDs of merge requests, for example `[1, 2]`' type Types::MergeRequestType, null: true diff --git a/changelogs/unreleased/ab-keyset-ambig-bug.yml b/changelogs/unreleased/ab-keyset-ambig-bug.yml new file mode 100644 index 00000000000..b4111794671 --- /dev/null +++ b/changelogs/unreleased/ab-keyset-ambig-bug.yml @@ -0,0 +1,5 @@ +--- +title: Fully qualify id columns for keyset pagination (Projects API) +merge_request: 29026 +author: +type: fixed diff --git a/changelogs/unreleased/remove_api_activity_logging_feature_flag.yml b/changelogs/unreleased/remove_api_activity_logging_feature_flag.yml new file mode 100644 index 00000000000..0b2ecb02d19 --- /dev/null +++ b/changelogs/unreleased/remove_api_activity_logging_feature_flag.yml @@ -0,0 +1,5 @@ +--- +title: Enable last user activity logging on the REST API +merge_request: 28755 +author: +type: added diff --git a/db/migrate/20171230123729_init_schema.rb b/db/migrate/20171230123729_init_schema.rb index a474ea2f925..79c9ebbb691 100644 --- a/db/migrate/20171230123729_init_schema.rb +++ b/db/migrate/20171230123729_init_schema.rb @@ -6,6 +6,7 @@ # rubocop:disable Migration/AddConcurrentForeignKey # rubocop:disable Style/WordArray # rubocop:disable Migration/AddLimitToStringColumns +# rubocop:disable Migration/Datetime class InitSchema < ActiveRecord::Migration[4.2] DOWNTIME = false diff --git a/db/migrate/20180116193854_create_lfs_file_locks.rb b/db/migrate/20180116193854_create_lfs_file_locks.rb index 2dd0e71916b..b94f69ad42e 100644 --- a/db/migrate/20180116193854_create_lfs_file_locks.rb +++ b/db/migrate/20180116193854_create_lfs_file_locks.rb @@ -9,7 +9,7 @@ class CreateLfsFileLocks < ActiveRecord::Migration[4.2] create_table :lfs_file_locks do |t| t.references :project, null: false, foreign_key: { on_delete: :cascade } t.references :user, null: false, index: true, foreign_key: { on_delete: :cascade } - t.datetime :created_at, null: false + t.datetime :created_at, null: false # rubocop:disable Migration/Datetime t.string :path, limit: 511 end diff --git a/db/migrate/20180503131624_create_remote_mirrors.rb b/db/migrate/20180503131624_create_remote_mirrors.rb index a079c1b3328..80090c14314 100644 --- a/db/migrate/20180503131624_create_remote_mirrors.rb +++ b/db/migrate/20180503131624_create_remote_mirrors.rb @@ -14,9 +14,9 @@ class CreateRemoteMirrors < ActiveRecord::Migration[4.2] t.string :url t.boolean :enabled, default: true t.string :update_status - t.datetime :last_update_at - t.datetime :last_successful_update_at - t.datetime :last_update_started_at + t.datetime :last_update_at # rubocop:disable Migration/Datetime + t.datetime :last_successful_update_at # rubocop:disable Migration/Datetime + t.datetime :last_update_started_at # rubocop:disable Migration/Datetime t.string :last_error t.boolean :only_protected_branches, default: false, null: false t.string :remote_name diff --git a/db/migrate/20200215225103_drop_forked_project_links_table.rb b/db/migrate/20200215225103_drop_forked_project_links_table.rb index f8dbd19980e..9c028d23dbc 100644 --- a/db/migrate/20200215225103_drop_forked_project_links_table.rb +++ b/db/migrate/20200215225103_drop_forked_project_links_table.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +# rubocop:disable Migration/Datetime + class DropForkedProjectLinksTable < ActiveRecord::Migration[6.0] include Gitlab::Database::MigrationHelpers diff --git a/doc/api/graphql/reference/gitlab_schema.graphql b/doc/api/graphql/reference/gitlab_schema.graphql index 50d5eeaf9ab..7fd6e78539e 100644 --- a/doc/api/graphql/reference/gitlab_schema.graphql +++ b/doc/api/graphql/reference/gitlab_schema.graphql @@ -6147,12 +6147,12 @@ type Project { """ mergeRequest( """ - The IID of the merge request, e.g., "1" + IID of the merge request, for example `1` """ iid: String """ - The list of IIDs of issues, e.g., [1, 2] + Array of IIDs of merge requests, for example `[1, 2]` """ iids: [String!] ): MergeRequest @@ -6177,12 +6177,12 @@ type Project { first: Int """ - The IID of the merge request, e.g., "1" + IID of the merge request, for example `1` """ iid: String """ - The list of IIDs of issues, e.g., [1, 2] + Array of IIDs of merge requests, for example `[1, 2]` """ iids: [String!] diff --git a/doc/api/graphql/reference/gitlab_schema.json b/doc/api/graphql/reference/gitlab_schema.json index c7281fcc638..a4210165c6d 100644 --- a/doc/api/graphql/reference/gitlab_schema.json +++ b/doc/api/graphql/reference/gitlab_schema.json @@ -18471,7 +18471,7 @@ "args": [ { "name": "iid", - "description": "The IID of the merge request, e.g., \"1\"", + "description": "IID of the merge request, for example `1`", "type": { "kind": "SCALAR", "name": "String", @@ -18481,7 +18481,7 @@ }, { "name": "iids", - "description": "The list of IIDs of issues, e.g., [1, 2]", + "description": "Array of IIDs of merge requests, for example `[1, 2]`", "type": { "kind": "LIST", "name": null, @@ -18512,7 +18512,7 @@ "args": [ { "name": "iid", - "description": "The IID of the merge request, e.g., \"1\"", + "description": "IID of the merge request, for example `1`", "type": { "kind": "SCALAR", "name": "String", @@ -18522,7 +18522,7 @@ }, { "name": "iids", - "description": "The list of IIDs of issues, e.g., [1, 2]", + "description": "Array of IIDs of merge requests, for example `[1, 2]`", "type": { "kind": "LIST", "name": null, diff --git a/doc/user/project/import/svn.md b/doc/user/project/import/svn.md index faf282c8ac3..cf034aafca9 100644 --- a/doc/user/project/import/svn.md +++ b/doc/user/project/import/svn.md @@ -100,9 +100,6 @@ Running SubGit in a mirror mode requires a [registration](https://subgit.com/pricing). Registration is free for open source, academic and startup projects. -We're currently working on deeper GitLab/SubGit integration. You may track our -progress at [this issue](https://gitlab.com/gitlab-org/gitlab/issues/990). - ### SubGit support For any questions related to SVN to GitLab migration with SubGit, you can diff --git a/lib/api/api.rb b/lib/api/api.rb index 51fc006ec08..cf6e7f18a5f 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -105,7 +105,7 @@ module API namespace do after do - ::Users::ActivityService.new(@current_user).execute if Feature.enabled?(:api_activity_logging) + ::Users::ActivityService.new(@current_user).execute end # Keep in alphabetical order diff --git a/lib/gitlab/background_migration/link_lfs_objects.rb b/lib/gitlab/background_migration/link_lfs_objects.rb deleted file mode 100644 index 69c03f617bf..00000000000 --- a/lib/gitlab/background_migration/link_lfs_objects.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BackgroundMigration - # Create missing LfsObjectsProject records for forks - class LinkLfsObjects - # Model definition used for migration - class ForkNetworkMember < ActiveRecord::Base - self.table_name = 'fork_network_members' - - def self.with_non_existing_lfs_objects - joins('JOIN lfs_objects_projects lop ON fork_network_members.forked_from_project_id = lop.project_id') - .where( - <<~SQL - NOT EXISTS ( - SELECT 1 - FROM lfs_objects_projects - WHERE lfs_objects_projects.project_id = fork_network_members.project_id - AND lfs_objects_projects.lfs_object_id = lop.lfs_object_id - ) - SQL - ) - end - end - - def perform(start_id, end_id) - # no-op as some queries times out - end - end - end -end diff --git a/lib/gitlab/experimentation.rb b/lib/gitlab/experimentation.rb index b8c630d1b12..68f3c9e332e 100644 --- a/lib/gitlab/experimentation.rb +++ b/lib/gitlab/experimentation.rb @@ -20,7 +20,7 @@ module Gitlab paid_signup_flow: { feature_toggle: :paid_signup_flow, environment: ::Gitlab.dev_env_or_com?, - enabled_ratio: 0.5, + enabled_ratio: 1, tracking_category: 'Growth::Acquisition::Experiment::PaidSignUpFlow' }, suggest_pipeline: { diff --git a/rubocop/cop/migration/datetime.rb b/rubocop/cop/migration/datetime.rb index 03ad3f3f601..5a6cdc74ca4 100644 --- a/rubocop/cop/migration/datetime.rb +++ b/rubocop/cop/migration/datetime.rb @@ -14,7 +14,7 @@ module RuboCop return unless in_migration?(node) node.each_descendant(:send) do |send_node| - method_name = node.children[1] + method_name = send_node.children[1] if method_name == :datetime || method_name == :timestamp add_offense(send_node, location: :selector, message: format(MSG, method_name)) diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 626517f8fa0..379cbe83a08 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -85,6 +85,24 @@ describe ProjectsFinder, :do_not_mock_admin_mode do end end + describe 'regression: Combination of id_before/id_after and joins requires fully qualified column names' do + context 'only returns projects with a project id less than given and matching search' do + subject { finder.execute.joins(:route) } + + let(:params) { { id_before: public_project.id }} + + it { is_expected.to eq([internal_project]) } + end + + context 'only returns projects with a project id greater than given and matching search' do + subject { finder.execute.joins(:route) } + + let(:params) { { id_after: internal_project.id }} + + it { is_expected.to eq([public_project]) } + end + end + describe 'filter by visibility_level' do before do private_project.add_maintainer(user) diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index c794db4cb0b..baebbbce631 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -13,13 +13,5 @@ describe API::API do it 'updates the users last_activity_on date' do expect { get api('/groups', user) }.to change { user.reload.last_activity_on }.to(Date.today) end - - context 'when the the api_activity_logging feature is disabled' do - it 'does not touch last_activity_on' do - stub_feature_flags(api_activity_logging: false) - - expect { get api('/groups', user) }.not_to change { user.reload.last_activity_on } - end - end end end diff --git a/spec/rubocop/cop/migration/datetime_spec.rb b/spec/rubocop/cop/migration/datetime_spec.rb index 0a771003100..e3023406dce 100644 --- a/spec/rubocop/cop/migration/datetime_spec.rb +++ b/spec/rubocop/cop/migration/datetime_spec.rb @@ -12,9 +12,69 @@ describe RuboCop::Cop::Migration::Datetime do subject(:cop) { described_class.new } - let(:migration_with_datetime) do + let(:create_table_migration_with_datetime) do %q( - class Users < ActiveRecord::Migration[4.2] + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.datetime :last_sign_in + end + end + end + ) + end + + let(:create_table_migration_with_timestamp) do + %q( + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.timestamp :last_sign_in + end + end + end + ) + end + + let(:create_table_migration_without_datetime) do + %q( + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.string :password + end + end + end + ) + end + + let(:create_table_migration_with_datetime_with_timezone) do + %q( + class Users < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + create_table :users do |t| + t.string :username, null: false + t.datetime_with_timezone :last_sign_in + end + end + end + ) + end + + let(:add_column_migration_with_datetime) do + %q( + class Users < ActiveRecord::Migration[6.0] DOWNTIME = false def change @@ -25,9 +85,9 @@ describe RuboCop::Cop::Migration::Datetime do ) end - let(:migration_with_timestamp) do + let(:add_column_migration_with_timestamp) do %q( - class Users < ActiveRecord::Migration[4.2] + class Users < ActiveRecord::Migration[6.0] DOWNTIME = false def change @@ -38,9 +98,9 @@ describe RuboCop::Cop::Migration::Datetime do ) end - let(:migration_without_datetime) do + let(:add_column_migration_without_datetime) do %q( - class Users < ActiveRecord::Migration[4.2] + class Users < ActiveRecord::Migration[6.0] DOWNTIME = false def change @@ -50,9 +110,9 @@ describe RuboCop::Cop::Migration::Datetime do ) end - let(:migration_with_datetime_with_timezone) do + let(:add_column_migration_with_datetime_with_timezone) do %q( - class Users < ActiveRecord::Migration[4.2] + class Users < ActiveRecord::Migration[6.0] DOWNTIME = false def change @@ -68,18 +128,54 @@ describe RuboCop::Cop::Migration::Datetime do allow(cop).to receive(:in_migration?).and_return(true) end - it 'registers an offense when the ":datetime" data type is used' do - inspect_source(migration_with_datetime) + it 'registers an offense when the ":datetime" data type is used on create_table' do + inspect_source(create_table_migration_with_datetime) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([8]) + expect(cop.offenses.first.message).to include('`datetime`') + end + end + + it 'registers an offense when the ":timestamp" data type is used on create_table' do + inspect_source(create_table_migration_with_timestamp) + + aggregate_failures do + expect(cop.offenses.size).to eq(1) + expect(cop.offenses.map(&:line)).to eq([8]) + expect(cop.offenses.first.message).to include('timestamp') + end + end + + it 'does not register an offense when the ":datetime" data type is not used on create_table' do + inspect_source(create_table_migration_without_datetime) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + + it 'does not register an offense when the ":datetime_with_timezone" data type is used on create_table' do + inspect_source(create_table_migration_with_datetime_with_timezone) + + aggregate_failures do + expect(cop.offenses.size).to eq(0) + end + end + + it 'registers an offense when the ":datetime" data type is used on add_column' do + inspect_source(add_column_migration_with_datetime) aggregate_failures do expect(cop.offenses.size).to eq(1) expect(cop.offenses.map(&:line)).to eq([7]) - expect(cop.offenses.first.message).to include('datetime') + expect(cop.offenses.first.message).to include('`datetime`') end end - it 'registers an offense when the ":timestamp" data type is used' do - inspect_source(migration_with_timestamp) + it 'registers an offense when the ":timestamp" data type is used on add_column' do + inspect_source(add_column_migration_with_timestamp) aggregate_failures do expect(cop.offenses.size).to eq(1) @@ -88,16 +184,16 @@ describe RuboCop::Cop::Migration::Datetime do end end - it 'does not register an offense when the ":datetime" data type is not used' do - inspect_source(migration_without_datetime) + it 'does not register an offense when the ":datetime" data type is not used on add_column' do + inspect_source(add_column_migration_without_datetime) aggregate_failures do expect(cop.offenses.size).to eq(0) end end - it 'does not register an offense when the ":datetime_with_timezone" data type is used' do - inspect_source(migration_with_datetime_with_timezone) + it 'does not register an offense when the ":datetime_with_timezone" data type is used on add_column' do + inspect_source(add_column_migration_with_datetime_with_timezone) aggregate_failures do expect(cop.offenses.size).to eq(0) @@ -107,10 +203,10 @@ describe RuboCop::Cop::Migration::Datetime do context 'outside of migration' do it 'registers no offense' do - inspect_source(migration_with_datetime) - inspect_source(migration_with_timestamp) - inspect_source(migration_without_datetime) - inspect_source(migration_with_datetime_with_timezone) + inspect_source(add_column_migration_with_datetime) + inspect_source(add_column_migration_with_timestamp) + inspect_source(add_column_migration_without_datetime) + inspect_source(add_column_migration_with_datetime_with_timezone) expect(cop.offenses.size).to eq(0) end