Merge pull request #15985 from opf/fix-acts_as_favorable

Fix and refactor acts as favorable and acts as watchable

config/initializers/acts_as_favorable.rb

@@ -1,8 +1,10 @@
 # Be sure to restart your server when you modify this file.
 
-# For development and non-eager load mode, we need to register the acts_as_favorable models manually
+# For development and non-eager load mode, we need to load models using acts_as_favorable manually
 # as no eager loading takes place
-Rails.application.config.after_initialize do
-  OpenProject::Acts::Favorable::Registry
-    .add(Project)
+Rails.application.config.to_prepare do
+  OpenProject::Acts::Favorable::Registry.add(
+    Project,
+    reset: true
+  )
 end

config/initializers/acts_as_watchable.rb

@@ -1,8 +1,16 @@
 # Be sure to restart your server when you modify this file.
 
-# In development and non-eager loaded mode, we need to register the acts_as_watchable models manually
+# For development and non-eager load mode, we need to load models using acts_as_watchable manually
 # as no eager loading takes place
-Rails.application.config.after_initialize do
-  OpenProject::Acts::Watchable::Registry
-    .add(WorkPackage, Message, Forum, News, Meeting, Wiki, WikiPage)
+Rails.application.config.to_prepare do
+  OpenProject::Acts::Watchable::Registry.add(
+    Forum,
+    Meeting,
+    Message,
+    News,
+    Wiki,
+    WikiPage,
+    WorkPackage,
+    reset: true
+  )
 end

config/routes.rb

@@ -184,13 +184,13 @@
   end
 
   # generic route for adding/removing watchers
-  scope ":object_type/:object_id", constraints: OpenProject::Acts::Watchable::Routes do
+  scope ":object_type/:object_id", constraints: OpenProject::Acts::Watchable::RouteConstraint do
     post "/watch" => "watchers#watch"
     delete "/unwatch" => "watchers#unwatch"
   end
 
   # generic route for adding/removing favorites
-  scope ":object_type/:object_id", constraints: OpenProject::Acts::Favorable::Routes do
+  scope ":object_type/:object_id", constraints: OpenProject::Acts::Favorable::RouteConstraint do
     post "/favorite" => "favorites#favorite"
     delete "/favorite" => "favorites#unfavorite"
   end

lib_static/open_project/acts/favorable.rb

@@ -46,11 +46,11 @@ module ClassMethods
         # acts_as_favorable expects that the including module defines a +visible?(user)+ method,
         # as it's used to identify whether a user can actually favorite the object.
         def acts_as_favorable
-          return if included_modules.include?(::OpenProject::Acts::Favorable::InstanceMethods)
-
-          OpenProject::Acts::Favorable::Registry.add(self)
+          return if included_modules.include?(InstanceMethods)
 
           class_eval do
+            prepend InstanceMethods
+
             has_many :favorites, as: :favored, dependent: :delete_all, validate: false
             has_many :favoring_users, through: :favorites, source: :user, validate: false
 
@@ -60,7 +60,7 @@ def acts_as_favorable
             }
           end
 
-          send :prepend, ::OpenProject::Acts::Favorable::InstanceMethods
+          Registry.add(self)
         end
       end
 

lib_static/open_project/acts/favorable/registry.rb

@@ -30,27 +30,7 @@ module OpenProject
   module Acts
     module Favorable
       module Registry
-        def self.models
-          @models ||= Set.new
-        end
-
-        def self.exists?(model)
-          models.include?(model)
-        end
-
-        def self.instance(model_name)
-          models.detect { |cls| cls.name == model_name.singularize.camelize }
-        end
-
-        def self.add(*models)
-          models.each do |model|
-            unless model.ancestors.include?(::OpenProject::Acts::Watchable)
-              raise ArgumentError.new("Model #{model} does not include acts_as_watchable")
-            end
-
-            self.models << model
-          end
-        end
+        extend RegistryMethods
       end
     end
   end

lib_static/open_project/acts/favorable/routes.rb → lib_static/open_project/acts/favorable/route_constraint.rb

@@ -29,7 +29,7 @@
 module OpenProject
   module Acts
     module Favorable
-      module Routes
+      module RouteConstraint
         def self.matches?(request)
           params = request.path_parameters
 

lib_static/open_project/acts/registry_methods.rb

@@ -0,0 +1,58 @@
+#-- 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 OpenProject
+  module Acts
+    module RegistryMethods
+      def instance(model_name)
+        models[model_name.singularize.camelize]
+      end
+
+      def add(*models, reset: false)
+        instance_methods_module = module_parent.const_get(:InstanceMethods)
+        acts_as_method_name = "acts_as_#{module_parent_name.demodulize.underscore}"
+
+        self.models.clear if reset
+
+        models.each do |model|
+          unless model.ancestors.include?(instance_methods_module)
+            raise ArgumentError.new("Model #{model} does not include #{acts_as_method_name}")
+          end
+
+          self.models[model.name] = model
+        end
+      end
+
+      private
+
+      def models
+        @models ||= Hash.new
+      end
+    end
+  end
+end

lib_static/open_project/acts/watchable.rb

@@ -25,8 +25,6 @@
 #
 # See COPYRIGHT and LICENSE files for more details.
 #++
-require_relative "watchable/registry"
-require_relative "watchable/routes"
 
 module OpenProject
   module Acts
@@ -54,11 +52,13 @@ module ClassMethods
         #                 is allowed to watch
 
         def acts_as_watchable(options = {})
-          return if included_modules.include?(::OpenProject::Acts::Watchable::InstanceMethods)
+          return if included_modules.include?(InstanceMethods)
 
           acts_as_watchable_enforce_project_association
 
           class_eval do
+            prepend InstanceMethods
+
             has_many :watchers, as: :watchable, dependent: :delete_all, validate: false
             has_many :watcher_users, through: :watchers, source: :user, validate: false
 
@@ -70,26 +70,23 @@ def acts_as_watchable(options = {})
             class_attribute :acts_as_watchable_options
 
             self.acts_as_watchable_options = options
-            ::OpenProject::Acts::Watchable::Registry.add(self)
           end
 
-          send :prepend, ::OpenProject::Acts::Watchable::InstanceMethods
+          Registry.add(self)
         end
 
         def acts_as_watchable_enforce_project_association
-          unless reflect_on_association(:project)
-            message = <<-MESSAGE
+          return if reflect_on_association(:project)
 
-              The #{self} model does not have an association to the Project model.
+          raise <<-MESSAGE
+            The #{self} model does not have an association to the Project model.
 
-              acts_as_watchable requires the including model to have such an association.
+            acts_as_watchable requires the including model to have such an association.
 
-              If no direct association exists, consider adding a
-                has_one :project, through: ...
-              association.
-            MESSAGE
-            raise message
-          end
+            If no direct association exists, consider adding a
+              has_one :project, through: ...
+            association.
+          MESSAGE
         end
       end
 

lib_static/open_project/acts/watchable/registry.rb

@@ -30,27 +30,7 @@ module OpenProject
   module Acts
     module Watchable
       module Registry
-        def self.models
-          @models ||= Set.new
-        end
-
-        def self.exists?(model)
-          models.include?(model)
-        end
-
-        def self.instance(model_name)
-          models.detect { |cls| cls.name == model_name.singularize.camelize }
-        end
-
-        def self.add(*models)
-          models.each do |model|
-            unless model.ancestors.include?(::OpenProject::Acts::Watchable)
-              raise ArgumentError.new("Model #{model} does not include acts_as_watchable")
-            end
-
-            self.models << model
-          end
-        end
+        extend RegistryMethods
       end
     end
   end

lib_static/open_project/acts/watchable/routes.rb → lib_static/open_project/acts/watchable/route_constraint.rb

@@ -25,12 +25,11 @@
 #
 # See COPYRIGHT and LICENSE files for more details.
 #++
+
 module OpenProject
   module Acts
     module Watchable
-      module Routes
-        mattr_accessor :models
-
+      module RouteConstraint
         def self.matches?(request)
           params = request.path_parameters
 

spec/lib/acts_as_watchable/lib/acts_as_watchable/routes_spec.rb → spec/lib/acts_as_watchable/lib/open_project/acts/favorable/route_constraint_spec.rb

@@ -28,39 +28,32 @@
 
 require "spec_helper"
 
-RSpec.describe OpenProject::Acts::Watchable::Routes do
-  let(:request) do
-    Struct.new(:type, :id) do
-      def path_parameters
-        { object_id: id, object_type: type }
-      end
-    end.new(type, id)
-  end
+RSpec.describe OpenProject::Acts::Favorable::RouteConstraint do
+  let(:request) { instance_double(ActionDispatch::Request, path_parameters:) }
+  let(:path_parameters) { { object_id: id, object_type: type } }
 
   describe "matches?" do
-    shared_examples_for "watched model" do
-      describe "for a valid id string" do
-        let(:id) { "1" }
-
-        it "is true" do
-          expect(described_class).to be_matches(request)
-        end
-      end
+    %w[
+      projects
+    ].each do |type|
+      describe "routing #{type} watches" do
+        let(:type) { type }
 
-      describe "for an invalid id string" do
-        let(:id) { "schmu" }
+        describe "for a valid id string" do
+          let(:id) { "1" }
 
-        it "is false" do
-          expect(described_class).not_to be_matches(request)
+          it "is true" do
+            expect(described_class).to be_matches(request)
+          end
         end
-      end
-    end
 
-    ["work_packages", "news", "forums", "messages", "wikis", "wiki_pages"].each do |type|
-      describe "routing #{type} watches" do
-        let(:type) { type }
+        describe "for an invalid id string" do
+          let(:id) { "schmu" }
 
-        it_behaves_like "watched model"
+          it "is false" do
+            expect(described_class).not_to be_matches(request)
+          end
+        end
       end
     end