From 86427c2b78af723be5c9bcd24f4b12ccf09e5764 Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Thu, 26 Aug 2021 10:48:39 +0200 Subject: [PATCH 01/21] Remove scheduling DigestJobs from WorkFlowJob --- app/workers/mails/digest_job.rb | 29 ------------------- app/workers/notifications/workflow_job.rb | 8 ----- .../notifications/workflow_job_spec.rb | 17 +---------- 3 files changed, 1 insertion(+), 53 deletions(-) diff --git a/app/workers/mails/digest_job.rb b/app/workers/mails/digest_job.rb index 0aa872ecf4..ffa99c93b5 100644 --- a/app/workers/mails/digest_job.rb +++ b/app/workers/mails/digest_job.rb @@ -29,35 +29,6 @@ #++ class Mails::DigestJob < Mails::DeliverJob - class << self - def schedule(notification) - # This alone is vulnerable to the edge case of the Mails::DigestJob - # having started but not completed when a new digest notification is generated. - # To cope with it, the Mails::DigestJob as its first action sets all digest notifications - # to being handled even though they are still processed. - return if digest_job_already_scheduled?(notification) - - set(wait_until: execution_time(notification.recipient)) - .perform_later(notification.recipient) - end - - private - - def execution_time(user) - zone = (user.time_zone || ActiveSupport::TimeZone.new('UTC')) - - zone.parse(Setting.notification_email_digest_time) + 1.day - end - - def digest_job_already_scheduled?(notification) - Notification - .mail_digest_before(recipient: notification.recipient, - time: notification.created_at) - .where.not(id: notification.id) - .exists? - end - end - private def render_mail diff --git a/app/workers/notifications/workflow_job.rb b/app/workers/notifications/workflow_job.rb index e562658b48..1b0c4593c2 100644 --- a/app/workers/notifications/workflow_job.rb +++ b/app/workers/notifications/workflow_job.rb @@ -73,13 +73,5 @@ class Notifications::WorkflowJob < ApplicationJob .new(notification) .call end - - Notification - .where(id: notification_ids) - .unread_mail_digest - .each do |notification| - Mails::DigestJob - .schedule(notification) - end end end diff --git a/spec/workers/notifications/workflow_job_spec.rb b/spec/workers/notifications/workflow_job_spec.rb index 8a8a2e1cc9..d253c8cc17 100644 --- a/spec/workers/notifications/workflow_job_spec.rb +++ b/spec/workers/notifications/workflow_job_spec.rb @@ -109,15 +109,8 @@ describe Notifications::WorkflowJob, type: :model do service_instance end - let!(:digest_job) do - allow(Mails::DigestJob) - .to receive(:schedule) - end - before do - scope = class_double(Notification, - unread_mail: [notifications.first], - unread_mail_digest: [notifications.last]) + scope = class_double(Notification, unread_mail: [notifications.first]) allow(Notification) .to receive(:where) @@ -131,14 +124,6 @@ describe Notifications::WorkflowJob, type: :model do expect(mail_service) .to have_received(:call) end - - it 'schedules a digest job for all notifications that are marked for the digest' do - perform_job - - expect(Mails::DigestJob) - .to have_received(:schedule) - .with(notifications.last) - end end end end From a701a6bdc20d0238f97b9a68db310a9f91461446 Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Thu, 26 Aug 2021 12:32:25 +0200 Subject: [PATCH 02/21] Rename 'digest' to 'reminder', first chunk. Not touching Notification yet --- app/models/notification.rb | 2 +- .../{mail_digest_before.rb => unsent_reminders_before.rb} | 6 +++--- app/models/project.rb | 2 +- app/workers/mails/{digest_job.rb => reminder_job.rb} | 6 +++--- ...igest_before_spec.rb => unsent_reminders_before_spec.rb} | 6 +++--- .../mails/{digest_job_spec.rb => reminder_job_spec.rb} | 4 ++-- 6 files changed, 13 insertions(+), 13 deletions(-) rename app/models/notifications/scopes/{mail_digest_before.rb => unsent_reminders_before.rb} (88%) rename app/workers/mails/{digest_job.rb => reminder_job.rb} (92%) rename spec/models/notifications/scopes/{mail_digest_before_spec.rb => unsent_reminders_before_spec.rb} (93%) rename spec/workers/mails/{digest_job_spec.rb => reminder_job_spec.rb} (97%) diff --git a/app/models/notification.rb b/app/models/notification.rb index a0bc3c289c..f6585e16cb 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -22,7 +22,7 @@ class Notification < ApplicationRecord belongs_to :resource, polymorphic: true include Scopes::Scoped - scopes :mail_digest_before, + scopes :unsent_reminders_before, :unread_mail, :unread_mail_digest, :recipient diff --git a/app/models/notifications/scopes/mail_digest_before.rb b/app/models/notifications/scopes/unsent_reminders_before.rb similarity index 88% rename from app/models/notifications/scopes/mail_digest_before.rb rename to app/models/notifications/scopes/unsent_reminders_before.rb index b20c706e4c..e46f623164 100644 --- a/app/models/notifications/scopes/mail_digest_before.rb +++ b/app/models/notifications/scopes/unsent_reminders_before.rb @@ -29,13 +29,13 @@ #++ module Notifications::Scopes - module MailDigestBefore + module UnsentRemindersBefore extend ActiveSupport::Concern class_methods do - # Return notifications of the user for which mail digest is to be sent and that is created before + # Return notifications for the user for who email reminders shall be sent and that were created before # the specified time. - def mail_digest_before(recipient:, time:) + def unsent_reminders_before(recipient:, time:) where(Notification.arel_table[:created_at].lteq(time)) .where(recipient: recipient) .where(read_mail_digest: false) diff --git a/app/models/project.rb b/app/models/project.rb index f663fb1173..8fa551314b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -44,7 +44,7 @@ class Project < ApplicationRecord has_many :members, -> { # TODO: check whether this should - # remaint to be limited to User only + # remain to be limited to User only includes(:principal, :roles) .merge(Principal.not_locked.user) .references(:principal, :roles) diff --git a/app/workers/mails/digest_job.rb b/app/workers/mails/reminder_job.rb similarity index 92% rename from app/workers/mails/digest_job.rb rename to app/workers/mails/reminder_job.rb index ffa99c93b5..fce4238e78 100644 --- a/app/workers/mails/digest_job.rb +++ b/app/workers/mails/reminder_job.rb @@ -28,13 +28,13 @@ # See COPYRIGHT and LICENSE files for more details. #++ -class Mails::DigestJob < Mails::DeliverJob +class Mails::ReminderJob < Mails::DeliverJob private def render_mail # Have to cast to array since the update in the subsequent block - # will result in the notification to not be found via the .mail_digest_before scope. - notification_ids = Notification.mail_digest_before(recipient: recipient, time: Time.current).pluck(:id) + # will result in the notification to not be found via the .unsent_reminders_before scope. + notification_ids = Notification.unsent_reminders_before(recipient: recipient, time: Time.current).pluck(:id) return nil if notification_ids.empty? diff --git a/spec/models/notifications/scopes/mail_digest_before_spec.rb b/spec/models/notifications/scopes/unsent_reminders_before_spec.rb similarity index 93% rename from spec/models/notifications/scopes/mail_digest_before_spec.rb rename to spec/models/notifications/scopes/unsent_reminders_before_spec.rb index 70253d290a..9e0f0a63fd 100644 --- a/spec/models/notifications/scopes/mail_digest_before_spec.rb +++ b/spec/models/notifications/scopes/unsent_reminders_before_spec.rb @@ -28,9 +28,9 @@ require 'spec_helper' -describe Notifications::Scopes::MailDigestBefore, type: :model do - describe '.mail_digest_before' do - subject(:scope) { ::Notification.mail_digest_before(recipient: recipient, time: time) } +describe Notifications::Scopes::UnsentRemindersBefore, type: :model do + describe '.unsent_reminders_before' do + subject(:scope) { ::Notification.unsent_reminders_before(recipient: recipient, time: time) } let(:recipient) do FactoryBot.create(:user) diff --git a/spec/workers/mails/digest_job_spec.rb b/spec/workers/mails/reminder_job_spec.rb similarity index 97% rename from spec/workers/mails/digest_job_spec.rb rename to spec/workers/mails/reminder_job_spec.rb index 6d3417febf..2f7fd3d1dd 100644 --- a/spec/workers/mails/digest_job_spec.rb +++ b/spec/workers/mails/reminder_job_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' -describe Mails::DigestJob, type: :model do +describe Mails::ReminderJob, type: :model do subject(:job) { described_class.perform_now(recipient) } let(:recipient) do @@ -46,7 +46,7 @@ describe Mails::DigestJob, type: :model do .and_return(Time.current) allow(Notification) - .to receive(:mail_digest_before) + .to receive(:unsent_reminders_before) .with(recipient: recipient, time: Time.current) .and_return(notifications) From 984d82f29f05c10569926c8193cdb95e73cc695a Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Fri, 27 Aug 2021 12:59:08 +0200 Subject: [PATCH 03/21] Don't send reminder mails for read notification The scope for unsent_remindes_before should not return Notifications that were already read in the in app notification center as we do not want to remind users of stuff they have already read. --- .../scopes/unsent_reminders_before.rb | 1 + .../scopes/unsent_reminders_before_spec.rb | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/app/models/notifications/scopes/unsent_reminders_before.rb b/app/models/notifications/scopes/unsent_reminders_before.rb index e46f623164..3456752ee7 100644 --- a/app/models/notifications/scopes/unsent_reminders_before.rb +++ b/app/models/notifications/scopes/unsent_reminders_before.rb @@ -38,6 +38,7 @@ module Notifications::Scopes def unsent_reminders_before(recipient:, time:) where(Notification.arel_table[:created_at].lteq(time)) .where(recipient: recipient) + .where("read_ian IS NULL OR read_ian IS FALSE") .where(read_mail_digest: false) end end diff --git a/spec/models/notifications/scopes/unsent_reminders_before_spec.rb b/spec/models/notifications/scopes/unsent_reminders_before_spec.rb index 9e0f0a63fd..10ca824c94 100644 --- a/spec/models/notifications/scopes/unsent_reminders_before_spec.rb +++ b/spec/models/notifications/scopes/unsent_reminders_before_spec.rb @@ -42,10 +42,12 @@ describe Notifications::Scopes::UnsentRemindersBefore, type: :model do let(:notification) do FactoryBot.create(:notification, recipient: notification_recipient, + read_ian: notification_read_ian, read_mail_digest: notification_read_mail_digest, created_at: notification_created_at) end let(:notification_read_mail_digest) { false } + let(:notification_read_ian) { false } let(:notification_created_at) { Time.current - 10.minutes } let(:notification_recipient) { recipient } @@ -58,35 +60,41 @@ describe Notifications::Scopes::UnsentRemindersBefore, type: :model do end end - context 'with a notification of the user for mail digests before the time' do + context 'with a unread and not reminded notification that was created before the time and for the user' do it 'returns the notification' do expect(scope) .to match_array([notification]) end end - context 'with a notification of the user for mail digests after the time' do + context 'with a unread and not reminded notification that was created after the time and for the user' do let(:notification_created_at) { Time.current + 10.minutes } it_behaves_like 'is empty' end - context 'with a notification of a different user for mail digests before the time' do + context 'with a unread and not reminded notification that was created before the time and for different user' do let(:notification_recipient) { FactoryBot.create(:user) } it_behaves_like 'is empty' end - context 'with a notification of a different user not for mail digests before the time' do + context 'with a unread and not reminded notification created before the time and for the user' do let(:notification_read_mail_digest) { nil } it_behaves_like 'is empty' end - context 'with a notification of a different user for already covered mail digests before the time' do + context 'with a unread but reminded notification created before the time and for the user' do let(:notification_read_mail_digest) { true } it_behaves_like 'is empty' end + + context 'with a read notification that was created before the time' do + let(:notification_read_ian) { true } + + it_behaves_like 'is empty' + end end end From 00699c8d47ec99434e7b2925e20fd0d1b5c98bf2 Mon Sep 17 00:00:00 2001 From: Wieland Lindenthal Date: Fri, 3 Sep 2021 15:42:03 +0200 Subject: [PATCH 04/21] Schedule reminder mail jobs --- .../cron/schedule_reminder_mails_job.rb | 113 ++++++++++++ .../notifications/digest_mail_spec.rb | 12 +- spec/support/schedule_reminder_mails.rb | 34 ++++ .../cron/schedule_reminder_mails_job_spec.rb | 167 ++++++++++++++++++ 4 files changed, 320 insertions(+), 6 deletions(-) create mode 100644 app/workers/cron/schedule_reminder_mails_job.rb create mode 100644 spec/support/schedule_reminder_mails.rb create mode 100644 spec/workers/cron/schedule_reminder_mails_job_spec.rb diff --git a/app/workers/cron/schedule_reminder_mails_job.rb b/app/workers/cron/schedule_reminder_mails_job.rb new file mode 100644 index 0000000000..5a3a45ccda --- /dev/null +++ b/app/workers/cron/schedule_reminder_mails_job.rb @@ -0,0 +1,113 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +module Cron + class ScheduleReminderMailsJob < CronJob + # runs every 30min step, so 00:00, 00:30, 01:00, 01:30... + self.cron_expression = '/30 * * * *' + + def perform + subscribers_of_current_slot_having_notifications do |subscriber_ids| + subscriber_ids.each do |recipient_id| + Mails::ReminderJob.perform_later(recipient_id) + end + end + end + + private + + def subscribers_of_current_slot_having_notifications + # Left outer join as not all user instances have preferences associated + # but we still want to select them + recipient_candidates = User.active + .left_joins(:preference) + .where(where_statement) + + subscriber_ids = Notification + .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) + .group(:recipient_id) + .pluck(:recipient_id) + + yield(subscriber_ids) + end + + def where_statement + current_timestamp_utc = Time.current.getutc + + age = age_statement(current_timestamp_utc) + <<-SQL.squish + #{age} < make_interval(mins=>30) + AND + #{age} >= make_interval(mins=>0) + SQL + end + + # Creates a SQL snippet for calculating the time between now and + # the reminder time slot with the each user's time zone + # @param [Time] current_timestamp_utc + def age_statement(current_timestamp_utc) + year = current_timestamp_utc.year + month = current_timestamp_utc.month + day = current_timestamp_utc.day + + case_statement = case_statement_zone_name_to_offset + + slot_time = Time.zone.parse(Setting.notification_email_digest_time) + + <<-SQL.squish + age( + now(), + make_timestamptz(#{year}, #{month}, #{day}, #{slot_time.hour}, #{slot_time.min}, 0, (#{case_statement})) + ) + SQL + end + + def case_statement_zone_name_to_offset + return @case_statement_zone_name_to_offset if @case_statement_zone_name_to_offset + + current_time = Time.current + statement = ActiveSupport::TimeZone.all.map do |zone| + offset = current_offset(zone, current_time) + "WHEN user_preferences.settings->>'time_zone' = '#{zone.name.gsub("'") { "''" }}' THEN '#{offset}'" + end + @case_statement_zone_name_to_offset = "CASE\n#{statement.join("\n")}\nELSE '+00:00'\nEND" + end + + # The real offset of a time zone depends of the moment we ask. Winter and summer time + # have different offsets of UTC. For instance, time zone "Berlin" in winter has +01:00 + # and in summer +02:00 + # @param [Time] time + # @param [TimeZone] zone + # @return [String] + def current_offset(zone, time = Time.current) + time.in_time_zone(zone).formatted_offset + end + end +end diff --git a/spec/features/notifications/digest_mail_spec.rb b/spec/features/notifications/digest_mail_spec.rb index 50ac533594..22a13c6085 100644 --- a/spec/features/notifications/digest_mail_spec.rb +++ b/spec/features/notifications/digest_mail_spec.rb @@ -49,6 +49,9 @@ describe "Digest email", type: :feature, js: true do end before do + allow(Setting).to receive(:notification_email_digest_time) + .and_return(hitting_reminder_slot_for(current_user)) + watched_work_package work_package involved_work_package @@ -121,12 +124,9 @@ describe "Digest email", type: :feature, js: true do involved_work_package.save! end - # Have to explicitly execute the delayed jobs. If we were to execute all - # by wrapping the above, work package altering code, inside a - # perform_enqueued_jobs block, the digest job would be executed right away - # so that the second update would trigger a new digest. But we want to test - # that only one digest is sent out - 5.times { perform_enqueued_jobs } + Cron::ScheduleReminderMailsJob.perform_later + perform_enqueued_jobs + perform_enqueued_jobs expect(ActionMailer::Base.deliveries.length) .to be 1 diff --git a/spec/support/schedule_reminder_mails.rb b/spec/support/schedule_reminder_mails.rb new file mode 100644 index 0000000000..b94f904bee --- /dev/null +++ b/spec/support/schedule_reminder_mails.rb @@ -0,0 +1,34 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +## +# Calculates a slot in the user's local time that hits for scheduling reminder mail jobs +def hitting_reminder_slot_for(user, current_utc_time = Time.current.getutc) + local_time = current_utc_time.in_time_zone(user.time_zone) + "#{'%02d' % local_time.hour}:#{local_time.min <= 30 ? '00' : '30'}" +end diff --git a/spec/workers/cron/schedule_reminder_mails_job_spec.rb b/spec/workers/cron/schedule_reminder_mails_job_spec.rb new file mode 100644 index 0000000000..4da5dd5059 --- /dev/null +++ b/spec/workers/cron/schedule_reminder_mails_job_spec.rb @@ -0,0 +1,167 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe Cron::ScheduleReminderMailsJob, type: :job do + # As it is hard to mock Postgres's "now()" method, in the specs here we need to adopt the slot time + # relative to the local time of the user that we want to hit. + let(:current_utc_time) { Time.current.getutc } + let(:slot_time) { hitting_reminder_slot_for(hitting_user, current_utc_time) } # ie. "08:00", "08:30" + + let(:hitting_user) { paris_user } + let(:paris_user) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } # time_zone in winter is +01:00 + let(:no_zone_user) { FactoryBot.create(:user) } # time_zone is nil + let(:notifications) { FactoryBot.create(:notification, recipient: hitting_user) } + + subject(:perform_job) do + described_class.new.perform + end + + before do + allow(Setting).to receive(:notification_email_digest_time).and_return(slot_time) + notifications + end + + context 'with paris_user as hitting_user' do + let(:moscow_user) { FactoryBot.create(:user, preferences: { time_zone: "Moscow" }) } # time_zone all year is +03:00 + let(:greenland_user) { FactoryBot.create(:user, preferences: { time_zone: "Greenland" }) } # time_zone in winter is -03:00 + let(:notifications) do + FactoryBot.create(:notification, recipient: hitting_user, created_at: 5.minutes.ago) + FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + FactoryBot.create(:notification, recipient: greenland_user, created_at: 5.minutes.ago) + FactoryBot.create(:notification, recipient: no_zone_user, created_at: 5.minutes.ago) + end + + before do + allow(Time).to receive(:current).and_return(current_utc_time) + end + + it 'schedules ReminderMailJobs for all users that subscribed for that slot in their local time' do + # `hitting_user` is `paris_user`. + # `slot_time` (expressed string in local time) is set to be at the beginning of the first or second half hour + # block of the started hour. + expect { perform_job } + .to enqueue_job(Mails::ReminderJob) + .with(hitting_user.id) + + # `moscow_user` is in a different time zone (higher offset than Paris) so should not hit for the same `slot_time` + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(moscow_user.id) + + # `greenland_user` is in a different time (lower offset than Paris) so should not hit for the same `slot_time` + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(greenland_user.id) + + # `no_zone_user` should fall back to UTC time zone and thus have lower offset as `paris_user` and not hit + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(no_zone_user.id) + end + end + + context 'when slot_time in UTC' do + let(:hitting_user) { no_zone_user } + let(:notifications) do + FactoryBot.create(:notification, recipient: hitting_user, created_at: 5.minutes.ago) + end + + it 'schedules a job for users without timezone set' do + expect { perform_job } + .to enqueue_job(Mails::ReminderJob) + .with(hitting_user.id) + end + end + + context 'when hitting user is not active' do + let(:hitting_user) do + paris_user.locked! + paris_user + end + let(:notifications) do + FactoryBot.create(:notification, recipient: hitting_user) + end + + it 'does not schedule for users that are not active' do + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(hitting_user.id) + end + end + + context 'with a user without notifications' do + # Create another user just as `hitting_user` but without notifications + let(:paris_user_without_notifications) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } + + it 'does not schedule reminder mail job' do + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(paris_user_without_notifications.id) + end + end + + context 'with a user with read IAN notifications' do + # Create another user just as `hitting_user` but without notifications + let(:paris_user_with_read_ian_notifications) do + FactoryBot.create(:user, preferences: { time_zone: "Paris" }) + end + let(:notifications) do + FactoryBot.create(:notification, + recipient: paris_user_with_read_ian_notifications, + read_ian: true) + end + + it 'does not schedule reminder mail job' do + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(paris_user_with_read_ian_notifications.id) + end + end + + context 'with a user with who already received a reminder for a notification' do + # Create another user just as `hitting_user` but without notifications + let(:paris_user_with_read_mail_digest_notifications) do + FactoryBot.create(:user, preferences: { time_zone: "Paris" }) + end + let(:notifications) do + FactoryBot.create(:notification, + recipient: paris_user_with_read_mail_digest_notifications, + read_mail_digest: true) + end + + it 'does not schedule reminder mail job' do + expect { perform_job } + .not_to enqueue_job(Mails::ReminderJob) + .with(paris_user_with_read_mail_digest_notifications.id) + end + end +end From 99c1387700925331c59dea6708ed235ea00281fb Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Sep 2021 10:52:36 +0200 Subject: [PATCH 05/21] extract `user to be notified` logic into scope --- app/models/user.rb | 3 +- .../having_reminder_mail_to_send_now.rb | 108 ++++++++++++ .../cron/schedule_reminder_mails_job.rb | 75 +------- .../having_reminder_mail_to_send_now_spec.rb | 161 ++++++++++++++++++ .../cron/schedule_reminder_mails_job_spec.rb | 152 +++-------------- 5 files changed, 299 insertions(+), 200 deletions(-) create mode 100644 app/models/users/scopes/having_reminder_mail_to_send_now.rb create mode 100644 spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb diff --git a/app/models/user.rb b/app/models/user.rb index cb21d88fc3..fafd653866 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -76,7 +76,8 @@ class User < Principal scopes :find_by_login, :newest, :notified_on_all, - :watcher_recipients + :watcher_recipients, + :having_reminder_mail_to_send_now def self.create_blocked_scope(scope, blocked) scope.where(blocked_condition(blocked)) diff --git a/app/models/users/scopes/having_reminder_mail_to_send_now.rb b/app/models/users/scopes/having_reminder_mail_to_send_now.rb new file mode 100644 index 0000000000..26b8c66b52 --- /dev/null +++ b/app/models/users/scopes/having_reminder_mail_to_send_now.rb @@ -0,0 +1,108 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See COPYRIGHT and LICENSE files for more details. +#++ + +module Users::Scopes + module HavingReminderMailToSendNow + extend ActiveSupport::Concern + + class_methods do + # Returns all users for which a reminder mails should be sent now. A user will be included if: + # * That user has an unread notification + # * The user hasn't been informed about the unread notification before + # * The user has configured reminder mails to be sent now. + def having_reminder_mail_to_send_now + # Left outer join as not all user instances have preferences associated + # but we still want to select them + recipient_candidates = User.active + .left_joins(:preference) + .where(where_statement) + + subscriber_ids = Notification + .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) + .group(:recipient_id) + .select(:recipient_id) + + User.where(id: subscriber_ids) + end + + def where_statement + current_timestamp_utc = Time.current.getutc + + age = age_statement(current_timestamp_utc) + <<-SQL.squish + #{age} < make_interval(mins=>30) + AND + #{age} >= make_interval(mins=>0) + SQL + end + + # Creates a SQL snippet for calculating the time between now and + # the reminder time slot with the each user's time zone + # @param [Time] current_timestamp_utc + def age_statement(current_timestamp_utc) + year = current_timestamp_utc.year + month = current_timestamp_utc.month + day = current_timestamp_utc.day + + case_statement = case_statement_zone_name_to_offset + + slot_time = Time.zone.parse(Setting.notification_email_digest_time) + + <<-SQL.squish + age( + now(), + make_timestamptz(#{year}, #{month}, #{day}, #{slot_time.hour}, #{slot_time.min}, 0, (#{case_statement})) + ) + SQL + end + + def case_statement_zone_name_to_offset + return @case_statement_zone_name_to_offset if @case_statement_zone_name_to_offset + + current_time = Time.current + statement = ActiveSupport::TimeZone.all.map do |zone| + offset = current_offset(zone, current_time) + "WHEN user_preferences.settings->>'time_zone' = '#{zone.name.gsub("'") { "''" }}' THEN '#{offset}'" + end + @case_statement_zone_name_to_offset = "CASE\n#{statement.join("\n")}\nELSE '+00:00'\nEND" + end + + # The real offset of a time zone depends of the moment we ask. Winter and summer time + # have different offsets of UTC. For instance, time zone "Berlin" in winter has +01:00 + # and in summer +02:00 + # @param [Time] time + # @param [TimeZone] zone + # @return [String] + def current_offset(zone, time = Time.current) + time.in_time_zone(zone).formatted_offset + end + end + end +end diff --git a/app/workers/cron/schedule_reminder_mails_job.rb b/app/workers/cron/schedule_reminder_mails_job.rb index 5a3a45ccda..cd3cc77e92 100644 --- a/app/workers/cron/schedule_reminder_mails_job.rb +++ b/app/workers/cron/schedule_reminder_mails_job.rb @@ -34,80 +34,9 @@ module Cron self.cron_expression = '/30 * * * *' def perform - subscribers_of_current_slot_having_notifications do |subscriber_ids| - subscriber_ids.each do |recipient_id| - Mails::ReminderJob.perform_later(recipient_id) - end + User.having_reminder_mail_to_send_now.pluck(:id).each do |subscriber_ids| + Mails::ReminderJob.perform_later(subscriber_ids) end end - - private - - def subscribers_of_current_slot_having_notifications - # Left outer join as not all user instances have preferences associated - # but we still want to select them - recipient_candidates = User.active - .left_joins(:preference) - .where(where_statement) - - subscriber_ids = Notification - .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) - .group(:recipient_id) - .pluck(:recipient_id) - - yield(subscriber_ids) - end - - def where_statement - current_timestamp_utc = Time.current.getutc - - age = age_statement(current_timestamp_utc) - <<-SQL.squish - #{age} < make_interval(mins=>30) - AND - #{age} >= make_interval(mins=>0) - SQL - end - - # Creates a SQL snippet for calculating the time between now and - # the reminder time slot with the each user's time zone - # @param [Time] current_timestamp_utc - def age_statement(current_timestamp_utc) - year = current_timestamp_utc.year - month = current_timestamp_utc.month - day = current_timestamp_utc.day - - case_statement = case_statement_zone_name_to_offset - - slot_time = Time.zone.parse(Setting.notification_email_digest_time) - - <<-SQL.squish - age( - now(), - make_timestamptz(#{year}, #{month}, #{day}, #{slot_time.hour}, #{slot_time.min}, 0, (#{case_statement})) - ) - SQL - end - - def case_statement_zone_name_to_offset - return @case_statement_zone_name_to_offset if @case_statement_zone_name_to_offset - - current_time = Time.current - statement = ActiveSupport::TimeZone.all.map do |zone| - offset = current_offset(zone, current_time) - "WHEN user_preferences.settings->>'time_zone' = '#{zone.name.gsub("'") { "''" }}' THEN '#{offset}'" - end - @case_statement_zone_name_to_offset = "CASE\n#{statement.join("\n")}\nELSE '+00:00'\nEND" - end - - # The real offset of a time zone depends of the moment we ask. Winter and summer time - # have different offsets of UTC. For instance, time zone "Berlin" in winter has +01:00 - # and in summer +02:00 - # @param [Time] time - # @param [TimeZone] zone - # @return [String] - def current_offset(zone, time = Time.current) - time.in_time_zone(zone).formatted_offset - end end end diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb new file mode 100644 index 0000000000..c61100ca9e --- /dev/null +++ b/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb @@ -0,0 +1,161 @@ +#-- encoding: UTF-8 + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2021 the OpenProject GmbH +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License version 3. +# +# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: +# Copyright (C) 2006-2013 Jean-Philippe Lang +# Copyright (C) 2010-2013 the ChiliProject Team +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# See docs/COPYRIGHT.rdoc for more details. +#++ + +require 'spec_helper' + +describe User, '.with_reminder_mail_to_send_now', type: :job do + subject(:scope) do + described_class.having_reminder_mail_to_send_now + end + + # As it is hard to mock Postgres's "now()" method, in the specs here we need to adopt the slot time + # relative to the local time of the user that we want to hit. + let(:current_utc_time) { Time.current.getutc } + let(:slot_time) { hitting_reminder_slot_for(hitting_user, current_utc_time) } # ie. "08:00", "08:30" + + let(:hitting_user) { paris_user } + let(:paris_user) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } # time_zone in winter is +01:00 + let(:moscow_user) { FactoryBot.create(:user, preferences: { time_zone: "Moscow" }) } # time_zone all year is +03:00 + let(:greenland_user) { FactoryBot.create(:user, preferences: { time_zone: "Greenland" }) } # time_zone in winter is -03:00 + let(:no_zone_user) { FactoryBot.create(:user) } # time_zone is nil + let(:notifications) { FactoryBot.create(:notification, recipient: hitting_user) } + + before do + allow(Setting).to receive(:notification_email_digest_time).and_return(slot_time) + allow(Time).to receive(:current).and_return(current_utc_time) + notifications + end + + context 'for a user whose local time is matching the configured time' do + let(:notifications) do + FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago) + end + + it 'contains the user' do + expect(scope) + .to match_array([paris_user]) + end + end + + context 'for a user whose local time is matching the configured time (in a non CET time zone)' do + let(:slot_time) { hitting_reminder_slot_for(moscow_user, current_utc_time) } + let(:notifications) do + FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + end + + it 'contains the user' do + expect(scope) + .to match_array([moscow_user]) + end + end + + context 'for a user whose local time is matching the configured time but without a notification' do + let(:notifications) do + # There is a notification for a different user + FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + end + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is matching the configured time but with an already read notification (IAN)' do + let(:notifications) do + FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago, read_ian: true) + end + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is matching the configured time but with an already read notification (reminder)' do + let(:notifications) do + FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago, read_mail_digest: true) + end + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is matching the configured time but with the user being inactive' do + let(:notifications) do + FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago) + end + + before do + paris_user.locked! + end + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is before the configured time' do + let(:notifications) do + FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + end + + it 'contains the user' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is after the configured time' do + let(:notifications) do + FactoryBot.create(:notification, recipient: greenland_user, created_at: 5.minutes.ago) + end + + it 'contains the user' do + expect(scope) + .to be_empty + end + end + + context 'for a user without a time zone' do + let(:slot_time) { hitting_reminder_slot_for(no_zone_user, current_utc_time) } + let(:notifications) do + FactoryBot.create(:notification, recipient: no_zone_user, created_at: 5.minutes.ago) + end + + it 'is including the user as UTC is assumed' do + expect(scope) + .to match_array([no_zone_user]) + end + end +end diff --git a/spec/workers/cron/schedule_reminder_mails_job_spec.rb b/spec/workers/cron/schedule_reminder_mails_job_spec.rb index 4da5dd5059..23a61cd8c1 100644 --- a/spec/workers/cron/schedule_reminder_mails_job_spec.rb +++ b/spec/workers/cron/schedule_reminder_mails_job_spec.rb @@ -31,137 +31,37 @@ require 'spec_helper' describe Cron::ScheduleReminderMailsJob, type: :job do - # As it is hard to mock Postgres's "now()" method, in the specs here we need to adopt the slot time - # relative to the local time of the user that we want to hit. - let(:current_utc_time) { Time.current.getutc } - let(:slot_time) { hitting_reminder_slot_for(hitting_user, current_utc_time) } # ie. "08:00", "08:30" + subject(:job) { described_class.perform_now } - let(:hitting_user) { paris_user } - let(:paris_user) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } # time_zone in winter is +01:00 - let(:no_zone_user) { FactoryBot.create(:user) } # time_zone is nil - let(:notifications) { FactoryBot.create(:notification, recipient: hitting_user) } - - subject(:perform_job) do - described_class.new.perform - end + let(:ids) { [23, 42] } before do - allow(Setting).to receive(:notification_email_digest_time).and_return(slot_time) - notifications - end - - context 'with paris_user as hitting_user' do - let(:moscow_user) { FactoryBot.create(:user, preferences: { time_zone: "Moscow" }) } # time_zone all year is +03:00 - let(:greenland_user) { FactoryBot.create(:user, preferences: { time_zone: "Greenland" }) } # time_zone in winter is -03:00 - let(:notifications) do - FactoryBot.create(:notification, recipient: hitting_user, created_at: 5.minutes.ago) - FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) - FactoryBot.create(:notification, recipient: greenland_user, created_at: 5.minutes.ago) - FactoryBot.create(:notification, recipient: no_zone_user, created_at: 5.minutes.ago) - end - - before do - allow(Time).to receive(:current).and_return(current_utc_time) - end - - it 'schedules ReminderMailJobs for all users that subscribed for that slot in their local time' do - # `hitting_user` is `paris_user`. - # `slot_time` (expressed string in local time) is set to be at the beginning of the first or second half hour - # block of the started hour. - expect { perform_job } - .to enqueue_job(Mails::ReminderJob) - .with(hitting_user.id) - - # `moscow_user` is in a different time zone (higher offset than Paris) so should not hit for the same `slot_time` - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(moscow_user.id) - - # `greenland_user` is in a different time (lower offset than Paris) so should not hit for the same `slot_time` - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(greenland_user.id) - - # `no_zone_user` should fall back to UTC time zone and thus have lower offset as `paris_user` and not hit - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(no_zone_user.id) - end - end - - context 'when slot_time in UTC' do - let(:hitting_user) { no_zone_user } - let(:notifications) do - FactoryBot.create(:notification, recipient: hitting_user, created_at: 5.minutes.ago) - end - - it 'schedules a job for users without timezone set' do - expect { perform_job } - .to enqueue_job(Mails::ReminderJob) - .with(hitting_user.id) - end - end - - context 'when hitting user is not active' do - let(:hitting_user) do - paris_user.locked! - paris_user - end - let(:notifications) do - FactoryBot.create(:notification, recipient: hitting_user) - end - - it 'does not schedule for users that are not active' do - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(hitting_user.id) - end - end - - context 'with a user without notifications' do - # Create another user just as `hitting_user` but without notifications - let(:paris_user_without_notifications) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } - - it 'does not schedule reminder mail job' do - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(paris_user_without_notifications.id) - end + scope = instance_double(ActiveRecord::Relation) + allow(User) + .to receive(:having_reminder_mail_to_send_now) + .and_return(scope) + + allow(scope) + .to receive(:pluck) + .with(:id) + .and_return(ids) end - context 'with a user with read IAN notifications' do - # Create another user just as `hitting_user` but without notifications - let(:paris_user_with_read_ian_notifications) do - FactoryBot.create(:user, preferences: { time_zone: "Paris" }) - end - let(:notifications) do - FactoryBot.create(:notification, - recipient: paris_user_with_read_ian_notifications, - read_ian: true) - end - - it 'does not schedule reminder mail job' do - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(paris_user_with_read_ian_notifications.id) - end - end - - context 'with a user with who already received a reminder for a notification' do - # Create another user just as `hitting_user` but without notifications - let(:paris_user_with_read_mail_digest_notifications) do - FactoryBot.create(:user, preferences: { time_zone: "Paris" }) - end - let(:notifications) do - FactoryBot.create(:notification, - recipient: paris_user_with_read_mail_digest_notifications, - read_mail_digest: true) - end - - it 'does not schedule reminder mail job' do - expect { perform_job } - .not_to enqueue_job(Mails::ReminderJob) - .with(paris_user_with_read_mail_digest_notifications.id) + describe '#perform' do + it 'schedules reminder jobs for every user with a reminder mails to be sent' do + expect { subject } + .to change(enqueued_jobs, :count) + .by(2) + + expect(Mails::ReminderJob) + .to have_been_enqueued + .exactly(:once) + .with(23) + + expect(Mails::ReminderJob) + .to have_been_enqueued + .exactly(:once) + .with(42) end end end From d025bf0ae051dc90229575b0d064eb8fc608032b Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Sep 2021 12:57:04 +0200 Subject: [PATCH 06/21] move notification schedule job --- .../{cron => notifications}/schedule_reminder_mails_job.rb | 4 ++-- .../schedule_reminder_mails_job_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename app/workers/{cron => notifications}/schedule_reminder_mails_job.rb (95%) rename spec/workers/{cron => notifications}/schedule_reminder_mails_job_spec.rb (96%) diff --git a/app/workers/cron/schedule_reminder_mails_job.rb b/app/workers/notifications/schedule_reminder_mails_job.rb similarity index 95% rename from app/workers/cron/schedule_reminder_mails_job.rb rename to app/workers/notifications/schedule_reminder_mails_job.rb index cd3cc77e92..ca55ed20ab 100644 --- a/app/workers/cron/schedule_reminder_mails_job.rb +++ b/app/workers/notifications/schedule_reminder_mails_job.rb @@ -28,8 +28,8 @@ # See docs/COPYRIGHT.rdoc for more details. #++ -module Cron - class ScheduleReminderMailsJob < CronJob +module Notifications + class ScheduleReminderMailsJob < Cron::CronJob # runs every 30min step, so 00:00, 00:30, 01:00, 01:30... self.cron_expression = '/30 * * * *' diff --git a/spec/workers/cron/schedule_reminder_mails_job_spec.rb b/spec/workers/notifications/schedule_reminder_mails_job_spec.rb similarity index 96% rename from spec/workers/cron/schedule_reminder_mails_job_spec.rb rename to spec/workers/notifications/schedule_reminder_mails_job_spec.rb index 23a61cd8c1..7ebe7dff82 100644 --- a/spec/workers/cron/schedule_reminder_mails_job_spec.rb +++ b/spec/workers/notifications/schedule_reminder_mails_job_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' -describe Cron::ScheduleReminderMailsJob, type: :job do +describe Notifications::ScheduleReminderMailsJob, type: :job do subject(:job) { described_class.perform_now } let(:ids) { [23, 42] } From 1aae2385662b1eaab647622a0b1cf1ace454f20b Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Sep 2021 13:09:42 +0200 Subject: [PATCH 07/21] include reminder feature spec at proper location --- .../notifications/digest_mail_spec.rb | 140 ------------------ .../notifications/reminder_mail_spec.rb | 78 +++++++++- 2 files changed, 76 insertions(+), 142 deletions(-) delete mode 100644 spec/features/notifications/digest_mail_spec.rb diff --git a/spec/features/notifications/digest_mail_spec.rb b/spec/features/notifications/digest_mail_spec.rb deleted file mode 100644 index 03b7b431ec..0000000000 --- a/spec/features/notifications/digest_mail_spec.rb +++ /dev/null @@ -1,140 +0,0 @@ -require 'spec_helper' -require 'support/pages/my/notifications' - -# TODO: This feature spec is to be replaced by the reminder_mail_spec.rb in the same directory. -describe "Digest email", type: :feature, js: true do - let!(:project) { FactoryBot.create :project, members: { current_user => role } } - let!(:mute_project) { FactoryBot.create :project, members: { current_user => role } } - let(:notification_settings_page) { Pages::My::Notifications.new(current_user) } - let(:role) { FactoryBot.create(:role, permissions: %i[view_work_packages]) } - let(:other_user) { FactoryBot.create(:user) } - let(:work_package) { FactoryBot.create(:work_package, project: project) } - let(:watched_work_package) { FactoryBot.create(:work_package, project: project, watcher_users: [current_user]) } - let(:involved_work_package) { FactoryBot.create(:work_package, project: project, assigned_to: current_user) } - - current_user do - FactoryBot.create :user, - notification_settings: [ - FactoryBot.build(:mail_notification_setting, - involved: false, - watched: false, - mentioned: false, - work_package_commented: false, - work_package_created: false, - work_package_processed: false, - work_package_prioritized: false, - work_package_scheduled: false, - all: false), - FactoryBot.build(:in_app_notification_setting, - involved: false, - watched: false, - mentioned: false, - work_package_commented: false, - work_package_created: false, - work_package_processed: false, - work_package_prioritized: false, - work_package_scheduled: false, - all: false), - FactoryBot.build(:mail_digest_notification_setting, - involved: true, - watched: true, - mentioned: true, - work_package_commented: true, - work_package_created: true, - work_package_processed: true, - work_package_prioritized: true, - work_package_scheduled: true, - all: false) - ] - end - - before do - allow(Setting).to receive(:notification_email_digest_time) - .and_return(hitting_reminder_slot_for(current_user)) - - watched_work_package - work_package - involved_work_package - - allow(CustomStyle.current) - .to receive(:logo).and_return(nil) - - ActiveJob::Base.queue_adapter.enqueued_jobs.clear - end - - it 'sends a digest mail based on the configuration', with_settings: { journal_aggregation_time_minutes: 0 } do - # Configure the digest - notification_settings_page.visit! - - notification_settings_page.expect_setting channel: :mail_digest, - project: nil, - involved: true, - mentioned: true, - watched: true, - work_package_commented: true, - work_package_created: true, - work_package_processed: true, - work_package_prioritized: true, - work_package_scheduled: true, - all: false - - notification_settings_page.configure_channel :mail_digest, - project: nil, - involved: false, - mentioned: true, - watched: true, - work_package_commented: false, - work_package_created: false, - work_package_processed: false, - work_package_prioritized: false, - work_package_scheduled: false, - all: false - - notification_settings_page.add_row(mute_project) - - notification_settings_page.configure_channel :mail_digest, - project: mute_project, - involved: false, - mentioned: false, - watched: false, - work_package_commented: false, - work_package_created: false, - work_package_processed: false, - work_package_prioritized: false, - work_package_scheduled: false, - all: false - - notification_settings_page.save - - # Perform some actions the user listens to - User.execute_as other_user do - note = <<~NOTE - Hey - @#{current_user.name} - - NOTE - - work_package.add_journal(other_user, note) - work_package.save! - - watched_work_package.subject = 'New watched work package subject' - watched_work_package.save! - - involved_work_package.description = 'New involved work package description' - involved_work_package.save! - end - - Cron::ScheduleReminderMailsJob.perform_later - perform_enqueued_jobs - perform_enqueued_jobs - - expect(ActionMailer::Base.deliveries.length) - .to be 1 - - expect(ActionMailer::Base.deliveries.first.subject) - .to eql "OpenProject - 1 unread notification including a mention" - end -end diff --git a/spec/features/notifications/reminder_mail_spec.rb b/spec/features/notifications/reminder_mail_spec.rb index 9e65361696..50ac70664c 100644 --- a/spec/features/notifications/reminder_mail_spec.rb +++ b/spec/features/notifications/reminder_mail_spec.rb @@ -41,7 +41,7 @@ describe "Reminder email", type: :feature, js: true do end end - context 'with the my page' do + context 'configuring via the my page' do let(:reminders_settings_page) { Pages::My::Reminders.new(current_user) } current_user do @@ -51,7 +51,7 @@ describe "Reminder email", type: :feature, js: true do it_behaves_like 'reminder settings' end - context 'with the user administration page' do + context 'configuring via the user administration page' do let(:reminders_settings_page) { Pages::Reminders::Settings.new(other_user) } let(:other_user) { FactoryBot.create :user } @@ -62,4 +62,78 @@ describe "Reminder email", type: :feature, js: true do it_behaves_like 'reminder settings' end + + context 'sending' do + let!(:project) { FactoryBot.create :project, members: { current_user => role } } + let!(:mute_project) { FactoryBot.create :project, members: { current_user => role } } + let(:role) { FactoryBot.create(:role, permissions: %i[view_work_packages]) } + let(:other_user) { FactoryBot.create(:user) } + let(:work_package) { FactoryBot.create(:work_package, project: project) } + let(:watched_work_package) { FactoryBot.create(:work_package, project: project, watcher_users: [current_user]) } + let(:involved_work_package) { FactoryBot.create(:work_package, project: project, assigned_to: current_user) } + + current_user do + FactoryBot.create :user, + notification_settings: [ + FactoryBot.build(:mail_digest_notification_setting, + involved: true, + watched: true, + mentioned: true, + work_package_commented: true, + work_package_created: true, + work_package_processed: true, + work_package_prioritized: true, + work_package_scheduled: true, + all: false) + ] + end + + before do + allow(Setting).to receive(:notification_email_digest_time) + .and_return(hitting_reminder_slot_for(current_user)) + + watched_work_package + work_package + involved_work_package + + allow(CustomStyle.current) + .to receive(:logo).and_return(nil) + + ActiveJob::Base.queue_adapter.enqueued_jobs.clear + end + + it 'sends a digest mail based on the configuration', with_settings: { journal_aggregation_time_minutes: 0 } do + # Perform some actions the user listens to + User.execute_as other_user do + note = <<~NOTE + Hey + @#{current_user.name} + + NOTE + + work_package.add_journal(other_user, note) + work_package.save! + + watched_work_package.subject = 'New watched work package subject' + watched_work_package.save! + + involved_work_package.description = 'New involved work package description' + involved_work_package.save! + end + + # The Job is triggered by time so we mock it and the jobs started by it being triggered + Notifications::ScheduleReminderMailsJob.perform_later + 2.times { perform_enqueued_jobs } + + expect(ActionMailer::Base.deliveries.length) + .to be 1 + + expect(ActionMailer::Base.deliveries.first.subject) + .to eql "OpenProject - 1 unread notification including a mention" + end + + end end From 7207e0b6bdd5e58731264fd4413c3903678302fe Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Sep 2021 15:08:19 +0200 Subject: [PATCH 08/21] use local time table to find reminder recipients --- .../having_reminder_mail_to_send_now.rb | 71 ++++++------------- .../schedule_reminder_mails_job.rb | 4 +- .../notifications/reminder_mail_spec.rb | 7 +- spec/support/schedule_reminder_mails.rb | 3 +- 4 files changed, 27 insertions(+), 58 deletions(-) diff --git a/app/models/users/scopes/having_reminder_mail_to_send_now.rb b/app/models/users/scopes/having_reminder_mail_to_send_now.rb index 26b8c66b52..0e5bc7e483 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send_now.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send_now.rb @@ -37,71 +37,42 @@ module Users::Scopes # * That user has an unread notification # * The user hasn't been informed about the unread notification before # * The user has configured reminder mails to be sent now. + # This assumes that users only have full hours specified for the time they desire + # to receive a reminder mail. def having_reminder_mail_to_send_now # Left outer join as not all user instances have preferences associated - # but we still want to select them - recipient_candidates = User.active - .left_joins(:preference) - .where(where_statement) + # but we still want to select them. + recipient_candidates = User + .active + .left_joins(:preference) + .joins(local_time_join) subscriber_ids = Notification .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) .group(:recipient_id) .select(:recipient_id) - User.where(id: subscriber_ids) + where(id: subscriber_ids) end - def where_statement - current_timestamp_utc = Time.current.getutc - - age = age_statement(current_timestamp_utc) - <<-SQL.squish - #{age} < make_interval(mins=>30) - AND - #{age} >= make_interval(mins=>0) - SQL - end - - # Creates a SQL snippet for calculating the time between now and - # the reminder time slot with the each user's time zone - # @param [Time] current_timestamp_utc - def age_statement(current_timestamp_utc) - year = current_timestamp_utc.year - month = current_timestamp_utc.month - day = current_timestamp_utc.day - - case_statement = case_statement_zone_name_to_offset - - slot_time = Time.zone.parse(Setting.notification_email_digest_time) - - <<-SQL.squish - age( - now(), - make_timestamptz(#{year}, #{month}, #{day}, #{slot_time.hour}, #{slot_time.min}, 0, (#{case_statement})) - ) + def local_time_join + <<~SQL.squish + JOIN (#{local_time_table}) AS local_times + ON COALESCE(user_preferences.settings->>'time_zone', 'UTC') = local_times.zone + AND local_times.time = '#{Setting.notification_email_digest_time}:00+00:00' SQL end - def case_statement_zone_name_to_offset - return @case_statement_zone_name_to_offset if @case_statement_zone_name_to_offset - + def local_time_table current_time = Time.current - statement = ActiveSupport::TimeZone.all.map do |zone| - offset = current_offset(zone, current_time) - "WHEN user_preferences.settings->>'time_zone' = '#{zone.name.gsub("'") { "''" }}' THEN '#{offset}'" - end - @case_statement_zone_name_to_offset = "CASE\n#{statement.join("\n")}\nELSE '+00:00'\nEND" - end - # The real offset of a time zone depends of the moment we ask. Winter and summer time - # have different offsets of UTC. For instance, time zone "Berlin" in winter has +01:00 - # and in summer +02:00 - # @param [Time] time - # @param [TimeZone] zone - # @return [String] - def current_offset(zone, time = Time.current) - time.in_time_zone(zone).formatted_offset + times_with_zones = ActiveSupport::TimeZone + .all + .map do |z| + [current_time.in_time_zone(z).strftime('%H:00:00+00:00'), z.name.gsub("'", "''")] + end + + "SELECT * FROM #{arel_table.grouping(Arel::Nodes::ValuesList.new(times_with_zones)).as('t(time, zone)').to_sql}" end end end diff --git a/app/workers/notifications/schedule_reminder_mails_job.rb b/app/workers/notifications/schedule_reminder_mails_job.rb index ca55ed20ab..21c0036cd7 100644 --- a/app/workers/notifications/schedule_reminder_mails_job.rb +++ b/app/workers/notifications/schedule_reminder_mails_job.rb @@ -30,8 +30,8 @@ module Notifications class ScheduleReminderMailsJob < Cron::CronJob - # runs every 30min step, so 00:00, 00:30, 01:00, 01:30... - self.cron_expression = '/30 * * * *' + # runs every hour, so 00:00, 01:00... + self.cron_expression = '0 * * * *' def perform User.having_reminder_mail_to_send_now.pluck(:id).each do |subscriber_ids| diff --git a/spec/features/notifications/reminder_mail_spec.rb b/spec/features/notifications/reminder_mail_spec.rb index 50ac70664c..0c136cbb5c 100644 --- a/spec/features/notifications/reminder_mail_spec.rb +++ b/spec/features/notifications/reminder_mail_spec.rb @@ -41,7 +41,7 @@ describe "Reminder email", type: :feature, js: true do end end - context 'configuring via the my page' do + context 'when configuring via the my page' do let(:reminders_settings_page) { Pages::My::Reminders.new(current_user) } current_user do @@ -51,7 +51,7 @@ describe "Reminder email", type: :feature, js: true do it_behaves_like 'reminder settings' end - context 'configuring via the user administration page' do + context 'when configuring via the user administration page' do let(:reminders_settings_page) { Pages::Reminders::Settings.new(other_user) } let(:other_user) { FactoryBot.create :user } @@ -63,7 +63,7 @@ describe "Reminder email", type: :feature, js: true do it_behaves_like 'reminder settings' end - context 'sending' do + describe 'sending' do let!(:project) { FactoryBot.create :project, members: { current_user => role } } let!(:mute_project) { FactoryBot.create :project, members: { current_user => role } } let(:role) { FactoryBot.create(:role, permissions: %i[view_work_packages]) } @@ -134,6 +134,5 @@ describe "Reminder email", type: :feature, js: true do expect(ActionMailer::Base.deliveries.first.subject) .to eql "OpenProject - 1 unread notification including a mention" end - end end diff --git a/spec/support/schedule_reminder_mails.rb b/spec/support/schedule_reminder_mails.rb index b94f904bee..ed4c0ee05f 100644 --- a/spec/support/schedule_reminder_mails.rb +++ b/spec/support/schedule_reminder_mails.rb @@ -29,6 +29,5 @@ ## # Calculates a slot in the user's local time that hits for scheduling reminder mail jobs def hitting_reminder_slot_for(user, current_utc_time = Time.current.getutc) - local_time = current_utc_time.in_time_zone(user.time_zone) - "#{'%02d' % local_time.hour}:#{local_time.min <= 30 ? '00' : '30'}" + current_utc_time.in_time_zone(user.time_zone).strftime('%H:00') end From 9114daf0d745f96bbaddbdc50edc37f76009b282 Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 14 Sep 2021 17:41:27 +0200 Subject: [PATCH 09/21] use daily_reminder settings to find reminder recipients --- .../having_reminder_mail_to_send_now.rb | 21 +- .../notifications/reminder_mail_spec.rb | 42 ++-- .../having_reminder_mail_to_send_now_spec.rb | 185 +++++++++++++++--- spec/support/schedule_reminder_mails.rb | 4 +- 4 files changed, 203 insertions(+), 49 deletions(-) diff --git a/app/models/users/scopes/having_reminder_mail_to_send_now.rb b/app/models/users/scopes/having_reminder_mail_to_send_now.rb index 0e5bc7e483..f3866b9103 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send_now.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send_now.rb @@ -37,8 +37,8 @@ module Users::Scopes # * That user has an unread notification # * The user hasn't been informed about the unread notification before # * The user has configured reminder mails to be sent now. - # This assumes that users only have full hours specified for the time they desire - # to receive a reminder mail. + # This assumes that users only have full hours specified for the times they desire + # to receive a reminder mail at. def having_reminder_mail_to_send_now # Left outer join as not all user instances have preferences associated # but we still want to select them. @@ -56,10 +56,25 @@ module Users::Scopes end def local_time_join + # Joins the times local to the user preferences and then checks whether: + # * reminders are enabled + # * any of the configured reminder time is the local time + # If no time zone is present, utc is assumed. + # If no reminder settings are present, sending a reminder at 08:00 local time is assumed. <<~SQL.squish JOIN (#{local_time_table}) AS local_times ON COALESCE(user_preferences.settings->>'time_zone', 'UTC') = local_times.zone - AND local_times.time = '#{Setting.notification_email_digest_time}:00+00:00' + AND ( + ( + user_preferences.settings->'daily_reminders'->'times' IS NULL + AND local_times.time = '08:00:00+00:00' + ) + OR + ( + (user_preferences.settings->'daily_reminders'->'enabled')::boolean + AND user_preferences.settings->'daily_reminders'->'times' ? local_times.time + ) + ) SQL end diff --git a/spec/features/notifications/reminder_mail_spec.rb b/spec/features/notifications/reminder_mail_spec.rb index 0c136cbb5c..037e556e0a 100644 --- a/spec/features/notifications/reminder_mail_spec.rb +++ b/spec/features/notifications/reminder_mail_spec.rb @@ -71,34 +71,40 @@ describe "Reminder email", type: :feature, js: true do let(:work_package) { FactoryBot.create(:work_package, project: project) } let(:watched_work_package) { FactoryBot.create(:work_package, project: project, watcher_users: [current_user]) } let(:involved_work_package) { FactoryBot.create(:work_package, project: project, assigned_to: current_user) } + let(:current_utc_time) { ActiveSupport::TimeZone['Hawaii'].parse("08:43").utc } current_user do - FactoryBot.create :user, - notification_settings: [ - FactoryBot.build(:mail_digest_notification_setting, - involved: true, - watched: true, - mentioned: true, - work_package_commented: true, - work_package_created: true, - work_package_processed: true, - work_package_prioritized: true, - work_package_scheduled: true, - all: false) - ] + FactoryBot.create( + :user, + preferences: { + time_zone: "Hawaii", + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("Hawaii", current_utc_time)] + } + }, + notification_settings: [ + FactoryBot.build(:mail_digest_notification_setting, + involved: true, + watched: true, + mentioned: true, + work_package_commented: true, + work_package_created: true, + work_package_processed: true, + work_package_prioritized: true, + work_package_scheduled: true, + all: false) + ] + ) end before do - allow(Setting).to receive(:notification_email_digest_time) - .and_return(hitting_reminder_slot_for(current_user)) + allow(Time).to receive(:current).and_return(current_utc_time) watched_work_package work_package involved_work_package - allow(CustomStyle.current) - .to receive(:logo).and_return(nil) - ActiveJob::Base.queue_adapter.enqueued_jobs.clear end diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb index c61100ca9e..cfd8c28b6d 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb @@ -35,29 +35,36 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do described_class.having_reminder_mail_to_send_now end - # As it is hard to mock Postgres's "now()" method, in the specs here we need to adopt the slot time + # As it is hard to mock PostgreSQL's "now()" method, in the specs here we need to adopt the slot time # relative to the local time of the user that we want to hit. let(:current_utc_time) { Time.current.getutc } - let(:slot_time) { hitting_reminder_slot_for(hitting_user, current_utc_time) } # ie. "08:00", "08:30" - let(:hitting_user) { paris_user } - let(:paris_user) { FactoryBot.create(:user, preferences: { time_zone: "Paris" }) } # time_zone in winter is +01:00 - let(:moscow_user) { FactoryBot.create(:user, preferences: { time_zone: "Moscow" }) } # time_zone all year is +03:00 - let(:greenland_user) { FactoryBot.create(:user, preferences: { time_zone: "Greenland" }) } # time_zone in winter is -03:00 - let(:no_zone_user) { FactoryBot.create(:user) } # time_zone is nil - let(:notifications) { FactoryBot.create(:notification, recipient: hitting_user) } + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: { + time_zone: "Paris", + daily_reminders: paris_user_daily_reminders + } + ) + end + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [hitting_reminder_slot_for("Paris", current_utc_time)] + } + end + let(:notifications) { FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago) } + let(:users) { [paris_user] } before do - allow(Setting).to receive(:notification_email_digest_time).and_return(slot_time) allow(Time).to receive(:current).and_return(current_utc_time) notifications + users end context 'for a user whose local time is matching the configured time' do - let(:notifications) do - FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago) - end - it 'contains the user' do expect(scope) .to match_array([paris_user]) @@ -65,10 +72,23 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do end context 'for a user whose local time is matching the configured time (in a non CET time zone)' do - let(:slot_time) { hitting_reminder_slot_for(moscow_user, current_utc_time) } + let(:moscow_user) do + FactoryBot.create( + :user, + firstname: 'Moscow', + preferences: { + time_zone: "Moscow", + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("Moscow", current_utc_time)] + } + } + ) + end let(:notifications) do FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) end + let(:users) { [moscow_user] } it 'contains the user' do expect(scope) @@ -76,10 +96,27 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do end end + context 'for a user whose local time is matching one of the configured times' do + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [ + hitting_reminder_slot_for("Paris", current_utc_time - 3.hours), + hitting_reminder_slot_for("Paris", current_utc_time), + hitting_reminder_slot_for("Paris", current_utc_time + 3.hours) + ] + } + end + + it 'contains the user' do + expect(scope) + .to match_array([paris_user]) + end + end + context 'for a user whose local time is matching the configured time but without a notification' do let(:notifications) do - # There is a notification for a different user - FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + nil end it 'is empty' do @@ -88,6 +125,56 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do end end + context 'for a user whose local time is matching the configured time but with the reminder being deactivated' do + let(:paris_user_daily_reminders) do + { + enabled: false, + times: [hitting_reminder_slot_for("Paris", current_utc_time)] + } + end + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user whose local time is matching the configured time but without a daily_reminders setting at 8:00' do + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: { + time_zone: "Paris" + } + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['Paris'].parse("08:00").utc } + + it 'contains the user' do + expect(scope) + .to match_array([paris_user]) + end + end + + context 'for a user whose local time is matching the configured time but without a daily_reminders setting at 10:00' do + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: { + time_zone: "Paris" + } + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['Paris'].parse("10:00").utc } + + it 'is empty' do + expect(scope) + .to be_empty + end + end + context 'for a user whose local time is matching the configured time but with an already read notification (IAN)' do let(:notifications) do FactoryBot.create(:notification, recipient: paris_user, created_at: 5.minutes.ago, read_ian: true) @@ -126,36 +213,82 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do end context 'for a user whose local time is before the configured time' do - let(:notifications) do - FactoryBot.create(:notification, recipient: moscow_user, created_at: 5.minutes.ago) + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [hitting_reminder_slot_for("Paris", current_utc_time + 1.hour)] + } end - it 'contains the user' do + it 'is empty' do expect(scope) .to be_empty end end context 'for a user whose local time is after the configured time' do - let(:notifications) do - FactoryBot.create(:notification, recipient: greenland_user, created_at: 5.minutes.ago) + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [hitting_reminder_slot_for("Paris", current_utc_time - 1.hour)] + } end - it 'contains the user' do + it 'is empty' do expect(scope) .to be_empty end end context 'for a user without a time zone' do - let(:slot_time) { hitting_reminder_slot_for(no_zone_user, current_utc_time) } - let(:notifications) do - FactoryBot.create(:notification, recipient: no_zone_user, created_at: 5.minutes.ago) + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: { + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("UTC", current_utc_time)] + } + } + ) end it 'is including the user as UTC is assumed' do expect(scope) - .to match_array([no_zone_user]) + .to match_array([paris_user]) + end + end + + context 'for a user without a time zone and daily_reminders at 08:00' do + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: {} + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("08:00").utc } + + it 'is including the user as UTC at 08:00 is assumed' do + expect(scope) + .to match_array([paris_user]) + end + end + + context 'for a user without a time zone and daily_reminders at 10:00' do + let(:paris_user) do + FactoryBot.create( + :user, + firstname: 'Paris', + preferences: {} + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } + + it 'is empty as UTC at 08:00 is assumed' do + expect(scope) + .to be_empty end end end diff --git a/spec/support/schedule_reminder_mails.rb b/spec/support/schedule_reminder_mails.rb index ed4c0ee05f..e30e8ab4d7 100644 --- a/spec/support/schedule_reminder_mails.rb +++ b/spec/support/schedule_reminder_mails.rb @@ -28,6 +28,6 @@ ## # Calculates a slot in the user's local time that hits for scheduling reminder mail jobs -def hitting_reminder_slot_for(user, current_utc_time = Time.current.getutc) - current_utc_time.in_time_zone(user.time_zone).strftime('%H:00') +def hitting_reminder_slot_for(time_zone, current_utc_time = Time.current.getutc) + current_utc_time.in_time_zone(ActiveSupport::TimeZone[time_zone]).strftime('%H:00:00+00:00') end From be0d53648d03fe770941b865dfb5cf579db38e51 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 14:53:45 +0200 Subject: [PATCH 10/21] remove duplicate I18n key --- config/locales/en.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/config/locales/en.yml b/config/locales/en.yml index 92eb5de015..1c302136b5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -905,7 +905,6 @@ en: firstname: "First name" group: "Group" groups: "Groups" - name: "Group name" id: "ID" is_default: "Default value" is_for_all: "For all projects" From 853cd81bbb9b2d3465c8afb43d93b8997c5fba1c Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 17:20:05 +0200 Subject: [PATCH 11/21] handle having missed a slot for the reminder mails --- app/models/user.rb | 2 +- ...now.rb => having_reminder_mail_to_send.rb} | 45 +++++++++---- .../schedule_reminder_mails_job.rb | 6 +- ...b => having_reminder_mail_to_send_spec.rb} | 50 +++++++++++++- .../schedule_reminder_mails_job_spec.rb | 66 ++++++++++++++----- 5 files changed, 136 insertions(+), 33 deletions(-) rename app/models/users/scopes/{having_reminder_mail_to_send_now.rb => having_reminder_mail_to_send.rb} (69%) rename spec/models/users/scopes/{having_reminder_mail_to_send_now_spec.rb => having_reminder_mail_to_send_spec.rb} (84%) diff --git a/app/models/user.rb b/app/models/user.rb index fafd653866..f3a440f0bd 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -77,7 +77,7 @@ class User < Principal :newest, :notified_on_all, :watcher_recipients, - :having_reminder_mail_to_send_now + :having_reminder_mail_to_send def self.create_blocked_scope(scope, blocked) scope.where(blocked_condition(blocked)) diff --git a/app/models/users/scopes/having_reminder_mail_to_send_now.rb b/app/models/users/scopes/having_reminder_mail_to_send.rb similarity index 69% rename from app/models/users/scopes/having_reminder_mail_to_send_now.rb rename to app/models/users/scopes/having_reminder_mail_to_send.rb index f3866b9103..14aab6442b 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send_now.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send.rb @@ -29,7 +29,7 @@ #++ module Users::Scopes - module HavingReminderMailToSendNow + module HavingReminderMailToSend extend ActiveSupport::Concern class_methods do @@ -39,13 +39,17 @@ module Users::Scopes # * The user has configured reminder mails to be sent now. # This assumes that users only have full hours specified for the times they desire # to receive a reminder mail at. - def having_reminder_mail_to_send_now + # @param [DateTime] earliest_time The earliest time to consider as a matching slot. All full hours from that time + # to now are included. + # Only the time part is used and that is floored to the hour (e.g. 2021-05-03 10:34:12+02:00 -> 08:00:00). + # Needs to be before the current time. + def having_reminder_mail_to_send(earliest_time) # Left outer join as not all user instances have preferences associated # but we still want to select them. recipient_candidates = User .active .left_joins(:preference) - .joins(local_time_join) + .joins(local_time_join(earliest_time)) subscriber_ids = Notification .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) @@ -55,14 +59,14 @@ module Users::Scopes where(id: subscriber_ids) end - def local_time_join + def local_time_join(earliest_time) # Joins the times local to the user preferences and then checks whether: # * reminders are enabled # * any of the configured reminder time is the local time # If no time zone is present, utc is assumed. # If no reminder settings are present, sending a reminder at 08:00 local time is assumed. <<~SQL.squish - JOIN (#{local_time_table}) AS local_times + JOIN (#{local_time_table(earliest_time)}) AS local_times ON COALESCE(user_preferences.settings->>'time_zone', 'UTC') = local_times.zone AND ( ( @@ -78,16 +82,31 @@ module Users::Scopes SQL end - def local_time_table - current_time = Time.current + def local_time_table(earliest_time) + times = hours_between_earliest_and_now(earliest_time) - times_with_zones = ActiveSupport::TimeZone - .all - .map do |z| - [current_time.in_time_zone(z).strftime('%H:00:00+00:00'), z.name.gsub("'", "''")] - end + values_list = Arel::Nodes::ValuesList.new(times_for_zones(times)) - "SELECT * FROM #{arel_table.grouping(Arel::Nodes::ValuesList.new(times_with_zones)).as('t(time, zone)').to_sql}" + "SELECT * FROM #{arel_table.grouping(values_list).as('t(time, zone)').to_sql}" + end + + def times_for_zones(times) + ActiveSupport::TimeZone + .all + .map do |z| + times.map do |time| + [time.in_time_zone(z).strftime('%H:00:00+00:00'), z.name.gsub("'", "''")] + end + end + .flatten(1) + end + + def hours_between_earliest_and_now(earliest_time) + raise ArgumentError if Time.current - earliest_time < 0 + + (0..(((Time.current - earliest_time) / 1.hour).round)).map do |i| + (earliest_time + i.hours).utc + end end end end diff --git a/app/workers/notifications/schedule_reminder_mails_job.rb b/app/workers/notifications/schedule_reminder_mails_job.rb index 21c0036cd7..6aea431fcd 100644 --- a/app/workers/notifications/schedule_reminder_mails_job.rb +++ b/app/workers/notifications/schedule_reminder_mails_job.rb @@ -34,9 +34,13 @@ module Notifications self.cron_expression = '0 * * * *' def perform - User.having_reminder_mail_to_send_now.pluck(:id).each do |subscriber_ids| + User.having_reminder_mail_to_send(run_at).pluck(:id).each do |subscriber_ids| Mails::ReminderJob.perform_later(subscriber_ids) end end + + def run_at + self.class.delayed_job.run_at + end end end diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb similarity index 84% rename from spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb rename to spec/models/users/scopes/having_reminder_mail_to_send_spec.rb index cfd8c28b6d..fe41aa2895 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_now_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb @@ -30,14 +30,15 @@ require 'spec_helper' -describe User, '.with_reminder_mail_to_send_now', type: :job do +describe User, '.with_reminder_mail_to_send', type: :job do subject(:scope) do - described_class.having_reminder_mail_to_send_now + described_class.having_reminder_mail_to_send(scope_time) end # As it is hard to mock PostgreSQL's "now()" method, in the specs here we need to adopt the slot time # relative to the local time of the user that we want to hit. let(:current_utc_time) { Time.current.getutc } + let(:scope_time) { Time.current } let(:paris_user) do FactoryBot.create( @@ -114,6 +115,42 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do end end + context 'for a user who has configured a slot between the earliest_time (in local time) and his current local time' do + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [ + hitting_reminder_slot_for("Paris", current_utc_time - 2.hours), + hitting_reminder_slot_for("Paris", current_utc_time + 3.hours) + ] + } + end + let(:scope_time) { Time.current - 2.hours } + + it 'contains the user' do + expect(scope) + .to match_array([paris_user]) + end + end + + context 'for a user who has configured a slot before the earliest_time (in local time) and after his current local time' do + let(:paris_user_daily_reminders) do + { + enabled: true, + times: [ + hitting_reminder_slot_for("Paris", current_utc_time - 3.hours), + hitting_reminder_slot_for("Paris", current_utc_time + 1.hour) + ] + } + end + let(:scope_time) { Time.current - 2.hours } + + it 'is empty' do + expect(scope) + .to be_empty + end + end + context 'for a user whose local time is matching the configured time but without a notification' do let(:notifications) do nil @@ -291,4 +328,13 @@ describe User, '.with_reminder_mail_to_send_now', type: :job do .to be_empty end end + + context 'when the provided scope_time is after the current time' do + let(:scope_time) { Time.current + 1.minute } + + it 'raises an error' do + expect { scope } + .to raise_error ArgumentError + end + end end diff --git a/spec/workers/notifications/schedule_reminder_mails_job_spec.rb b/spec/workers/notifications/schedule_reminder_mails_job_spec.rb index 7ebe7dff82..5d5cf8569f 100644 --- a/spec/workers/notifications/schedule_reminder_mails_job_spec.rb +++ b/spec/workers/notifications/schedule_reminder_mails_job_spec.rb @@ -31,14 +31,28 @@ require 'spec_helper' describe Notifications::ScheduleReminderMailsJob, type: :job do - subject(:job) { described_class.perform_now } + subject(:job) { scheduled_job.invoke_job } + + let(:scheduled_job) do + described_class.ensure_scheduled! + + Delayed::Job.first + end let(:ids) { [23, 42] } + let(:run_at) { scheduled_job.run_at } before do + # We need to access the job as stored in the database to get at the run_at time persisted there + allow(ActiveJob::Base) + .to receive(:queue_adapter) + .and_return(ActiveJob::QueueAdapters::DelayedJobAdapter.new) + + scheduled_job.update_column(:run_at, run_at) + scope = instance_double(ActiveRecord::Relation) allow(User) - .to receive(:having_reminder_mail_to_send_now) + .to receive(:having_reminder_mail_to_send) .and_return(scope) allow(scope) @@ -48,20 +62,40 @@ describe Notifications::ScheduleReminderMailsJob, type: :job do end describe '#perform' do - it 'schedules reminder jobs for every user with a reminder mails to be sent' do - expect { subject } - .to change(enqueued_jobs, :count) - .by(2) - - expect(Mails::ReminderJob) - .to have_been_enqueued - .exactly(:once) - .with(23) - - expect(Mails::ReminderJob) - .to have_been_enqueued - .exactly(:once) - .with(42) + shared_examples_for 'schedules reminder mails' do + it 'schedules reminder jobs for every user with a reminder mails to be sent' do + expect { subject } + .to change(Delayed::Job, :count) + .by(2) + + jobs = Delayed::Job.all.map do |job| + YAML.safe_load(job.handler, permitted_classes: [ActiveJob::QueueAdapters::DelayedJobAdapter::JobWrapper]) + end + + reminder_jobs = jobs.select { |job| job.job_data['job_class'] == "Mails::ReminderJob" } + + expect(reminder_jobs[0].job_data['arguments']) + .to match_array([23]) + + expect(reminder_jobs[1].job_data['arguments']) + .to match_array([42]) + end + + it 'queries with the intended job execution time (which might have been missed due to high load)' do + subject + + expect(User) + .to have_received(:having_reminder_mail_to_send) + .with(run_at) + end + end + + it_behaves_like 'schedules reminder mails' + + context 'with a job that missed some runs' do + let(:run_at) { scheduled_job.run_at - 3.hours } + + it_behaves_like 'schedules reminder mails' end end end From fff18d0e3157ffc34bcda77f9964bc625681fc76 Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 17:28:27 +0200 Subject: [PATCH 12/21] safeguard against large time differences --- app/models/users/scopes/having_reminder_mail_to_send.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/users/scopes/having_reminder_mail_to_send.rb b/app/models/users/scopes/having_reminder_mail_to_send.rb index 14aab6442b..a64b0ff78e 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send.rb @@ -104,7 +104,7 @@ module Users::Scopes def hours_between_earliest_and_now(earliest_time) raise ArgumentError if Time.current - earliest_time < 0 - (0..(((Time.current - earliest_time) / 1.hour).round)).map do |i| + (0..[((Time.current - earliest_time) / 1.hour).round, 24].min).map do |i| (earliest_time + i.hours).utc end end From 750b300d20db2577aea81ba003daab9f47fd193d Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 17:45:12 +0200 Subject: [PATCH 13/21] remove global digest_time configuration --- .../settings/mail_notifications_settings/show.html.erb | 8 -------- config/locales/en.yml | 2 -- config/settings.yml | 2 -- 3 files changed, 12 deletions(-) diff --git a/app/views/admin/settings/mail_notifications_settings/show.html.erb b/app/views/admin/settings/mail_notifications_settings/show.html.erb index 1f1e33984b..4be305d8f1 100644 --- a/app/views/admin/settings/mail_notifications_settings/show.html.erb +++ b/app/views/admin/settings/mail_notifications_settings/show.html.erb @@ -46,14 +46,6 @@ See COPYRIGHT and LICENSE files for more details. <%= t(:'settings.notifications.delay_minutes_explanation') %> -
- <%= setting_time_field :notification_email_digest_time, - container_class: '-xslim', - unit: 'UTC' %> -
- <%= t(:'settings.notifications.email_digest_explanation') %> -
-
<%= t(:setting_emails_header) %> & <%= t(:setting_emails_footer) %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 1c302136b5..4452ea7520 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2452,7 +2452,6 @@ en: setting_enabled_projects_columns: "Visible in project list" setting_notification_retention_period_days: "Notification retention period" setting_notification_email_delay_minutes: "Email sending delay" - setting_notification_email_digest_time: "Email digest time" setting_feeds_enabled: "Enable Feeds" setting_feeds_limit: "Feed content limit" setting_file_max_size_displayed: "Max size of text files displayed inline" @@ -2535,7 +2534,6 @@ en: Set the number of days notification events for users (the source for in-app notifications) will be kept in the system. Any events older than this time will be deleted. delay_minutes_explanation: "Email sending can be delayed to allow users with configured in app notification to confirm the notification within the application before a mail is sent out. Users who read a notification within the application will not receive an email for the already read notification." - email_digest_explanation: "Once a day, an email digest can be sent out containing a collection of all the notification users subscribed to. The setting is relative to each users configured time zone, so e.g. 8:00 will be executed at 7:00 UTC for users in UTC+1 and 9:00 UTC for those in UTC-1." events_explanation: 'Governs for which event an email is sent out. Work packages are excluded from this list as the notifications for them can be configured specifically for every user.' display: first_date_of_week_and_year_set: > diff --git a/config/settings.yml b/config/settings.yml index 0f28c025fe..eeb05506f3 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -385,5 +385,3 @@ notification_retention_period_days: notification_email_delay_minutes: default: 15 format: int -notification_email_digest_time: - default: '08:00' From 57ed3c8fd3b26a24850ac283da0f1182d58ea82e Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 17:58:27 +0200 Subject: [PATCH 14/21] add indices to support finding notification recipients --- ...656_add_user_preference_settings_indices.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 db/migrate/20210915154656_add_user_preference_settings_indices.rb diff --git a/db/migrate/20210915154656_add_user_preference_settings_indices.rb b/db/migrate/20210915154656_add_user_preference_settings_indices.rb new file mode 100644 index 0000000000..fe288fd0d7 --- /dev/null +++ b/db/migrate/20210915154656_add_user_preference_settings_indices.rb @@ -0,0 +1,18 @@ +class AddUserPreferenceSettingsIndices < ActiveRecord::Migration[6.1] + def change + add_index :user_preferences, + "(settings->'daily_reminders'->'enabled')", + using: :gin, + name: 'index_user_prefs_settings_daily_reminders_enabled' + + add_index :user_preferences, + "(settings->'daily_reminders'->'times')", + using: :gin, + name: 'index_user_prefs_settings_daily_reminders_times' + + add_index :user_preferences, + "(settings->'time_zone')", + using: :gin, + name: 'index_user_prefs_settings_time_zone' + end +end From 718b2df8f5fa93f9e1474b0d85838510fcfc255d Mon Sep 17 00:00:00 2001 From: ulferts Date: Wed, 15 Sep 2021 18:07:35 +0200 Subject: [PATCH 15/21] mock the delayed_job backend --- spec/features/notifications/reminder_mail_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/features/notifications/reminder_mail_spec.rb b/spec/features/notifications/reminder_mail_spec.rb index 037e556e0a..74e0625d5b 100644 --- a/spec/features/notifications/reminder_mail_spec.rb +++ b/spec/features/notifications/reminder_mail_spec.rb @@ -106,6 +106,12 @@ describe "Reminder email", type: :feature, js: true do involved_work_package ActiveJob::Base.queue_adapter.enqueued_jobs.clear + + # There is no delayed_job associated when using the testing backend of ActiveJob + # so we have to mock it. + allow(Notifications::ScheduleReminderMailsJob) + .to receive(:delayed_job) + .and_return(instance_double(Delayed::Backend::ActiveRecord::Job, run_at: Time.current.utc)) end it 'sends a digest mail based on the configuration', with_settings: { journal_aggregation_time_minutes: 0 } do From 53989fbc6a1055834fa6ec7990548ae5cb750b61 Mon Sep 17 00:00:00 2001 From: ulferts Date: Mon, 20 Sep 2021 15:35:38 +0200 Subject: [PATCH 16/21] Rename variable Co-authored-by: Wieland Lindenthal --- app/workers/notifications/schedule_reminder_mails_job.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/notifications/schedule_reminder_mails_job.rb b/app/workers/notifications/schedule_reminder_mails_job.rb index 6aea431fcd..0307edda80 100644 --- a/app/workers/notifications/schedule_reminder_mails_job.rb +++ b/app/workers/notifications/schedule_reminder_mails_job.rb @@ -34,8 +34,8 @@ module Notifications self.cron_expression = '0 * * * *' def perform - User.having_reminder_mail_to_send(run_at).pluck(:id).each do |subscriber_ids| - Mails::ReminderJob.perform_later(subscriber_ids) + User.having_reminder_mail_to_send(run_at).pluck(:id).each do |user_id| + Mails::ReminderJob.perform_later(user_id) end end From f0692790c575371a798b8da37e0a587ab6ea2b80 Mon Sep 17 00:00:00 2001 From: ulferts Date: Mon, 20 Sep 2021 15:33:52 +0200 Subject: [PATCH 17/21] add test case for user without preferences --- .../having_reminder_mail_to_send_spec.rb | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb index fe41aa2895..875a376d90 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' -describe User, '.with_reminder_mail_to_send', type: :job do +describe User, '.having_reminder_mail_to_send', type: :job do subject(:scope) do described_class.having_reminder_mail_to_send(scope_time) end @@ -337,4 +337,30 @@ describe User, '.with_reminder_mail_to_send', type: :job do .to raise_error ArgumentError end end + + context 'for a user without preferences at 08:00' do + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("08:00").utc } + + before do + paris_user.pref.destroy + end + + it 'is including the user as UTC at 08:00 is assumed' do + expect(scope) + .to match_array([paris_user]) + end + end + + context 'for a user without preferences at 10:00' do + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } + + before do + paris_user.pref.destroy + end + + it 'is empty as UTC at 08:00 is assumed' do + expect(scope) + .to be_empty + end + end end From 7aa850fb6590903300ec58d8e7a3dfe534684c9f Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 21 Sep 2021 09:44:31 +0200 Subject: [PATCH 18/21] fix spec type Co-authored-by: Wieland Lindenthal --- spec/models/users/scopes/having_reminder_mail_to_send_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb index 875a376d90..3fdfc76e0b 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb @@ -30,7 +30,7 @@ require 'spec_helper' -describe User, '.having_reminder_mail_to_send', type: :job do +describe User, '.having_reminder_mail_to_send', type: :model do subject(:scope) do described_class.having_reminder_mail_to_send(scope_time) end From 4cd9dac8dd5cef17f5f6de631e300ede4694b1cb Mon Sep 17 00:00:00 2001 From: ulferts Date: Tue, 21 Sep 2021 18:09:04 +0200 Subject: [PATCH 19/21] take non hour time zone offsets into account --- .../scopes/having_reminder_mail_to_send.rb | 51 ++++++--- .../schedule_reminder_mails_job.rb | 4 +- .../notifications/reminder_mail_spec.rb | 10 +- .../having_reminder_mail_to_send_spec.rb | 107 +++++++++++++++++- 4 files changed, 148 insertions(+), 24 deletions(-) diff --git a/app/models/users/scopes/having_reminder_mail_to_send.rb b/app/models/users/scopes/having_reminder_mail_to_send.rb index a64b0ff78e..7ff5c829fa 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send.rb @@ -36,20 +36,25 @@ module Users::Scopes # Returns all users for which a reminder mails should be sent now. A user will be included if: # * That user has an unread notification # * The user hasn't been informed about the unread notification before - # * The user has configured reminder mails to be sent now. + # * The user has configured reminder mails to be within the time frame between the provided time and now. # This assumes that users only have full hours specified for the times they desire # to receive a reminder mail at. - # @param [DateTime] earliest_time The earliest time to consider as a matching slot. All full hours from that time + # @param [DateTime] earliest_time The earliest time to consider as a matching slot. All quarter hours from that time # to now are included. - # Only the time part is used and that is floored to the hour (e.g. 2021-05-03 10:34:12+02:00 -> 08:00:00). + # Only the time part is used which is moved forward to the next quarter hour (e.g. 2021-05-03 10:34:12+02:00 -> 08:45:00). + # This is done because time zones always have a mod(15) == 0 minutes offset. # Needs to be before the current time. def having_reminder_mail_to_send(earliest_time) + local_times = local_times_from(earliest_time) + + return none if local_times.empty? + # Left outer join as not all user instances have preferences associated # but we still want to select them. recipient_candidates = User .active .left_joins(:preference) - .joins(local_time_join(earliest_time)) + .joins(local_time_join(local_times)) subscriber_ids = Notification .unsent_reminders_before(recipient: recipient_candidates, time: Time.current) @@ -59,14 +64,16 @@ module Users::Scopes where(id: subscriber_ids) end - def local_time_join(earliest_time) + def local_time_join(local_times) # Joins the times local to the user preferences and then checks whether: # * reminders are enabled # * any of the configured reminder time is the local time # If no time zone is present, utc is assumed. # If no reminder settings are present, sending a reminder at 08:00 local time is assumed. <<~SQL.squish - JOIN (#{local_time_table(earliest_time)}) AS local_times + JOIN ( + SELECT * FROM #{arel_table.grouping(Arel::Nodes::ValuesList.new(local_times)).as('t(time, zone)').to_sql} + ) AS local_times ON COALESCE(user_preferences.settings->>'time_zone', 'UTC') = local_times.zone AND ( ( @@ -82,12 +89,10 @@ module Users::Scopes SQL end - def local_time_table(earliest_time) - times = hours_between_earliest_and_now(earliest_time) + def local_times_from(earliest_time) + times = quarters_between_earliest_and_now(earliest_time) - values_list = Arel::Nodes::ValuesList.new(times_for_zones(times)) - - "SELECT * FROM #{arel_table.grouping(values_list).as('t(time, zone)').to_sql}" + times_for_zones(times) end def times_for_zones(times) @@ -95,19 +100,33 @@ module Users::Scopes .all .map do |z| times.map do |time| - [time.in_time_zone(z).strftime('%H:00:00+00:00'), z.name.gsub("'", "''")] + local_time = time.in_time_zone(z) + + # Since only full hours can be configured, we can disregard any local time that is not + # a full hour. + next if local_time.min != 0 + + [local_time.strftime('%H:00:00+00:00'), z.name.gsub("'", "''")] end end .flatten(1) + .compact end - def hours_between_earliest_and_now(earliest_time) - raise ArgumentError if Time.current - earliest_time < 0 + def quarters_between_earliest_and_now(earliest_time) + latest_time = Time.current + raise ArgumentError if latest_time < earliest_time || (latest_time - earliest_time) > 1.day + + quarters = ((latest_time - earliest_time) / 60 / 15).floor - (0..[((Time.current - earliest_time) / 1.hour).round, 24].min).map do |i| - (earliest_time + i.hours).utc + (1..quarters).each_with_object([next_quarter_hour(earliest_time)]) do |i, times| + times << times.last + i * 15.minutes end end + + def next_quarter_hour(time) + (time + (time.min % 15 == 0 ? 0.minutes : (15 - time.min % 15).minutes)) + end end end end diff --git a/app/workers/notifications/schedule_reminder_mails_job.rb b/app/workers/notifications/schedule_reminder_mails_job.rb index 0307edda80..bc0f0e4530 100644 --- a/app/workers/notifications/schedule_reminder_mails_job.rb +++ b/app/workers/notifications/schedule_reminder_mails_job.rb @@ -30,8 +30,8 @@ module Notifications class ScheduleReminderMailsJob < Cron::CronJob - # runs every hour, so 00:00, 01:00... - self.cron_expression = '0 * * * *' + # runs every quarter of an hour, so 00:00, 00:15... + self.cron_expression = '*/15 * * * *' def perform User.having_reminder_mail_to_send(run_at).pluck(:id).each do |user_id| diff --git a/spec/features/notifications/reminder_mail_spec.rb b/spec/features/notifications/reminder_mail_spec.rb index 51e9e38d7f..b34dfb59a8 100644 --- a/spec/features/notifications/reminder_mail_spec.rb +++ b/spec/features/notifications/reminder_mail_spec.rb @@ -71,7 +71,13 @@ describe "Reminder email", type: :feature, js: true do let(:work_package) { FactoryBot.create(:work_package, project: project) } let(:watched_work_package) { FactoryBot.create(:work_package, project: project, watcher_users: [current_user]) } let(:involved_work_package) { FactoryBot.create(:work_package, project: project, assigned_to: current_user) } - let(:current_utc_time) { ActiveSupport::TimeZone['Hawaii'].parse("08:43").utc } + # The run_at time of the delayed job used for scheduling the reminder mails + # needs to be within a time frame eligible for sending out mails for the chose + # time zone. For the time zone Hawaii (UTC-10) this means between 8:00:00 and 8:14:59 UTC. + # The job is scheduled to run every 15 min so the run_at will in production always move between the quarters of an hour. + # The current time can be way behind that. + let(:current_utc_time) { ActiveSupport::TimeZone['Hawaii'].parse("08:34:10").utc } + let(:job_run_at) { ActiveSupport::TimeZone['Hawaii'].parse("08:00").utc } current_user do FactoryBot.create( @@ -112,7 +118,7 @@ describe "Reminder email", type: :feature, js: true do # so we have to mock it. allow(Notifications::ScheduleReminderMailsJob) .to receive(:delayed_job) - .and_return(instance_double(Delayed::Backend::ActiveRecord::Job, run_at: Time.current.utc)) + .and_return(instance_double(Delayed::Backend::ActiveRecord::Job, run_at: job_run_at)) end it 'sends a digest mail based on the configuration', with_settings: { journal_aggregation_time_minutes: 0 } do diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb index 3fdfc76e0b..ceb106b5ee 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb @@ -37,8 +37,8 @@ describe User, '.having_reminder_mail_to_send', type: :model do # As it is hard to mock PostgreSQL's "now()" method, in the specs here we need to adopt the slot time # relative to the local time of the user that we want to hit. - let(:current_utc_time) { Time.current.getutc } - let(:scope_time) { Time.current } + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("08:10:59") } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("08:00") } let(:paris_user) do FactoryBot.create( @@ -72,6 +72,16 @@ describe User, '.having_reminder_mail_to_send', type: :model do end end + context 'for a user whose local time is not matching the configured time' do + let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("08:20:59") } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("08:15") } + + it 'is empty' do + expect(scope) + .to be_empty + end + end + context 'for a user whose local time is matching the configured time (in a non CET time zone)' do let(:moscow_user) do FactoryBot.create( @@ -125,7 +135,7 @@ describe User, '.having_reminder_mail_to_send', type: :model do ] } end - let(:scope_time) { Time.current - 2.hours } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("06:00") } it 'contains the user' do expect(scope) @@ -186,7 +196,8 @@ describe User, '.having_reminder_mail_to_send', type: :model do } ) end - let(:current_utc_time) { ActiveSupport::TimeZone['Paris'].parse("08:00").utc } + let(:current_utc_time) { ActiveSupport::TimeZone['Paris'].parse("08:09").utc } + let(:scope_time) { ActiveSupport::TimeZone['Paris'].parse("08:00") } it 'contains the user' do expect(scope) @@ -205,6 +216,91 @@ describe User, '.having_reminder_mail_to_send', type: :model do ) end let(:current_utc_time) { ActiveSupport::TimeZone['Paris'].parse("10:00").utc } + let(:scope_time) { ActiveSupport::TimeZone['Paris'].parse("10:00") } + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user who is in a 45 min time zone and having reminder set to 8:00 and being executed at 8:10' do + let(:kathmandu_user) do + FactoryBot.create( + :user, + firstname: 'Kathmandu', + preferences: { + time_zone: "Kathmandu", + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("Asia/Kathmandu", current_utc_time)] + } + } + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("08:10").utc } + let(:scope_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("08:00").utc } + let(:notifications) do + FactoryBot.create(:notification, recipient: kathmandu_user, created_at: 5.minutes.ago) + end + + let(:users) { [kathmandu_user] } + + it 'contains the user' do + expect(scope) + .to match_array([kathmandu_user]) + end + end + + context 'for a user who is in a 45 min time zone and having reminder set to 8:00 and being executed at 8:40' do + let(:kathmandu_user) do + FactoryBot.create( + :user, + firstname: 'Kathmandu', + preferences: { + time_zone: "Kathmandu", + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("Asia/Kathmandu", current_utc_time)] + } + } + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("08:40").utc } + let(:scope_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("08:30").utc } + let(:notifications) do + FactoryBot.create(:notification, recipient: kathmandu_user, created_at: 5.minutes.ago) + end + + let(:users) { [kathmandu_user] } + + it 'is empty' do + expect(scope) + .to be_empty + end + end + + context 'for a user who is in a 45 min time zone and having reminder set to 8:00 and being executed at 7:55' do + let(:kathmandu_user) do + FactoryBot.create( + :user, + firstname: 'Kathmandu', + preferences: { + time_zone: "Kathmandu", + daily_reminders: { + enabled: true, + times: [hitting_reminder_slot_for("Asia/Kathmandu", current_utc_time)] + } + } + ) + end + let(:current_utc_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("07:55").utc } + let(:scope_time) { ActiveSupport::TimeZone['Asia/Kathmandu'].parse("07:45").utc } + let(:notifications) do + FactoryBot.create(:notification, recipient: kathmandu_user, created_at: 5.minutes.ago) + end + + let(:users) { [kathmandu_user] } it 'is empty' do expect(scope) @@ -322,6 +418,7 @@ describe User, '.having_reminder_mail_to_send', type: :model do ) end let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } it 'is empty as UTC at 08:00 is assumed' do expect(scope) @@ -340,6 +437,7 @@ describe User, '.having_reminder_mail_to_send', type: :model do context 'for a user without preferences at 08:00' do let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("08:00").utc } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("08:00").utc } before do paris_user.pref.destroy @@ -353,6 +451,7 @@ describe User, '.having_reminder_mail_to_send', type: :model do context 'for a user without preferences at 10:00' do let(:current_utc_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } + let(:scope_time) { ActiveSupport::TimeZone['UTC'].parse("10:00").utc } before do paris_user.pref.destroy From a9f1de155918db028c732a53f309c390d8d08e7c Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 23 Sep 2021 13:43:06 +0200 Subject: [PATCH 20/21] Increase spec understandability Co-authored-by: Wieland Lindenthal --- spec/models/users/scopes/having_reminder_mail_to_send_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb index ceb106b5ee..c04d8052ef 100644 --- a/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb +++ b/spec/models/users/scopes/having_reminder_mail_to_send_spec.rb @@ -153,7 +153,7 @@ describe User, '.having_reminder_mail_to_send', type: :model do ] } end - let(:scope_time) { Time.current - 2.hours } + let(:scope_time) { current_utc_time - 2.hours } it 'is empty' do expect(scope) From a9543d3404e76daf93036db93a640eb70548a8c5 Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 23 Sep 2021 14:19:05 +0200 Subject: [PATCH 21/21] linting --- app/models/users/scopes/having_reminder_mail_to_send.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/users/scopes/having_reminder_mail_to_send.rb b/app/models/users/scopes/having_reminder_mail_to_send.rb index 7ff5c829fa..787d592567 100644 --- a/app/models/users/scopes/having_reminder_mail_to_send.rb +++ b/app/models/users/scopes/having_reminder_mail_to_send.rb @@ -120,12 +120,12 @@ module Users::Scopes quarters = ((latest_time - earliest_time) / 60 / 15).floor (1..quarters).each_with_object([next_quarter_hour(earliest_time)]) do |i, times| - times << times.last + i * 15.minutes + times << (times.last + (i * 15.minutes)) end end def next_quarter_hour(time) - (time + (time.min % 15 == 0 ? 0.minutes : (15 - time.min % 15).minutes)) + (time + (time.min % 15 == 0 ? 0.minutes : (15 - (time.min % 15)).minutes)) end end end