From 793c97ddc5747fd45cccfcca8d778c2a48b92eff Mon Sep 17 00:00:00 2001 From: David Kim Date: Tue, 25 Aug 2020 17:59:21 +0930 Subject: [PATCH 01/18] Add merge_request_reviewers table and model It's added to prepare for dedicated reviewers section for MRs on EE --- app/models/merge_request.rb | 2 ++ app/models/merge_request_reviewer.rb | 8 +++++ .../add-reviewers-to-merge-requests.yml | 5 +++ ...25071735_create_merge_request_reviewers.rb | 15 +++++++++ db/schema_migrations/20200825071735 | 1 + db/structure.sql | 31 +++++++++++++++++++ spec/lib/gitlab/import_export/all_models.yml | 2 ++ spec/models/merge_request_reviewer_spec.rb | 18 +++++++++++ spec/models/merge_request_spec.rb | 1 + 9 files changed, 83 insertions(+) create mode 100644 app/models/merge_request_reviewer.rb create mode 100644 changelogs/unreleased/add-reviewers-to-merge-requests.yml create mode 100644 db/migrate/20200825071735_create_merge_request_reviewers.rb create mode 100644 db/schema_migrations/20200825071735 create mode 100644 spec/models/merge_request_reviewer_spec.rb diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ff72aa7f563c74..88c660b12b89dd 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -80,6 +80,8 @@ def merge_request_diff has_many :merge_request_assignees has_many :assignees, class_name: "User", through: :merge_request_assignees + has_many :merge_request_reviewers + has_many :reviewers, class_name: "User", through: :merge_request_reviewers has_many :user_mentions, class_name: "MergeRequestUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :deployment_merge_requests diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb new file mode 100644 index 00000000000000..b01b8591c79ee3 --- /dev/null +++ b/app/models/merge_request_reviewer.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class MergeRequestReviewer < ApplicationRecord + belongs_to :merge_request + belongs_to :reviewer, class_name: "User", foreign_key: :user_id, inverse_of: :merge_request_assignees + + validates :reviewer, uniqueness: { scope: :merge_request_id } +end diff --git a/changelogs/unreleased/add-reviewers-to-merge-requests.yml b/changelogs/unreleased/add-reviewers-to-merge-requests.yml new file mode 100644 index 00000000000000..6c5c4a1e4b86f7 --- /dev/null +++ b/changelogs/unreleased/add-reviewers-to-merge-requests.yml @@ -0,0 +1,5 @@ +--- +title: Add merge_request_reviewers table +merge_request: 40358 +author: +type: added diff --git a/db/migrate/20200825071735_create_merge_request_reviewers.rb b/db/migrate/20200825071735_create_merge_request_reviewers.rb new file mode 100644 index 00000000000000..16ddf0d77fa7d4 --- /dev/null +++ b/db/migrate/20200825071735_create_merge_request_reviewers.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateMergeRequestReviewers < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :merge_request_reviewers do |t| + t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false + t.references :merge_request, foreign_key: { on_delete: :cascade }, index: true, null: false + t.datetime_with_timezone :created_at, null: false + end + end +end diff --git a/db/schema_migrations/20200825071735 b/db/schema_migrations/20200825071735 new file mode 100644 index 00000000000000..fe108bfb9cf705 --- /dev/null +++ b/db/schema_migrations/20200825071735 @@ -0,0 +1 @@ +cd8574318fae1f2bb021b53d4e453c6b64c763f0e7cc8836cdb8b12963ff0e18 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index bec2f11356c560..63c5ce6b5e04c4 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13124,6 +13124,22 @@ CREATE SEQUENCE public.merge_request_metrics_id_seq ALTER SEQUENCE public.merge_request_metrics_id_seq OWNED BY public.merge_request_metrics.id; +CREATE TABLE public.merge_request_reviewers ( + id bigint NOT NULL, + user_id bigint NOT NULL, + merge_request_id bigint NOT NULL, + created_at timestamp with time zone NOT NULL +); + +CREATE SEQUENCE public.merge_request_reviewers_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + +ALTER SEQUENCE public.merge_request_reviewers_id_seq OWNED BY public.merge_request_reviewers.id; + CREATE TABLE public.merge_request_user_mentions ( id bigint NOT NULL, merge_request_id integer NOT NULL, @@ -17104,6 +17120,8 @@ ALTER TABLE ONLY public.merge_request_diffs ALTER COLUMN id SET DEFAULT nextval( ALTER TABLE ONLY public.merge_request_metrics ALTER COLUMN id SET DEFAULT nextval('public.merge_request_metrics_id_seq'::regclass); +ALTER TABLE ONLY public.merge_request_reviewers ALTER COLUMN id SET DEFAULT nextval('public.merge_request_reviewers_id_seq'::regclass); + ALTER TABLE ONLY public.merge_request_user_mentions ALTER COLUMN id SET DEFAULT nextval('public.merge_request_user_mentions_id_seq'::regclass); ALTER TABLE ONLY public.merge_requests ALTER COLUMN id SET DEFAULT nextval('public.merge_requests_id_seq'::regclass); @@ -18216,6 +18234,9 @@ ALTER TABLE ONLY public.merge_request_diffs ALTER TABLE ONLY public.merge_request_metrics ADD CONSTRAINT merge_request_metrics_pkey PRIMARY KEY (id); +ALTER TABLE ONLY public.merge_request_reviewers + ADD CONSTRAINT merge_request_reviewers_pkey PRIMARY KEY (id); + ALTER TABLE ONLY public.merge_request_user_mentions ADD CONSTRAINT merge_request_user_mentions_pkey PRIMARY KEY (id); @@ -20041,6 +20062,10 @@ CREATE INDEX index_merge_request_metrics_on_target_project_id ON public.merge_re CREATE INDEX index_merge_request_metrics_on_target_project_id_merged_at ON public.merge_request_metrics USING btree (target_project_id, merged_at); +CREATE INDEX index_merge_request_reviewers_on_merge_request_id ON public.merge_request_reviewers USING btree (merge_request_id); + +CREATE INDEX index_merge_request_reviewers_on_user_id ON public.merge_request_reviewers USING btree (user_id); + CREATE UNIQUE INDEX index_merge_request_user_mentions_on_note_id ON public.merge_request_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL); CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON public.merge_requests_closing_issues USING btree (issue_id); @@ -22232,6 +22257,9 @@ ALTER TABLE ONLY public.board_labels ALTER TABLE ONLY public.merge_request_blocks ADD CONSTRAINT fk_rails_364d4bea8b FOREIGN KEY (blocked_merge_request_id) REFERENCES public.merge_requests(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.merge_request_reviewers + ADD CONSTRAINT fk_rails_3704a66140 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.analytics_cycle_analytics_project_stages ADD CONSTRAINT fk_rails_3829e49b66 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; @@ -23006,6 +23034,9 @@ ALTER TABLE ONLY public.alert_management_alert_assignees ALTER TABLE ONLY public.geo_hashed_storage_attachments_events ADD CONSTRAINT fk_rails_d496b088e9 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE; +ALTER TABLE ONLY public.merge_request_reviewers + ADD CONSTRAINT fk_rails_d9fec24b9d FOREIGN KEY (merge_request_id) REFERENCES public.merge_requests(id) ON DELETE CASCADE; + ALTER TABLE ONLY public.jira_imports ADD CONSTRAINT fk_rails_da617096ce FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE SET NULL; diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 37b5d8a1021805..37b18506735013 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -120,6 +120,7 @@ merge_requests: - award_emoji - author - assignee +- reviewers - updated_by - milestone - iteration @@ -147,6 +148,7 @@ merge_requests: - latest_merge_request_diff - pipelines_for_merge_request - merge_request_assignees +- merge_request_reviewers - suggestions - diff_note_positions - unresolved_notes diff --git a/spec/models/merge_request_reviewer_spec.rb b/spec/models/merge_request_reviewer_spec.rb new file mode 100644 index 00000000000000..3d9b3aee6c46f6 --- /dev/null +++ b/spec/models/merge_request_reviewer_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestReviewer do + let(:merge_request) { create(:merge_request) } + + subject { merge_request.merge_request_reviewers.build(reviewer: create(:user)) } + + describe 'associations' do + it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') } + it { is_expected.to belong_to(:reviewer).class_name('User') } + end + + describe 'validations' do + it { is_expected.to validate_uniqueness_of(:reviewer).scoped_to(:merge_request_id) } + end +end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index da84b2b6a8f964..82ddc9caad81e3 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -18,6 +18,7 @@ it { is_expected.to belong_to(:source_project).class_name('Project') } it { is_expected.to belong_to(:merge_user).class_name("User") } it { is_expected.to have_many(:assignees).through(:merge_request_assignees) } + it { is_expected.to have_many(:reviewers).through(:merge_request_reviewers) } it { is_expected.to have_many(:merge_request_diffs) } it { is_expected.to have_many(:user_mentions).class_name("MergeRequestUserMention") } it { is_expected.to belong_to(:milestone) } -- GitLab From 68ed25107c1cdd128891594ea190bb0c45ead14a Mon Sep 17 00:00:00 2001 From: David Kim Date: Wed, 26 Aug 2020 18:51:57 +0930 Subject: [PATCH 02/18] Add ability to set reviewers on MRs --- .../merge_requests/application_controller.rb | 1 + app/models/concerns/issuable.rb | 4 ++ app/services/issuable_base_service.rb | 16 ++++--- ee/app/helpers/ee/form_helper.rb | 45 +++++++++++++++++++ ee/app/models/ee/merge_request.rb | 4 ++ ee/app/models/license.rb | 1 + ee/app/services/ee/issuable_base_service.rb | 18 ++++++++ 7 files changed, 84 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/merge_requests/application_controller.rb b/app/controllers/projects/merge_requests/application_controller.rb index 0bb4e0fb5ee35a..921da788ad2616 100644 --- a/app/controllers/projects/merge_requests/application_controller.rb +++ b/app/controllers/projects/merge_requests/application_controller.rb @@ -43,6 +43,7 @@ def merge_request_params_attributes :discussion_locked, label_ids: [], assignee_ids: [], + reviewer_ids: [], update_task: [:index, :checked, :line_number, :line_source] ] end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 46cd9649c2d2df..2d6327dca62500 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -177,6 +177,10 @@ def has_multiple_assignees? assignees.count > 1 end + def allows_reviewers? + false + end + def supports_weight? false end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 65a73dadc2e7c5..87cd8fd2667011 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -35,6 +35,7 @@ def filter_params(issuable) end filter_assignee(issuable) + filter_reviewer(issuable) filter_milestone filter_labels end @@ -46,7 +47,7 @@ def filter_assignee(issuable) params[:assignee_ids] = params[:assignee_ids].first(1) end - assignee_ids = params[:assignee_ids].select { |assignee_id| assignee_can_read?(issuable, assignee_id) } + assignee_ids = params[:assignee_ids].select { |assignee_id| user_can_read?(issuable, assignee_id) } if params[:assignee_ids].map(&:to_s) == [IssuableFinder::Params::NONE] params[:assignee_ids] = [] @@ -57,15 +58,20 @@ def filter_assignee(issuable) end end - def assignee_can_read?(issuable, assignee_id) - new_assignee = User.find_by_id(assignee_id) + def filter_reviewer(issuable) + # Reviewer is only available for EE + params.delete(:reviewer_ids) + end + + def user_can_read?(issuable, user_id) + new_user = User.find_by_id(user_id) - return false unless new_assignee + return false unless new_user ability_name = :"read_#{issuable.to_ability_name}" resource = issuable.persisted? ? issuable : project - can?(new_assignee, ability_name, resource) + can?(new_user, ability_name, resource) end def filter_milestone diff --git a/ee/app/helpers/ee/form_helper.rb b/ee/app/helpers/ee/form_helper.rb index ba0ec340d1593a..c786a6e93ccda4 100644 --- a/ee/app/helpers/ee/form_helper.rb +++ b/ee/app/helpers/ee/form_helper.rb @@ -9,5 +9,50 @@ def issue_supports_multiple_assignees? def merge_request_supports_multiple_assignees? @merge_request&.allows_multiple_assignees? end + + def reviewers_dropdown_options(issuable_type) + dropdown_data = { + toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', + title: 'Select reviewer', + filter: true, + dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-reviewer', + placeholder: _('Search users'), + data: { + first_user: current_user&.username, + null_user: true, + current_user: true, + project_id: (@target_project || @project)&.id, field_name: "#{issuable_type}[reviewer_ids][]", + default_label: 'Unassigned', + 'max-select': 1, + 'dropdown-header': 'Reviewer', + multi_select: true, + 'input-meta': 'name', + 'always-show-selectbox': true, + current_user_info: UserSerializer.new.represent(current_user) + } + } + + type = issuable_type.to_s + + if type == 'merge_request' && merge_request_supports_reviewers? + dropdown_data = multiple_reviewers_dropdown_options(dropdown_data) + end + + dropdown_data + end + + def multiple_reviewers_dropdown_options(options) + new_options = options.dup + + new_options[:title] = 'Select reviewer(s)' + new_options[:data][:'dropdown-header'] = 'Reviewer(s)' + new_options[:data].delete(:'max-select') + + new_options + end + + def merge_request_supports_reviewers? + @merge_request&.allows_reviewers? + end end end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 7df5eb8f08c33b..225fa7477674ab 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -126,6 +126,10 @@ def allows_multiple_assignees? project.feature_available?(:multiple_merge_request_assignees) end + def allows_reviewers? + project.feature_available?(:merge_request_reviewers) + end + def visible_blocking_merge_requests(user) Ability.merge_requests_readable_by_user(blocking_merge_requests, user) end diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index a6c4ba8533e6f7..dc576c1a725cfe 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -91,6 +91,7 @@ class License < ApplicationRecord ldap_group_sync_filter merge_pipelines merge_request_performance_metrics + merge_request_reviewers admin_merge_request_approvers_rules merge_trains metrics_reports diff --git a/ee/app/services/ee/issuable_base_service.rb b/ee/app/services/ee/issuable_base_service.rb index 93e29849b540d2..6f6f22085c3975 100644 --- a/ee/app/services/ee/issuable_base_service.rb +++ b/ee/app/services/ee/issuable_base_service.rb @@ -24,6 +24,24 @@ def filter_params(issuable) super end + def filter_reviewer(issuable) + return if params[:reviewer_ids].blank? + + unless can_admin_issuable?(issuable) && issuable.allows_reviewers? + params.delete(:reviewer_ids) + end + + reviewer_ids = params[:reviewer_ids].select { |reviewer_id| user_can_read?(issuable, reviewer_id) } + + if params[:reviewer_ids].map(&:to_s) == [IssuableFinder::Params::NONE] + params[:reviewer_ids] = [] + elsif reviewer_ids.any? + params[:reviewer_ids] = reviewer_ids + else + params.delete(:reviewer_ids) + end + end + override :filter_labels def filter_labels super -- GitLab From 020d81b3de25e249da3a43228b5a7a9cd45d6ce1 Mon Sep 17 00:00:00 2001 From: David Kim Date: Thu, 27 Aug 2020 13:24:39 +0930 Subject: [PATCH 03/18] Add reviewers to serializers --- app/serializers/merge_request_basic_entity.rb | 1 + app/serializers/merge_request_reviewer_entity.rb | 9 +++++++++ .../merge_request_sidebar_extras_entity.rb | 4 ++++ .../serializers/ee/merge_request_reviewer_entity.rb | 11 +++++++++++ 4 files changed, 25 insertions(+) create mode 100644 app/serializers/merge_request_reviewer_entity.rb create mode 100644 ee/app/serializers/ee/merge_request_reviewer_entity.rb diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 82baf4a4a78b0f..0fa447f07cce76 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/app/serializers/merge_request_basic_entity.rb @@ -9,6 +9,7 @@ class MergeRequestBasicEntity < Grape::Entity expose :milestone, using: API::Entities::Milestone expose :labels, using: LabelEntity expose :assignees, using: API::Entities::UserBasic + expose :reviewers, using: API::Entities::UserBasic expose :task_status, :task_status_short expose :lock_version, :lock_version end diff --git a/app/serializers/merge_request_reviewer_entity.rb b/app/serializers/merge_request_reviewer_entity.rb new file mode 100644 index 00000000000000..fefd116014fbaf --- /dev/null +++ b/app/serializers/merge_request_reviewer_entity.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class MergeRequestReviewerEntity < ::API::Entities::UserBasic + expose :can_merge do |reviewer, options| + options[:merge_request]&.can_be_merged_by?(reviewer) + end +end + +MergeRequestReviewerEntity.prepend_if_ee('EE::MergeRequestReviewerEntity') diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb index 7276509c363bcc..c46e4dc13a2075 100644 --- a/app/serializers/merge_request_sidebar_extras_entity.rb +++ b/app/serializers/merge_request_sidebar_extras_entity.rb @@ -4,4 +4,8 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity expose :assignees do |merge_request| MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request) end + + expose :reviewers do |merge_request| + MergeRequestReviewerEntity.represent(merge_request.reviewers, merge_request: merge_request) + end end diff --git a/ee/app/serializers/ee/merge_request_reviewer_entity.rb b/ee/app/serializers/ee/merge_request_reviewer_entity.rb new file mode 100644 index 00000000000000..20e3ea5fb3a907 --- /dev/null +++ b/ee/app/serializers/ee/merge_request_reviewer_entity.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module EE + module MergeRequestReviewerEntity + extend ActiveSupport::Concern + + prepended do + expose :gitlab_employee?, as: :is_gitlab_employee, if: proc { ::Gitlab.com? && ::Feature.enabled?(:gitlab_employee_badge) } + end + end +end -- GitLab From 5c2028bd87400e58178e77797d73676271c70d89 Mon Sep 17 00:00:00 2001 From: David Kim Date: Thu, 27 Aug 2020 17:08:31 +0930 Subject: [PATCH 04/18] Split migration into three and replace validation with unique index --- app/models/merge_request_reviewer.rb | 2 -- ...25071735_create_merge_request_reviewers.rb | 13 ++++++++--- ..._foreign_key_to_merge_request_reviewers.rb | 22 +++++++++++++++++++ ..._foreign_key_to_merge_request_reviewers.rb | 22 +++++++++++++++++++ db/schema_migrations/20200827060911 | 1 + db/schema_migrations/20200827060932 | 1 + db/structure.sql | 2 +- spec/models/merge_request_reviewer_spec.rb | 4 ---- 8 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20200827060911_add_merge_request_foreign_key_to_merge_request_reviewers.rb create mode 100644 db/migrate/20200827060932_add_user_foreign_key_to_merge_request_reviewers.rb create mode 100644 db/schema_migrations/20200827060911 create mode 100644 db/schema_migrations/20200827060932 diff --git a/app/models/merge_request_reviewer.rb b/app/models/merge_request_reviewer.rb index b01b8591c79ee3..1cb49c0cd76a56 100644 --- a/app/models/merge_request_reviewer.rb +++ b/app/models/merge_request_reviewer.rb @@ -3,6 +3,4 @@ class MergeRequestReviewer < ApplicationRecord belongs_to :merge_request belongs_to :reviewer, class_name: "User", foreign_key: :user_id, inverse_of: :merge_request_assignees - - validates :reviewer, uniqueness: { scope: :merge_request_id } end diff --git a/db/migrate/20200825071735_create_merge_request_reviewers.rb b/db/migrate/20200825071735_create_merge_request_reviewers.rb index 16ddf0d77fa7d4..45451476bb02e0 100644 --- a/db/migrate/20200825071735_create_merge_request_reviewers.rb +++ b/db/migrate/20200825071735_create_merge_request_reviewers.rb @@ -5,11 +5,18 @@ class CreateMergeRequestReviewers < ActiveRecord::Migration[6.0] DOWNTIME = false - def change + def up create_table :merge_request_reviewers do |t| - t.references :user, foreign_key: { on_delete: :cascade }, index: true, null: false - t.references :merge_request, foreign_key: { on_delete: :cascade }, index: true, null: false + t.bigint :user_id, null: false + t.bigint :merge_request_id, null: false t.datetime_with_timezone :created_at, null: false end + + add_index :merge_request_reviewers, [:merge_request_id, :user_id], unique: true + add_index :merge_request_reviewers, :user_id + end + + def down + drop_table :merge_request_reviewers end end diff --git a/db/migrate/20200827060911_add_merge_request_foreign_key_to_merge_request_reviewers.rb b/db/migrate/20200827060911_add_merge_request_foreign_key_to_merge_request_reviewers.rb new file mode 100644 index 00000000000000..dc3356375fdaec --- /dev/null +++ b/db/migrate/20200827060911_add_merge_request_foreign_key_to_merge_request_reviewers.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMergeRequestForeignKeyToMergeRequestReviewers < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_foreign_key :merge_request_reviewers, :merge_requests, column: :merge_request_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey + end + end + + def down + with_lock_retries do + remove_foreign_key :merge_request_reviewers, column: :merge_request_id + end + end +end diff --git a/db/migrate/20200827060932_add_user_foreign_key_to_merge_request_reviewers.rb b/db/migrate/20200827060932_add_user_foreign_key_to_merge_request_reviewers.rb new file mode 100644 index 00000000000000..d6c6985a668399 --- /dev/null +++ b/db/migrate/20200827060932_add_user_foreign_key_to_merge_request_reviewers.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +# See https://docs.gitlab.com/ee/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddUserForeignKeyToMergeRequestReviewers < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + with_lock_retries do + add_foreign_key :merge_request_reviewers, :users, column: :user_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey + end + end + + def down + with_lock_retries do + remove_foreign_key :merge_request_reviewers, column: :user_id + end + end +end diff --git a/db/schema_migrations/20200827060911 b/db/schema_migrations/20200827060911 new file mode 100644 index 00000000000000..a93302067cd59e --- /dev/null +++ b/db/schema_migrations/20200827060911 @@ -0,0 +1 @@ +124f6ba79f71e2de510741b22d3dd5cf15378b5476c759484bd814377a644256 \ No newline at end of file diff --git a/db/schema_migrations/20200827060932 b/db/schema_migrations/20200827060932 new file mode 100644 index 00000000000000..af7d00f21f2124 --- /dev/null +++ b/db/schema_migrations/20200827060932 @@ -0,0 +1 @@ +ba2e32b4836062631308937023470d31b3f808b468999ba15374c3b953377402 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 63c5ce6b5e04c4..6ceaf303d6ef18 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -20062,7 +20062,7 @@ CREATE INDEX index_merge_request_metrics_on_target_project_id ON public.merge_re CREATE INDEX index_merge_request_metrics_on_target_project_id_merged_at ON public.merge_request_metrics USING btree (target_project_id, merged_at); -CREATE INDEX index_merge_request_reviewers_on_merge_request_id ON public.merge_request_reviewers USING btree (merge_request_id); +CREATE UNIQUE INDEX index_merge_request_reviewers_on_merge_request_id_and_user_id ON public.merge_request_reviewers USING btree (merge_request_id, user_id); CREATE INDEX index_merge_request_reviewers_on_user_id ON public.merge_request_reviewers USING btree (user_id); diff --git a/spec/models/merge_request_reviewer_spec.rb b/spec/models/merge_request_reviewer_spec.rb index 3d9b3aee6c46f6..55199cd96ad997 100644 --- a/spec/models/merge_request_reviewer_spec.rb +++ b/spec/models/merge_request_reviewer_spec.rb @@ -11,8 +11,4 @@ it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') } it { is_expected.to belong_to(:reviewer).class_name('User') } end - - describe 'validations' do - it { is_expected.to validate_uniqueness_of(:reviewer).scoped_to(:merge_request_id) } - end end -- GitLab From 6f02131d37f6b584d59d4c25f9b6661b712256df Mon Sep 17 00:00:00 2001 From: David Kim Date: Fri, 28 Aug 2020 17:13:51 +0930 Subject: [PATCH 05/18] Add new reviewers to fixtures --- .../fixtures/api/schemas/entities/merge_request_basic.json | 7 +++++++ .../api/schemas/entities/merge_request_sidebar_extras.json | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/spec/fixtures/api/schemas/entities/merge_request_basic.json b/spec/fixtures/api/schemas/entities/merge_request_basic.json index ac0a031445558c..3c19528d71bdeb 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_basic.json +++ b/spec/fixtures/api/schemas/entities/merge_request_basic.json @@ -15,6 +15,13 @@ "$ref": "../public_api/v4/user/basic.json" } }, + "reviewers": { + "type": ["array"], + "items": { + "type": "object", + "$ref": "../public_api/v4/user/basic.json" + } + }, "milestone": { "type": [ "object", "null" ] }, diff --git a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json b/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json index 11076ec73ded71..c54ae551d6a8c0 100644 --- a/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json +++ b/spec/fixtures/api/schemas/entities/merge_request_sidebar_extras.json @@ -17,6 +17,10 @@ "assignees": { "type": "array", "items": { "$ref": "../public_api/v4/user/basic.json" } + }, + "reviewers": { + "type": "array", + "items": { "$ref": "../public_api/v4/user/basic.json" } } }, "additionalProperties": false -- GitLab From 33afb3920b26313e93bd444ba50b854b17efddbf Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 31 Aug 2020 17:00:00 +0930 Subject: [PATCH 06/18] Clean up reviewers_dropdown_options --- ee/app/helpers/ee/form_helper.rb | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/ee/app/helpers/ee/form_helper.rb b/ee/app/helpers/ee/form_helper.rb index c786a6e93ccda4..7a0c8853984d77 100644 --- a/ee/app/helpers/ee/form_helper.rb +++ b/ee/app/helpers/ee/form_helper.rb @@ -13,7 +13,7 @@ def merge_request_supports_multiple_assignees? def reviewers_dropdown_options(issuable_type) dropdown_data = { toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', - title: 'Select reviewer', + title: 'Select reviewer(s)', filter: true, dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-reviewer', placeholder: _('Search users'), @@ -23,36 +23,13 @@ def reviewers_dropdown_options(issuable_type) current_user: true, project_id: (@target_project || @project)&.id, field_name: "#{issuable_type}[reviewer_ids][]", default_label: 'Unassigned', - 'max-select': 1, - 'dropdown-header': 'Reviewer', + 'dropdown-header': 'Reviewer(s)', multi_select: true, 'input-meta': 'name', 'always-show-selectbox': true, current_user_info: UserSerializer.new.represent(current_user) } } - - type = issuable_type.to_s - - if type == 'merge_request' && merge_request_supports_reviewers? - dropdown_data = multiple_reviewers_dropdown_options(dropdown_data) - end - - dropdown_data - end - - def multiple_reviewers_dropdown_options(options) - new_options = options.dup - - new_options[:title] = 'Select reviewer(s)' - new_options[:data][:'dropdown-header'] = 'Reviewer(s)' - new_options[:data].delete(:'max-select') - - new_options - end - - def merge_request_supports_reviewers? - @merge_request&.allows_reviewers? end end end -- GitLab From d9dc5a1dd8eb45c82ae39e2dcfdfa668dcbcfb70 Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 31 Aug 2020 17:27:36 +0930 Subject: [PATCH 07/18] Make merge request reviewers beta feature while we're working on it --- ee/app/models/ee/merge_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 225fa7477674ab..962dac60365129 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -127,7 +127,7 @@ def allows_multiple_assignees? end def allows_reviewers? - project.feature_available?(:merge_request_reviewers) + project.beta_feature_available?(:merge_request_reviewers) end def visible_blocking_merge_requests(user) -- GitLab From 0aeb881cc48dedc6dc1826a48442d58352573f45 Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 31 Aug 2020 19:55:28 +0930 Subject: [PATCH 08/18] Expose reviewers only when the reviewer feature is enabled --- .../merge_request_sidebar_extras_entity.rb | 2 +- ...erge_request_sidebar_extras_entity_spec.rb | 51 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) create mode 100644 spec/serializers/merge_request_sidebar_extras_entity_spec.rb diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb index c46e4dc13a2075..d8aa1613e43948 100644 --- a/app/serializers/merge_request_sidebar_extras_entity.rb +++ b/app/serializers/merge_request_sidebar_extras_entity.rb @@ -5,7 +5,7 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request) end - expose :reviewers do |merge_request| + expose :reviewers, if: -> (m) { m.project.beta_feature_available?(:merge_request_reviewers) } do |merge_request| MergeRequestReviewerEntity.represent(merge_request.reviewers, merge_request: merge_request) end end diff --git a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb new file mode 100644 index 00000000000000..b4882d42955068 --- /dev/null +++ b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequestSidebarExtrasEntity do + let_it_be(:assignee, reload: true) { create(:user) } + let_it_be(:reviewer, reload: true) { create(:user) } + + let(:user) { create(:user) } + let(:project) { create :project, :repository } + let(:merge_request) do + create(:merge_request, source_project: project, + target_project: project, + assignees: [assignee], + reviewers: [reviewer]) + end + + let(:request) { double('request', current_user: user, project: project) } + + let(:entity) { described_class.new(merge_request, request: request).as_json } + + describe '#assignees' do + it 'contains assignees attributes' do + expect(entity[:assignees].count).to be 1 + expect(entity[:assignees].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url, :can_merge + ) + end + end + + describe '#reviewers' do + context 'when merge_request_reviewers feature is disabled' do + it 'does not contain assignees attributes' do + stub_licensed_features(merge_request_reviewers: false) + + expect(entity[:reviewers]).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + it 'does not include code navigation properties' do + stub_licensed_features(merge_request_reviewers: true) + + expect(entity[:reviewers].count).to be 1 + expect(entity[:reviewers].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url, :can_merge + ) + end + end + end +end -- GitLab From e8c33b2b19ebd533f7726e1d4841d53be2a141c4 Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 31 Aug 2020 20:09:03 +0930 Subject: [PATCH 09/18] Add specs for allows_reviewers? --- ee/spec/models/merge_request_spec.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 0067f09ca6fe24..87a06e75f2c4d7 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -51,6 +51,24 @@ end end + describe '#allows_reviewers?' do + it 'returns false without merge_request_reviewers license' do + stub_licensed_features(merge_request_reviewers: false) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(false) + end + + it 'returns true with merge_request_reviewers license' do + stub_licensed_features(merge_request_reviewers: true) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(true) + end + end + describe '#participants' do context 'with approval rule' do before do -- GitLab From d51ca5b7b6dcb2f1720924dccc5e6975a89dd7f3 Mon Sep 17 00:00:00 2001 From: David Kim Date: Wed, 2 Sep 2020 14:52:02 +0930 Subject: [PATCH 10/18] Move reviewers feature to Core --- app/helpers/form_helper.rb | 22 +++++++++++++++++++ app/models/merge_request.rb | 4 ++++ .../merge_request_sidebar_extras_entity.rb | 2 +- app/services/issuable_base_service.rb | 6 ----- app/services/merge_requests/base_service.rb | 20 +++++++++++++++++ ee/app/models/ee/merge_request.rb | 4 ---- ee/app/services/ee/issuable_base_service.rb | 18 --------------- ee/spec/models/merge_request_spec.rb | 18 --------------- spec/models/issue_spec.rb | 10 +++++++++ spec/models/merge_request_spec.rb | 18 +++++++++++++++ 10 files changed, 75 insertions(+), 47 deletions(-) diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 67bfeb22d92d6a..2ae69393fb3a54 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -55,6 +55,28 @@ def assignees_dropdown_options(issuable_type) dropdown_data end + def reviewers_dropdown_options(issuable_type) + { + toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', + title: 'Select reviewer(s)', + filter: true, + dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-reviewer', + placeholder: _('Search users'), + data: { + first_user: current_user&.username, + null_user: true, + current_user: true, + project_id: (@target_project || @project)&.id, field_name: "#{issuable_type}[reviewer_ids][]", + default_label: 'Unassigned', + 'dropdown-header': 'Reviewer(s)', + multi_select: true, + 'input-meta': 'name', + 'always-show-selectbox': true, + current_user_info: UserSerializer.new.represent(current_user) + } + } + end + # Overwritten def issue_supports_multiple_assignees? false diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 9588c678a3a7cc..2eb606b8cddc9b 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1633,6 +1633,10 @@ def ensure_metrics end end + def allows_reviewers? + Feature.enabled?(:merge_request_reviewers, project) + end + private def with_rebase_lock diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb index d8aa1613e43948..b883d81410c454 100644 --- a/app/serializers/merge_request_sidebar_extras_entity.rb +++ b/app/serializers/merge_request_sidebar_extras_entity.rb @@ -5,7 +5,7 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request) end - expose :reviewers, if: -> (m) { m.project.beta_feature_available?(:merge_request_reviewers) } do |merge_request| + expose :reviewers, if: -> (m) { merge_request.allows_reviewers? } do |merge_request| MergeRequestReviewerEntity.represent(merge_request.reviewers, merge_request: merge_request) end end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 87cd8fd2667011..35e91328a4170e 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -35,7 +35,6 @@ def filter_params(issuable) end filter_assignee(issuable) - filter_reviewer(issuable) filter_milestone filter_labels end @@ -58,11 +57,6 @@ def filter_assignee(issuable) end end - def filter_reviewer(issuable) - # Reviewer is only available for EE - params.delete(:reviewer_ids) - end - def user_can_read?(issuable, user_id) new_user = User.find_by_id(user_id) diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index 7e301f311e9864..ecddda18fe95ff 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -87,6 +87,26 @@ def filter_params(merge_request) unless merge_request.can_allow_collaboration?(current_user) params.delete(:allow_collaboration) end + + filter_reviewer + end + + def filter_reviewer(merge_request) + return if params[:reviewer_ids].blank? + + unless can_admin_issuable?(merge_request) && merge_request.allows_reviewers? + params.delete(:reviewer_ids) + end + + reviewer_ids = params[:reviewer_ids].select { |reviewer_id| user_can_read?(merge_request, reviewer_id) } + + if params[:reviewer_ids].map(&:to_s) == [IssuableFinder::Params::NONE] + params[:reviewer_ids] = [] + elsif reviewer_ids.any? + params[:reviewer_ids] = reviewer_ids + else + params.delete(:reviewer_ids) + end end def merge_request_metrics_service(merge_request) diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 70b34ca266511e..7f56b9af51f61b 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -126,10 +126,6 @@ def allows_multiple_assignees? project.feature_available?(:multiple_merge_request_assignees) end - def allows_reviewers? - project.beta_feature_available?(:merge_request_reviewers) - end - def visible_blocking_merge_requests(user) Ability.merge_requests_readable_by_user(blocking_merge_requests, user) end diff --git a/ee/app/services/ee/issuable_base_service.rb b/ee/app/services/ee/issuable_base_service.rb index 6f6f22085c3975..93e29849b540d2 100644 --- a/ee/app/services/ee/issuable_base_service.rb +++ b/ee/app/services/ee/issuable_base_service.rb @@ -24,24 +24,6 @@ def filter_params(issuable) super end - def filter_reviewer(issuable) - return if params[:reviewer_ids].blank? - - unless can_admin_issuable?(issuable) && issuable.allows_reviewers? - params.delete(:reviewer_ids) - end - - reviewer_ids = params[:reviewer_ids].select { |reviewer_id| user_can_read?(issuable, reviewer_id) } - - if params[:reviewer_ids].map(&:to_s) == [IssuableFinder::Params::NONE] - params[:reviewer_ids] = [] - elsif reviewer_ids.any? - params[:reviewer_ids] = reviewer_ids - else - params.delete(:reviewer_ids) - end - end - override :filter_labels def filter_labels super diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 62ffc2488a7d35..cd0af978ee282d 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -51,24 +51,6 @@ end end - describe '#allows_reviewers?' do - it 'returns false without merge_request_reviewers license' do - stub_licensed_features(merge_request_reviewers: false) - - merge_request = build_stubbed(:merge_request) - - expect(merge_request.allows_reviewers?).to be(false) - end - - it 'returns true with merge_request_reviewers license' do - stub_licensed_features(merge_request_reviewers: true) - - merge_request = build_stubbed(:merge_request) - - expect(merge_request.allows_reviewers?).to be(true) - end - end - describe '#participants' do context 'with approval rule' do before do diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index ff6d448561d9ba..6ff3c34bd325f4 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -1211,4 +1211,14 @@ end end end + + describe '#allows_reviewers?' do + it 'returns false as issues do not support reviewers feature' do + stub_feature_flags(merge_request_reviewers: true) + + issue = build_stubbed(:issue) + + expect(issue.allows_reviewers?).to be(false) + end + end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 80612264a38e50..4da01aabd6c36a 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -4112,4 +4112,22 @@ def create_build(pipeline, coverage, name) expect(context[:label_url_method]).to eq(:project_merge_requests_url) end end + + describe '#allows_reviewers?' do + it 'returns false without merge_request_reviewers feature' do + stub_feature_flags(merge_request_reviewers: false) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(false) + end + + it 'returns true with merge_request_reviewers feature' do + stub_feature_flags(merge_request_reviewers: true) + + merge_request = build_stubbed(:merge_request) + + expect(merge_request.allows_reviewers?).to be(true) + end + end end -- GitLab From 1fa5f4434a740aa492778f0a527de490692251bd Mon Sep 17 00:00:00 2001 From: David Kim Date: Wed, 2 Sep 2020 18:27:08 +0930 Subject: [PATCH 11/18] Fix failing specs and some more service specs --- .../merge_request_sidebar_extras_entity.rb | 2 +- app/services/merge_requests/base_service.rb | 4 +- ...erge_request_sidebar_extras_entity_spec.rb | 4 +- .../merge_requests/create_service_spec.rb | 4 ++ .../merge_requests/update_service_spec.rb | 23 +++++++ .../services/merge_request_shared_examples.rb | 62 +++++++++++++++++++ 6 files changed, 95 insertions(+), 4 deletions(-) create mode 100644 spec/support/shared_examples/services/merge_request_shared_examples.rb diff --git a/app/serializers/merge_request_sidebar_extras_entity.rb b/app/serializers/merge_request_sidebar_extras_entity.rb index b883d81410c454..9db8e52abef629 100644 --- a/app/serializers/merge_request_sidebar_extras_entity.rb +++ b/app/serializers/merge_request_sidebar_extras_entity.rb @@ -5,7 +5,7 @@ class MergeRequestSidebarExtrasEntity < IssuableSidebarExtrasEntity MergeRequestAssigneeEntity.represent(merge_request.assignees, merge_request: merge_request) end - expose :reviewers, if: -> (m) { merge_request.allows_reviewers? } do |merge_request| + expose :reviewers, if: -> (m) { m.allows_reviewers? } do |merge_request| MergeRequestReviewerEntity.represent(merge_request.reviewers, merge_request: merge_request) end end diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb index ecddda18fe95ff..d38f62bca9036b 100644 --- a/app/services/merge_requests/base_service.rb +++ b/app/services/merge_requests/base_service.rb @@ -88,7 +88,7 @@ def filter_params(merge_request) params.delete(:allow_collaboration) end - filter_reviewer + filter_reviewer(merge_request) end def filter_reviewer(merge_request) @@ -96,6 +96,8 @@ def filter_reviewer(merge_request) unless can_admin_issuable?(merge_request) && merge_request.allows_reviewers? params.delete(:reviewer_ids) + + return end reviewer_ids = params[:reviewer_ids].select { |reviewer_id| user_can_read?(merge_request, reviewer_id) } diff --git a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb index b4882d42955068..8b7adf091d1928 100644 --- a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb +++ b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb @@ -31,7 +31,7 @@ describe '#reviewers' do context 'when merge_request_reviewers feature is disabled' do it 'does not contain assignees attributes' do - stub_licensed_features(merge_request_reviewers: false) + stub_feature_flags(merge_request_reviewers: false) expect(entity[:reviewers]).to be_nil end @@ -39,7 +39,7 @@ context 'when merge_request_reviewers feature is enabled' do it 'does not include code navigation properties' do - stub_licensed_features(merge_request_reviewers: true) + stub_feature_flags(merge_request_reviewers: true) expect(entity[:reviewers].count).to be 1 expect(entity[:reviewers].first.keys).to include( diff --git a/spec/services/merge_requests/create_service_spec.rb b/spec/services/merge_requests/create_service_spec.rb index bb62e594e7aae9..196ba7bdabf095 100644 --- a/spec/services/merge_requests/create_service_spec.rb +++ b/spec/services/merge_requests/create_service_spec.rb @@ -338,6 +338,10 @@ end end end + + it_behaves_like 'reviewer_ids filter' do + let(:execute) { service.execute } + end end it_behaves_like 'issuable record that supports quick actions' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index c3433c8c9d2970..10dd9a403cacc6 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -161,6 +161,29 @@ def update_merge_request(opts) expect(@merge_request.merge_params["force_remove_source_branch"]).to eq("1") end end + + it_behaves_like 'reviewer_ids filter' do + let(:opts) { {} } + let(:execute) { update_merge_request(opts) } + end + + context 'with an existing reviewer' do + let(:merge_request) do + create(:merge_request, :simple, source_project: project, reviewer_ids: [user2.id]) + end + + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + let(:opts) { { reviewer_ids: [IssuableFinder::Params::NONE] } } + + it 'removes reviewers' do + expect(update_merge_request(opts).reviewers).to eq [] + end + end + end end context 'after_save callback to store_mentions' do diff --git a/spec/support/shared_examples/services/merge_request_shared_examples.rb b/spec/support/shared_examples/services/merge_request_shared_examples.rb new file mode 100644 index 00000000000000..a703264021705a --- /dev/null +++ b/spec/support/shared_examples/services/merge_request_shared_examples.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'reviewer_ids filter' do + context 'filter_reviewer' do + let(:opts) { super().merge(reviewer_ids_param) } + + context 'without reviewer_ids' do + let(:reviewer_ids_param) { {} } + + it 'contains no reviewer_ids' do + expect(execute.reviewers).to eq [] + end + end + + context 'with reviewer_ids' do + let(:reviewer_ids_param) { { reviewer_ids: [reviewer1.id, reviewer2.id, reviewer3.id] } } + + let(:reviewer1) { create(:user) } + let(:reviewer2) { create(:user) } + let(:reviewer3) { create(:user) } + + context 'when the current user can admin the merge_request' do + context 'when merge_request_reviewer feature is enabled' do + before do + stub_feature_flags(merge_request_reviewer: true) + end + + context 'with reviewers who can read the merge_request' do + before do + project.add_developer(reviewer1) + project.add_developer(reviewer2) + end + + it 'contains reviewers who can read the merge_request' do + expect(execute.reviewers).to contain_exactly(reviewer1, reviewer2) + end + end + end + + context 'when merge_request_reviewer feature is disabled' do + before do + stub_feature_flags(merge_request_reviewer: false) + end + + it 'contains no reviewers' do + expect(execute.reviewers).to eq [] + end + end + end + + context 'when the current_user cannot admin the merge_request' do + before do + project.add_developer(user) + end + + it 'contains no reviewers' do + expect(execute.reviewers).to eq [] + end + end + end + end +end -- GitLab From b5f7994c984e2e8d77397f76d6ed1caf8e75b0d1 Mon Sep 17 00:00:00 2001 From: David Kim Date: Wed, 2 Sep 2020 18:40:14 +0930 Subject: [PATCH 12/18] Remove duplicate entry from EE helper --- ee/app/helpers/ee/form_helper.rb | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/ee/app/helpers/ee/form_helper.rb b/ee/app/helpers/ee/form_helper.rb index 7a0c8853984d77..ba0ec340d1593a 100644 --- a/ee/app/helpers/ee/form_helper.rb +++ b/ee/app/helpers/ee/form_helper.rb @@ -9,27 +9,5 @@ def issue_supports_multiple_assignees? def merge_request_supports_multiple_assignees? @merge_request&.allows_multiple_assignees? end - - def reviewers_dropdown_options(issuable_type) - dropdown_data = { - toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', - title: 'Select reviewer(s)', - filter: true, - dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-reviewer', - placeholder: _('Search users'), - data: { - first_user: current_user&.username, - null_user: true, - current_user: true, - project_id: (@target_project || @project)&.id, field_name: "#{issuable_type}[reviewer_ids][]", - default_label: 'Unassigned', - 'dropdown-header': 'Reviewer(s)', - multi_select: true, - 'input-meta': 'name', - 'always-show-selectbox': true, - current_user_info: UserSerializer.new.represent(current_user) - } - } - end end end -- GitLab From 45d310ed70b294c6b1bf6a9c8f3c8a5832b9cfaa Mon Sep 17 00:00:00 2001 From: David Kim Date: Thu, 3 Sep 2020 12:05:12 +0930 Subject: [PATCH 13/18] Address comments and add more specs --- app/serializers/merge_request_basic_entity.rb | 2 +- app/services/issuable_base_service.rb | 6 ++-- .../merge_request_basic_entity_spec.rb | 27 +++++++++++++++- ...erge_request_sidebar_extras_entity_spec.rb | 32 +++++++++++-------- 4 files changed, 49 insertions(+), 18 deletions(-) diff --git a/app/serializers/merge_request_basic_entity.rb b/app/serializers/merge_request_basic_entity.rb index 0fa447f07cce76..ef1177e9967f9c 100644 --- a/app/serializers/merge_request_basic_entity.rb +++ b/app/serializers/merge_request_basic_entity.rb @@ -9,7 +9,7 @@ class MergeRequestBasicEntity < Grape::Entity expose :milestone, using: API::Entities::Milestone expose :labels, using: LabelEntity expose :assignees, using: API::Entities::UserBasic - expose :reviewers, using: API::Entities::UserBasic + expose :reviewers, if: -> (m) { m.allows_reviewers? }, using: API::Entities::UserBasic expose :task_status, :task_status_short expose :lock_version, :lock_version end diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 35e91328a4170e..51c161a5e6046f 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -58,14 +58,14 @@ def filter_assignee(issuable) end def user_can_read?(issuable, user_id) - new_user = User.find_by_id(user_id) + user = User.find_by_id(user_id) - return false unless new_user + return false unless user ability_name = :"read_#{issuable.to_ability_name}" resource = issuable.persisted? ? issuable : project - can?(new_user, ability_name, resource) + can?(user, ability_name, resource) end def filter_milestone diff --git a/spec/serializers/merge_request_basic_entity_spec.rb b/spec/serializers/merge_request_basic_entity_spec.rb index 1cddd87e91732c..4a8bcd72d9cc80 100644 --- a/spec/serializers/merge_request_basic_entity_spec.rb +++ b/spec/serializers/merge_request_basic_entity_spec.rb @@ -3,7 +3,8 @@ require 'spec_helper' RSpec.describe MergeRequestBasicEntity do - let(:resource) { build(:merge_request) } + let(:resource) { build(:merge_request, params) } + let(:params) { {} } subject do described_class.new(resource).as_json @@ -14,4 +15,28 @@ expect(subject[:merge_status]).to eq 'checking' end + + describe '#reviewers' do + let(:params) { { reviewers: [reviewer] } } + let(:reviewer) { build(:user) } + + context 'when merge_request_reviewers feature is disabled' do + it 'does not contain assignees attributes' do + stub_feature_flags(merge_request_reviewers: false) + + expect(subject[:reviewers]).to be_nil + end + end + + context 'when merge_request_reviewers feature is enabled' do + it 'contains reviewers attributes' do + stub_feature_flags(merge_request_reviewers: true) + + expect(subject[:reviewers].count).to be 1 + expect(subject[:reviewers].first.keys).to include( + :id, :name, :username, :state, :avatar_url, :web_url + ) + end + end + end end diff --git a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb index 8b7adf091d1928..74e9956c8a0305 100644 --- a/spec/serializers/merge_request_sidebar_extras_entity_spec.rb +++ b/spec/serializers/merge_request_sidebar_extras_entity_spec.rb @@ -3,21 +3,27 @@ require 'spec_helper' RSpec.describe MergeRequestSidebarExtrasEntity do - let_it_be(:assignee, reload: true) { create(:user) } - let_it_be(:reviewer, reload: true) { create(:user) } - - let(:user) { create(:user) } - let(:project) { create :project, :repository } - let(:merge_request) do - create(:merge_request, source_project: project, - target_project: project, - assignees: [assignee], - reviewers: [reviewer]) + let_it_be(:assignee) { build(:user) } + let_it_be(:reviewer) { build(:user) } + let_it_be(:user) { build(:user) } + let_it_be(:project) { create :project, :repository } + + let(:params) do + { + source_project: project, + target_project: project, + assignees: [assignee], + reviewers: [reviewer] + } + end + + let(:resource) do + build(:merge_request, params) end let(:request) { double('request', current_user: user, project: project) } - let(:entity) { described_class.new(merge_request, request: request).as_json } + let(:entity) { described_class.new(resource, request: request).as_json } describe '#assignees' do it 'contains assignees attributes' do @@ -30,7 +36,7 @@ describe '#reviewers' do context 'when merge_request_reviewers feature is disabled' do - it 'does not contain assignees attributes' do + it 'does not contain reviewers attributes' do stub_feature_flags(merge_request_reviewers: false) expect(entity[:reviewers]).to be_nil @@ -38,7 +44,7 @@ end context 'when merge_request_reviewers feature is enabled' do - it 'does not include code navigation properties' do + it 'contains reviewers attributes' do stub_feature_flags(merge_request_reviewers: true) expect(entity[:reviewers].count).to be 1 -- GitLab From c135dfa76aacad6a1a2719c0d41ea3e03d6eacfe Mon Sep 17 00:00:00 2001 From: David Kim Date: Thu, 3 Sep 2020 14:16:10 +0930 Subject: [PATCH 14/18] Remove helper method change to move it to the frontend MR --- app/helpers/form_helper.rb | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/app/helpers/form_helper.rb b/app/helpers/form_helper.rb index 2ae69393fb3a54..67bfeb22d92d6a 100644 --- a/app/helpers/form_helper.rb +++ b/app/helpers/form_helper.rb @@ -55,28 +55,6 @@ def assignees_dropdown_options(issuable_type) dropdown_data end - def reviewers_dropdown_options(issuable_type) - { - toggle_class: 'js-user-search js-assignee-search js-multiselect js-save-user-data', - title: 'Select reviewer(s)', - filter: true, - dropdown_class: 'dropdown-menu-user dropdown-menu-selectable dropdown-menu-reviewer', - placeholder: _('Search users'), - data: { - first_user: current_user&.username, - null_user: true, - current_user: true, - project_id: (@target_project || @project)&.id, field_name: "#{issuable_type}[reviewer_ids][]", - default_label: 'Unassigned', - 'dropdown-header': 'Reviewer(s)', - multi_select: true, - 'input-meta': 'name', - 'always-show-selectbox': true, - current_user_info: UserSerializer.new.represent(current_user) - } - } - end - # Overwritten def issue_supports_multiple_assignees? false -- GitLab From 320abd99f715d71a126f1976ac822a5f95240a74 Mon Sep 17 00:00:00 2001 From: David Kim Date: Fri, 4 Sep 2020 12:52:44 +0930 Subject: [PATCH 15/18] Add feature flag config --- .../feature_flags/development/merge-request-reviewers.yml | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 config/feature_flags/development/merge-request-reviewers.yml diff --git a/config/feature_flags/development/merge-request-reviewers.yml b/config/feature_flags/development/merge-request-reviewers.yml new file mode 100644 index 00000000000000..6669395c04ad29 --- /dev/null +++ b/config/feature_flags/development/merge-request-reviewers.yml @@ -0,0 +1,7 @@ +--- +name: merge-request-reviewers +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40488 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245190 +group: group::source code +type: development +default_enabled: false -- GitLab From 253cfaee2e8ae9758555a0a8d03b342108199b3a Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 4 Sep 2020 11:30:12 +0000 Subject: [PATCH 16/18] Apply 1 suggestion(s) to 1 file(s) --- config/feature_flags/development/merge-request-reviewers.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/merge-request-reviewers.yml b/config/feature_flags/development/merge-request-reviewers.yml index 6669395c04ad29..2180662b9df3d5 100644 --- a/config/feature_flags/development/merge-request-reviewers.yml +++ b/config/feature_flags/development/merge-request-reviewers.yml @@ -1,5 +1,5 @@ --- -name: merge-request-reviewers +name: merge_request_reviewers introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40488 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/245190 group: group::source code -- GitLab From ec6195ff5b902246fa0800d8acf0c10133c59ab5 Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 7 Sep 2020 13:05:36 +0930 Subject: [PATCH 17/18] Rename FF config file name to match the correct FF name --- .../{merge-request-reviewers.yml => merge_request_reviewers.yml} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename config/feature_flags/development/{merge-request-reviewers.yml => merge_request_reviewers.yml} (100%) diff --git a/config/feature_flags/development/merge-request-reviewers.yml b/config/feature_flags/development/merge_request_reviewers.yml similarity index 100% rename from config/feature_flags/development/merge-request-reviewers.yml rename to config/feature_flags/development/merge_request_reviewers.yml -- GitLab From 4e76ded663c90fdb253d73da8a744a1a614bda14 Mon Sep 17 00:00:00 2001 From: David Kim Date: Mon, 7 Sep 2020 16:05:35 +0930 Subject: [PATCH 18/18] Remove the license entry that was added incorrectly --- ee/app/models/license.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/ee/app/models/license.rb b/ee/app/models/license.rb index be476e2c103eee..4bd8f978cae4be 100644 --- a/ee/app/models/license.rb +++ b/ee/app/models/license.rb @@ -89,7 +89,6 @@ class License < ApplicationRecord ldap_group_sync_filter merge_pipelines merge_request_performance_metrics - merge_request_reviewers admin_merge_request_approvers_rules merge_trains metrics_reports -- GitLab