From b466fba10c157a45df0b5767d8c04ba95e5725b1 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 19 Nov 2024 15:51:10 +0300 Subject: [PATCH 01/38] [#59433] Define database model for reminders https://community.openproject.org/work_packages/59433 --- app/models/remindable.rb | 35 ++++++++++++++++++ app/models/reminder.rb | 32 ++++++++++++++++ app/models/work_package.rb | 1 + db/migrate/20241119131205_create_reminders.rb | 12 ++++++ spec/models/concerns/remindable_spec.rb | 37 +++++++++++++++++++ spec/models/reminder_spec.rb | 36 ++++++++++++++++++ 6 files changed, 153 insertions(+) create mode 100644 app/models/remindable.rb create mode 100644 app/models/reminder.rb create mode 100644 db/migrate/20241119131205_create_reminders.rb create mode 100644 spec/models/concerns/remindable_spec.rb create mode 100644 spec/models/reminder_spec.rb diff --git a/app/models/remindable.rb b/app/models/remindable.rb new file mode 100644 index 000000000000..d55e65bf912b --- /dev/null +++ b/app/models/remindable.rb @@ -0,0 +1,35 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Remindable + extend ActiveSupport::Concern + + included do + has_many :reminders, as: :remindable, dependent: :destroy + end +end diff --git a/app/models/reminder.rb b/app/models/reminder.rb new file mode 100644 index 000000000000..13365419f1be --- /dev/null +++ b/app/models/reminder.rb @@ -0,0 +1,32 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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. +#++ + +class Reminder < ApplicationRecord + belongs_to :remindable, polymorphic: true + belongs_to :creator, class_name: "User" +end diff --git a/app/models/work_package.rb b/app/models/work_package.rb index e02d14aa81dd..270106799161 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -41,6 +41,7 @@ class WorkPackage < ApplicationRecord include WorkPackages::Relations include ::Scopes::Scoped include HasMembers + include Remindable include OpenProject::Journal::AttachmentHelper diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb new file mode 100644 index 000000000000..0bac66cccf07 --- /dev/null +++ b/db/migrate/20241119131205_create_reminders.rb @@ -0,0 +1,12 @@ +class CreateReminders < ActiveRecord::Migration[7.1] + def change + create_table :reminders do |t| + t.references :remindable, polymorphic: true, null: false + t.references :creator, null: false, foreign_key: { to_table: :users } + t.datetime :remind_at, null: false + t.text :notes + + t.timestamps + end + end +end diff --git a/spec/models/concerns/remindable_spec.rb b/spec/models/concerns/remindable_spec.rb new file mode 100644 index 000000000000..c86420aa8f73 --- /dev/null +++ b/spec/models/concerns/remindable_spec.rb @@ -0,0 +1,37 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" + +RSpec.describe Remindable do + let(:remindable) { build_stubbed(:work_package) } + + describe "Associations" do + it { expect(remindable).to have_many(:reminders) } + end +end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb new file mode 100644 index 000000000000..6e22d9a83dda --- /dev/null +++ b/spec/models/reminder_spec.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" + +RSpec.describe Reminder do + describe "Associations" do + it { is_expected.to belong_to(:remindable) } + it { is_expected.to belong_to(:creator).class_name("User") } + end +end From c8c015ea35f872e743af65f0445b53815ad0a7fe Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 20 Nov 2024 12:29:39 +0300 Subject: [PATCH 02/38] chore[Op#59433]: relocate remindable concern --- app/models/{ => concerns}/remindable.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename app/models/{ => concerns}/remindable.rb (100%) diff --git a/app/models/remindable.rb b/app/models/concerns/remindable.rb similarity index 100% rename from app/models/remindable.rb rename to app/models/concerns/remindable.rb From 78e2a7a3bfb00187c779b5e64f319e745ec9621e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 20 Nov 2024 12:30:12 +0300 Subject: [PATCH 03/38] feat[Op#59435]: define reminders contract --- app/contracts/reminders/base_contract.rb | 73 +++++++++++ app/contracts/reminders/create_contract.rb | 32 +++++ app/contracts/reminders/delete_contract.rb | 36 +++++ config/initializers/permissions.rb | 5 + config/locales/en.yml | 4 +- .../contracts/reminders/base_contract_spec.rb | 123 ++++++++++++++++++ .../reminders/delete_contract_spec.rb | 48 +++++++ spec/factories/reminders_factory.rb | 36 +++++ 8 files changed, 356 insertions(+), 1 deletion(-) create mode 100644 app/contracts/reminders/base_contract.rb create mode 100644 app/contracts/reminders/create_contract.rb create mode 100644 app/contracts/reminders/delete_contract.rb create mode 100644 spec/contracts/reminders/base_contract_spec.rb create mode 100644 spec/contracts/reminders/delete_contract_spec.rb create mode 100644 spec/factories/reminders_factory.rb diff --git a/app/contracts/reminders/base_contract.rb b/app/contracts/reminders/base_contract.rb new file mode 100644 index 000000000000..18c4cd6c11c4 --- /dev/null +++ b/app/contracts/reminders/base_contract.rb @@ -0,0 +1,73 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class BaseContract < ::ModelContract + attribute :creator_id + attribute :remindable_id + attribute :remindable_type + attribute :remind_at + attribute :notes + + validate :validate_creator_exists + validate :validate_acting_user + validate :validate_remindable_exists + validate :validate_manage_reminders_permissions + validate :validate_remind_at_is_in_future + + def self.model = Reminder + + private + + def validate_creator_exists + errors.add :creator, :not_found unless User.exists?(model.creator_id) + end + + def validate_acting_user + errors.add :creator, :invalid unless model.creator_id == user.id + end + + def validate_remindable_exists + errors.add :remindable, :not_found if model.remindable.blank? + end + + def validate_remind_at_is_in_future + if model.remind_at.present? && model.remind_at < Time.current + errors.add :remind_at, :datetime_must_be_in_future + end + end + + def validate_manage_reminders_permissions + return if errors.added?(:remindable, :not_found) + + unless user.allowed_in_project?(:manage_own_reminders, model.remindable.project) + errors.add :base, :error_unauthorized + end + end + end +end diff --git a/app/contracts/reminders/create_contract.rb b/app/contracts/reminders/create_contract.rb new file mode 100644 index 000000000000..6bc3c01eb76a --- /dev/null +++ b/app/contracts/reminders/create_contract.rb @@ -0,0 +1,32 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class CreateContract < BaseContract + end +end diff --git a/app/contracts/reminders/delete_contract.rb b/app/contracts/reminders/delete_contract.rb new file mode 100644 index 000000000000..06895868e6c4 --- /dev/null +++ b/app/contracts/reminders/delete_contract.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class DeleteContract < ::DeleteContract + delete_permission -> { + # The user can delete the reminder if they created it + model.creator_id == user.id + } + end +end diff --git a/config/initializers/permissions.rb b/config/initializers/permissions.rb index 51b4d6184a41..e82b531caf2d 100644 --- a/config/initializers/permissions.rb +++ b/config/initializers/permissions.rb @@ -212,6 +212,11 @@ {}, permissible_on: :project_query, require: :loggedin + + map.permission :manage_own_reminders, + {}, + permissible_on: :project, + require: :member end map.project_module :work_package_tracking, order: 90 do |wpt| diff --git a/config/locales/en.yml b/config/locales/en.yml index 008c15973abf..81e66307eb52 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -999,6 +999,7 @@ en: not_an_integer: "is not an integer." not_an_iso_date: "is not a valid date. Required format: YYYY-MM-DD." not_same_project: "doesn't belong to the same project." + datetime_must_be_in_future: "must be in the future." odd: "must be odd." regex_match_failed: "does not match the regular expression %{expression}." regex_invalid: "could not be validated with the associated regular expression." @@ -3108,6 +3109,7 @@ en: permission_select_project_modules: "Select project modules" permission_share_work_packages: "Share work packages" permission_manage_types: "Select types" + permission_manage_own_reminders: "Create own reminders" permission_view_project: "View projects" permission_view_changesets: "View repository revisions in OpenProject" permission_view_commit_author_statistics: "View commit author statistics" @@ -3490,7 +3492,7 @@ en: If this option is active, login requests will redirect to the configured omniauth provider. The login dropdown and sign-in page will be disabled.
- Note: Unless you also disable password logins, with this option enabled, + Note: Unless you also disable password logins, with this option enabled, users can still log in internally by visiting the %{internal_path} login page. attachments: whitelist_text_html: > diff --git a/spec/contracts/reminders/base_contract_spec.rb b/spec/contracts/reminders/base_contract_spec.rb new file mode 100644 index 000000000000..3005a15155f2 --- /dev/null +++ b/spec/contracts/reminders/base_contract_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "contracts/shared/model_contract_shared_context" + +RSpec.describe Reminders::BaseContract do + include_context "ModelContract shared context" + + let(:contract) { described_class.new(reminder, user) } + let(:user) { build_stubbed(:admin) } + let(:creator) { user } + let(:reminder) { build_stubbed(:reminder, creator:) } + + before do + User.current = user + allow(User).to receive(:exists?).with(user.id).and_return(true) + end + + describe "admin user" do + it_behaves_like "contract is valid" + end + + describe "non-admin user" do + context "with valid permissions" do + let(:user) { build_stubbed(:user) } + + before do + mock_permissions_for(user) do |mock| + mock.allow_in_project(:manage_own_reminders, project: reminder.remindable.project) + end + end + + it_behaves_like "contract is valid" + end + + context "without valid permissions" do + let(:user) { build_stubbed(:user) } + + it_behaves_like "contract is invalid", base: :error_unauthorized + end + end + + describe "validate creator exists" do + context "when creator does not exist" do + before { allow(User).to receive(:exists?).with(user.id).and_return(false) } + + it_behaves_like "contract is invalid", creator: :not_found + end + end + + describe "validate acting user" do + context "when the current user is different from the remindable acting user" do + let(:different_user) { build_stubbed(:user) } + + before do + allow(User).to receive(:exists?).with(different_user.id).and_return(true) + reminder.creator = different_user + end + + it_behaves_like "contract is invalid", creator: :invalid + end + end + + describe "validate remindable object" do + context "when remindable is blank" do + before { reminder.remindable = nil } + + it_behaves_like "contract is invalid", remindable: :not_found + end + + context "when remindable is a work package" do + let(:work_package) { build_stubbed(:work_package) } + + before { reminder.remindable = work_package } + + it_behaves_like "contract is valid" + end + end + + describe "validate remind at is in future" do + context "when remind at is in the past" do + before { reminder.remind_at = 1.day.ago } + + it_behaves_like "contract is invalid", remind_at: :datetime_must_be_in_future + end + + context "when remind at is in the future" do + before { reminder.remind_at = 1.day.from_now } + + it_behaves_like "contract is valid" + end + end + + include_examples "contract reuses the model errors" +end diff --git a/spec/contracts/reminders/delete_contract_spec.rb b/spec/contracts/reminders/delete_contract_spec.rb new file mode 100644 index 000000000000..2d884059023b --- /dev/null +++ b/spec/contracts/reminders/delete_contract_spec.rb @@ -0,0 +1,48 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "contracts/shared/model_contract_shared_context" + +RSpec.describe Reminders::DeleteContract do + include_context "ModelContract shared context" + + let(:contract) { described_class.new(reminder, current_user) } + let(:current_user) { build_stubbed(:admin) } + let(:reminder) { build_stubbed(:reminder, creator: current_user) } + + context "when user is different from the one that created the reminder" do + let(:another_user) { build_stubbed(:admin) } + + before { reminder.creator = another_user } + + it_behaves_like "contract user is unauthorized" + end + + include_examples "contract reuses the model errors" +end diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb new file mode 100644 index 000000000000..95e55d5e7ad8 --- /dev/null +++ b/spec/factories/reminders_factory.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +FactoryBot.define do + factory :reminder do + remindable factory: :work_package_journal + creator factory: :user + remind_at { 1.day.from_now } + notes { "This is a reminder" } + end +end From 6137d1b42724edf4a8eff3c2817828c3861b40d1 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 20 Nov 2024 13:09:24 +0300 Subject: [PATCH 04/38] feat[Op#59434]: add notification reason `reminder` --- app/models/notification.rb | 6 ++--- ...notification_representer_rendering_spec.rb | 2 +- .../v3/notifications/property_factory_spec.rb | 2 +- spec/models/notification_spec.rb | 24 +++++++++++++++++++ 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/app/models/notification.rb b/app/models/notification.rb index 65dff4c23ab5..2b2fbc5c6e4e 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -40,11 +40,11 @@ class Notification < ApplicationRecord responsible: 9, date_alert_start_date: 10, date_alert_due_date: 11, - shared: 12 + shared: 12, + reminder: 13 }.freeze - enum reason: REASONS, - _prefix: true + enum :reason, REASONS, prefix: true belongs_to :recipient, class_name: "User" belongs_to :actor, class_name: "User" diff --git a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb index c0fb12c42cbd..f146213883db 100644 --- a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb +++ b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb @@ -103,7 +103,7 @@ end describe "reason" do - (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date]).each do |notification_reason| + (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date reminder]).each do |notification_reason| context "for a #{notification_reason} reason" do let(:reason) { notification_reason } diff --git a/spec/lib/api/v3/notifications/property_factory_spec.rb b/spec/lib/api/v3/notifications/property_factory_spec.rb index 3ad53e613c57..653a567d1060 100644 --- a/spec/lib/api/v3/notifications/property_factory_spec.rb +++ b/spec/lib/api/v3/notifications/property_factory_spec.rb @@ -106,7 +106,7 @@ end end - (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date]).each do |possible_reason| + (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date reminder]).each do |possible_reason| context "for a #{possible_reason} notification" do let(:reason) { possible_reason } diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 7e9c73044222..0007a19b60d3 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -28,6 +28,30 @@ require "spec_helper" RSpec.describe Notification do + describe "Enums" do + it do + expect(subject).to define_enum_for(:reason) + .with_values( + mentioned: 0, + assigned: 1, + watched: 2, + subscribed: 3, + commented: 4, + created: 5, + processed: 6, + prioritized: 7, + scheduled: 8, + responsible: 9, + date_alert_start_date: 10, + date_alert_due_date: 11, + shared: 12, + reminder: 13 + ) + .with_prefix + .backed_by_column_of_type(:integer) + end + end + describe ".save" do context "for a non existing journal (e.g. because it has been deleted)" do let(:notification) { build(:notification) } From 40d0756211cca1d20eb97fe824cead4452ef8858 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 20 Nov 2024 18:56:21 +0300 Subject: [PATCH 05/38] fix[Op#59433]: track reminder job id for job scheduling control --- app/models/reminder.rb | 4 ++++ db/migrate/20241119131205_create_reminders.rb | 1 + spec/factories/reminders_factory.rb | 6 +++++- spec/models/reminder_spec.rb | 14 ++++++++++++++ 4 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/models/reminder.rb b/app/models/reminder.rb index 13365419f1be..5e894f91152e 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -29,4 +29,8 @@ class Reminder < ApplicationRecord belongs_to :remindable, polymorphic: true belongs_to :creator, class_name: "User" + + def scheduled? + job_id.present? + end end diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 0bac66cccf07..461b29cd42dd 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -4,6 +4,7 @@ def change t.references :remindable, polymorphic: true, null: false t.references :creator, null: false, foreign_key: { to_table: :users } t.datetime :remind_at, null: false + t.string :job_id t.text :notes t.timestamps diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index 95e55d5e7ad8..4e3b124b864f 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -28,9 +28,13 @@ FactoryBot.define do factory :reminder do - remindable factory: :work_package_journal + remindable factory: :work_package creator factory: :user remind_at { 1.day.from_now } notes { "This is a reminder" } + + trait :scheduled do + job_id { SecureRandom.uuid } + end end end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index 6e22d9a83dda..e87cff1a9903 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -33,4 +33,18 @@ it { is_expected.to belong_to(:remindable) } it { is_expected.to belong_to(:creator).class_name("User") } end + + describe "#scheduled?" do + context "when job_id is present" do + subject { build(:reminder, :scheduled) } + + it { is_expected.to be_scheduled } + end + + context "when job_id is not present" do + subject { build(:reminder) } + + it { is_expected.not_to be_scheduled } + end + end end From 73aa20c5aa4be3210648837b16cdde04f10521ab Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 20 Nov 2024 18:59:15 +0300 Subject: [PATCH 06/38] feat[Op#59435]: define reminders create service Once a reminder is scheduled, track the job id- this way in case of rescheduling (snoozing reminder) there is a record we can rely on to have GoodJob reschedule directly. Further, we use this as a control to ensure unique job schedules. TODO: Once a notification is created from a reminder, create a "reminder_notification" - join table that tracks notifications created from reminders for further management --- app/services/reminders/create_service.rb | 39 +++++++++++++ .../reminders/set_attributes_service.rb | 32 +++++++++++ .../reminders/schedule_reminder_job.rb | 56 +++++++++++++++++++ .../services/reminders/create_service_spec.rb | 52 +++++++++++++++++ 4 files changed, 179 insertions(+) create mode 100644 app/services/reminders/create_service.rb create mode 100644 app/services/reminders/set_attributes_service.rb create mode 100644 app/workers/reminders/schedule_reminder_job.rb create mode 100644 spec/services/reminders/create_service_spec.rb diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb new file mode 100644 index 000000000000..e5f6997c2898 --- /dev/null +++ b/app/services/reminders/create_service.rb @@ -0,0 +1,39 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class CreateService < ::BaseServices::Create + def after_perform(service_call) + reminder = service_call.result + job = Reminders::ScheduleReminderJob.schedule(reminder) + reminder.update!(job_id: job.job_id) + + service_call + end + end +end diff --git a/app/services/reminders/set_attributes_service.rb b/app/services/reminders/set_attributes_service.rb new file mode 100644 index 000000000000..c1cbff56aa64 --- /dev/null +++ b/app/services/reminders/set_attributes_service.rb @@ -0,0 +1,32 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class SetAttributesService < ::BaseServices::SetAttributes + end +end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb new file mode 100644 index 000000000000..195b185bfadb --- /dev/null +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -0,0 +1,56 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 Reminders + class ScheduleReminderJob < ApplicationJob + queue_with_priority :notification + + def self.schedule(reminder) + set(wait_until: reminder.remind_at).perform_later(reminder) + end + + def perform(reminder) + return if reminder.scheduled? + + create_notification_from_reminder(reminder) + end + + private + + def create_notification_from_reminder(reminder) + Notifications::CreateService + .new(user: reminder.creator) + .call( + actor_id: reminder.creator_id, + recipient_id: reminder.creator_id, + resource: reminder.remindable, + reason: :reminder + ) + end + end +end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb new file mode 100644 index 000000000000..8f75884076fa --- /dev/null +++ b/spec/services/reminders/create_service_spec.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "services/base_services/behaves_like_create_service" + +RSpec.describe Reminders::CreateService, type: :model do + it_behaves_like "BaseServices create service" do + let(:factory) { :reminder } + + before do + allow(model_instance).to receive(:update!).and_return(true) + end + + it "schedules a reminder job" do + allow(Reminders::ScheduleReminderJob).to receive(:schedule) + .with(model_instance) + .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1)) + + subject + + expect(model_instance).to have_received(:update!).with(job_id: 1) + end + end +end From fdb36b6db846c84e391a7c4cbd499040ef666fe1 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 21 Nov 2024 14:19:15 +0300 Subject: [PATCH 07/38] tests[Op#59435]: add schedule reminder job spec --- .../reminders/schedule_reminder_job_spec.rb | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 spec/workers/reminders/schedule_reminder_job_spec.rb diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb new file mode 100644 index 000000000000..835aa7cf39a4 --- /dev/null +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -0,0 +1,74 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" + +RSpec.describe Reminders::ScheduleReminderJob do + describe ".schedule" do + let(:reminder) { create(:reminder) } + + subject { described_class.schedule(reminder) } + + it "enqueues a ScheduleReminderJob" do + expect { subject } + .to have_enqueued_job(described_class) + .at(reminder.remind_at) + .with(reminder) + end + end + + describe "#perform" do + let(:reminder) { create(:reminder) } + + subject { described_class.new.perform(reminder) } + + it "creates a notification from the reminder" do + notification_svc = nil + expect { notification_svc = subject }.to change(Notification, :count).by(1) + + aggregate_failures "notification attributes" do + notification = notification_svc.result + + expect(notification.actor_id).to eq(reminder.creator_id) + expect(notification.recipient_id).to eq(reminder.creator_id) + expect(notification.resource).to eq(reminder.remindable) + expect(notification.reason).to eq("reminder") + end + end + + context "when the reminder is already scheduled" do + before do + reminder.update_column(:job_id, SecureRandom.uuid) + end + + it "does not create a notification from the reminder" do + expect { subject }.not_to change(Notification, :count) + end + end + end +end From b29cfd9e7728c026269bbbe28ba67a9064b4a09f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 21 Nov 2024 14:21:22 +0300 Subject: [PATCH 08/38] fix[Op#59433]: switch from `update!` to `update_column` for job id update `update_column` is faster and updates the DB directly, skipping validations; which are not needed in this case. It is more important that we guarantee the job id is written into the database irrespective of any side effects from model validations --- app/services/reminders/create_service.rb | 2 +- spec/services/reminders/create_service_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb index e5f6997c2898..426c08819590 100644 --- a/app/services/reminders/create_service.rb +++ b/app/services/reminders/create_service.rb @@ -31,7 +31,7 @@ class CreateService < ::BaseServices::Create def after_perform(service_call) reminder = service_call.result job = Reminders::ScheduleReminderJob.schedule(reminder) - reminder.update!(job_id: job.job_id) + reminder.update_column(:job_id, job.job_id) service_call end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 8f75884076fa..288b0450d238 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -36,7 +36,7 @@ let(:factory) { :reminder } before do - allow(model_instance).to receive(:update!).and_return(true) + allow(model_instance).to receive(:update_column).and_return(true) end it "schedules a reminder job" do @@ -46,7 +46,7 @@ subject - expect(model_instance).to have_received(:update!).with(job_id: 1) + expect(model_instance).to have_received(:update_column).with(:job_id, 1) end end end From 67f524a9c54d3a94e77220066d6c6722fe06e776 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 21 Nov 2024 14:51:26 +0300 Subject: [PATCH 09/38] Add Notification assoc tests --- spec/models/notification_spec.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index 0007a19b60d3..e615c033932c 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -28,6 +28,13 @@ require "spec_helper" RSpec.describe Notification do + describe "Associations" do + it { is_expected.to belong_to(:journal) } + it { is_expected.to belong_to(:resource) } + it { is_expected.to belong_to(:actor).class_name("User") } + it { is_expected.to belong_to(:recipient).class_name("User") } + end + describe "Enums" do it do expect(subject).to define_enum_for(:reason) From 09622465b98b52b304cef706b59ae20aa481c127 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 21 Nov 2024 14:56:06 +0300 Subject: [PATCH 10/38] Keep track of reminder notifications Arises from: 73aa20c5aa4be3210648837b16cdde04f10521ab Once a reminder is created we need to keep track of: (1) The scheduled job: this is done via Reminder#job_id which corresponds to a GoodJob::Job#id (2) Any Notification(s) resulting from the reminder This is important in order to easily handle any modifications to the reminder such as snoozing, editing & deleting reminders --- app/models/reminder.rb | 2 ++ app/models/reminder_notification.rb | 4 +++ .../reminders/schedule_reminder_job.rb | 1 + ...121113638_create_reminder_notifications.rb | 10 ++++++ spec/models/reminder_notification_spec.rb | 36 +++++++++++++++++++ spec/models/reminder_spec.rb | 1 + .../reminders/schedule_reminder_job_spec.rb | 2 +- 7 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 app/models/reminder_notification.rb create mode 100644 db/migrate/20241121113638_create_reminder_notifications.rb create mode 100644 spec/models/reminder_notification_spec.rb diff --git a/app/models/reminder.rb b/app/models/reminder.rb index 5e894f91152e..b2fca7cc9ae8 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -30,6 +30,8 @@ class Reminder < ApplicationRecord belongs_to :remindable, polymorphic: true belongs_to :creator, class_name: "User" + has_many :reminder_notifications, dependent: :destroy + def scheduled? job_id.present? end diff --git a/app/models/reminder_notification.rb b/app/models/reminder_notification.rb new file mode 100644 index 000000000000..cbdd70905a9b --- /dev/null +++ b/app/models/reminder_notification.rb @@ -0,0 +1,4 @@ +class ReminderNotification < ApplicationRecord + belongs_to :reminder + belongs_to :notification +end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 195b185bfadb..e87d47ffb9b7 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -38,6 +38,7 @@ def perform(reminder) return if reminder.scheduled? create_notification_from_reminder(reminder) + .on_success { |service_result| ReminderNotification.create!(reminder:, notification: service_result.result) } end private diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb new file mode 100644 index 000000000000..576e07c8f1e6 --- /dev/null +++ b/db/migrate/20241121113638_create_reminder_notifications.rb @@ -0,0 +1,10 @@ +class CreateReminderNotifications < ActiveRecord::Migration[7.1] + def change + create_table :reminder_notifications do |t| + t.references :reminder, foreign_key: true + t.references :notification, foreign_key: true + + t.timestamps + end + end +end diff --git a/spec/models/reminder_notification_spec.rb b/spec/models/reminder_notification_spec.rb new file mode 100644 index 000000000000..95c8193813c2 --- /dev/null +++ b/spec/models/reminder_notification_spec.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" + +RSpec.describe ReminderNotification do + describe "Associations" do + it { is_expected.to belong_to(:reminder) } + it { is_expected.to belong_to(:notification) } + end +end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index e87cff1a9903..bb1398e1125f 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -32,6 +32,7 @@ describe "Associations" do it { is_expected.to belong_to(:remindable) } it { is_expected.to belong_to(:creator).class_name("User") } + it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) } end describe "#scheduled?" do diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 835aa7cf39a4..a24ced8c5109 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -49,7 +49,7 @@ it "creates a notification from the reminder" do notification_svc = nil - expect { notification_svc = subject }.to change(Notification, :count).by(1) + expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(ReminderNotification, :count).by(1) aggregate_failures "notification attributes" do notification = notification_svc.result From fe32e9562618e823c4b2a45f6154f5a9ce111079 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 00:10:13 +0300 Subject: [PATCH 11/38] Specify `API::V3::Notifications::PropertyFactory` to look up descendants const_defined? looks up ancestors OR descendants by default. Which resulted in a NameError ```ruby Failure/Error: "API::V3::Notifications::PropertyFactory::#{property_key.camelcase}".constantize NameError: uninitialized constant API::V3::Notifications::PropertyFactory::Reminder ``` To enforce descendants lookup, provide `false` as the second arg --- lib/api/v3/notifications/property_factory.rb | 7 +------ .../notification_representer_rendering_spec.rb | 2 +- spec/lib/api/v3/notifications/property_factory_spec.rb | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/api/v3/notifications/property_factory.rb b/lib/api/v3/notifications/property_factory.rb index f5786d253ef3..5b450656df9b 100644 --- a/lib/api/v3/notifications/property_factory.rb +++ b/lib/api/v3/notifications/property_factory.rb @@ -88,12 +88,7 @@ def concrete_factory_for(notification) end @concrete_factory_for ||= Hash.new do |h, property_key| - h[property_key] = if property_key == "shared" - # for some reasons - # API::V3::Notifications::PropertyFactory.const_defined?(property_key.camelcase) - # returns true for shared only to fail on the constantize later on. - API::V3::Notifications::PropertyFactory::Default - elsif API::V3::Notifications::PropertyFactory.const_defined?(property_key.camelcase) + h[property_key] = if API::V3::Notifications::PropertyFactory.const_defined?(property_key.camelcase, false) "API::V3::Notifications::PropertyFactory::#{property_key.camelcase}".constantize else API::V3::Notifications::PropertyFactory::Default diff --git a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb index f146213883db..c0fb12c42cbd 100644 --- a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb +++ b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb @@ -103,7 +103,7 @@ end describe "reason" do - (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date reminder]).each do |notification_reason| + (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date]).each do |notification_reason| context "for a #{notification_reason} reason" do let(:reason) { notification_reason } diff --git a/spec/lib/api/v3/notifications/property_factory_spec.rb b/spec/lib/api/v3/notifications/property_factory_spec.rb index 653a567d1060..3ad53e613c57 100644 --- a/spec/lib/api/v3/notifications/property_factory_spec.rb +++ b/spec/lib/api/v3/notifications/property_factory_spec.rb @@ -106,7 +106,7 @@ end end - (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date reminder]).each do |possible_reason| + (Notification::REASONS.keys - %i[date_alert_start_date date_alert_due_date]).each do |possible_reason| context "for a #{possible_reason} notification" do let(:reason) { possible_reason } From b4715902960d7ff6ae19a1ab590c091efb4ed1cf Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 01:54:06 +0300 Subject: [PATCH 12/38] add notification reason "reminder" translation --- config/locales/js-en.yml | 1 + .../center/in-app-notification-center.component.ts | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index 322eeb3246f7..44daa9c4dd6d 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -677,6 +677,7 @@ en: prioritized: "Prioritized" dateAlert: "Date alert" shared: "Shared" + reminder: "Reminder" date_alerts: milestone_date: "Milestone date" overdue: "Overdue" diff --git a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts index 641983075d1f..70b4f0c8eea2 100644 --- a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts +++ b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts @@ -101,6 +101,10 @@ export class InAppNotificationCenterComponent implements OnInit { key: 'shared', title: this.I18n.t('js.notifications.reasons.shared'), }, + { + key: 'reminder', + title: this.I18n.t('js.notifications.reasons.reminder'), + }, ]; selectedFilter = this.reasonMenuItems.find((item) => item.key === this.urlParams.get('name'))?.title; From 07c2588ab26fdb4a3686c3fdd16863f9d6e86514 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 12:27:45 +0300 Subject: [PATCH 13/38] rename notes to note --- app/contracts/reminders/base_contract.rb | 2 +- db/migrate/20241119131205_create_reminders.rb | 2 +- spec/factories/reminders_factory.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/contracts/reminders/base_contract.rb b/app/contracts/reminders/base_contract.rb index 18c4cd6c11c4..abdf358a7a1d 100644 --- a/app/contracts/reminders/base_contract.rb +++ b/app/contracts/reminders/base_contract.rb @@ -32,7 +32,7 @@ class BaseContract < ::ModelContract attribute :remindable_id attribute :remindable_type attribute :remind_at - attribute :notes + attribute :note validate :validate_creator_exists validate :validate_acting_user diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 461b29cd42dd..454e830fefdc 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -5,7 +5,7 @@ def change t.references :creator, null: false, foreign_key: { to_table: :users } t.datetime :remind_at, null: false t.string :job_id - t.text :notes + t.text :note t.timestamps end diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index 4e3b124b864f..3903d27be4e1 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -31,7 +31,7 @@ remindable factory: :work_package creator factory: :user remind_at { 1.day.from_now } - notes { "This is a reminder" } + note { "This is a reminder" } trait :scheduled do job_id { SecureRandom.uuid } From fdf09b9aa8208c22fcf9fb97e90234d01508591f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 12:32:51 +0300 Subject: [PATCH 14/38] Separate schedule status from notification status on reminders As reminders are generally scheduled in the future, we need to keet track of (1) scheduled status - make it easy to track down the (good) job for edit/reschedule actions (2) notified at timestamp - prevents duplicate notifications, skip schedule when already notified --- app/models/reminder.rb | 5 ++--- app/workers/reminders/schedule_reminder_job.rb | 16 +++++++++++++--- db/migrate/20241119131205_create_reminders.rb | 1 + spec/factories/reminders_factory.rb | 4 ++++ spec/models/reminder_spec.rb | 14 ++++++++++++++ spec/services/reminders/create_service_spec.rb | 6 +++--- .../reminders/schedule_reminder_job_spec.rb | 8 ++++++-- 7 files changed, 43 insertions(+), 11 deletions(-) diff --git a/app/models/reminder.rb b/app/models/reminder.rb index b2fca7cc9ae8..bc4d9802b5a8 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -32,7 +32,6 @@ class Reminder < ApplicationRecord has_many :reminder_notifications, dependent: :destroy - def scheduled? - job_id.present? - end + def notified? = notified_at.present? + def scheduled? = job_id.present? end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index e87d47ffb9b7..260940b51a8d 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -35,10 +35,20 @@ def self.schedule(reminder) end def perform(reminder) - return if reminder.scheduled? + return if reminder.notified? - create_notification_from_reminder(reminder) - .on_success { |service_result| ReminderNotification.create!(reminder:, notification: service_result.result) } + create_notification_service = create_notification_from_reminder(reminder) + + create_notification_service.on_success do |service_result| + ReminderNotification.create!(reminder:, notification: service_result.result) + reminder.update_column(:notified_at, Time.current) + end + + create_notification_service.on_failure do |service_result| + Rails.logger.error do + "Failed to create notification for reminder #{reminder.id}: #{service_result.message}" + end + end end private diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 454e830fefdc..bcc84c73ae97 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -4,6 +4,7 @@ def change t.references :remindable, polymorphic: true, null: false t.references :creator, null: false, foreign_key: { to_table: :users } t.datetime :remind_at, null: false + t.datetime :notified_at t.string :job_id t.text :note diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index 3903d27be4e1..e6335941afaa 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -36,5 +36,9 @@ trait :scheduled do job_id { SecureRandom.uuid } end + + trait :notified do + notified_at { remind_at + 1.second } + end end end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index bb1398e1125f..f8a6bbb6cb77 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -35,6 +35,20 @@ it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) } end + describe "#notified?" do + context "when notified_at is present" do + subject { build(:reminder, :notified) } + + it { is_expected.to be_notified } + end + + context "when notified_at is not present" do + subject { build(:reminder) } + + it { is_expected.not_to be_notified } + end + end + describe "#scheduled?" do context "when job_id is present" do subject { build(:reminder, :scheduled) } diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 288b0450d238..78ae257ed710 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -37,15 +37,15 @@ before do allow(model_instance).to receive(:update_column).and_return(true) - end - - it "schedules a reminder job" do allow(Reminders::ScheduleReminderJob).to receive(:schedule) .with(model_instance) .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1)) + end + it "schedules a reminder job" do subject + expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) expect(model_instance).to have_received(:update_column).with(:job_id, 1) end end diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index a24ced8c5109..4466834a88e2 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -59,11 +59,15 @@ expect(notification.resource).to eq(reminder.remindable) expect(notification.reason).to eq("reminder") end + + aggregate_failures "marks the reminder as notified" do + expect(reminder.reload).to be_notified + end end - context "when the reminder is already scheduled" do + context "when the reminder is already notified" do before do - reminder.update_column(:job_id, SecureRandom.uuid) + reminder.update_column(:notified_at, Time.current) end it "does not create a notification from the reminder" do From d6cd81047452969be06d2ba2ac9afee8a906ab9a Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 12:33:57 +0300 Subject: [PATCH 15/38] Add uniqueness index on reminder notification A notification cannot be linked to more than one reminder, every reminder should generate it's own unique notification. --- db/migrate/20241121113638_create_reminder_notifications.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb index 576e07c8f1e6..eaa711b431e5 100644 --- a/db/migrate/20241121113638_create_reminder_notifications.rb +++ b/db/migrate/20241121113638_create_reminder_notifications.rb @@ -6,5 +6,9 @@ def change t.timestamps end + + add_index :reminder_notifications, :notification_id, + unique: true, + name: "index_reminder_notifications_unique" end end From 4c01600681791d0f712ace7f180dbb605ee0248a Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 12:53:46 +0300 Subject: [PATCH 16/38] Remove reminder notification actor Semantically on the recepient is relevant, as the actor is a reminder --- app/workers/reminders/schedule_reminder_job.rb | 1 - spec/workers/reminders/schedule_reminder_job_spec.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 260940b51a8d..395c95123dc6 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -57,7 +57,6 @@ def create_notification_from_reminder(reminder) Notifications::CreateService .new(user: reminder.creator) .call( - actor_id: reminder.creator_id, recipient_id: reminder.creator_id, resource: reminder.remindable, reason: :reminder diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 4466834a88e2..4b3ed9fca65c 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -54,7 +54,6 @@ aggregate_failures "notification attributes" do notification = notification_svc.result - expect(notification.actor_id).to eq(reminder.creator_id) expect(notification.recipient_id).to eq(reminder.creator_id) expect(notification.resource).to eq(reminder.remindable) expect(notification.reason).to eq("reminder") From ae3f77da803f3eac97e5e12121f1266f807b7d31 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 22 Nov 2024 22:52:03 +0300 Subject: [PATCH 17/38] fix[Op#59433]: simplify reminder model to directly linked to notification Introduce simple status enums to track various reminder states --- app/models/reminder.rb | 6 ++-- app/models/reminder_notification.rb | 4 --- app/services/reminders/create_service.rb | 2 +- .../reminders/schedule_reminder_job.rb | 3 +- db/migrate/20241119131205_create_reminders.rb | 5 +-- ...121113638_create_reminder_notifications.rb | 14 -------- spec/factories/reminders_factory.rb | 4 ++- spec/models/reminder_notification_spec.rb | 36 ------------------- spec/models/reminder_spec.rb | 32 ++++------------- .../services/reminders/create_service_spec.rb | 4 +-- .../reminders/schedule_reminder_job_spec.rb | 12 ++++--- 11 files changed, 26 insertions(+), 96 deletions(-) delete mode 100644 app/models/reminder_notification.rb delete mode 100644 db/migrate/20241121113638_create_reminder_notifications.rb delete mode 100644 spec/models/reminder_notification_spec.rb diff --git a/app/models/reminder.rb b/app/models/reminder.rb index bc4d9802b5a8..8218ea972218 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -29,9 +29,7 @@ class Reminder < ApplicationRecord belongs_to :remindable, polymorphic: true belongs_to :creator, class_name: "User" + belongs_to :notification, optional: true - has_many :reminder_notifications, dependent: :destroy - - def notified? = notified_at.present? - def scheduled? = job_id.present? + enum :status, { pending: 0, scheduled: 1, notified: 2, done: 9 } end diff --git a/app/models/reminder_notification.rb b/app/models/reminder_notification.rb deleted file mode 100644 index cbdd70905a9b..000000000000 --- a/app/models/reminder_notification.rb +++ /dev/null @@ -1,4 +0,0 @@ -class ReminderNotification < ApplicationRecord - belongs_to :reminder - belongs_to :notification -end diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb index 426c08819590..7674e63e059c 100644 --- a/app/services/reminders/create_service.rb +++ b/app/services/reminders/create_service.rb @@ -31,7 +31,7 @@ class CreateService < ::BaseServices::Create def after_perform(service_call) reminder = service_call.result job = Reminders::ScheduleReminderJob.schedule(reminder) - reminder.update_column(:job_id, job.job_id) + reminder.update_columns(status: :scheduled, job_id: job.job_id) service_call end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 395c95123dc6..95d64057b92f 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -40,8 +40,7 @@ def perform(reminder) create_notification_service = create_notification_from_reminder(reminder) create_notification_service.on_success do |service_result| - ReminderNotification.create!(reminder:, notification: service_result.result) - reminder.update_column(:notified_at, Time.current) + reminder.update_columns(status: :notified, notification_id: service_result.result.id) end create_notification_service.on_failure do |service_result| diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index bcc84c73ae97..e74d16016420 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -3,10 +3,11 @@ def change create_table :reminders do |t| t.references :remindable, polymorphic: true, null: false t.references :creator, null: false, foreign_key: { to_table: :users } + t.references :notification, foreign_key: true t.datetime :remind_at, null: false - t.datetime :notified_at + t.integer :status, default: 0, null: false t.string :job_id - t.text :note + t.string :note t.timestamps end diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb deleted file mode 100644 index eaa711b431e5..000000000000 --- a/db/migrate/20241121113638_create_reminder_notifications.rb +++ /dev/null @@ -1,14 +0,0 @@ -class CreateReminderNotifications < ActiveRecord::Migration[7.1] - def change - create_table :reminder_notifications do |t| - t.references :reminder, foreign_key: true - t.references :notification, foreign_key: true - - t.timestamps - end - - add_index :reminder_notifications, :notification_id, - unique: true, - name: "index_reminder_notifications_unique" - end -end diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index e6335941afaa..cda7d6a84e9c 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -38,7 +38,9 @@ end trait :notified do - notified_at { remind_at + 1.second } + job_id { SecureRandom.uuid } + status { :notified } + notification factory: :notification end end end diff --git a/spec/models/reminder_notification_spec.rb b/spec/models/reminder_notification_spec.rb deleted file mode 100644 index 95c8193813c2..000000000000 --- a/spec/models/reminder_notification_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 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. -#++ - -require "spec_helper" - -RSpec.describe ReminderNotification do - describe "Associations" do - it { is_expected.to belong_to(:reminder) } - it { is_expected.to belong_to(:notification) } - end -end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index f8a6bbb6cb77..a65bcfd8857a 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -32,34 +32,14 @@ describe "Associations" do it { is_expected.to belong_to(:remindable) } it { is_expected.to belong_to(:creator).class_name("User") } - it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) } + it { is_expected.to belong_to(:notification).optional } end - describe "#notified?" do - context "when notified_at is present" do - subject { build(:reminder, :notified) } - - it { is_expected.to be_notified } - end - - context "when notified_at is not present" do - subject { build(:reminder) } - - it { is_expected.not_to be_notified } - end - end - - describe "#scheduled?" do - context "when job_id is present" do - subject { build(:reminder, :scheduled) } - - it { is_expected.to be_scheduled } - end - - context "when job_id is not present" do - subject { build(:reminder) } - - it { is_expected.not_to be_scheduled } + describe "Enums" do + it do + expect(subject).to define_enum_for(:status) + .with_values({ pending: 0, scheduled: 1, notified: 2, done: 9 }) + .backed_by_column_of_type(:integer) end end end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 78ae257ed710..76a89a0bc27f 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -36,7 +36,7 @@ let(:factory) { :reminder } before do - allow(model_instance).to receive(:update_column).and_return(true) + allow(model_instance).to receive(:update_columns) allow(Reminders::ScheduleReminderJob).to receive(:schedule) .with(model_instance) .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1)) @@ -46,7 +46,7 @@ subject expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) - expect(model_instance).to have_received(:update_column).with(:job_id, 1) + expect(model_instance).to have_received(:update_columns).with(status: :scheduled, job_id: 1) end end end diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 4b3ed9fca65c..036d251e164a 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -49,11 +49,11 @@ it "creates a notification from the reminder" do notification_svc = nil - expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(ReminderNotification, :count).by(1) + expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(reminder, :status).to("notified") - aggregate_failures "notification attributes" do - notification = notification_svc.result + notification = notification_svc.result + aggregate_failures "notification attributes" do expect(notification.recipient_id).to eq(reminder.creator_id) expect(notification.resource).to eq(reminder.remindable) expect(notification.reason).to eq("reminder") @@ -61,16 +61,20 @@ aggregate_failures "marks the reminder as notified" do expect(reminder.reload).to be_notified + expect(reminder.notification_id).to eq(notification.id) end end context "when the reminder is already notified" do + let(:reminder) { build_stubbed(:reminder, :notified) } + before do - reminder.update_column(:notified_at, Time.current) + allow(Notifications::CreateService).to receive(:new).and_call_original end it "does not create a notification from the reminder" do expect { subject }.not_to change(Notification, :count) + expect(Notifications::CreateService).not_to have_received(:new) end end end From 9627c5b8fc7f49bb58cdc817a74a4f1d32aaa393 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Sat, 23 Nov 2024 01:05:47 +0300 Subject: [PATCH 18/38] Reminder note should not be more than 80 chars --- app/contracts/reminders/base_contract.rb | 9 +++++++++ spec/contracts/reminders/base_contract_spec.rb | 14 ++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/app/contracts/reminders/base_contract.rb b/app/contracts/reminders/base_contract.rb index abdf358a7a1d..b539f96f6361 100644 --- a/app/contracts/reminders/base_contract.rb +++ b/app/contracts/reminders/base_contract.rb @@ -28,6 +28,8 @@ module Reminders class BaseContract < ::ModelContract + MAX_NOTE_CHARS_LENGTH = 80 + attribute :creator_id attribute :remindable_id attribute :remindable_type @@ -39,6 +41,7 @@ class BaseContract < ::ModelContract validate :validate_remindable_exists validate :validate_manage_reminders_permissions validate :validate_remind_at_is_in_future + validate :validate_note_length def self.model = Reminder @@ -62,6 +65,12 @@ def validate_remind_at_is_in_future end end + def validate_note_length + if model.note.present? && model.note.length > MAX_NOTE_CHARS_LENGTH + errors.add :note, :too_long, count: MAX_NOTE_CHARS_LENGTH + end + end + def validate_manage_reminders_permissions return if errors.added?(:remindable, :not_found) diff --git a/spec/contracts/reminders/base_contract_spec.rb b/spec/contracts/reminders/base_contract_spec.rb index 3005a15155f2..1dc9fcd654d4 100644 --- a/spec/contracts/reminders/base_contract_spec.rb +++ b/spec/contracts/reminders/base_contract_spec.rb @@ -119,5 +119,19 @@ end end + describe "validate note length" do + context "when note is too long" do + before { reminder.note = "a" * (described_class::MAX_NOTE_CHARS_LENGTH + 1) } + + it_behaves_like "contract is invalid", note: :too_long + end + + context "when note is within the limit" do + before { reminder.note = "a" * described_class::MAX_NOTE_CHARS_LENGTH } + + it_behaves_like "contract is valid" + end + end + include_examples "contract reuses the model errors" end From 89bd340e93bb3f3d26006468dbdfff8c10d3b4d4 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 Nov 2024 11:13:42 +0300 Subject: [PATCH 19/38] Ensure notifications are unique to reminders Add uniqueness index on NOT null ``` -- add_index(:reminders, :notification_id, {:unique=>true, :where=>"notification_id IS NOT NULL", :name=>"index_reminders_on_notification_id_unique"}) (1.0ms) CREATE UNIQUE INDEX "index_reminders_on_notification_id_unique" ON "reminders" ("notification_id") WHERE notification_id IS NOT NULL ``` On notification delete, nullify the reminder record- at present reminders are a persistent record (so are notifications, but that could change). --- app/models/notification.rb | 2 ++ db/migrate/20241119131205_create_reminders.rb | 5 +++++ spec/models/notification_spec.rb | 2 ++ spec/models/reminder_spec.rb | 4 ++++ 4 files changed, 13 insertions(+) diff --git a/app/models/notification.rb b/app/models/notification.rb index 2b2fbc5c6e4e..99407a2e29dd 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -51,6 +51,8 @@ class Notification < ApplicationRecord belongs_to :journal belongs_to :resource, polymorphic: true + has_one :reminder, dependent: :nullify + include Scopes::Scoped scopes :unsent_reminders_before, :mail_reminder_unsent, diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index e74d16016420..87e5e5d6fa49 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -11,5 +11,10 @@ def change t.timestamps end + + add_index :reminders, :notification_id, + unique: true, + where: "notification_id IS NOT NULL", + name: "index_reminders_on_notification_id_unique" end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index e615c033932c..b850207b6e3c 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -33,6 +33,8 @@ it { is_expected.to belong_to(:resource) } it { is_expected.to belong_to(:actor).class_name("User") } it { is_expected.to belong_to(:recipient).class_name("User") } + + it { is_expected.to have_one(:reminder).dependent(:nullify) } end describe "Enums" do diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index a65bcfd8857a..479a03fd39be 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -42,4 +42,8 @@ .backed_by_column_of_type(:integer) end end + + describe "DB Indexes" do + it { is_expected.to have_db_index(:notification_id).unique(true) } + end end From b9a5fd0cd5fe5d1560fd003e08ea0f56cc35d884 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 Nov 2024 17:35:03 +0300 Subject: [PATCH 20/38] Revert back to reminder state inference model (1) Notified - a `notification_id` exists (2) Scheduled - a `job_id` exists Reverts fdf09b9aa8208c22fcf9fb97e90234d01508591f --- app/models/reminder.rb | 3 +- app/services/reminders/create_service.rb | 2 +- .../reminders/schedule_reminder_job.rb | 2 +- db/migrate/20241119131205_create_reminders.rb | 1 - spec/factories/reminders_factory.rb | 1 - spec/models/reminder_spec.rb | 34 +++++++++++++++---- .../services/reminders/create_service_spec.rb | 2 +- .../reminders/schedule_reminder_job_spec.rb | 4 +-- 8 files changed, 34 insertions(+), 15 deletions(-) diff --git a/app/models/reminder.rb b/app/models/reminder.rb index 8218ea972218..a2271dfa5c43 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -31,5 +31,6 @@ class Reminder < ApplicationRecord belongs_to :creator, class_name: "User" belongs_to :notification, optional: true - enum :status, { pending: 0, scheduled: 1, notified: 2, done: 9 } + def notified? = notification_id.present? + def scheduled? = job_id.present? end diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb index 7674e63e059c..c5108c749adc 100644 --- a/app/services/reminders/create_service.rb +++ b/app/services/reminders/create_service.rb @@ -31,7 +31,7 @@ class CreateService < ::BaseServices::Create def after_perform(service_call) reminder = service_call.result job = Reminders::ScheduleReminderJob.schedule(reminder) - reminder.update_columns(status: :scheduled, job_id: job.job_id) + reminder.update_columns(job_id: job.job_id) service_call end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 95d64057b92f..51ec59593cd1 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -40,7 +40,7 @@ def perform(reminder) create_notification_service = create_notification_from_reminder(reminder) create_notification_service.on_success do |service_result| - reminder.update_columns(status: :notified, notification_id: service_result.result.id) + reminder.update_columns(notification_id: service_result.result.id) end create_notification_service.on_failure do |service_result| diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 87e5e5d6fa49..47d7677799ed 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -5,7 +5,6 @@ def change t.references :creator, null: false, foreign_key: { to_table: :users } t.references :notification, foreign_key: true t.datetime :remind_at, null: false - t.integer :status, default: 0, null: false t.string :job_id t.string :note diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index cda7d6a84e9c..29a45bfee8fe 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -39,7 +39,6 @@ trait :notified do job_id { SecureRandom.uuid } - status { :notified } notification factory: :notification end end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index 479a03fd39be..fdb8a5f85f29 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -35,15 +35,35 @@ it { is_expected.to belong_to(:notification).optional } end - describe "Enums" do - it do - expect(subject).to define_enum_for(:status) - .with_values({ pending: 0, scheduled: 1, notified: 2, done: 9 }) - .backed_by_column_of_type(:integer) + describe "DB Indexes" do + it { is_expected.to have_db_index(:notification_id).unique(true) } + end + + describe "#notified?" do + it "returns true if notification_id is present" do + reminder = build_stubbed(:reminder, :notified) + + expect(reminder).to be_notified + end + + it "returns false if notification_id is not present" do + reminder = build(:reminder, notification_id: nil) + + expect(reminder).not_to be_notified end end - describe "DB Indexes" do - it { is_expected.to have_db_index(:notification_id).unique(true) } + describe "#scheduled?" do + it "returns true if job_id is present" do + reminder = build_stubbed(:reminder, :scheduled) + + expect(reminder).to be_scheduled + end + + it "returns false if job_id is not present" do + reminder = build(:reminder, job_id: nil) + + expect(reminder).not_to be_scheduled + end end end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 76a89a0bc27f..509c88c9bfaf 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -46,7 +46,7 @@ subject expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) - expect(model_instance).to have_received(:update_columns).with(status: :scheduled, job_id: 1) + expect(model_instance).to have_received(:update_columns).with(job_id: 1) end end end diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 036d251e164a..95db3509756d 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -49,7 +49,7 @@ it "creates a notification from the reminder" do notification_svc = nil - expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(reminder, :status).to("notified") + expect { notification_svc = subject }.to change(Notification, :count).by(1) notification = notification_svc.result @@ -61,7 +61,7 @@ aggregate_failures "marks the reminder as notified" do expect(reminder.reload).to be_notified - expect(reminder.notification_id).to eq(notification.id) + expect(reminder.notification).to eq(notification) end end From a13494ce642fc7994e82d0d8488cc6241ad1a8d8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 25 Nov 2024 19:12:47 +0300 Subject: [PATCH 21/38] Add reminder note as an embedded notification detail --- config/locales/en.yml | 1 + .../notifications/notification_representer.rb | 2 +- .../property_factory/reminder.rb | 45 +++++++++++++++ .../v3/values/property_generic_representer.rb | 33 +++++++++++ .../v3/values/schemas/value_schema_factory.rb | 13 +++-- ...notification_representer_rendering_spec.rb | 55 +++++++++++++++++++ 6 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 lib/api/v3/notifications/property_factory/reminder.rb create mode 100644 lib/api/v3/values/property_generic_representer.rb diff --git a/config/locales/en.yml b/config/locales/en.yml index 203fec0fad30..d71685662228 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1448,6 +1448,7 @@ en: login: "Username" mail: "Email" name: "Name" + note: "Note" password: "Password" priority: "Priority" project: "Project" diff --git a/lib/api/v3/notifications/notification_representer.rb b/lib/api/v3/notifications/notification_representer.rb index 5b67c4dd9a3d..6984fe235187 100644 --- a/lib/api/v3/notifications/notification_representer.rb +++ b/lib/api/v3/notifications/notification_representer.rb @@ -104,7 +104,7 @@ def _type "Notification" end - self.to_eager_load = %i[actor journal] + self.to_eager_load = %i[actor journal reminder] end end end diff --git a/lib/api/v3/notifications/property_factory/reminder.rb b/lib/api/v3/notifications/property_factory/reminder.rb new file mode 100644 index 000000000000..e8bd4e210c2f --- /dev/null +++ b/lib/api/v3/notifications/property_factory/reminder.rb @@ -0,0 +1,45 @@ +# --copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 API::V3::Notifications::PropertyFactory + module Reminder + extend ::API::V3::Utilities::PathHelper + + module_function + + def for(notification) + return [] unless notification.reminder + + [ + ::API::V3::Values::PropertyGenericRepresenter + .new(::API::V3::Values::PropertyModel.new(:note, notification.reminder.note), + self_link: api_v3_paths.notification_detail(notification.id, 0)) + ] + end + end +end diff --git a/lib/api/v3/values/property_generic_representer.rb b/lib/api/v3/values/property_generic_representer.rb new file mode 100644 index 000000000000..ed814ef2fa38 --- /dev/null +++ b/lib/api/v3/values/property_generic_representer.rb @@ -0,0 +1,33 @@ +# --copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 API::V3::Values + class PropertyGenericRepresenter < PropertyRepresenter + property :value + end +end diff --git a/lib/api/v3/values/schemas/value_schema_factory.rb b/lib/api/v3/values/schemas/value_schema_factory.rb index de98a68e279e..441bb6734729 100644 --- a/lib/api/v3/values/schemas/value_schema_factory.rb +++ b/lib/api/v3/values/schemas/value_schema_factory.rb @@ -29,7 +29,7 @@ module API::V3::Values::Schemas module ValueSchemaFactory extend ::API::V3::Utilities::PathHelper - SUPPORTED = %w(start_date due_date date).freeze + SUPPORTED = %w(start_date due_date date note).freeze module_function @@ -64,10 +64,13 @@ def i18n_for(property) I18n.t("attributes.#{property}") end - def type_for(_property) - # This is but a stub. Currently, only 'start_date' and 'due_date' - # need to be supported so this simple approach works. - "Date" + def type_for(property) + case property + when "start_date", "due_date", "date" + "Date" + when "note" + "String" + end end end end diff --git a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb index c0fb12c42cbd..bb3aef99788e 100644 --- a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb +++ b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb @@ -267,5 +267,60 @@ .at_path("_embedded/details") end end + + shared_examples_for "embeds a Values::Property for reminder note" do + it "embeds a Values::Property" do + expect(generated) + .to be_json_eql("Values::Property".to_json) + .at_path("_embedded/details/0/_type") + end + + it "has a note value for the `property` property" do + expect(generated) + .to be_json_eql("note".to_json) + .at_path("_embedded/details/0/property") + end + + it "has a reminder`s note for the value" do + expect(generated) + .to be_json_eql(notification.reminder.note.to_json) + .at_path("_embedded/details/0/value") + end + end + + context "for a reminder when embedding" do + let(:reminder) { build_stubbed(:reminder) } + let(:reason) { :reminder } + let(:embed_links) { true } + + before do + allow(notification).to receive(:reminder).and_return(reminder) + end + + it_behaves_like "embeds a Values::Property for reminder note" + end + + context "for a reminder when not embedding" do + let(:reminder) { build_stubbed(:reminder) } + let(:reason) { :reminder } + let(:embed_links) { false } + + before do + allow(notification).to receive(:reminder).and_return(reminder) + end + + it_behaves_like "embeds a Values::Property for reminder note" + end + + context "for a reminder with no notification" do + let(:reminder) { nil } + let(:reason) { :reminder } + + it "has an empty details array" do + expect(generated) + .to have_json_size(0) + .at_path("_embedded/details") + end + end end end From 43da3e06fb136ac20427f687df33c35400380a5c Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Tue, 26 Nov 2024 13:03:20 +0300 Subject: [PATCH 22/38] remove incorrect type --- spec/services/reminders/create_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 509c88c9bfaf..65dde3e508b7 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -31,7 +31,7 @@ require "spec_helper" require "services/base_services/behaves_like_create_service" -RSpec.describe Reminders::CreateService, type: :model do +RSpec.describe Reminders::CreateService do it_behaves_like "BaseServices create service" do let(:factory) { :reminder } From b6367992bc9e9bc943603ad2d0e983c8c8afb1e9 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 27 Nov 2024 11:56:43 +0300 Subject: [PATCH 23/38] Revert "fix[Op#59433]: simplify reminder model to directly linked to notification" This reverts commit ae3f77da803f3eac97e5e12121f1266f807b7d31. Maintain a persistent record of reminder notifications as notifications are always retrievable by the user, e.g. via email, viewing "all" notifications. A reminder notification is only created IF there are no active unread notifications, hence when rescheduling, snoozing or editing a reminder then ensure all previous notifications are marked as read- as we should not have duplicate notifications for a given reminder. --- app/models/notification.rb | 3 +- app/models/reminder.rb | 13 +++++-- app/models/reminder_notification.rb | 4 +++ .../reminders/schedule_reminder_job.rb | 4 +-- db/migrate/20241119131205_create_reminders.rb | 8 +---- ...121113638_create_reminder_notifications.rb | 14 ++++++++ spec/factories/reminders_factory.rb | 18 ++++++++-- spec/models/notification_spec.rb | 3 +- spec/models/reminder_notification_spec.rb | 36 +++++++++++++++++++ spec/models/reminder_spec.rb | 25 +++++++------ .../services/reminders/create_service_spec.rb | 2 +- .../reminders/schedule_reminder_job_spec.rb | 16 ++++----- 12 files changed, 107 insertions(+), 39 deletions(-) create mode 100644 app/models/reminder_notification.rb create mode 100644 db/migrate/20241121113638_create_reminder_notifications.rb create mode 100644 spec/models/reminder_notification_spec.rb diff --git a/app/models/notification.rb b/app/models/notification.rb index 99407a2e29dd..e21493cc10db 100644 --- a/app/models/notification.rb +++ b/app/models/notification.rb @@ -51,7 +51,8 @@ class Notification < ApplicationRecord belongs_to :journal belongs_to :resource, polymorphic: true - has_one :reminder, dependent: :nullify + has_one :reminder_notification, dependent: :destroy + has_one :reminder, through: :reminder_notification include Scopes::Scoped scopes :unsent_reminders_before, diff --git a/app/models/reminder.rb b/app/models/reminder.rb index a2271dfa5c43..d960aa724cc2 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -29,8 +29,15 @@ class Reminder < ApplicationRecord belongs_to :remindable, polymorphic: true belongs_to :creator, class_name: "User" - belongs_to :notification, optional: true - def notified? = notification_id.present? - def scheduled? = job_id.present? + has_many :reminder_notifications, dependent: :destroy + has_many :notifications, through: :reminder_notifications + + def unread_notifications? + notifications.exists?(read_ian: [false, nil]) + end + + def scheduled? + job_id.present? + end end diff --git a/app/models/reminder_notification.rb b/app/models/reminder_notification.rb new file mode 100644 index 000000000000..cbdd70905a9b --- /dev/null +++ b/app/models/reminder_notification.rb @@ -0,0 +1,4 @@ +class ReminderNotification < ApplicationRecord + belongs_to :reminder + belongs_to :notification +end diff --git a/app/workers/reminders/schedule_reminder_job.rb b/app/workers/reminders/schedule_reminder_job.rb index 51ec59593cd1..522dad565bc0 100644 --- a/app/workers/reminders/schedule_reminder_job.rb +++ b/app/workers/reminders/schedule_reminder_job.rb @@ -35,12 +35,12 @@ def self.schedule(reminder) end def perform(reminder) - return if reminder.notified? + return if reminder.unread_notifications? create_notification_service = create_notification_from_reminder(reminder) create_notification_service.on_success do |service_result| - reminder.update_columns(notification_id: service_result.result.id) + ReminderNotification.create!(reminder:, notification: service_result.result) end create_notification_service.on_failure do |service_result| diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 47d7677799ed..454e830fefdc 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -3,17 +3,11 @@ def change create_table :reminders do |t| t.references :remindable, polymorphic: true, null: false t.references :creator, null: false, foreign_key: { to_table: :users } - t.references :notification, foreign_key: true t.datetime :remind_at, null: false t.string :job_id - t.string :note + t.text :note t.timestamps end - - add_index :reminders, :notification_id, - unique: true, - where: "notification_id IS NOT NULL", - name: "index_reminders_on_notification_id_unique" end end diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb new file mode 100644 index 000000000000..eaa711b431e5 --- /dev/null +++ b/db/migrate/20241121113638_create_reminder_notifications.rb @@ -0,0 +1,14 @@ +class CreateReminderNotifications < ActiveRecord::Migration[7.1] + def change + create_table :reminder_notifications do |t| + t.references :reminder, foreign_key: true + t.references :notification, foreign_key: true + + t.timestamps + end + + add_index :reminder_notifications, :notification_id, + unique: true, + name: "index_reminder_notifications_unique" + end +end diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index 29a45bfee8fe..982038d75e77 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -37,9 +37,21 @@ job_id { SecureRandom.uuid } end - trait :notified do - job_id { SecureRandom.uuid } - notification factory: :notification + trait :with_unread_notifications do + after(:create) do |reminder| + create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: false)) + end + end + + trait :with_read_notifications do + after(:create) do |reminder| + create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: true)) + end end end + + factory :reminder_notification do + reminder + notification + end end diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb index b850207b6e3c..c21c920e2617 100644 --- a/spec/models/notification_spec.rb +++ b/spec/models/notification_spec.rb @@ -34,7 +34,8 @@ it { is_expected.to belong_to(:actor).class_name("User") } it { is_expected.to belong_to(:recipient).class_name("User") } - it { is_expected.to have_one(:reminder).dependent(:nullify) } + it { is_expected.to have_one(:reminder_notification).dependent(:destroy) } + it { is_expected.to have_one(:reminder).through(:reminder_notification) } end describe "Enums" do diff --git a/spec/models/reminder_notification_spec.rb b/spec/models/reminder_notification_spec.rb new file mode 100644 index 000000000000..95c8193813c2 --- /dev/null +++ b/spec/models/reminder_notification_spec.rb @@ -0,0 +1,36 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" + +RSpec.describe ReminderNotification do + describe "Associations" do + it { is_expected.to belong_to(:reminder) } + it { is_expected.to belong_to(:notification) } + end +end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index fdb8a5f85f29..8b7304640124 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -32,24 +32,27 @@ describe "Associations" do it { is_expected.to belong_to(:remindable) } it { is_expected.to belong_to(:creator).class_name("User") } - it { is_expected.to belong_to(:notification).optional } + it { is_expected.to have_many(:reminder_notifications).dependent(:destroy) } + it { is_expected.to have_many(:notifications).through(:reminder_notifications) } end - describe "DB Indexes" do - it { is_expected.to have_db_index(:notification_id).unique(true) } - end + describe "#unread_notifications?" do + context "with an unread notification" do + subject { create(:reminder, :with_unread_notifications) } + + it { is_expected.to be_an_unread_notification } + end - describe "#notified?" do - it "returns true if notification_id is present" do - reminder = build_stubbed(:reminder, :notified) + context "with no unread notifications" do + subject { create(:reminder, :with_read_notifications) } - expect(reminder).to be_notified + it { is_expected.not_to be_an_unread_notification } end - it "returns false if notification_id is not present" do - reminder = build(:reminder, notification_id: nil) + context "with no notifications" do + subject { build(:reminder, notifications: []) } - expect(reminder).not_to be_notified + it { is_expected.not_to be_an_unread_notification } end end diff --git a/spec/services/reminders/create_service_spec.rb b/spec/services/reminders/create_service_spec.rb index 65dde3e508b7..b220ee90aa75 100644 --- a/spec/services/reminders/create_service_spec.rb +++ b/spec/services/reminders/create_service_spec.rb @@ -36,7 +36,7 @@ let(:factory) { :reminder } before do - allow(model_instance).to receive(:update_columns) + allow(model_instance).to receive(:update_columns).and_return(true) allow(Reminders::ScheduleReminderJob).to receive(:schedule) .with(model_instance) .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 1)) diff --git a/spec/workers/reminders/schedule_reminder_job_spec.rb b/spec/workers/reminders/schedule_reminder_job_spec.rb index 95db3509756d..2928e20cb68a 100644 --- a/spec/workers/reminders/schedule_reminder_job_spec.rb +++ b/spec/workers/reminders/schedule_reminder_job_spec.rb @@ -49,32 +49,28 @@ it "creates a notification from the reminder" do notification_svc = nil - expect { notification_svc = subject }.to change(Notification, :count).by(1) - - notification = notification_svc.result + expect { notification_svc = subject }.to change(Notification, :count).by(1) & change(ReminderNotification, :count).by(1) aggregate_failures "notification attributes" do + notification = notification_svc.result + expect(notification.recipient_id).to eq(reminder.creator_id) expect(notification.resource).to eq(reminder.remindable) expect(notification.reason).to eq("reminder") end - aggregate_failures "marks the reminder as notified" do - expect(reminder.reload).to be_notified - expect(reminder.notification).to eq(notification) + aggregate_failures "marks reminder as having unread notifications" do + expect(reminder.reload).to be_an_unread_notification end end context "when the reminder is already notified" do - let(:reminder) { build_stubbed(:reminder, :notified) } - before do - allow(Notifications::CreateService).to receive(:new).and_call_original + create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: false)) end it "does not create a notification from the reminder" do expect { subject }.not_to change(Notification, :count) - expect(Notifications::CreateService).not_to have_received(:new) end end end From 724f899033fec593b627e34a958c3862c80e82ab Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 27 Nov 2024 18:17:25 +0300 Subject: [PATCH 24/38] Define set reminders update service --- app/contracts/reminders/update_contract.rb | 41 +++++++ app/models/reminder.rb | 4 + app/services/reminders/update_service.rb | 63 +++++++++++ .../reminders/update_contract_spec.rb | 68 ++++++++++++ spec/models/reminder_spec.rb | 5 +- .../services/reminders/update_service_spec.rb | 102 ++++++++++++++++++ 6 files changed, 282 insertions(+), 1 deletion(-) create mode 100644 app/contracts/reminders/update_contract.rb create mode 100644 app/services/reminders/update_service.rb create mode 100644 spec/contracts/reminders/update_contract_spec.rb create mode 100644 spec/services/reminders/update_service_spec.rb diff --git a/app/contracts/reminders/update_contract.rb b/app/contracts/reminders/update_contract.rb new file mode 100644 index 000000000000..e09a94cdc81f --- /dev/null +++ b/app/contracts/reminders/update_contract.rb @@ -0,0 +1,41 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class UpdateContract < BaseContract + validate :unchangeable_attributes + + private + + def unchangeable_attributes + if model.remindable_changed? || model.creator_id_changed? + errors.add(:base, :unchangeable_attributes) + end + end + end +end diff --git a/app/models/reminder.rb b/app/models/reminder.rb index d960aa724cc2..0c8bb474b614 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -37,6 +37,10 @@ def unread_notifications? notifications.exists?(read_ian: [false, nil]) end + def unread_notifications + notifications.where(read_ian: [false, nil]) + end + def scheduled? job_id.present? end diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb new file mode 100644 index 000000000000..dce0e6b06880 --- /dev/null +++ b/app/services/reminders/update_service.rb @@ -0,0 +1,63 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class UpdateService < ::BaseServices::Update + def after_perform(service_call) + reminder = service_call.result + + if remind_at_changed?(reminder) + destroy_scheduled_reminder_job(reminder.job_id) if reminder.scheduled? + mark_unread_notifications_as_read_for(reminder) if reminder.unread_notifications? + + job = Reminders::ScheduleReminderJob.schedule(reminder) + reminder.update_columns(job_id: job.job_id) + end + + service_call + end + + private + + def remind_at_changed?(reminder) + # For some reason reminder.remind_at_changed? returns false + # so we assume a change if remind_at is present in the params (would have passed contract validation) + params.key?(:remind_at) && reminder.remind_at == params[:remind_at] + end + + def destroy_scheduled_reminder_job(job_id) + return unless job = GoodJob::Job.find_by(id: job_id) + + job.destroy unless job.finished? + end + + def mark_unread_notifications_as_read_for(reminder) + reminder.unread_notifications.update_all(read_ian: true) + end + end +end diff --git a/spec/contracts/reminders/update_contract_spec.rb b/spec/contracts/reminders/update_contract_spec.rb new file mode 100644 index 000000000000..1feb81a88fd2 --- /dev/null +++ b/spec/contracts/reminders/update_contract_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "contracts/shared/model_contract_shared_context" + +RSpec.describe Reminders::UpdateContract do + include_context "ModelContract shared context" + + let(:contract) { described_class.new(reminder, user) } + let(:user) { build_stubbed(:admin) } + let(:creator) { user } + let(:reminder) { build_stubbed(:reminder, creator:) } + + before do + User.current = user + allow(User).to receive(:exists?).with(user.id).and_return(true) + end + + describe "validate unchangeable attributes" do + context "when remindable changed" do + before do + reminder.remindable = build_stubbed(:work_package) + end + + it_behaves_like "contract is invalid", base: :unchangeable_attributes + end + + context "when creator_id changed" do + before do + new_creator = build_stubbed(:user) + reminder.creator = new_creator + allow(User).to receive(:exists?).with(new_creator.id).and_return(true) + end + + it_behaves_like "contract is invalid", base: :unchangeable_attributes + end + end + + include_examples "contract reuses the model errors" +end diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index 8b7304640124..8e77a7bcc14e 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -41,18 +41,21 @@ subject { create(:reminder, :with_unread_notifications) } it { is_expected.to be_an_unread_notification } + it { expect(subject.unread_notifications).to be_present } end context "with no unread notifications" do subject { create(:reminder, :with_read_notifications) } it { is_expected.not_to be_an_unread_notification } + it { expect(subject.unread_notifications).to be_empty } end context "with no notifications" do - subject { build(:reminder, notifications: []) } + subject { build_stubbed(:reminder) } it { is_expected.not_to be_an_unread_notification } + it { expect(subject.unread_notifications).to be_empty } end end diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb new file mode 100644 index 000000000000..b3cc66b9785e --- /dev/null +++ b/spec/services/reminders/update_service_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "services/base_services/behaves_like_update_service" + +RSpec.describe Reminders::UpdateService do + it_behaves_like "BaseServices update service" do + let(:factory) { :reminder } + end + + describe "remind_at changed" do + subject { described_class.new(user:, model: model_instance).call(call_attributes) } + + let(:model_instance) { create(:reminder, :scheduled, creator: user, job_id: 1) } + let(:user) { create(:admin) } + let(:call_attributes) { { remind_at: 2.days.from_now } } + + before do + allow(Reminders::ScheduleReminderJob).to receive(:schedule) + .with(model_instance) + .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 2)) + end + + context "with an existing unfinished scheduled job" do + let(:job) { instance_double(GoodJob::Job, finished?: false, destroy: true) } + + before do + allow(GoodJob::Job).to receive(:find_by).with(id: "1").and_return(job) + end + + it "reschedules the reminder" do + expect { subject }.to change(model_instance, :job_id).from("1").to("2") + + aggregate_failures "destroy existing job" do + expect(GoodJob::Job).to have_received(:find_by).with(id: "1") + expect(job).to have_received(:destroy) + end + + aggregate_failures "schedule new job" do + expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) + end + end + end + + context "with an existing finished scheduled job" do + let(:job) { instance_double(GoodJob::Job, finished?: true, destroy: true) } + + before do + allow(GoodJob::Job).to receive(:find_by).with(id: "1").and_return(job) + end + + it "schedules a new job" do + expect { subject }.to change(model_instance, :job_id).from("1").to("2") + + aggregate_failures "does NOT destroy existing job" do + expect(GoodJob::Job).to have_received(:find_by).with(id: "1") + expect(job).not_to have_received(:destroy) + end + + aggregate_failures "schedule new job" do + expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) + end + end + end + + context "with remind_at attribute in non-utc timezone" do + let(:call_attributes) { { remind_at: 2.days.from_now.in_time_zone("Africa/Nairobi") } } + + it "schedules the reminder" do + expect { subject }.to change(model_instance, :job_id).from("1").to("2") + end + end + end +end From c72487d26e7b9dc21be9c6d5b30432b58b93ddf8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 27 Nov 2024 18:38:09 +0300 Subject: [PATCH 25/38] Ensure unchangeable reminder attributes are protected --- app/contracts/reminders/update_contract.rb | 2 +- .../reminders/update_contract_spec.rb | 4 +-- .../services/reminders/update_service_spec.rb | 34 +++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/app/contracts/reminders/update_contract.rb b/app/contracts/reminders/update_contract.rb index e09a94cdc81f..de462cfec17d 100644 --- a/app/contracts/reminders/update_contract.rb +++ b/app/contracts/reminders/update_contract.rb @@ -34,7 +34,7 @@ class UpdateContract < BaseContract def unchangeable_attributes if model.remindable_changed? || model.creator_id_changed? - errors.add(:base, :unchangeable_attributes) + errors.add(:base, :unchangeable) end end end diff --git a/spec/contracts/reminders/update_contract_spec.rb b/spec/contracts/reminders/update_contract_spec.rb index 1feb81a88fd2..3f58ed19b0d2 100644 --- a/spec/contracts/reminders/update_contract_spec.rb +++ b/spec/contracts/reminders/update_contract_spec.rb @@ -50,7 +50,7 @@ reminder.remindable = build_stubbed(:work_package) end - it_behaves_like "contract is invalid", base: :unchangeable_attributes + it_behaves_like "contract is invalid", base: :unchangeable end context "when creator_id changed" do @@ -60,7 +60,7 @@ allow(User).to receive(:exists?).with(new_creator.id).and_return(true) end - it_behaves_like "contract is invalid", base: :unchangeable_attributes + it_behaves_like "contract is invalid", base: :unchangeable end end diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb index b3cc66b9785e..1f4822a6f1df 100644 --- a/spec/services/reminders/update_service_spec.rb +++ b/spec/services/reminders/update_service_spec.rb @@ -99,4 +99,38 @@ end end end + + describe "unchangeable attributes" do + let(:original_creator) { create(:user) } + let(:original_remindable) { create(:work_package) } + let(:model_instance) { create(:reminder, creator: original_creator, remindable: original_remindable) } + + context "when attempting to update the creator" do + subject { described_class.new(user: model_instance.creator, model: model_instance).call(creator: another_user) } + + let(:another_user) { create(:user) } + + it "does not update the creator", :aggregate_failures do + update_svc = subject + + expect(update_svc).to be_a_failure + expect(update_svc.result.reload.creator).to eq(original_creator) + expect(update_svc.message).to eq("Creator is invalid. may not be accessed. cannot be changed.") + end + end + + context "when attempting to update the remindable" do + subject { described_class.new(user: model_instance.creator, model: model_instance).call(remindable: another_remindable) } + + let(:another_remindable) { create(:work_package) } + + it "does not update the remindable", :aggregate_failures do + update_svc = subject + + expect(update_svc).to be_a_failure + expect(update_svc.result.reload.remindable).to eq(original_remindable) + expect(update_svc.message).to include("cannot be changed.") + end + end + end end From cd74446e4cafc51812c6a2c84a032c7caeb58a7e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 19:15:04 +0300 Subject: [PATCH 26/38] Ensure existing unread notifications are marked as read When a reminder schedule is updated, any existing unread notifications are marked as read; and new notification is scheduled --- spec/factories/reminders_factory.rb | 4 ++-- spec/services/reminders/update_service_spec.rb | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index 982038d75e77..c1b1bd0d50db 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -39,13 +39,13 @@ trait :with_unread_notifications do after(:create) do |reminder| - create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: false)) + create(:reminder_notification, reminder:, notification: create(:notification, read_ian: false)) end end trait :with_read_notifications do after(:create) do |reminder| - create(:reminder_notification, reminder: reminder, notification: create(:notification, read_ian: true)) + create(:reminder_notification, reminder:, notification: create(:notification, read_ian: true)) end end end diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb index 1f4822a6f1df..441597c7fa9d 100644 --- a/spec/services/reminders/update_service_spec.rb +++ b/spec/services/reminders/update_service_spec.rb @@ -39,7 +39,7 @@ describe "remind_at changed" do subject { described_class.new(user:, model: model_instance).call(call_attributes) } - let(:model_instance) { create(:reminder, :scheduled, creator: user, job_id: 1) } + let(:model_instance) { create(:reminder, :scheduled, :with_unread_notifications, creator: user, job_id: 1) } let(:user) { create(:admin) } let(:call_attributes) { { remind_at: 2.days.from_now } } @@ -64,6 +64,11 @@ expect(job).to have_received(:destroy) end + aggregate_failures "marks unread notifications as read" do + expect(model_instance.notifications.count).to eq(1) + expect(model_instance.unread_notifications.count).to eq(0) + end + aggregate_failures "schedule new job" do expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) end From 6602153e7be70a783d6d265c86f8a82f3fb6ebb2 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 19:39:59 +0300 Subject: [PATCH 27/38] reload model instance to verify state change In CI env the change is not seen for some odd reason- passes locally ```ruby expected `Reminder#job_id` to have changed from "1" to "2", but did not change ``` --- spec/services/reminders/update_service_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb index 441597c7fa9d..73bcdbf78e80 100644 --- a/spec/services/reminders/update_service_spec.rb +++ b/spec/services/reminders/update_service_spec.rb @@ -57,7 +57,7 @@ end it "reschedules the reminder" do - expect { subject }.to change(model_instance, :job_id).from("1").to("2") + expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") aggregate_failures "destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") @@ -83,7 +83,7 @@ end it "schedules a new job" do - expect { subject }.to change(model_instance, :job_id).from("1").to("2") + expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") aggregate_failures "does NOT destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") @@ -100,7 +100,7 @@ let(:call_attributes) { { remind_at: 2.days.from_now.in_time_zone("Africa/Nairobi") } } it "schedules the reminder" do - expect { subject }.to change(model_instance, :job_id).from("1").to("2") + expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") end end end From dca1fa369d0dff34757d3d5f141739341e58eb4f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 20:08:34 +0300 Subject: [PATCH 28/38] write into updated_at timestamp when marking notifications as read When using AR #update_all, a direct SQL update is performed which means updated_at is not updated (lol) --- app/services/reminders/update_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb index dce0e6b06880..8ad833d7c102 100644 --- a/app/services/reminders/update_service.rb +++ b/app/services/reminders/update_service.rb @@ -57,7 +57,7 @@ def destroy_scheduled_reminder_job(job_id) end def mark_unread_notifications_as_read_for(reminder) - reminder.unread_notifications.update_all(read_ian: true) + reminder.unread_notifications.update_all(read_ian: true, updated_at: Time.zone.now) end end end From 64cecd3241164a0ec7b9d4f56e9aa2c3bd9b8ab8 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 20:20:42 +0300 Subject: [PATCH 29/38] Unify Reminder#unread_notifactions query exists? can be chained to where without having adverse effects See: https://docs.rubocop.org/rubocop-rails/cops_rails.html#railswhereexists --- app/models/reminder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/reminder.rb b/app/models/reminder.rb index 0c8bb474b614..6c1eb71ef413 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -34,7 +34,7 @@ class Reminder < ApplicationRecord has_many :notifications, through: :reminder_notifications def unread_notifications? - notifications.exists?(read_ian: [false, nil]) + unread_notifications.exists? end def unread_notifications From c70546b4a4f177aeaa4472e7b11768bfe9df19c6 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 20:30:56 +0300 Subject: [PATCH 30/38] Reset model job id in before hook to reliably check for state change Failing in CI - perhaps when ran in global context --- spec/services/reminders/update_service_spec.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb index 73bcdbf78e80..311088074782 100644 --- a/spec/services/reminders/update_service_spec.rb +++ b/spec/services/reminders/update_service_spec.rb @@ -39,11 +39,12 @@ describe "remind_at changed" do subject { described_class.new(user:, model: model_instance).call(call_attributes) } - let(:model_instance) { create(:reminder, :scheduled, :with_unread_notifications, creator: user, job_id: 1) } + let(:model_instance) { create(:reminder, :scheduled, :with_unread_notifications, creator: user) } let(:user) { create(:admin) } let(:call_attributes) { { remind_at: 2.days.from_now } } before do + model_instance.update(job_id: 1) allow(Reminders::ScheduleReminderJob).to receive(:schedule) .with(model_instance) .and_return(instance_double(Reminders::ScheduleReminderJob, job_id: 2)) @@ -53,15 +54,16 @@ let(:job) { instance_double(GoodJob::Job, finished?: false, destroy: true) } before do - allow(GoodJob::Job).to receive(:find_by).with(id: "1").and_return(job) + allow(GoodJob::Job).to receive(:find_by).and_return(job) end it "reschedules the reminder" do - expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") + subject aggregate_failures "destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") expect(job).to have_received(:destroy) + expect(model_instance.reload.job_id).to eq("2") end aggregate_failures "marks unread notifications as read" do @@ -79,15 +81,16 @@ let(:job) { instance_double(GoodJob::Job, finished?: true, destroy: true) } before do - allow(GoodJob::Job).to receive(:find_by).with(id: "1").and_return(job) + allow(GoodJob::Job).to receive(:find_by).and_return(job) end it "schedules a new job" do - expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") + subject aggregate_failures "does NOT destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") expect(job).not_to have_received(:destroy) + expect(model_instance.reload.job_id).to eq("2") end aggregate_failures "schedule new job" do @@ -100,7 +103,8 @@ let(:call_attributes) { { remind_at: 2.days.from_now.in_time_zone("Africa/Nairobi") } } it "schedules the reminder" do - expect { subject }.to change(model_instance.reload, :job_id).from("1").to("2") + subject + expect(model_instance.reload.job_id).to eq("2") end end end From 2bebbd5de3638174df9fdafff190bef7e7495117 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 21:29:43 +0300 Subject: [PATCH 31/38] perform time comparison in unix epoch for consistency --- app/services/reminders/update_service.rb | 2 +- spec/services/reminders/update_service_spec.rb | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb index 8ad833d7c102..795fbacfc427 100644 --- a/app/services/reminders/update_service.rb +++ b/app/services/reminders/update_service.rb @@ -47,7 +47,7 @@ def after_perform(service_call) def remind_at_changed?(reminder) # For some reason reminder.remind_at_changed? returns false # so we assume a change if remind_at is present in the params (would have passed contract validation) - params.key?(:remind_at) && reminder.remind_at == params[:remind_at] + params.key?(:remind_at) && reminder.remind_at.to_i == params[:remind_at].to_i end def destroy_scheduled_reminder_job(job_id) diff --git a/spec/services/reminders/update_service_spec.rb b/spec/services/reminders/update_service_spec.rb index 311088074782..be14bd68b154 100644 --- a/spec/services/reminders/update_service_spec.rb +++ b/spec/services/reminders/update_service_spec.rb @@ -58,12 +58,11 @@ end it "reschedules the reminder" do - subject + expect { subject }.to change(model_instance, :job_id).from("1").to("2") aggregate_failures "destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") expect(job).to have_received(:destroy) - expect(model_instance.reload.job_id).to eq("2") end aggregate_failures "marks unread notifications as read" do @@ -72,6 +71,7 @@ end aggregate_failures "schedule new job" do + expect(model_instance.remind_at.to_i).to eq(call_attributes[:remind_at].to_i) expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) end end @@ -85,15 +85,15 @@ end it "schedules a new job" do - subject + expect { subject }.to change(model_instance, :job_id).from("1").to("2") aggregate_failures "does NOT destroy existing job" do expect(GoodJob::Job).to have_received(:find_by).with(id: "1") expect(job).not_to have_received(:destroy) - expect(model_instance.reload.job_id).to eq("2") end aggregate_failures "schedule new job" do + expect(model_instance.remind_at.to_i).to eq(call_attributes[:remind_at].to_i) expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) end end @@ -102,9 +102,13 @@ context "with remind_at attribute in non-utc timezone" do let(:call_attributes) { { remind_at: 2.days.from_now.in_time_zone("Africa/Nairobi") } } - it "schedules the reminder" do - subject - expect(model_instance.reload.job_id).to eq("2") + it "reschedules the reminder" do + expect { subject }.to change(model_instance, :job_id).from("1").to("2") + + aggregate_failures "schedule new job" do + expect(model_instance.remind_at.to_i).to eq(call_attributes[:remind_at].to_i) + expect(Reminders::ScheduleReminderJob).to have_received(:schedule).with(model_instance) + end end end end From 93e55276da7ed43d86fdf24a1d499bca9b8dda8f Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Thu, 28 Nov 2024 22:07:52 +0300 Subject: [PATCH 32/38] relax remind at change check to simple presence check In case of wonky time comparison issues, we'd rather have the reminder rescheduled with minimal tolerance than not having the reminder set at all --- app/services/reminders/update_service.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb index 795fbacfc427..bc34f4bb75f9 100644 --- a/app/services/reminders/update_service.rb +++ b/app/services/reminders/update_service.rb @@ -31,7 +31,7 @@ class UpdateService < ::BaseServices::Update def after_perform(service_call) reminder = service_call.result - if remind_at_changed?(reminder) + if remind_at_changed? destroy_scheduled_reminder_job(reminder.job_id) if reminder.scheduled? mark_unread_notifications_as_read_for(reminder) if reminder.unread_notifications? @@ -44,10 +44,10 @@ def after_perform(service_call) private - def remind_at_changed?(reminder) + def remind_at_changed? # For some reason reminder.remind_at_changed? returns false # so we assume a change if remind_at is present in the params (would have passed contract validation) - params.key?(:remind_at) && reminder.remind_at.to_i == params[:remind_at].to_i + params[:remind_at].present? end def destroy_scheduled_reminder_job(job_id) From be456ee9f737fe8ca3ee3b61c3d799cabd4e40b2 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 29 Nov 2024 12:39:53 +0300 Subject: [PATCH 33/38] Extract common reminder service helpers --- .../concerns/reminders/service_helpers.rb | 46 +++++++++++++++++++ app/services/reminders/update_service.rb | 16 ++----- 2 files changed, 50 insertions(+), 12 deletions(-) create mode 100644 app/services/concerns/reminders/service_helpers.rb diff --git a/app/services/concerns/reminders/service_helpers.rb b/app/services/concerns/reminders/service_helpers.rb new file mode 100644 index 000000000000..0865cbfd4152 --- /dev/null +++ b/app/services/concerns/reminders/service_helpers.rb @@ -0,0 +1,46 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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 Reminders + module ServiceHelpers + extend ActiveSupport::Concern + + def destroy_scheduled_reminder_job(reminder) + return unless reminder.scheduled? + return unless job = GoodJob::Job.find_by(id: reminder.job_id) + + job.destroy unless job.finished? + end + + def mark_unread_notifications_as_read_for(reminder) + return unless reminder.unread_notifications? + + reminder.unread_notifications.update_all(read_ian: true, updated_at: Time.zone.now) + end + end +end diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb index bc34f4bb75f9..b18795f96c50 100644 --- a/app/services/reminders/update_service.rb +++ b/app/services/reminders/update_service.rb @@ -28,12 +28,14 @@ module Reminders class UpdateService < ::BaseServices::Update + include Reminders::ServiceHelpers + def after_perform(service_call) reminder = service_call.result if remind_at_changed? - destroy_scheduled_reminder_job(reminder.job_id) if reminder.scheduled? - mark_unread_notifications_as_read_for(reminder) if reminder.unread_notifications? + destroy_scheduled_reminder_job(reminder) + mark_unread_notifications_as_read_for(reminder) job = Reminders::ScheduleReminderJob.schedule(reminder) reminder.update_columns(job_id: job.job_id) @@ -49,15 +51,5 @@ def remind_at_changed? # so we assume a change if remind_at is present in the params (would have passed contract validation) params[:remind_at].present? end - - def destroy_scheduled_reminder_job(job_id) - return unless job = GoodJob::Job.find_by(id: job_id) - - job.destroy unless job.finished? - end - - def mark_unread_notifications_as_read_for(reminder) - reminder.unread_notifications.update_all(read_ian: true, updated_at: Time.zone.now) - end end end From 36e23d9d5512c5a319cc432d28b65773982ade28 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 29 Nov 2024 16:30:08 +0300 Subject: [PATCH 34/38] Add reminder completion via delete service --- app/models/reminder.rb | 6 +- app/services/reminders/delete_service.rb | 39 ++++++++ db/migrate/20241119131205_create_reminders.rb | 1 + spec/factories/reminders_factory.rb | 5 + spec/models/reminder_spec.rb | 20 ++++ .../services/reminders/delete_service_spec.rb | 98 +++++++++++++++++++ 6 files changed, 168 insertions(+), 1 deletion(-) create mode 100644 app/services/reminders/delete_service.rb create mode 100644 spec/services/reminders/delete_service_spec.rb diff --git a/app/models/reminder.rb b/app/models/reminder.rb index 6c1eb71ef413..1228420fc19d 100644 --- a/app/models/reminder.rb +++ b/app/models/reminder.rb @@ -41,7 +41,11 @@ def unread_notifications notifications.where(read_ian: [false, nil]) end + def completed? + completed_at.present? + end + def scheduled? - job_id.present? + job_id.present? && !completed? end end diff --git a/app/services/reminders/delete_service.rb b/app/services/reminders/delete_service.rb new file mode 100644 index 000000000000..8250ffe12f8f --- /dev/null +++ b/app/services/reminders/delete_service.rb @@ -0,0 +1,39 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 2012-2024 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 Reminders + class DeleteService < ::BaseServices::Delete + include Reminders::ServiceHelpers + + def destroy(reminder) + destroy_scheduled_reminder_job(reminder) + mark_unread_notifications_as_read_for(reminder) + reminder.update(completed_at: Time.zone.now) + end + end +end diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index 454e830fefdc..cb2a73255894 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -4,6 +4,7 @@ def change t.references :remindable, polymorphic: true, null: false t.references :creator, null: false, foreign_key: { to_table: :users } t.datetime :remind_at, null: false + t.datetime :completed_at t.string :job_id t.text :note diff --git a/spec/factories/reminders_factory.rb b/spec/factories/reminders_factory.rb index c1b1bd0d50db..0c195743a1e9 100644 --- a/spec/factories/reminders_factory.rb +++ b/spec/factories/reminders_factory.rb @@ -37,6 +37,11 @@ job_id { SecureRandom.uuid } end + trait :completed do + job_id { SecureRandom.uuid } + completed_at { Time.zone.now } + end + trait :with_unread_notifications do after(:create) do |reminder| create(:reminder_notification, reminder:, notification: create(:notification, read_ian: false)) diff --git a/spec/models/reminder_spec.rb b/spec/models/reminder_spec.rb index 8e77a7bcc14e..3c3a9c675a7a 100644 --- a/spec/models/reminder_spec.rb +++ b/spec/models/reminder_spec.rb @@ -71,5 +71,25 @@ expect(reminder).not_to be_scheduled end + + it "returns false if completed_at is present" do + reminder = build(:reminder, :scheduled, :completed) + + expect(reminder).not_to be_scheduled + end + end + + describe "#completed?" do + it "returns true if completed_at is present" do + reminder = build(:reminder, :completed) + + expect(reminder).to be_completed + end + + it "returns false if completed_at is not present" do + reminder = build(:reminder, completed_at: nil) + + expect(reminder).not_to be_completed + end end end diff --git a/spec/services/reminders/delete_service_spec.rb b/spec/services/reminders/delete_service_spec.rb new file mode 100644 index 000000000000..40607fecb3c9 --- /dev/null +++ b/spec/services/reminders/delete_service_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require "spec_helper" +require "services/base_services/behaves_like_delete_service" + +RSpec.describe Reminders::DeleteService do + it_behaves_like "BaseServices delete service" do + let(:factory) { :reminder } + + before do + allow(model_instance).to receive(:update).and_return(model_destroy_result) + end + end + + describe "Remove reminder" do + subject { described_class.new(user:, model: model_instance).call } + + let(:model_instance) { create(:reminder, :scheduled, :with_unread_notifications, creator: user) } + let(:user) { create(:admin) } + + context "with an existing unfinished scheduled job" do + let(:job) { instance_double(GoodJob::Job, finished?: false, destroy: true) } + + before do + model_instance.update(job_id: 1, completed_at: nil) + allow(GoodJob::Job).to receive(:find_by).and_return(job) + end + + it "completes the reminder" do + expect { subject }.to change(model_instance, :completed_at).from(nil) + + aggregate_failures "destroy existing job" do + expect(GoodJob::Job).to have_received(:find_by).with(id: "1") + expect(job).to have_received(:destroy) + end + + aggregate_failures "marks unread notifications as read" do + expect(model_instance.notifications.count).to eq(1) + expect(model_instance.unread_notifications.count).to eq(0) + end + + aggregate_failures "marks the reminder as complete" do + expect(model_instance).to be_completed + end + end + end + + context "with an existing finished scheduled job" do + let(:job) { instance_double(GoodJob::Job, finished?: true, destroy: true) } + + before do + model_instance.update(job_id: 1, completed_at: nil) + allow(GoodJob::Job).to receive(:find_by).and_return(job) + end + + it "completes the reminder" do + expect { subject }.to change(model_instance, :completed_at).from(nil) + + aggregate_failures "does NOT destroy existing job" do + expect(GoodJob::Job).to have_received(:find_by).with(id: "1") + expect(job).not_to have_received(:destroy) + end + + aggregate_failures "marks the reminder as complete" do + expect(model_instance).to be_completed + end + end + end + end +end From 17b94f6f55432d105cf5e7129ae1fdcc77d1bf4e Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 29 Nov 2024 17:13:29 +0300 Subject: [PATCH 35/38] migrate existing roles to include manage own reminder permission --- ...opulate_manage_own_reminders_permission.rb | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 db/migrate/20241129135602_populate_manage_own_reminders_permission.rb diff --git a/db/migrate/20241129135602_populate_manage_own_reminders_permission.rb b/db/migrate/20241129135602_populate_manage_own_reminders_permission.rb new file mode 100644 index 000000000000..586be7102374 --- /dev/null +++ b/db/migrate/20241129135602_populate_manage_own_reminders_permission.rb @@ -0,0 +1,40 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + +require Rails.root.join("db/migrate/migration_utils/permission_adder") + +class PopulateManageOwnRemindersPermission < ActiveRecord::Migration[7.1] + def up + ::Migration::MigrationUtils::PermissionAdder + .add(:view_project, + :manage_own_reminders) + end + + # No-op + def down; end +end From d65b624a7fd7b2b49cb9893a456d225323d2e21d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Fri, 29 Nov 2024 17:13:45 +0300 Subject: [PATCH 36/38] add missing license blurb --- db/migrate/20241119131205_create_reminders.rb | 28 +++++++++++++++++++ ...121113638_create_reminder_notifications.rb | 28 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/db/migrate/20241119131205_create_reminders.rb b/db/migrate/20241119131205_create_reminders.rb index cb2a73255894..1ad0989de80a 100644 --- a/db/migrate/20241119131205_create_reminders.rb +++ b/db/migrate/20241119131205_create_reminders.rb @@ -1,3 +1,31 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + class CreateReminders < ActiveRecord::Migration[7.1] def change create_table :reminders do |t| diff --git a/db/migrate/20241121113638_create_reminder_notifications.rb b/db/migrate/20241121113638_create_reminder_notifications.rb index eaa711b431e5..0e92b9dab99e 100644 --- a/db/migrate/20241121113638_create_reminder_notifications.rb +++ b/db/migrate/20241121113638_create_reminder_notifications.rb @@ -1,3 +1,31 @@ +#-- copyright +# OpenProject is an open source project management software. +# Copyright (C) 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. +#++ + class CreateReminderNotifications < ActiveRecord::Migration[7.1] def change create_table :reminder_notifications do |t| From 4bb700718c67f3fda377f13bfed4cdc575ee5f18 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 2 Dec 2024 16:59:45 +0300 Subject: [PATCH 37/38] Further decompose reminders service helpers --- app/services/concerns/reminders/service_helpers.rb | 11 +++++++++++ app/services/reminders/create_service.rb | 6 +++--- app/services/reminders/update_service.rb | 10 +--------- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/app/services/concerns/reminders/service_helpers.rb b/app/services/concerns/reminders/service_helpers.rb index 0865cbfd4152..b6e27e2d446a 100644 --- a/app/services/concerns/reminders/service_helpers.rb +++ b/app/services/concerns/reminders/service_helpers.rb @@ -30,6 +30,17 @@ module Reminders module ServiceHelpers extend ActiveSupport::Concern + def reschedule_reminder(reminder) + destroy_scheduled_reminder_job(reminder) + mark_unread_notifications_as_read_for(reminder) + schedule_new_reminder_job(reminder) + end + + def schedule_new_reminder_job(reminder) + job = Reminders::ScheduleReminderJob.schedule(reminder) + reminder.update_columns(job_id: job.job_id) + end + def destroy_scheduled_reminder_job(reminder) return unless reminder.scheduled? return unless job = GoodJob::Job.find_by(id: reminder.job_id) diff --git a/app/services/reminders/create_service.rb b/app/services/reminders/create_service.rb index c5108c749adc..340f980a289f 100644 --- a/app/services/reminders/create_service.rb +++ b/app/services/reminders/create_service.rb @@ -28,10 +28,10 @@ module Reminders class CreateService < ::BaseServices::Create + include Reminders::ServiceHelpers + def after_perform(service_call) - reminder = service_call.result - job = Reminders::ScheduleReminderJob.schedule(reminder) - reminder.update_columns(job_id: job.job_id) + schedule_new_reminder_job(service_call.result) service_call end diff --git a/app/services/reminders/update_service.rb b/app/services/reminders/update_service.rb index b18795f96c50..9a1bd002e1cd 100644 --- a/app/services/reminders/update_service.rb +++ b/app/services/reminders/update_service.rb @@ -31,15 +31,7 @@ class UpdateService < ::BaseServices::Update include Reminders::ServiceHelpers def after_perform(service_call) - reminder = service_call.result - - if remind_at_changed? - destroy_scheduled_reminder_job(reminder) - mark_unread_notifications_as_read_for(reminder) - - job = Reminders::ScheduleReminderJob.schedule(reminder) - reminder.update_columns(job_id: job.job_id) - end + reschedule_reminder(service_call.result) if remind_at_changed? service_call end From 220ca16ef02239a0b3294e8d51640619825f7718 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Mon, 2 Dec 2024 17:29:37 +0300 Subject: [PATCH 38/38] Revert "Add reminder note as an embedded notification detail" This reverts commit a13494ce642fc7994e82d0d8488cc6241ad1a8d8. --- config/locales/en.yml | 1 - .../notifications/notification_representer.rb | 2 +- .../property_factory/reminder.rb | 45 --------------- .../v3/values/property_generic_representer.rb | 33 ----------- .../v3/values/schemas/value_schema_factory.rb | 13 ++--- ...notification_representer_rendering_spec.rb | 55 ------------------- 6 files changed, 6 insertions(+), 143 deletions(-) delete mode 100644 lib/api/v3/notifications/property_factory/reminder.rb delete mode 100644 lib/api/v3/values/property_generic_representer.rb diff --git a/config/locales/en.yml b/config/locales/en.yml index 5ab4b6bae1b7..3d2a4e308d20 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1510,7 +1510,6 @@ en: login: "Username" mail: "Email" name: "Name" - note: "Note" password: "Password" priority: "Priority" project: "Project" diff --git a/lib/api/v3/notifications/notification_representer.rb b/lib/api/v3/notifications/notification_representer.rb index 6984fe235187..5b67c4dd9a3d 100644 --- a/lib/api/v3/notifications/notification_representer.rb +++ b/lib/api/v3/notifications/notification_representer.rb @@ -104,7 +104,7 @@ def _type "Notification" end - self.to_eager_load = %i[actor journal reminder] + self.to_eager_load = %i[actor journal] end end end diff --git a/lib/api/v3/notifications/property_factory/reminder.rb b/lib/api/v3/notifications/property_factory/reminder.rb deleted file mode 100644 index e8bd4e210c2f..000000000000 --- a/lib/api/v3/notifications/property_factory/reminder.rb +++ /dev/null @@ -1,45 +0,0 @@ -# --copyright -# OpenProject is an open source project management software. -# Copyright (C) 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 API::V3::Notifications::PropertyFactory - module Reminder - extend ::API::V3::Utilities::PathHelper - - module_function - - def for(notification) - return [] unless notification.reminder - - [ - ::API::V3::Values::PropertyGenericRepresenter - .new(::API::V3::Values::PropertyModel.new(:note, notification.reminder.note), - self_link: api_v3_paths.notification_detail(notification.id, 0)) - ] - end - end -end diff --git a/lib/api/v3/values/property_generic_representer.rb b/lib/api/v3/values/property_generic_representer.rb deleted file mode 100644 index ed814ef2fa38..000000000000 --- a/lib/api/v3/values/property_generic_representer.rb +++ /dev/null @@ -1,33 +0,0 @@ -# --copyright -# OpenProject is an open source project management software. -# Copyright (C) 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 API::V3::Values - class PropertyGenericRepresenter < PropertyRepresenter - property :value - end -end diff --git a/lib/api/v3/values/schemas/value_schema_factory.rb b/lib/api/v3/values/schemas/value_schema_factory.rb index 441bb6734729..de98a68e279e 100644 --- a/lib/api/v3/values/schemas/value_schema_factory.rb +++ b/lib/api/v3/values/schemas/value_schema_factory.rb @@ -29,7 +29,7 @@ module API::V3::Values::Schemas module ValueSchemaFactory extend ::API::V3::Utilities::PathHelper - SUPPORTED = %w(start_date due_date date note).freeze + SUPPORTED = %w(start_date due_date date).freeze module_function @@ -64,13 +64,10 @@ def i18n_for(property) I18n.t("attributes.#{property}") end - def type_for(property) - case property - when "start_date", "due_date", "date" - "Date" - when "note" - "String" - end + def type_for(_property) + # This is but a stub. Currently, only 'start_date' and 'due_date' + # need to be supported so this simple approach works. + "Date" end end end diff --git a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb index bb3aef99788e..c0fb12c42cbd 100644 --- a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb +++ b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb @@ -267,60 +267,5 @@ .at_path("_embedded/details") end end - - shared_examples_for "embeds a Values::Property for reminder note" do - it "embeds a Values::Property" do - expect(generated) - .to be_json_eql("Values::Property".to_json) - .at_path("_embedded/details/0/_type") - end - - it "has a note value for the `property` property" do - expect(generated) - .to be_json_eql("note".to_json) - .at_path("_embedded/details/0/property") - end - - it "has a reminder`s note for the value" do - expect(generated) - .to be_json_eql(notification.reminder.note.to_json) - .at_path("_embedded/details/0/value") - end - end - - context "for a reminder when embedding" do - let(:reminder) { build_stubbed(:reminder) } - let(:reason) { :reminder } - let(:embed_links) { true } - - before do - allow(notification).to receive(:reminder).and_return(reminder) - end - - it_behaves_like "embeds a Values::Property for reminder note" - end - - context "for a reminder when not embedding" do - let(:reminder) { build_stubbed(:reminder) } - let(:reason) { :reminder } - let(:embed_links) { false } - - before do - allow(notification).to receive(:reminder).and_return(reminder) - end - - it_behaves_like "embeds a Values::Property for reminder note" - end - - context "for a reminder with no notification" do - let(:reminder) { nil } - let(:reason) { :reminder } - - it "has an empty details array" do - expect(generated) - .to have_json_size(0) - .at_path("_embedded/details") - end - end end end