From 793c97ddc5747fd45cccfcca8d778c2a48b92eff Mon Sep 17 00:00:00 2001 From: David Kim Date: Tue, 25 Aug 2020 17:59:21 +0930 Subject: [PATCH 1/2] 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 5c2028bd87400e58178e77797d73676271c70d89 Mon Sep 17 00:00:00 2001 From: David Kim Date: Thu, 27 Aug 2020 17:08:31 +0930 Subject: [PATCH 2/2] 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