diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index eae6d1f87bfd98d19f668140dd4bb99cc6317036..3e427fbe2f8995c49cbc91ceb31416c8652d0ab3 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -390,3 +390,5 @@ def acting_user end end end + +NotificationRecipientService.prepend(EE::NotificationRecipientService) diff --git a/app/workers/new_note_worker.rb b/app/workers/new_note_worker.rb index 98f9f45e6085270b66c63c3d1f81f2c57e6068dd..6d5ed3ddbdd633ab436b71223261fce8d99ff432 100644 --- a/app/workers/new_note_worker.rb +++ b/app/workers/new_note_worker.rb @@ -23,3 +23,5 @@ def skip_notification?(note) end # rubocop: enable CodeReuse/ActiveRecord end + +NewNoteWorker.prepend(EE::NewNoteWorker) diff --git a/db/schema.rb b/db/schema.rb index 4ed00987fc2c522755291578c9771c955be93c29..d1e2a6bd53d9f58539f91e35af5f4c5e9629cf54 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1878,6 +1878,7 @@ t.integer "cached_markdown_version" t.text "change_position" t.boolean "resolved_by_push" + t.bigint "review_id" t.index ["author_id"], name: "index_notes_on_author_id", using: :btree t.index ["commit_id"], name: "index_notes_on_commit_id", using: :btree t.index ["created_at"], name: "index_notes_on_created_at", using: :btree @@ -1887,6 +1888,7 @@ t.index ["noteable_id", "noteable_type"], name: "index_notes_on_noteable_id_and_noteable_type", using: :btree t.index ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree t.index ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree + t.index ["review_id"], name: "index_notes_on_review_id", using: :btree end create_table "notification_settings", force: :cascade do |t| @@ -2556,6 +2558,16 @@ t.index ["user_id"], name: "index_resource_label_events_on_user_id", using: :btree end + create_table "reviews", id: :bigserial, force: :cascade do |t| + t.integer "author_id" + t.integer "merge_request_id", null: false + t.integer "project_id", null: false + t.datetime_with_timezone "created_at", null: false + t.index ["author_id"], name: "index_reviews_on_author_id", using: :btree + t.index ["merge_request_id"], name: "index_reviews_on_merge_request_id", using: :btree + t.index ["project_id"], name: "index_reviews_on_project_id", using: :btree + end + create_table "routes", force: :cascade do |t| t.integer "source_id", null: false t.string "source_type", null: false @@ -3296,6 +3308,7 @@ add_foreign_key "namespaces", "projects", column: "file_template_project_id", name: "fk_319256d87a", on_delete: :nullify add_foreign_key "note_diff_files", "notes", column: "diff_note_id", on_delete: :cascade add_foreign_key "notes", "projects", name: "fk_99e097b079", on_delete: :cascade + add_foreign_key "notes", "reviews", name: "fk_2e82291620", on_delete: :nullify add_foreign_key "notification_settings", "users", name: "fk_0c95e91db7", on_delete: :cascade add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id" add_foreign_key "operations_feature_flags", "projects", on_delete: :cascade @@ -3360,6 +3373,9 @@ add_foreign_key "resource_label_events", "labels", on_delete: :nullify add_foreign_key "resource_label_events", "merge_requests", on_delete: :cascade add_foreign_key "resource_label_events", "users", on_delete: :nullify + add_foreign_key "reviews", "merge_requests", on_delete: :cascade + add_foreign_key "reviews", "projects", on_delete: :cascade + add_foreign_key "reviews", "users", column: "author_id", on_delete: :nullify add_foreign_key "saml_providers", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "services", "projects", name: "fk_71cce407f9", on_delete: :cascade add_foreign_key "slack_integrations", "services", on_delete: :cascade diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md index 1708052e0845703978f5cfddb927091d9626b09d..5f273ca6848e83aef72cfb700e67b75aa4c074c3 100644 --- a/doc/user/discussions/index.md +++ b/doc/user/discussions/index.md @@ -337,6 +337,12 @@ bottom of the screen with two buttons: Alternatively, every pending comment has a button to finish the entire review. ![Review submission](img/review_preview.png) + +Submitting the review will send a single email to every notifiable user of the +merge request with all the comments associated to it. + +Replying to this email will, consequentially, create a new comment on the associated merge request. + ## Filtering notes > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/issues/26723) in GitLab 11.5. diff --git a/ee/app/controllers/projects/merge_requests/drafts_controller.rb b/ee/app/controllers/projects/merge_requests/drafts_controller.rb index d276740258ae860b435f015074a367bc7a67770a..0b233160167366a554ca91e748cc9061cb3ab2a5 100644 --- a/ee/app/controllers/projects/merge_requests/drafts_controller.rb +++ b/ee/app/controllers/projects/merge_requests/drafts_controller.rb @@ -41,9 +41,13 @@ def destroy end def publish - DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true)) + result = DraftNotes::PublishService.new(merge_request, current_user).execute(draft_note(allow_nil: true)) - head :ok + if result[:status] == :success + head :ok + else + render json: { message: result[:message] }, status: result[:status] + end end def discard diff --git a/ee/app/mailers/ee/notify.rb b/ee/app/mailers/ee/notify.rb index 36cac430e1948c4aabb7b6e7b7c9a67f79fd52e5..d72f1286f28d4aa2b492f7b390f16f103af8ba51 100644 --- a/ee/app/mailers/ee/notify.rb +++ b/ee/app/mailers/ee/notify.rb @@ -13,6 +13,7 @@ module Notify include ::Emails::CsvExport include ::Emails::ServiceDesk include ::Emails::Epics + include ::Emails::Reviews end attr_reader :group diff --git a/ee/app/mailers/emails/reviews.rb b/ee/app/mailers/emails/reviews.rb new file mode 100644 index 0000000000000000000000000000000000000000..21316a2d95a298d2ccd20a190d6c244fbefac761 --- /dev/null +++ b/ee/app/mailers/emails/reviews.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Emails + module Reviews + def new_review_email(recipient_id, review_id) + setup_review_email(review_id, recipient_id) + + mail_answer_thread(@merge_request, review_thread_options(recipient_id)) + end + + private + + def review_thread_options(recipient_id) + { + from: sender(@author.id), + to: recipient(recipient_id), + subject: subject("#{@merge_request.title} (#{@merge_request.to_reference})") + } + end + + def setup_review_email(review_id, recipient_id) + review = Review.find_by_id(review_id) + + @notes = review.notes + @author = review.author + @author_name = review.author_name + @project = review.project + @merge_request = review.merge_request + @target_url = project_merge_request_url(@project, @merge_request) + @sent_notification = SentNotification.record(@merge_request, recipient_id, reply_key) + end + end +end diff --git a/ee/app/models/draft_note.rb b/ee/app/models/draft_note.rb index 97c94fb8183526fb0f1dc166323225f8851cc3b4..3e240253fea16a7de126a96dee9d7e282e9b3b1f 100644 --- a/ee/app/models/draft_note.rb +++ b/ee/app/models/draft_note.rb @@ -13,6 +13,8 @@ class DraftNote < ActiveRecord::Base # Text with quick actions filtered out attr_accessor :rendered_note + attr_accessor :review + belongs_to :author, class_name: 'User' belongs_to :merge_request @@ -91,6 +93,7 @@ def publish_params attrs.concat(DIFF_ATTRS) if on_diff? params = slice(*attrs) params[:in_reply_to_discussion_id] = discussion_id if discussion_id.present? + params[:review_id] = review.id if review.present? params end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index 2a0c300cf9c349ebada65e0895b834804d0d96bd..3145c850a268a280ec478bdf9ebf7608f24dea45 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -11,6 +11,7 @@ module MergeRequest prepended do include Elastic::MergeRequestsSearch + has_many :reviews, inverse_of: :merge_request has_many :approvals, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent has_many :approved_by_users, through: :approvals, source: :user has_many :approvers, as: :target, dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent diff --git a/ee/app/models/ee/note.rb b/ee/app/models/ee/note.rb index bc262b394a9d2cadcb168d52308b56eb1d8afc34..4e3e4ad183629e2dc28ab56e74dba1062d926d27 100644 --- a/ee/app/models/ee/note.rb +++ b/ee/app/models/ee/note.rb @@ -9,6 +9,8 @@ module Note include ::ObjectStorage::BackgroundMove include Elastic::NotesSearch + belongs_to :review, inverse_of: :notes + scope :searchable, -> { where(system: false) } end diff --git a/ee/app/models/ee/project.rb b/ee/app/models/ee/project.rb index daaec91d7443846825582d593b5d305d6e453ed9..5274cb66f48c9fcf4fbc57e333cad0484c2de864 100644 --- a/ee/app/models/ee/project.rb +++ b/ee/app/models/ee/project.rb @@ -41,6 +41,7 @@ module Project has_one :gitlab_slack_application_service has_one :tracing_setting, class_name: 'ProjectTracingSetting' + has_many :reviews, inverse_of: :project has_many :approvers, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :approver_groups, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :audit_events, as: :entity diff --git a/ee/app/models/ee/user.rb b/ee/app/models/ee/user.rb index 431a7b9764cab8f7bd0d2db39030ae2f33289b96..41f9577f1bd7ff1b1c95a2050d7d364700f3e3ea 100644 --- a/ee/app/models/ee/user.rb +++ b/ee/app/models/ee/user.rb @@ -27,6 +27,7 @@ module User delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=, to: :namespace + has_many :reviews, foreign_key: :author_id, inverse_of: :author has_many :epics, foreign_key: :author_id has_many :assigned_epics, foreign_key: :assignee_id, class_name: "Epic" has_many :path_locks, dependent: :destroy # rubocop: disable Cop/ActiveRecordDependent diff --git a/ee/app/models/review.rb b/ee/app/models/review.rb new file mode 100644 index 0000000000000000000000000000000000000000..0d49236a26e5b853da2293892f37c02ad6fc98c1 --- /dev/null +++ b/ee/app/models/review.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class Review < ApplicationRecord + include Participable + include Mentionable + + belongs_to :author, class_name: 'User', foreign_key: :author_id, inverse_of: :reviews + belongs_to :merge_request, inverse_of: :reviews + belongs_to :project, inverse_of: :reviews + + has_many :notes, -> { order(:id) }, inverse_of: :review + + delegate :name, to: :author, prefix: true + + participant :author + + def all_references(current_user = nil, extractor: nil) + ext = super + + notes.each do |note| + note.all_references(current_user, extractor: ext) + end + + ext + end +end diff --git a/ee/app/services/draft_notes/base_service.rb b/ee/app/services/draft_notes/base_service.rb index 6012edd89566f49e2fc983e4d87d4d9e1acaab04..3cdfc8aa363b37cba29b8c5c041d5083f6256b0d 100644 --- a/ee/app/services/draft_notes/base_service.rb +++ b/ee/app/services/draft_notes/base_service.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module DraftNotes - class BaseService + class BaseService < ::BaseService attr_accessor :merge_request, :current_user, :params def initialize(merge_request, current_user, params = nil) diff --git a/ee/app/services/draft_notes/publish_service.rb b/ee/app/services/draft_notes/publish_service.rb index fc60ba50b3f4856f427ec171e5e46420646e67ad..0472cbda577a75f46b00cef9725c3f618719d9e8 100644 --- a/ee/app/services/draft_notes/publish_service.rb +++ b/ee/app/services/draft_notes/publish_service.rb @@ -8,6 +8,11 @@ def execute(draft = nil) else publish_draft_notes end + + success + rescue ActiveRecord::RecordInvalid => e + message = "Unable to save #{e.record.class.name}: #{e.record.errors.full_messages.join(", ")} " + error(message) end private @@ -20,7 +25,21 @@ def publish_draft_note(draft) end def publish_draft_notes - draft_notes.each(&method(:create_note_from_draft)) + return if draft_notes.empty? + + if Feature.enabled?(:batch_review_notification, project) + review = Review.create!(author: current_user, merge_request: merge_request, project: project) + + draft_notes.map do |draft_note| + draft_note.review = review + create_note_from_draft(draft_note) + end + + notification_service.async.new_review(review) + else + draft_notes.each(&method(:create_note_from_draft)) + end + draft_notes.delete_all MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(merge_request) @@ -33,6 +52,8 @@ def create_note_from_draft(draft) note = Notes::CreateService.new(draft.project, draft.author, draft.publish_params).execute set_discussion_resolve_status(note, draft) + + note end def set_discussion_resolve_status(note, draft_note) diff --git a/ee/app/services/ee/notification_recipient_service.rb b/ee/app/services/ee/notification_recipient_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..168d0e4718a8126b5bc1d95cf53b85d812e9b9a0 --- /dev/null +++ b/ee/app/services/ee/notification_recipient_service.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module EE + module NotificationRecipientService + extend ActiveSupport::Concern + + class_methods do + def build_new_review_recipients(*args) + NotificationRecipientService::Builder::NewReview.new(*args).notification_recipients + end + end + + prepended do + module Builder + class NewReview < ::NotificationRecipientService::Builder::Base + attr_reader :review + def initialize(review) + @review = review + end + + def target + review.merge_request + end + + def project + review.project + end + + def group + project.group + end + + def build! + add_participants(review.author) + add_mentions(review.author, target: review) + add_project_watchers + add_custom_notifications + add_subscribed_users + end + + # A new review is a batch of new notes + # therefore new_note subscribers should also + # receive incoming new reviews + def custom_action + :new_note + end + + def acting_user + review.author + end + end + end + end + end +end diff --git a/ee/app/services/ee/notification_service.rb b/ee/app/services/ee/notification_service.rb index 904fa01c09c1f2031500faa5d6f2dd718034c6b5..7b93c37587dd7b165d074a5c5d432feb9ca0d6a8 100644 --- a/ee/app/services/ee/notification_service.rb +++ b/ee/app/services/ee/notification_service.rb @@ -6,6 +6,11 @@ module EE module NotificationService extend ::Gitlab::Utils::Override + # Notify users on new review in system + def new_review(review) + send_new_review_notification(review) + end + # When we add approvers to a merge request we should send an email to: # # * the new approvers @@ -70,6 +75,14 @@ def prometheus_alerts_fired(project, alerts) private + def send_new_review_notification(review) + recipients = ::NotificationRecipientService.build_new_review_recipients(review) + + recipients.each do |recipient| + mailer.new_review_email(recipient.user.id, review.id).deliver_later + end + end + def add_mr_approvers_email(merge_request, approvers, current_user) approvers.each do |approver| mailer.add_merge_request_approver_email(approver.id, merge_request.id, current_user.id).deliver_later @@ -77,7 +90,7 @@ def add_mr_approvers_email(merge_request, approvers, current_user) end def approve_mr_email(merge_request, project, current_user) - recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: 'approve') + recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'approve') recipients.each do |recipient| mailer.approved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later @@ -85,7 +98,7 @@ def approve_mr_email(merge_request, project, current_user) end def unapprove_mr_email(merge_request, project, current_user) - recipients = NotificationRecipientService.build_recipients(merge_request, current_user, action: 'unapprove') + recipients = ::NotificationRecipientService.build_recipients(merge_request, current_user, action: 'unapprove') recipients.each do |recipient| mailer.unapproved_merge_request_email(recipient.user.id, merge_request.id, current_user.id).deliver_later @@ -110,7 +123,7 @@ def send_service_desk_notification(note) def epic_status_change_email(target, current_user, status) action = status == 'reopened' ? 'reopen' : 'close' - recipients = NotificationRecipientService.build_recipients( + recipients = ::NotificationRecipientService.build_recipients( target, current_user, action: action diff --git a/ee/app/services/ee/users/migrate_to_ghost_user_service.rb b/ee/app/services/ee/users/migrate_to_ghost_user_service.rb index 7ad44d3ef1abe5ebab63695f70477a4b83625cbe..05253b388d70cb5cfc747ef1e6f9ac6121b771a1 100644 --- a/ee/app/services/ee/users/migrate_to_ghost_user_service.rb +++ b/ee/app/services/ee/users/migrate_to_ghost_user_service.rb @@ -8,6 +8,7 @@ module MigrateToGhostUserService def migrate_records migrate_epics migrate_vulnerabilities_feedback + migrate_reviews super end @@ -21,6 +22,10 @@ def migrate_epics def migrate_vulnerabilities_feedback user.vulnerability_feedback.update_all(author_id: ghost_user.id) end + + def migrate_reviews + user.reviews.update_all(author_id: ghost_user.id) + end end end end diff --git a/ee/app/views/notify/new_review_email.html.haml b/ee/app/views/notify/new_review_email.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..181dafa3f3c7bad31a21bdc7d8b48940bf08a447 --- /dev/null +++ b/ee/app/views/notify/new_review_email.html.haml @@ -0,0 +1,17 @@ +%table{ border: "0", cellpadding:"0", cellspacing: "0", style: "width:100%;margin:0 auto;border-collapse:separate;border-spacing:0;" } + %tbody + %tr + %td{ style: "font-family:'Helvetica Neue',Helvetica,Arial,sans-serif;background-color:#ffffff;text-align:left;overflow:hidden;" } + %table{ border: "0", cellpadding: "0", cellspacing: "0", style: "width:100%;border-collapse:separate;border-spacing:0;" } + %tbody + %tr + %td{ style: "color:#333333;border-bottom:1px solid #ededed;font-size:15px;font-weight:bold;line-height:1.4;padding: 20px 0;" } + Merge request + = link_to(@merge_request.to_reference(@project), project_merge_request_url(@project, @merge_request)) + was reviewed by + = link_to(@author_name, user_url(@author)) + %tr + %td{ style: "overflow:hidden;font-size:14px;line-height:1.4;display:grid;" } + - @notes.each do |note| + - target_url = project_merge_request_url(@project, @merge_request, anchor: "note_#{note.id}") + = render 'note_email', note: note, diff_limit: 3, target_url: target_url, note_style: "border-bottom:1px solid #ededed;" diff --git a/ee/app/views/notify/new_review_email.text.erb b/ee/app/views/notify/new_review_email.text.erb new file mode 100644 index 0000000000000000000000000000000000000000..22789e6bb73585c237c855b76aff6495b4a514cd --- /dev/null +++ b/ee/app/views/notify/new_review_email.text.erb @@ -0,0 +1,10 @@ +<%= "Merge request #{merge_request_url(@merge_request)} was reviewed by #{@author_name}" %> + +-- +<% @notes.each_with_index do |note, index| %> + <%= render 'note_email', note: note, diff_limit: 3 %> + + <% if index != @notes.length-1 %> +-- + <% end %> +<% end %> diff --git a/ee/app/workers/ee/new_note_worker.rb b/ee/app/workers/ee/new_note_worker.rb new file mode 100644 index 0000000000000000000000000000000000000000..eeb408b053c0b6ee40b9695c45093ebdedffca83 --- /dev/null +++ b/ee/app/workers/ee/new_note_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module EE + module NewNoteWorker + extend ActiveSupport::Concern + + private + + # If a note belongs to a review + # We do not want to send a standalone + # notification + def skip_notification?(note) + note.review.present? + end + end +end diff --git a/ee/db/migrate/20181127130125_create_reviews.rb b/ee/db/migrate/20181127130125_create_reviews.rb new file mode 100644 index 0000000000000000000000000000000000000000..e033d5833154b30d6982beef1ddd3eea4e5f614f --- /dev/null +++ b/ee/db/migrate/20181127130125_create_reviews.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class CreateReviews < ActiveRecord::Migration[5.0] + DOWNTIME = false + + def up + create_table :reviews, id: :bigserial do |t| + t.references :author, index: true, references: :users + t.references :merge_request, index: true, null: false, foreign_key: { on_delete: :cascade } + t.references :project, index: true, null: false, foreign_key: { on_delete: :cascade } + t.datetime_with_timezone :created_at, null: false + end + + add_foreign_key :reviews, :users, column: :author_id, on_delete: :nullify # rubocop:disable Migration/AddConcurrentForeignKey + end + + def down + remove_foreign_key :reviews, column: :author_id + + drop_table :reviews + end +end diff --git a/ee/db/migrate/20181127133629_add_review_id_to_notes.rb b/ee/db/migrate/20181127133629_add_review_id_to_notes.rb new file mode 100644 index 0000000000000000000000000000000000000000..8c32756b56a61f2761326962010af88c124dc7d8 --- /dev/null +++ b/ee/db/migrate/20181127133629_add_review_id_to_notes.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddReviewIdToNotes < ActiveRecord::Migration[5.0] + DOWNTIME = false + + def change + add_column :notes, :review_id, :bigint + end +end diff --git a/ee/db/migrate/20181205093951_add_review_foreign_key_to_notes.rb b/ee/db/migrate/20181205093951_add_review_foreign_key_to_notes.rb new file mode 100644 index 0000000000000000000000000000000000000000..e80699cc788e4b3700b51ec32068ed876cc31b36 --- /dev/null +++ b/ee/db/migrate/20181205093951_add_review_foreign_key_to_notes.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class AddReviewForeignKeyToNotes < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_foreign_key(:notes, :reviews, column: :review_id, on_delete: :nullify) + add_concurrent_index :notes, :review_id + end + + def down + remove_foreign_key :notes, column: :review_id + remove_concurrent_index(:notes, :review_id) + end +end diff --git a/ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb b/ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb index 57e694612993d8ece51f112e9c7ed81e4e0ce808..d3b9a041515d4da9cffa8fc5bfed8c2418e08da6 100644 --- a/ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb +++ b/ee/spec/controllers/projects/merge_requests/drafts_controller_spec.rb @@ -193,6 +193,22 @@ def update_draft_note(overrides = {}) end end + context 'when PublishService errors' do + it 'returns message and 500 response' do + create(:draft_note, merge_request: merge_request, author: user) + error_message = "Something went wrong" + + expect_next_instance_of(DraftNotes::PublishService) do |service| + allow(service).to receive(:execute).and_return({ message: error_message, status: :error }) + end + + post :publish, params + + expect(response).to have_gitlab_http_status(:error) + expect(json_response["message"]).to include(error_message) + end + end + it 'publishes draft notes with position' do diff_refs = project.commit(RepoHelpers.sample_commit.id).try(:diff_refs) diff --git a/ee/spec/factories/reviews.rb b/ee/spec/factories/reviews.rb new file mode 100644 index 0000000000000000000000000000000000000000..7cf752f1cd91a5e15e3d829cd52ae488c36ff0f8 --- /dev/null +++ b/ee/spec/factories/reviews.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +FactoryBot.define do + factory :review do + merge_request + association :project, :repository + author factory: :user + end +end diff --git a/ee/spec/lib/gitlab/import_export/all_models.yml b/ee/spec/lib/gitlab/import_export/all_models.yml index d4df9b591ad148adc4160c73d3109793d2981c62..c812fe6ca94637bd4a59ac00bd7485bcf860efc2 100644 --- a/ee/spec/lib/gitlab/import_export/all_models.yml +++ b/ee/spec/lib/gitlab/import_export/all_models.yml @@ -5,6 +5,7 @@ issues: milestone: - boards merge_requests: +- reviews - approvals - approvers - approver_groups @@ -69,6 +70,7 @@ project: - packages - tracing_setting - webide_pipelines +- reviews prometheus_metrics: - project - prometheus_alerts @@ -82,3 +84,10 @@ epic_issues: - epic tracing_setting: - project +notes: +- review +reviews: +- project +- merge_request +- author +- notes diff --git a/ee/spec/lib/gitlab/import_export/safe_model_attributes.yml b/ee/spec/lib/gitlab/import_export/safe_model_attributes.yml index 42bb276064e6a3f53a174efc4aae1ffe681b7531..2a9cdbdc666cdbb6a6ab2d726f748825e613fa32 100644 --- a/ee/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/ee/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -1,3 +1,5 @@ --- ProjectTracingSetting: - external_url +Note: +- review_id diff --git a/ee/spec/mailers/notify_spec.rb b/ee/spec/mailers/notify_spec.rb index 8b48904d68f89fa516aea7ecf07d56588b8a9b7d..1a41107f7345382c15d7700fdca8b757a672b4a4 100644 --- a/ee/spec/mailers/notify_spec.rb +++ b/ee/spec/mailers/notify_spec.rb @@ -227,6 +227,47 @@ end end + describe 'merge request reviews' do + let(:review) { create(:review, project: project, merge_request: merge_request) } + let(:notes) { create_list(:notes, 3, review: review, project: project, author: review.author, noteable: merge_request) } + + subject { described_class.new_review_email(recipient.id, review.id) } + + it_behaves_like 'an answer to an existing thread with reply-by-email enabled' do + let(:model) { review.merge_request } + end + it_behaves_like 'it should show Gmail Actions View Merge request link' + it_behaves_like 'an unsubscribeable thread' + + it 'is sent to the given recipient as the author' do + sender = subject.header[:from].addrs[0] + + aggregate_failures do + expect(sender.display_name).to eq(review.author_name) + expect(sender.address).to eq(gitlab_sender) + expect(subject).to deliver_to(recipient.notification_email) + end + end + + it 'contains the message from the notes of the review' do + review.notes.each do |note| + is_expected.to have_body_text note.note + end + end + + it 'contains review author name' do + is_expected.to have_body_text review.author_name + end + + it 'has the correct subject and body' do + aggregate_failures do + is_expected.to have_subject "Re: #{project.name} | #{merge_request.title} (#{merge_request.to_reference})" + + is_expected.to have_body_text project_merge_request_path(project, merge_request) + end + end + end + describe 'mirror was hard failed' do let(:project) { create(:project, :mirror, :import_hard_failed) } diff --git a/ee/spec/models/ee/note_spec.rb b/ee/spec/models/ee/note_spec.rb index c4d0f12c405fa269f60388c1026f1c49a8011ad1..4bd8e86dd46ff42bb20272e83a626357ef7f0494 100644 --- a/ee/spec/models/ee/note_spec.rb +++ b/ee/spec/models/ee/note_spec.rb @@ -3,6 +3,10 @@ describe Note do include ::EE::GeoHelpers + describe 'associations' do + it { is_expected.to belong_to(:review).inverse_of(:notes) } + end + context 'object storage with background upload' do before do stub_uploads_object_storage(AttachmentUploader, background_upload: true) diff --git a/ee/spec/models/ee/user_spec.rb b/ee/spec/models/ee/user_spec.rb index b8da526ed966a1887c96d38db4d05c994110acfb..87992cff16899db2c0bb3b02ac3a2818a19a2789 100644 --- a/ee/spec/models/ee/user_spec.rb +++ b/ee/spec/models/ee/user_spec.rb @@ -4,6 +4,7 @@ describe 'associations' do subject { build(:user) } + it { is_expected.to have_many(:reviews) } it { is_expected.to have_many(:vulnerability_feedback) } end diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 92197898c5e6ee7391175db59b14eafc8b02cef8..412776be56246220536136dba743a3cb327195e7 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -8,6 +8,7 @@ subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) } describe 'associations' do + it { is_expected.to have_many(:reviews).inverse_of(:merge_request) } it { is_expected.to have_many(:approvals).dependent(:delete_all) } it { is_expected.to have_many(:approvers).dependent(:delete_all) } it { is_expected.to have_many(:approver_groups).dependent(:delete_all) } diff --git a/ee/spec/models/project_spec.rb b/ee/spec/models/project_spec.rb index 1757de33a20e67e866005a4ce965cf612c426231..9fdd11844083af9df3de06d24a2de049ba17675e 100644 --- a/ee/spec/models/project_spec.rb +++ b/ee/spec/models/project_spec.rb @@ -17,6 +17,7 @@ it { is_expected.to have_one(:import_state).class_name('ProjectImportState') } it { is_expected.to have_one(:repository_state).class_name('ProjectRepositoryState').inverse_of(:project) } + it { is_expected.to have_many(:reviews).inverse_of(:project) } it { is_expected.to have_many(:path_locks) } it { is_expected.to have_many(:vulnerability_feedback) } it { is_expected.to have_many(:sourced_pipelines) } diff --git a/ee/spec/models/review_spec.rb b/ee/spec/models/review_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9dd8b90feee3b6cf431e9ca6ddec63af24931228 --- /dev/null +++ b/ee/spec/models/review_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Review do + describe 'associations' do + it { is_expected.to belong_to(:author).class_name('User').with_foreign_key(:author_id).inverse_of(:reviews) } + it { is_expected.to belong_to(:merge_request).inverse_of(:reviews).touch(false) } + it { is_expected.to belong_to(:project).inverse_of(:reviews) } + + it { is_expected.to have_many(:notes).order(:id).inverse_of(:review) } + end + + describe 'modules' do + it { is_expected.to include_module(Participable) } + it { is_expected.to include_module(Mentionable) } + end + + describe '#all_references' do + it 'returns an extractor with the correct referenced users' do + user1 = create(:user, username: "foo") + user2 = create(:user, username: "bar") + review = create(:review) + project = review.project + author = review.author + + create(:note, review: review, project: project, author: author, note: "cc @foo @non_existent") + create(:note, review: review, project: project, author: author, note: "cc @bar") + + expect(review.all_references(author).users).to match_array([user1, user2]) + end + end + + describe '#participants' do + it 'includes the review author' do + project = create(:project, :public) + merge_request = create(:merge_request, source_project: project) + review = create(:review, project: project, merge_request: merge_request) + create(:note, review: review, noteable: merge_request, project: project, author: review.author) + + expect(review.participants).to include(review.author) + end + end +end diff --git a/ee/spec/services/draft_notes/publish_service_spec.rb b/ee/spec/services/draft_notes/publish_service_spec.rb index 8558f9f562c761a2e6dff94cb003df293b79efad..4b1d2d51dafb72a397d6f4a56cfdc5c6cee728fc 100644 --- a/ee/spec/services/draft_notes/publish_service_spec.rb +++ b/ee/spec/services/draft_notes/publish_service_spec.rb @@ -10,18 +10,84 @@ def publish(draft: nil) DraftNotes::PublishService.new(merge_request, user).execute(draft) end - it 'publishes a single draft note' do - drafts = create_list(:draft_note, 2, merge_request: merge_request, author: user) + context 'single draft note' do + let!(:drafts) { create_list(:draft_note, 2, merge_request: merge_request, author: user) } - expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) - expect(DraftNote.count).to eq(1) + it 'publishes' do + expect { publish(draft: drafts.first) }.to change { DraftNote.count }.by(-1).and change { Note.count }.by(1) + expect(DraftNote.count).to eq(1) + end + + it 'does not skip notification' do + expect(Notes::CreateService).to receive(:new).with(project, user, drafts.first.publish_params).and_call_original + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:new_note) + end + + result = publish(draft: drafts.first) + + expect(result[:status]).to eq(:success) + end end - it 'publishes all draft notes for a user in a merge request' do - create_list(:draft_note, 2, merge_request: merge_request, author: user) + context 'multiple draft notes' do + before do + create_list(:draft_note, 2, merge_request: merge_request, author: user) + end - expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2) - expect(DraftNote.count).to eq(0) + context 'when review fails to create' do + before do + expect_next_instance_of(Review) do |review| + allow(review).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(review)) + end + end + + it 'does not publish any draft note' do + expect { publish }.not_to change { DraftNote.count } + end + + it 'returns an error' do + result = publish + + expect(result[:status]).to eq(:error) + expect(result[:message]).to match(/Unable to save Review/) + end + end + + it 'returns success' do + result = publish + + expect(result[:status]).to eq(:success) + end + + it 'publishes all draft notes for a user in a merge request' do + expect { publish }.to change { DraftNote.count }.by(-2).and change { Note.count }.by(2).and change { Review.count }.by(1) + expect(DraftNote.count).to eq(0) + end + + context 'when batch_review_notification feature is enabled' do + it 'sends batch notification' do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive_message_chain(:async, :new_review).with(kind_of(Review)) + end + + publish + end + end + + context 'when batch_review_notification feature is disabled' do + it 'send a notification for each note' do + stub_feature_flags(batch_review_notification: false) + + 2.times do + expect_next_instance_of(NotificationService) do |notification_service| + expect(notification_service).to receive(:new_note).with(kind_of(Note)) + end + end + + publish + end + end end it 'only publishes the draft notes belonging to the current user' do @@ -54,8 +120,8 @@ def publish(draft: nil) end context 'with drafts that resolve discussions' do - let(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } - let(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) } + let!(:note) { create(:discussion_note_on_merge_request, noteable: merge_request, project: project) } + let!(:draft_note) { create(:draft_note, merge_request: merge_request, author: user, resolve_discussion: true, discussion_id: note.discussion.reply_id) } it 'resolves the discussion' do publish(draft: draft_note) diff --git a/ee/spec/services/ee/notification_recipient_service_spec.rb b/ee/spec/services/ee/notification_recipient_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..929fe3614e263c053a4e50c0564808a911a819e3 --- /dev/null +++ b/ee/spec/services/ee/notification_recipient_service_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe NotificationRecipientService do + subject(:service) { described_class } + + let(:project) { create(:project, :public) } + let(:other_projects) { create_list(:project, 5, :public) } + + describe '#build_new_review_recipients' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: merge_request.author) } + let(:notes) { create_list(:note_on_merge_request, 3, review: review, noteable: review.merge_request, project: review.project) } + + shared_examples 'no N+1 queries' do + it 'avoids N+1 queries', :request_store do + create_user + + service.build_new_review_recipients(review) + + control_count = ActiveRecord::QueryRecorder.new do + service.build_new_review_recipients(review) + end + + create_user + + expect { service.build_new_review_recipients(review) }.not_to exceed_query_limit(control_count) + end + end + + context 'when there are multiple watchers' do + def create_user + watcher = create(:user) + create(:notification_setting, source: project, user: watcher, level: :watch) + + other_projects.each do |other_project| + create(:notification_setting, source: other_project, user: watcher, level: :watch) + end + end + + include_examples 'no N+1 queries' + end + + context 'when there are multiple subscribers' do + def create_user + subscriber = create(:user) + merge_request.subscriptions.create(user: subscriber, project: project, subscribed: true) + end + + include_examples 'no N+1 queries' + + context 'when the project is private' do + before do + project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE) + end + + include_examples 'no N+1 queries' + end + end + end +end diff --git a/ee/spec/services/ee/notification_service_spec.rb b/ee/spec/services/ee/notification_service_spec.rb index 2fc64f4e408c72404b6b9da55f76241cbf5c1207..0e0a309f18ffca37a2860cb7e38721ca5877602f 100644 --- a/ee/spec/services/ee/notification_service_spec.rb +++ b/ee/spec/services/ee/notification_service_spec.rb @@ -121,6 +121,90 @@ def self.it_should_not_email! end end + context 'new review' do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + let(:reviewer) { create(:user) } + let(:merge_request) { create(:merge_request, source_project: project, assignee: user, author: create(:user)) } + let(:review) { create(:review, merge_request: merge_request, project: project, author: reviewer) } + let(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, author: reviewer, review: review) } + + before do + build_team(review.project, merge_request) + project.add_maintainer(merge_request.author) + project.add_maintainer(reviewer) + project.add_maintainer(merge_request.assignee) + + create(:diff_note_on_merge_request, + project: project, + noteable: merge_request, + author: reviewer, + review: review, + note: "cc @mention") + end + + it 'sends emails' do + expect(Notify).not_to receive(:new_review_email).with(review.author.id, review.id) + expect(Notify).not_to receive(:new_review_email).with(@unsubscriber.id, review.id) + expect(Notify).to receive(:new_review_email).with(merge_request.assignee.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(merge_request.author.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_watcher.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@u_mentioned.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@watcher_and_subscriber.id, review.id).and_call_original + expect(Notify).to receive(:new_review_email).with(@subscribed_participant.id, review.id).and_call_original + + subject.new_review(review) + end + + def build_team(project, merge_request) + @u_watcher = create_global_setting_for(create(:user), :watch) + @u_participating = create_global_setting_for(create(:user), :participating) + @u_participant_mentioned = create_global_setting_for(create(:user, username: 'participant'), :participating) + @u_disabled = create_global_setting_for(create(:user), :disabled) + @u_mentioned = create_global_setting_for(create(:user, username: 'mention'), :mention) + @u_committer = create(:user, username: 'committer') + @u_not_mentioned = create_global_setting_for(create(:user, username: 'regular'), :participating) + @u_outsider_mentioned = create(:user, username: 'outsider') + @u_custom_global = create_global_setting_for(create(:user, username: 'custom_global'), :custom) + + # subscribers + @subscriber = create :user + @unsubscriber = create :user + @subscribed_participant = create_global_setting_for(create(:user, username: 'subscribed_participant'), :participating) + @watcher_and_subscriber = create_global_setting_for(create(:user), :watch) + + # User to be participant by default + # This user does not contain any record in notification settings table + # It should be treated with a :participating notification_level + @u_lazy_participant = create(:user, username: 'lazy-participant') + + @u_guest_watcher = create_user_with_notification(:watch, 'guest_watching') + @u_guest_custom = create_user_with_notification(:custom, 'guest_custom') + + project.add_maintainer(@u_watcher) + project.add_maintainer(@u_participating) + project.add_maintainer(@u_participant_mentioned) + project.add_maintainer(@u_disabled) + project.add_maintainer(@u_mentioned) + project.add_maintainer(@u_committer) + project.add_maintainer(@u_not_mentioned) + project.add_maintainer(@u_lazy_participant) + project.add_maintainer(@u_custom_global) + project.add_maintainer(@subscriber) + project.add_maintainer(@unsubscriber) + project.add_maintainer(@subscribed_participant) + project.add_maintainer(@watcher_and_subscriber) + + merge_request.subscriptions.create(user: @unsubscribed_mentioned, subscribed: false) + merge_request.subscriptions.create(user: @subscriber, subscribed: true) + merge_request.subscriptions.create(user: @subscribed_participant, subscribed: true) + merge_request.subscriptions.create(user: @unsubscriber, subscribed: false) + # Make the watcher a subscriber to detect dupes + merge_request.subscriptions.create(user: @watcher_and_subscriber, subscribed: true) + end + end + describe 'mirror hard failed' do let(:user) { create(:user) } diff --git a/ee/spec/services/users/migrate_to_ghost_user_service_spec.rb b/ee/spec/services/users/migrate_to_ghost_user_service_spec.rb index 241862460a4e75e74f51054d9eff5050ac2077db..8194743a686558336707f29fd326bc95e17282f1 100644 --- a/ee/spec/services/users/migrate_to_ghost_user_service_spec.rb +++ b/ee/spec/services/users/migrate_to_ghost_user_service_spec.rb @@ -28,4 +28,13 @@ let(:created_record) { create(:vulnerability_feedback, author: user) } end end + + context 'reviews' do + let!(:user) { create(:user) } + let(:service) { described_class.new(user) } + + include_examples "migrating a deleted user's associated records to the ghost user", Review, [:author] do + let(:created_record) { create(:review, author: user) } + end + end end diff --git a/ee/spec/workers/new_note_worker_spec.rb b/ee/spec/workers/new_note_worker_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..1e8aa9065d67f811938b452bb47b5a868c1650b8 --- /dev/null +++ b/ee/spec/workers/new_note_worker_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe NewNoteWorker do + context 'when skip_notification' do + it 'does not create a new note notification' do + note = create(:note, :with_review) + + expect_any_instance_of(NotificationService).not_to receive(:new_note) + + subject.perform(note.id) + end + end +end diff --git a/spec/factories/notes.rb b/spec/factories/notes.rb index 2d1f48bf249ebc739191ffe34b582b86b3683a29..f2c9d6db3ccd6e3ef3fb527280e7f21ab181f0dd 100644 --- a/spec/factories/notes.rb +++ b/spec/factories/notes.rb @@ -122,6 +122,10 @@ system true end + trait :with_review do + review + end + trait :downvote do note "thumbsdown" end