From bebced8fc9039c5e8a3774af183a8dd9c244f297 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 6 Feb 2018 15:54:11 +0100 Subject: [PATCH 01/20] creating background job --- app/services/issues/base_service.rb | 6 ++++++ app/services/issues/create_service.rb | 2 ++ app/services/issues/update_service.rb | 7 +++++++ app/workers/all_queues.yml | 1 + app/workers/issue_due_worker.rb | 11 +++++++++++ 5 files changed, 27 insertions(+) create mode 100644 app/workers/issue_due_worker.rb diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 9f6cfc0f6d3..af25cf2c819 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -7,6 +7,12 @@ module Issues hook_data end + def schedule_due_date_email(issuable) + return if issuable.due_date.nil? + + IssueDueWorker.perform_at(issuable.due_date.to_time, issuable.id) + end + def reopen_service Issues::ReopenService end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 0307634c0b6..dc461c7c520 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -27,6 +27,8 @@ module Issues todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) + # TODO: Create the scheduled due date email + schedule_due_date_email(issuable) super end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index d7aa7e2347e..e8fdf9901c1 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -13,6 +13,10 @@ module Issues spam_check(issue, current_user) end + def after_update(issue) + schedule_due_date_email(issue) + end + def handle_changes(issue, options) old_associations = options.fetch(:old_associations, {}) old_labels = old_associations.fetch(:labels, []) @@ -23,6 +27,9 @@ module Issues todo_service.mark_pending_todos_as_done(issue, current_user) end + # TODO: If due date doesn't change, don't bother updating the due date + # email worker + if issue.previous_changes.include?('title') || issue.previous_changes.include?('description') todo_service.update_issue(issue, current_user, old_mentioned_users) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index f65e8385ac8..93ad55fdd70 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -78,6 +78,7 @@ - group_destroy - invalid_gpg_signature_update - irker +- issue_due - merge - namespaceless_project_destroy - new_issue diff --git a/app/workers/issue_due_worker.rb b/app/workers/issue_due_worker.rb new file mode 100644 index 00000000000..ba7f06ffc2f --- /dev/null +++ b/app/workers/issue_due_worker.rb @@ -0,0 +1,11 @@ +class IssueDueWorker + include ApplicationWorker + + def perform(issue_id) + issue = Issue.find_by_id(issue_id) + # How do we want to deal with noops? + if issue.due_date == Date.today + # execute + end + end +end From 5f39f9b30853291ec83df2383cd2e7863b72fb27 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 6 Feb 2018 19:09:59 +0100 Subject: [PATCH 02/20] Send email to recipients --- app/mailers/emails/issues.rb | 6 ++++++ app/services/notification_recipient_service.rb | 10 ++++++++-- app/services/notification_service.rb | 13 +++++++++++++ app/views/notify/issue_due_email.html.haml | 14 ++++++++++++++ app/views/notify/issue_due_email.text.erb | 8 ++++++++ app/workers/issue_due_worker.rb | 3 +-- 6 files changed, 50 insertions(+), 4 deletions(-) create mode 100644 app/views/notify/issue_due_email.html.haml create mode 100644 app/views/notify/issue_due_email.text.erb diff --git a/app/mailers/emails/issues.rb b/app/mailers/emails/issues.rb index b33131becd3..392cc0bee03 100644 --- a/app/mailers/emails/issues.rb +++ b/app/mailers/emails/issues.rb @@ -6,6 +6,12 @@ module Emails mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id, reason)) end + def issue_due_email(recipient_id, issue_id, reason = nil) + setup_issue_mail(issue_id, recipient_id) + + mail_new_thread(@issue, issue_thread_options(@issue.author_id, recipient_id, reason)) + end + def new_mention_in_issue_email(recipient_id, issue_id, updated_by_user_id, reason = nil) setup_issue_mail(issue_id, recipient_id) mail_answer_thread(@issue, issue_thread_options(updated_by_user_id, recipient_id, reason)) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index e4be953e810..c7c64356e30 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -230,14 +230,20 @@ module NotificationRecipientService add_subscribed_users - if [:new_issue, :new_merge_request].include?(custom_action) + if [:new_issue, :new_merge_request, :due_date_issue].include?(custom_action) # These will all be participants as well, but adding with the :mention # type ensures that users with the mention notification level will # receive them, too. add_mentions(current_user, target: target) # Add the assigned users, if any - assignees = custom_action == :new_issue ? target.assignees : target.assignee + assignees = case custom_action + when :new_issue, :due_date_issue + target.assignees + else + target.assignee + end + # We use the `:participating` notification level in order to match existing legacy behavior as captured # in existing specs (notification_service_spec.rb ~ line 507) add_recipients(assignees, :participating, NotificationReason::ASSIGNED) if assignees diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d7d2cde1004..aade82f5cf1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -363,6 +363,19 @@ class NotificationService end end + def issue_due_email(issue) + recipients = NotificationRecipientService.build_recipients( + issue, + issue.author, + action: "due_date", + skip_current_user: false, + ) + + recipients.each do |recipient| + mailer.send(:issue_due_email, recipient.user.id, issue.id, recipient.reason).deliver_later + end + end + protected def new_resource_email(target, method) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml new file mode 100644 index 00000000000..30ba53cf801 --- /dev/null +++ b/app/views/notify/issue_due_email.html.haml @@ -0,0 +1,14 @@ +- if Gitlab::CurrentSettings.email_author_in_body + %p.details + #{link_to @issue.author_name, user_url(@issue.author)} updated the issue's due date: + +- if @issue.assignees.any? + %p + Assignee: #{@issue.assignee_list} + +%p + This Issue has a new due date: #{ @issue.due_date } + +- if @issue.description + %div + = markdown(@issue.description, pipeline: :email, author: @issue.author) diff --git a/app/views/notify/issue_due_email.text.erb b/app/views/notify/issue_due_email.text.erb new file mode 100644 index 00000000000..1ac98bd9d93 --- /dev/null +++ b/app/views/notify/issue_due_email.text.erb @@ -0,0 +1,8 @@ +An Issue had its due date updated. + +Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> +Author: <%= @issue.author_name %> +Assignee: <%= @issue.assignee_list %> +New Due Date: <%= @issue.due_date %> + +<%= @issue.description %> diff --git a/app/workers/issue_due_worker.rb b/app/workers/issue_due_worker.rb index ba7f06ffc2f..ddd61d7b912 100644 --- a/app/workers/issue_due_worker.rb +++ b/app/workers/issue_due_worker.rb @@ -3,9 +3,8 @@ class IssueDueWorker def perform(issue_id) issue = Issue.find_by_id(issue_id) - # How do we want to deal with noops? if issue.due_date == Date.today - # execute + NotificationService.new.issue_due_email(issue) end end end From 247096eeb809e749289fa3544b1b006ba820a785 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 6 Feb 2018 19:19:45 +0100 Subject: [PATCH 03/20] Remove dead comment --- app/services/issues/create_service.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index dc461c7c520..a253020b9c3 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -27,7 +27,6 @@ module Issues todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) - # TODO: Create the scheduled due date email schedule_due_date_email(issuable) super From a67f848674e408eb8da73a11386e62d97bb2c73e Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 6 Feb 2018 23:06:45 +0100 Subject: [PATCH 04/20] Add new queue to sidekick_queues --- config/sidekiq_queues.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 554502c5d83..5c0bf091b18 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -48,6 +48,7 @@ - [authorized_projects, 1] - [expire_build_instance_artifacts, 1] - [group_destroy, 1] + - [issue_due, 1] - [irker, 1] - [namespaceless_project_destroy, 1] - [project_cache, 1] From ff830bc1e17e574927893305c33c6e31955cd35a Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 6 Feb 2018 23:07:22 +0100 Subject: [PATCH 05/20] Make linter happy --- app/services/notification_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index aade82f5cf1..f6d9020b9a1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -368,7 +368,7 @@ class NotificationService issue, issue.author, action: "due_date", - skip_current_user: false, + skip_current_user: false ) recipients.each do |recipient| From 9d81d5aa893b20ebedcbdafc7d23d651d4dbee89 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Sat, 10 Feb 2018 14:38:31 +0100 Subject: [PATCH 06/20] Use cron for sending emails --- app/services/issues/base_service.rb | 6 ------ app/services/issues/create_service.rb | 1 - app/services/issues/update_service.rb | 7 ------- app/workers/all_queues.yml | 2 +- app/workers/issue_due_worker.rb | 6 +++--- config/initializers/1_settings.rb | 4 ++++ config/sidekiq_queues.yml | 1 - 7 files changed, 8 insertions(+), 19 deletions(-) diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index af25cf2c819..9f6cfc0f6d3 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -7,12 +7,6 @@ module Issues hook_data end - def schedule_due_date_email(issuable) - return if issuable.due_date.nil? - - IssueDueWorker.perform_at(issuable.due_date.to_time, issuable.id) - end - def reopen_service Issues::ReopenService end diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index a253020b9c3..0307634c0b6 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -27,7 +27,6 @@ module Issues todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) - schedule_due_date_email(issuable) super end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index e8fdf9901c1..d7aa7e2347e 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -13,10 +13,6 @@ module Issues spam_check(issue, current_user) end - def after_update(issue) - schedule_due_date_email(issue) - end - def handle_changes(issue, options) old_associations = options.fetch(:old_associations, {}) old_labels = old_associations.fetch(:labels, []) @@ -27,9 +23,6 @@ module Issues todo_service.mark_pending_todos_as_done(issue, current_user) end - # TODO: If due date doesn't change, don't bother updating the due date - # email worker - if issue.previous_changes.include?('title') || issue.previous_changes.include?('description') todo_service.update_issue(issue, current_user, old_mentioned_users) diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index 93ad55fdd70..7598e579c26 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -18,6 +18,7 @@ - cronjob:stuck_import_jobs - cronjob:stuck_merge_jobs - cronjob:trending_projects +- cronjob:issue_due - gcp_cluster:cluster_install_app - gcp_cluster:cluster_provision @@ -78,7 +79,6 @@ - group_destroy - invalid_gpg_signature_update - irker -- issue_due - merge - namespaceless_project_destroy - new_issue diff --git a/app/workers/issue_due_worker.rb b/app/workers/issue_due_worker.rb index ddd61d7b912..90d2cc09f1b 100644 --- a/app/workers/issue_due_worker.rb +++ b/app/workers/issue_due_worker.rb @@ -1,9 +1,9 @@ class IssueDueWorker include ApplicationWorker + include CronjobQueue - def perform(issue_id) - issue = Issue.find_by_id(issue_id) - if issue.due_date == Date.today + def perform + Issue.where(due_date: Date.today).find_each do |issue| NotificationService.new.issue_due_email(issue) end end diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 53cf0010d8e..023dc8a273d 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -431,6 +431,10 @@ Settings.cron_jobs['pages_domain_verification_cron_worker'] ||= Settingslogic.ne Settings.cron_jobs['pages_domain_verification_cron_worker']['cron'] ||= '*/15 * * * *' Settings.cron_jobs['pages_domain_verification_cron_worker']['job_class'] = 'PagesDomainVerificationCronWorker' +Settings.cron_jobs['issue_due_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['issue_due_worker']['cron'] ||= '50 00 * * *' +Settings.cron_jobs['issue_due_worker']['job_class'] = 'IssueDueWorker' + # # GitLab Shell # diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 5c0bf091b18..554502c5d83 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -48,7 +48,6 @@ - [authorized_projects, 1] - [expire_build_instance_artifacts, 1] - [group_destroy, 1] - - [issue_due, 1] - [irker, 1] - [namespaceless_project_destroy, 1] - [project_cache, 1] From 99fde7da158971b9dbed5dd06aadb9ef89339fab Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Sat, 10 Feb 2018 14:38:45 +0100 Subject: [PATCH 07/20] Update email body (html+text) --- app/views/notify/issue_due_email.html.haml | 2 +- app/views/notify/issue_due_email.text.erb | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml index 30ba53cf801..a59a55516d3 100644 --- a/app/views/notify/issue_due_email.html.haml +++ b/app/views/notify/issue_due_email.html.haml @@ -7,7 +7,7 @@ Assignee: #{@issue.assignee_list} %p - This Issue has a new due date: #{ @issue.due_date } + This issue is due on: #{ @issue.due_date } - if @issue.description %div diff --git a/app/views/notify/issue_due_email.text.erb b/app/views/notify/issue_due_email.text.erb index 1ac98bd9d93..3c7a57a8a2e 100644 --- a/app/views/notify/issue_due_email.text.erb +++ b/app/views/notify/issue_due_email.text.erb @@ -1,8 +1,7 @@ -An Issue had its due date updated. +The following issue is due on <%= @issue.due_date %>: Issue <%= @issue.iid %>: <%= url_for(project_issue_url(@issue.project, @issue)) %> Author: <%= @issue.author_name %> Assignee: <%= @issue.assignee_list %> -New Due Date: <%= @issue.due_date %> <%= @issue.description %> From 260282b0ac292f1ec7c6f2ed0eb23142175f898a Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Sat, 10 Feb 2018 14:54:45 +0100 Subject: [PATCH 08/20] Create cron issue due spec skeleton --- spec/workers/issue_due_worker_spec.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 spec/workers/issue_due_worker_spec.rb diff --git a/spec/workers/issue_due_worker_spec.rb b/spec/workers/issue_due_worker_spec.rb new file mode 100644 index 00000000000..c85f7270a2b --- /dev/null +++ b/spec/workers/issue_due_worker_spec.rb @@ -0,0 +1,10 @@ +require 'spec_helper' + +describe IssueDueWorker do + describe 'perform' do + let(:worker) { described_class.new } + + it 'finds issues due on the day run' do + end + end +end From 556546a9a6e8019917d927cda2404a3fb1a992cd Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 13 Feb 2018 09:39:44 +0100 Subject: [PATCH 09/20] Add changelog entry --- changelogs/unreleased/16957-issue-due-email.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/16957-issue-due-email.yml diff --git a/changelogs/unreleased/16957-issue-due-email.yml b/changelogs/unreleased/16957-issue-due-email.yml new file mode 100644 index 00000000000..18dc66db125 --- /dev/null +++ b/changelogs/unreleased/16957-issue-due-email.yml @@ -0,0 +1,5 @@ +--- +title: Add cron job to email users on issue due date +merge_request: 16957 +author: @stuartnelson3 +type: added From 7bd912783ae20a841acb8f62f6218b46e8703ce8 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 13 Feb 2018 10:12:36 +0100 Subject: [PATCH 10/20] Add spec for issue_due_worker --- spec/workers/issue_due_worker_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/workers/issue_due_worker_spec.rb b/spec/workers/issue_due_worker_spec.rb index c85f7270a2b..a83a4a2d355 100644 --- a/spec/workers/issue_due_worker_spec.rb +++ b/spec/workers/issue_due_worker_spec.rb @@ -5,6 +5,13 @@ describe IssueDueWorker do let(:worker) { described_class.new } it 'finds issues due on the day run' do + issue1 = create(:issue, :opened, due_date: Date.today) + issue3 = create(:issue, :opened, due_date: 3.days.from_now) + issue4 = create(:issue, :opened, due_date: 4.days.from_now) + + expect_any_instance_of(NotificationService).to receive(:issue_due_email).with(issue1) + + worker.perform end end end From a562db39e0db6520952c8bf3db3892655e1e25cc Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 13 Feb 2018 10:17:16 +0100 Subject: [PATCH 11/20] Fix changelog (linting) --- changelogs/unreleased/16957-issue-due-email.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/unreleased/16957-issue-due-email.yml b/changelogs/unreleased/16957-issue-due-email.yml index 18dc66db125..308f72e6a67 100644 --- a/changelogs/unreleased/16957-issue-due-email.yml +++ b/changelogs/unreleased/16957-issue-due-email.yml @@ -1,5 +1,5 @@ --- title: Add cron job to email users on issue due date merge_request: 16957 -author: @stuartnelson3 +author: Stuart Nelson type: added From 3c5d5be2a0aaa75452a92cdc93ac56b0ccd61022 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Tue, 13 Feb 2018 10:51:40 +0100 Subject: [PATCH 12/20] Remove 'useless' variable assignment --- spec/workers/issue_due_worker_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/workers/issue_due_worker_spec.rb b/spec/workers/issue_due_worker_spec.rb index a83a4a2d355..9004edbee3a 100644 --- a/spec/workers/issue_due_worker_spec.rb +++ b/spec/workers/issue_due_worker_spec.rb @@ -6,8 +6,8 @@ describe IssueDueWorker do it 'finds issues due on the day run' do issue1 = create(:issue, :opened, due_date: Date.today) - issue3 = create(:issue, :opened, due_date: 3.days.from_now) - issue4 = create(:issue, :opened, due_date: 4.days.from_now) + create(:issue, :opened, due_date: 3.days.from_now) + create(:issue, :opened, due_date: 4.days.from_now) expect_any_instance_of(NotificationService).to receive(:issue_due_email).with(issue1) From a11b4ee683bb20a8795464e0b033c21f22bc7975 Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Wed, 14 Feb 2018 09:30:05 +0100 Subject: [PATCH 13/20] Update html email --- app/views/notify/issue_due_email.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml index a59a55516d3..c552c612098 100644 --- a/app/views/notify/issue_due_email.html.haml +++ b/app/views/notify/issue_due_email.html.haml @@ -1,13 +1,13 @@ - if Gitlab::CurrentSettings.email_author_in_body %p.details - #{link_to @issue.author_name, user_url(@issue.author)} updated the issue's due date: + Issue created by #{link_to @issue.author_name, user_url(@issue.author)} is due: - if @issue.assignees.any? %p Assignee: #{@issue.assignee_list} %p - This issue is due on: #{ @issue.due_date } + This issue is due on: #{@issue.due_date} - if @issue.description %div From f4c8517fec6d53e079f465d594ddef531e12c0af Mon Sep 17 00:00:00 2001 From: Stuart Nelson Date: Wed, 14 Feb 2018 10:16:37 +0100 Subject: [PATCH 14/20] Add failing notification_recipient_service spec --- .../notification_recipient_service_spec.rb | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 spec/services/notification_recipient_service_spec.rb diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb new file mode 100644 index 00000000000..7d826377579 --- /dev/null +++ b/spec/services/notification_recipient_service_spec.rb @@ -0,0 +1,22 @@ +require 'spec_helper' + +describe NotificationRecipientService do + describe 'build_recipients' do + it 'due_date' do + # These folks are being filtered out because they can't receive notifications + # notification_recipient.rb#85 + user = create(:user) + assignee = create(:user) + issue = create(:issue, :opened, due_date: Date.today, author: user, assignees: [assignee]) + + recipients = described_class.build_recipients( + issue, + issue.author, + action: "due_date", + skip_current_user: false + ) + + expect(recipients).to match_array([user, assignee]) + end + end +end From ddb23d3b2ba7c646cff6a5d21957194fc3474418 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 30 Mar 2018 12:58:06 +0100 Subject: [PATCH 15/20] Send issue due emails to all participants --- .../notification_recipient_service.rb | 4 +-- app/services/notification_service.rb | 2 +- .../notification_recipient_service_spec.rb | 22 ------------- spec/services/notification_service_spec.rb | 31 +++++++++++++++++++ 4 files changed, 34 insertions(+), 25 deletions(-) delete mode 100644 spec/services/notification_recipient_service_spec.rb diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index c7c64356e30..c4da679defe 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -230,7 +230,7 @@ module NotificationRecipientService add_subscribed_users - if [:new_issue, :new_merge_request, :due_date_issue].include?(custom_action) + if [:new_issue, :new_merge_request].include?(custom_action) # These will all be participants as well, but adding with the :mention # type ensures that users with the mention notification level will # receive them, too. @@ -238,7 +238,7 @@ module NotificationRecipientService # Add the assigned users, if any assignees = case custom_action - when :new_issue, :due_date_issue + when :new_issue target.assignees else target.assignee diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index f6d9020b9a1..9d6cfd21266 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -363,7 +363,7 @@ class NotificationService end end - def issue_due_email(issue) + def issue_due(issue) recipients = NotificationRecipientService.build_recipients( issue, issue.author, diff --git a/spec/services/notification_recipient_service_spec.rb b/spec/services/notification_recipient_service_spec.rb deleted file mode 100644 index 7d826377579..00000000000 --- a/spec/services/notification_recipient_service_spec.rb +++ /dev/null @@ -1,22 +0,0 @@ -require 'spec_helper' - -describe NotificationRecipientService do - describe 'build_recipients' do - it 'due_date' do - # These folks are being filtered out because they can't receive notifications - # notification_recipient.rb#85 - user = create(:user) - assignee = create(:user) - issue = create(:issue, :opened, due_date: Date.today, author: user, assignees: [assignee]) - - recipients = described_class.build_recipients( - issue, - issue.author, - action: "due_date", - skip_current_user: false - ) - - expect(recipients).to match_array([user, assignee]) - end - end -end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 3943148f0db..1d117d9abc7 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -933,6 +933,37 @@ describe NotificationService, :mailer do let(:notification_trigger) { notification.issue_moved(issue, new_issue, @u_disabled) } end end + + describe '#issue_due' do + it 'sends email to issue notification recipients' do + notification.issue_due(issue) + + should_email(issue.assignees.first) + should_email(issue.author) + should_email(@u_watcher) + should_email(@u_guest_watcher) + should_email(@u_participant_mentioned) + should_email(@subscriber) + should_email(@watcher_and_subscriber) + should_not_email(@unsubscriber) + should_not_email(@u_participating) + should_not_email(@u_disabled) + should_not_email(@u_lazy_participant) + end + + it 'sends the email from the author' do + notification.issue_due(issue) + email = find_email_for(@subscriber) + + expect(email.header[:from].display_names).to eq([issue.author.name]) + end + + it_behaves_like 'participating notifications' do + let(:participant) { create(:user, username: 'user-participant') } + let(:issuable) { issue } + let(:notification_trigger) { notification.issue_due(issue) } + end + end end describe 'Merge Requests' do From 5ab75649f3ea00b64cb63e7e5283100c6b70cfb5 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 30 Mar 2018 13:25:46 +0100 Subject: [PATCH 16/20] Only send issue due emails to participants and custom subscribers --- app/models/notification_setting.rb | 3 ++- app/services/notification_recipient_service.rb | 3 ++- app/services/notification_service.rb | 3 ++- app/views/notify/issue_due_email.html.haml | 2 +- ...21048_add_issue_due_to_notification_settings.rb | 9 +++++++++ db/schema.rb | 3 ++- doc/api/notification_settings.md | 4 ++++ doc/workflow/notifications.md | 14 +++++++------- spec/services/notification_service_spec.rb | 13 ++++++++++--- 9 files changed, 39 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20180330121048_add_issue_due_to_notification_settings.rb diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index f6d9b0215fc..9195408551f 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -47,7 +47,8 @@ class NotificationSetting < ActiveRecord::Base ].freeze EXCLUDED_WATCHER_EVENTS = [ - :push_to_merge_request + :push_to_merge_request, + :issue_due ].push(*EXCLUDED_PARTICIPATING_EVENTS).freeze def self.find_or_create_for(source) diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index c4da679defe..f5e140bccee 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -204,10 +204,11 @@ module NotificationRecipientService attr_reader :action attr_reader :previous_assignee attr_reader :skip_current_user - def initialize(target, current_user, action:, previous_assignee: nil, skip_current_user: true) + def initialize(target, current_user, action:, custom_action: nil, previous_assignee: nil, skip_current_user: true) @target = target @current_user = current_user @action = action + @custom_action = custom_action @previous_assignee = previous_assignee @skip_current_user = skip_current_user end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 7d3c2856e93..274161df946 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -377,7 +377,8 @@ class NotificationService recipients = NotificationRecipientService.build_recipients( issue, issue.author, - action: "due_date", + action: 'due', + custom_action: :issue_due, skip_current_user: false ) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml index c552c612098..600e183cfdf 100644 --- a/app/views/notify/issue_due_email.html.haml +++ b/app/views/notify/issue_due_email.html.haml @@ -1,6 +1,6 @@ - if Gitlab::CurrentSettings.email_author_in_body %p.details - Issue created by #{link_to @issue.author_name, user_url(@issue.author)} is due: + #{link_to @issue.author_name, user_url(@issue.author)}'s issue is due soon. - if @issue.assignees.any? %p diff --git a/db/migrate/20180330121048_add_issue_due_to_notification_settings.rb b/db/migrate/20180330121048_add_issue_due_to_notification_settings.rb new file mode 100644 index 00000000000..c64a481fcf0 --- /dev/null +++ b/db/migrate/20180330121048_add_issue_due_to_notification_settings.rb @@ -0,0 +1,9 @@ +class AddIssueDueToNotificationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :notification_settings, :issue_due, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 9aaefcf1c8d..a96242e6587 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180327101207) do +ActiveRecord::Schema.define(version: 20180330121048) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1311,6 +1311,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "failed_pipeline" t.boolean "success_pipeline" t.boolean "push_to_merge_request" + t.boolean "issue_due" end add_index "notification_settings", ["source_id", "source_type"], name: "index_notification_settings_on_source_id_and_source_type", using: :btree diff --git a/doc/api/notification_settings.md b/doc/api/notification_settings.md index f05ae647577..682b90361bd 100644 --- a/doc/api/notification_settings.md +++ b/doc/api/notification_settings.md @@ -23,6 +23,7 @@ new_issue reopen_issue close_issue reassign_issue +issue_due new_merge_request push_to_merge_request reopen_merge_request @@ -75,6 +76,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab | `reopen_issue` | boolean | no | Enable/disable this notification | | `close_issue` | boolean | no | Enable/disable this notification | | `reassign_issue` | boolean | no | Enable/disable this notification | +| `issue_due` | boolean | no | Enable/disable this notification | | `new_merge_request` | boolean | no | Enable/disable this notification | | `push_to_merge_request` | boolean | no | Enable/disable this notification | | `reopen_merge_request` | boolean | no | Enable/disable this notification | @@ -142,6 +144,7 @@ curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab | `reopen_issue` | boolean | no | Enable/disable this notification | | `close_issue` | boolean | no | Enable/disable this notification | | `reassign_issue` | boolean | no | Enable/disable this notification | +| `issue_due` | boolean | no | Enable/disable this notification | | `new_merge_request` | boolean | no | Enable/disable this notification | | `push_to_merge_request` | boolean | no | Enable/disable this notification | | `reopen_merge_request` | boolean | no | Enable/disable this notification | @@ -166,6 +169,7 @@ Example responses: "reopen_issue": false, "close_issue": false, "reassign_issue": false, + "issue_due": false, "new_merge_request": false, "push_to_merge_request": false, "reopen_merge_request": false, diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index c4095ee0f69..f1501c81b27 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -86,6 +86,7 @@ In most of the below cases, the notification will be sent to: | Close issue | | | Reassign issue | The above, plus the old assignee | | Reopen issue | | +| Due issue | Participants and Custom notification level with this event selected | | New merge request | | | Push to merge request | Participants and Custom notification level with this event selected | | Reassign merge request | The above, plus the old assignee | @@ -96,15 +97,14 @@ In most of the below cases, the notification will be sent to: | Failed pipeline | The author of the pipeline | | Successful pipeline | The author of the pipeline, if they have the custom notification setting for successful pipelines set | - In addition, if the title or description of an Issue or Merge Request is changed, notifications will be sent to any **new** mentions by `@username` as if they had been mentioned in the original text. -You won't receive notifications for Issues, Merge Requests or Milestones -created by yourself. You will only receive automatic notifications when -somebody else comments or adds changes to the ones that you've created or -mentions you. +You won't receive notifications for Issues, Merge Requests or Milestones created +by yourself (except when an issue is due). You will only receive automatic +notifications when somebody else comments or adds changes to the ones that +you've created or mentions you. ### Email Headers @@ -122,7 +122,7 @@ Notification emails include headers that provide extra content about the notific | X-GitLab-NotificationReason | The reason for being notified. "mentioned", "assigned", etc | #### X-GitLab-NotificationReason -This header holds the reason for the notification to have been sent out, +This header holds the reason for the notification to have been sent out, where reason can be `mentioned`, `assigned`, `own_activity`, etc. Only one reason is sent out according to its priority: - `own_activity` @@ -130,7 +130,7 @@ Only one reason is sent out according to its priority: - `mentioned` The reason in this header will also be shown in the footer of the notification email. For example an email with the -reason `assigned` will have this sentence in the footer: +reason `assigned` will have this sentence in the footer: `"You are receiving this email because you have been assigned an item on {configured GitLab hostname}"` **Note: Only reasons listed above have been implemented so far** diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index e7c7706b484..cd10a13814e 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -935,16 +935,23 @@ describe NotificationService, :mailer do end describe '#issue_due' do - it 'sends email to issue notification recipients' do + before do + update_custom_notification(:issue_due, @u_guest_custom, resource: project) + update_custom_notification(:issue_due, @u_custom_global) + end + + it 'sends email to issue notification recipients, excluding watchers' do notification.issue_due(issue) should_email(issue.assignees.first) should_email(issue.author) - should_email(@u_watcher) - should_email(@u_guest_watcher) + should_email(@u_guest_custom) + should_email(@u_custom_global) should_email(@u_participant_mentioned) should_email(@subscriber) should_email(@watcher_and_subscriber) + should_not_email(@u_watcher) + should_not_email(@u_guest_watcher) should_not_email(@unsubscriber) should_not_email(@u_participating) should_not_email(@u_disabled) From 2db218f8bf186c509c927ce3e9d0502fee4f8349 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 30 Mar 2018 14:05:20 +0100 Subject: [PATCH 17/20] Send emails for issues due tomorrow Also, refactor the mail sending slightly: instead of one worker sending all emails, create a worker per project with issues due, which will send all emails for that project. --- app/models/issue.rb | 1 + app/workers/all_queues.yml | 4 +++- app/workers/concerns/mail_scheduler_queue.rb | 7 ++++++ app/workers/issue_due_scheduler_worker.rb | 10 ++++++++ app/workers/issue_due_worker.rb | 10 -------- .../mail_scheduler/issue_due_worker.rb | 14 +++++++++++ .../unreleased/16957-issue-due-email.yml | 2 +- config/initializers/1_settings.rb | 6 ++--- config/sidekiq_queues.yml | 1 + doc/user/project/issues/due_dates.md | 3 +++ .../issue_due_scheduler_worker_spec.rb | 23 +++++++++++++++++++ spec/workers/issue_due_worker_spec.rb | 17 -------------- .../mail_scheduler/issue_due_worker_spec.rb | 21 +++++++++++++++++ 13 files changed, 87 insertions(+), 32 deletions(-) create mode 100644 app/workers/concerns/mail_scheduler_queue.rb create mode 100644 app/workers/issue_due_scheduler_worker.rb delete mode 100644 app/workers/issue_due_worker.rb create mode 100644 app/workers/mail_scheduler/issue_due_worker.rb create mode 100644 spec/workers/issue_due_scheduler_worker_spec.rb delete mode 100644 spec/workers/issue_due_worker_spec.rb create mode 100644 spec/workers/mail_scheduler/issue_due_worker_spec.rb diff --git a/app/models/issue.rb b/app/models/issue.rb index 7bfc45c1f43..f65cd8bf896 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -48,6 +48,7 @@ class Issue < ActiveRecord::Base scope :without_due_date, -> { where(due_date: nil) } scope :due_before, ->(date) { where('issues.due_date < ?', date) } scope :due_between, ->(from_date, to_date) { where('issues.due_date >= ?', from_date).where('issues.due_date <= ?', to_date) } + scope :due_tomorrow, -> { where(due_date: Date.tomorrow) } scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') } scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') } diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index ef445055f6e..9aea3bad27b 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -18,7 +18,7 @@ - cronjob:stuck_import_jobs - cronjob:stuck_merge_jobs - cronjob:trending_projects -- cronjob:issue_due +- cronjob:issue_due_scheduler - gcp_cluster:cluster_install_app - gcp_cluster:cluster_provision @@ -40,6 +40,8 @@ - github_importer:github_import_stage_import_pull_requests - github_importer:github_import_stage_import_repository +- mail_scheduler:mail_scheduler_issue_due + - object_storage_upload - object_storage:object_storage_background_move - object_storage:object_storage_migrate_uploads diff --git a/app/workers/concerns/mail_scheduler_queue.rb b/app/workers/concerns/mail_scheduler_queue.rb new file mode 100644 index 00000000000..9df55ad9522 --- /dev/null +++ b/app/workers/concerns/mail_scheduler_queue.rb @@ -0,0 +1,7 @@ +module MailSchedulerQueue + extend ActiveSupport::Concern + + included do + queue_namespace :mail_scheduler + end +end diff --git a/app/workers/issue_due_scheduler_worker.rb b/app/workers/issue_due_scheduler_worker.rb new file mode 100644 index 00000000000..9c06304bff6 --- /dev/null +++ b/app/workers/issue_due_scheduler_worker.rb @@ -0,0 +1,10 @@ +class IssueDueSchedulerWorker + include ApplicationWorker + include CronjobQueue + + def perform + Issue.opened.due_tomorrow.group(:project_id).pluck(:project_id).each do |project_id| + MailScheduler::IssueDueWorker.perform_async(project_id) + end + end +end diff --git a/app/workers/issue_due_worker.rb b/app/workers/issue_due_worker.rb deleted file mode 100644 index 90d2cc09f1b..00000000000 --- a/app/workers/issue_due_worker.rb +++ /dev/null @@ -1,10 +0,0 @@ -class IssueDueWorker - include ApplicationWorker - include CronjobQueue - - def perform - Issue.where(due_date: Date.today).find_each do |issue| - NotificationService.new.issue_due_email(issue) - end - end -end diff --git a/app/workers/mail_scheduler/issue_due_worker.rb b/app/workers/mail_scheduler/issue_due_worker.rb new file mode 100644 index 00000000000..b06079d68ca --- /dev/null +++ b/app/workers/mail_scheduler/issue_due_worker.rb @@ -0,0 +1,14 @@ +module MailScheduler + class IssueDueWorker + include ApplicationWorker + include MailSchedulerQueue + + def perform(project_id) + notification_service = NotificationService.new + + Issue.opened.due_tomorrow.in_projects(project_id).preload(:project).find_each do |issue| + notification_service.issue_due(issue) + end + end + end +end diff --git a/changelogs/unreleased/16957-issue-due-email.yml b/changelogs/unreleased/16957-issue-due-email.yml index 308f72e6a67..83944ca4f73 100644 --- a/changelogs/unreleased/16957-issue-due-email.yml +++ b/changelogs/unreleased/16957-issue-due-email.yml @@ -1,5 +1,5 @@ --- title: Add cron job to email users on issue due date -merge_request: 16957 +merge_request: 17985 author: Stuart Nelson type: added diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb index 1ddf380c56d..c8a4c1af50f 100644 --- a/config/initializers/1_settings.rb +++ b/config/initializers/1_settings.rb @@ -453,9 +453,9 @@ Settings.cron_jobs['pages_domain_verification_cron_worker'] ||= Settingslogic.ne Settings.cron_jobs['pages_domain_verification_cron_worker']['cron'] ||= '*/15 * * * *' Settings.cron_jobs['pages_domain_verification_cron_worker']['job_class'] = 'PagesDomainVerificationCronWorker' -Settings.cron_jobs['issue_due_worker'] ||= Settingslogic.new({}) -Settings.cron_jobs['issue_due_worker']['cron'] ||= '50 00 * * *' -Settings.cron_jobs['issue_due_worker']['job_class'] = 'IssueDueWorker' +Settings.cron_jobs['issue_due_scheduler_worker'] ||= Settingslogic.new({}) +Settings.cron_jobs['issue_due_scheduler_worker']['cron'] ||= '50 00 * * *' +Settings.cron_jobs['issue_due_scheduler_worker']['job_class'] = 'IssueDueSchedulerWorker' # # GitLab Shell diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index c811034b29d..47fbbed44cf 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -34,6 +34,7 @@ - [email_receiver, 2] - [emails_on_push, 2] - [mailers, 2] + - [mail_scheduler, 2] - [invalid_gpg_signature_update, 2] - [create_gpg_signature, 2] - [rebase, 2] diff --git a/doc/user/project/issues/due_dates.md b/doc/user/project/issues/due_dates.md index e0c405353ce..400f581e513 100644 --- a/doc/user/project/issues/due_dates.md +++ b/doc/user/project/issues/due_dates.md @@ -35,5 +35,8 @@ Due dates also appear in your [todos list](../../../workflow/todos.md). ![Issues with due dates in the todos](img/due_dates_todos.png) +The day before an open issue is due, an email will be sent to all participants +of the issue. + [ce-3614]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3614 [permissions]: ../../permissions.md#project diff --git a/spec/workers/issue_due_scheduler_worker_spec.rb b/spec/workers/issue_due_scheduler_worker_spec.rb new file mode 100644 index 00000000000..eff5855834c --- /dev/null +++ b/spec/workers/issue_due_scheduler_worker_spec.rb @@ -0,0 +1,23 @@ +require 'spec_helper' + +describe IssueDueSchedulerWorker do + describe '#perform' do + it 'schedules one MailScheduler::IssueDueWorker per project with open issues due tomorrow' do + project1 = create(:project) + project2 = create(:project) + project_closed_issue = create(:project) + project_issue_due_another_day = create(:project) + + create(:issue, :opened, project: project1, due_date: Date.tomorrow) + create(:issue, :opened, project: project1, due_date: Date.tomorrow) + create(:issue, :opened, project: project2, due_date: Date.tomorrow) + create(:issue, :closed, project: project_closed_issue, due_date: Date.tomorrow) + create(:issue, :opened, project: project_issue_due_another_day, due_date: Date.today) + + expect(MailScheduler::IssueDueWorker).to receive(:perform_async).with(project1.id) + expect(MailScheduler::IssueDueWorker).to receive(:perform_async).with(project2.id) + + described_class.new.perform + end + end +end diff --git a/spec/workers/issue_due_worker_spec.rb b/spec/workers/issue_due_worker_spec.rb deleted file mode 100644 index 9004edbee3a..00000000000 --- a/spec/workers/issue_due_worker_spec.rb +++ /dev/null @@ -1,17 +0,0 @@ -require 'spec_helper' - -describe IssueDueWorker do - describe 'perform' do - let(:worker) { described_class.new } - - it 'finds issues due on the day run' do - issue1 = create(:issue, :opened, due_date: Date.today) - create(:issue, :opened, due_date: 3.days.from_now) - create(:issue, :opened, due_date: 4.days.from_now) - - expect_any_instance_of(NotificationService).to receive(:issue_due_email).with(issue1) - - worker.perform - end - end -end diff --git a/spec/workers/mail_scheduler/issue_due_worker_spec.rb b/spec/workers/mail_scheduler/issue_due_worker_spec.rb new file mode 100644 index 00000000000..48ac1b8a1a4 --- /dev/null +++ b/spec/workers/mail_scheduler/issue_due_worker_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe MailScheduler::IssueDueWorker do + describe '#perform' do + let(:worker) { described_class.new } + let(:project) { create(:project) } + + it 'sends emails for open issues due tomorrow in the project specified' do + issue1 = create(:issue, :opened, project: project, due_date: Date.tomorrow) + issue2 = create(:issue, :opened, project: project, due_date: Date.tomorrow) + create(:issue, :closed, project: project, due_date: Date.tomorrow) # closed + create(:issue, :opened, project: project, due_date: 2.days.from_now) # due on another day + create(:issue, :opened, due_date: Date.tomorrow) # different project + + expect_any_instance_of(NotificationService).to receive(:issue_due).with(issue1) + expect_any_instance_of(NotificationService).to receive(:issue_due).with(issue2) + + worker.perform(project.id) + end + end +end From 86bf99a63bfd1ef456da54d8f31c23c46e678f0c Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 10 Apr 2018 11:29:40 +0100 Subject: [PATCH 18/20] Clarify that due date emails are in the server's timezone --- doc/user/project/issues/due_dates.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/doc/user/project/issues/due_dates.md b/doc/user/project/issues/due_dates.md index 400f581e513..1bf8b776c2e 100644 --- a/doc/user/project/issues/due_dates.md +++ b/doc/user/project/issues/due_dates.md @@ -36,7 +36,8 @@ Due dates also appear in your [todos list](../../../workflow/todos.md). ![Issues with due dates in the todos](img/due_dates_todos.png) The day before an open issue is due, an email will be sent to all participants -of the issue. +of the issue. Both the due date and the day before are calculated using the +server's timezone. [ce-3614]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3614 [permissions]: ../../permissions.md#project From fd4a08203448d5ade7471549fe17169fb19c6484 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 17 Apr 2018 12:44:22 +0100 Subject: [PATCH 19/20] Make issue due email more consistent with other mailers --- app/views/notify/issue_due_email.html.haml | 8 +++----- spec/services/notification_service_spec.rb | 2 ++ 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/views/notify/issue_due_email.html.haml b/app/views/notify/issue_due_email.html.haml index 600e183cfdf..e81144b8fcb 100644 --- a/app/views/notify/issue_due_email.html.haml +++ b/app/views/notify/issue_due_email.html.haml @@ -1,13 +1,11 @@ -- if Gitlab::CurrentSettings.email_author_in_body - %p.details - #{link_to @issue.author_name, user_url(@issue.author)}'s issue is due soon. +%p.details + #{link_to @issue.author_name, user_url(@issue.author)}'s issue is due soon. - if @issue.assignees.any? %p Assignee: #{@issue.assignee_list} - %p - This issue is due on: #{@issue.due_date} + This issue is due on: #{@issue.due_date.to_s(:medium)} - if @issue.description %div diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index cd10a13814e..55bbe954491 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -936,6 +936,8 @@ describe NotificationService, :mailer do describe '#issue_due' do before do + issue.update!(due_date: Date.today) + update_custom_notification(:issue_due, @u_guest_custom, resource: project) update_custom_notification(:issue_due, @u_custom_global) end From ab650e7c6753d2f4c418496ad74dc87e243a5b2b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 17 Apr 2018 12:44:45 +0100 Subject: [PATCH 20/20] Use `bulk_perform_async` to schedule issue due emails --- app/workers/issue_due_scheduler_worker.rb | 6 +++--- spec/workers/issue_due_scheduler_worker_spec.rb | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/workers/issue_due_scheduler_worker.rb b/app/workers/issue_due_scheduler_worker.rb index 9c06304bff6..16ab5d069e0 100644 --- a/app/workers/issue_due_scheduler_worker.rb +++ b/app/workers/issue_due_scheduler_worker.rb @@ -3,8 +3,8 @@ class IssueDueSchedulerWorker include CronjobQueue def perform - Issue.opened.due_tomorrow.group(:project_id).pluck(:project_id).each do |project_id| - MailScheduler::IssueDueWorker.perform_async(project_id) - end + project_ids = Issue.opened.due_tomorrow.group(:project_id).pluck(:project_id).map { |id| [id] } + + MailScheduler::IssueDueWorker.bulk_perform_async(project_ids) end end diff --git a/spec/workers/issue_due_scheduler_worker_spec.rb b/spec/workers/issue_due_scheduler_worker_spec.rb index eff5855834c..7b60835fd26 100644 --- a/spec/workers/issue_due_scheduler_worker_spec.rb +++ b/spec/workers/issue_due_scheduler_worker_spec.rb @@ -14,8 +14,7 @@ describe IssueDueSchedulerWorker do create(:issue, :closed, project: project_closed_issue, due_date: Date.tomorrow) create(:issue, :opened, project: project_issue_due_another_day, due_date: Date.today) - expect(MailScheduler::IssueDueWorker).to receive(:perform_async).with(project1.id) - expect(MailScheduler::IssueDueWorker).to receive(:perform_async).with(project2.id) + expect(MailScheduler::IssueDueWorker).to receive(:bulk_perform_async).with([[project1.id], [project2.id]]) described_class.new.perform end