Fix and refactor acts as favorable and acts as watchable #15985
Conversation
…chable Scope module is included in ApplicationRecord, so all models havel it included, but we want to check that only models that have acts_as_... method called on them are added to the registry, so we should use InstanceMethods instead.
To prevent copy pasting prbolem
toy
changed the title
toy
force-pushed
the
fix-acts_as_favorable
branch
from
abc1250 to
8847502
Compare
…plicitly adding to registry
…as_watchable initializers to_prepare is also called every time code is reloaded
toy
force-pushed
the
fix-acts_as_favorable
branch
from
8847502 to
2c2c4d8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…:Favorable::RouteConstraint Can't come up with a good name for shared module to extract code from RouteConstraint modules and specs
Storing classes in development may lead to problems when reloading
classes, as new versions of the class will be appended to the list, but
`instance` method will still find the old versions of the classes.
I considered adding caching for production environment, but additional
code is not justified by 3 times faster execution.
```ruby
require "benchmark/ips"
h = {"Project" => Project}
Benchmark.ips do |x|
x.report("constantize") { "Project".constantize }
x.report("hash access") { h["Project"] }
x.compare!
end
```
```
Warming up --------------------------------------
constantize 1.163M i/100ms
hash access 3.397M i/100ms
Calculating -------------------------------------
constantize 11.842M (± 1.5%) i/s - 59.291M in
5.007752s
hash access 34.379M (± 1.3%) i/s - 173.239M in
5.039989s
Comparison:
hash access: 34378546.0 i/s
constantize: 11842325.0 i/s - 2.90x slower
```
toy
force-pushed
the
fix-acts_as_favorable
branch
from
2c2c4d8 to
87e9803
Compare
| .add(WorkPackage, Message, Forum, News, Meeting, Wiki, WikiPage) | ||
| Rails.application.config.to_prepare do | ||
| Forum | ||
| Meeting |
There was a problem hiding this comment.
This model is coming from meeting module, so it seems more appropriate to move this line to modules/meeting/lib/open_project/meeting/engine.rb, but all modules are loaded through Gemfile.modules, so not sure what is better - keeping it here or moving it there.
| raise ArgumentError.new("Model #{model} does not include #{acts_as_method_name}") | ||
| end | ||
|
|
||
| class_names << model.name |
There was a problem hiding this comment.
Why did we change the behavior here to put the class names (as string) in the set and not the classes anymore? This requires us to use constantize in the instance method. When we still put the class names in the set, we can use the same models.detect method we used in the old implementation.
There was a problem hiding this comment.
I guess this is due to this:
Both registries were storing model classes, so in development new versions of those models were added when reloaded, but the registry still returned the first model class that was registered
But this could be fixed in the to_prepare call to simply clear out the registry.
There was a problem hiding this comment.
So you prefer a separate method to clear registry (or an argument of add method) over storing names of models? I'll have a look myself if it looks better
There was a problem hiding this comment.
Using Hash both resolves model reloading problem and doesn't need walking the set using detect, but also added explicit resetting of registries in to_prepare
On the surface, it is arguably the exact same thing albeit a bit more explicit. I prefer having some notion of what classes have it rather than relying on autoloading, as developers might simply forget it. With the registry, this could not be the case (as soon as they started using/testing the feature, the controllers were checking the registry to see which classes existed) |
This will alternatively resolve the problem with duplicate classes in development on reload without the need to explicitly reset the registry
toy
force-pushed
the
fix-acts_as_favorable
branch
from
c60686c to
73424a7
Compare
Fixed:
Favorable::Registrywas copied without changing the module inside fromWatchableand method name from"acts_as_watchable"Watchableto check that only models that haveacts_as_watchablecalled in them were allowed, butWatchableis included inApplicationRecord, soWatchable::InstanceMethodsshould be used instead. Also theacts_as_watchablewas registering model before including the moduleRefactored:
No need to explicitly callRegistry.add(Model)in initialisers, it is enough to just reference models to trigger autoloadRoutestoRouteConstraintfor clarityRegistryMethods