-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🧹 Clean up services #2109
🧹 Clean up services #2109
Conversation
This commit will reconcile the services that are overrides for Hyrax with the Hyrax 5.0.0rc2 version. There were a number of overrides that were changed to the decorator pattern.
@@ -209,7 +118,7 @@ def self.create_user_collection_type | |||
# If calling from Abilities, pass the ability. | |||
# If you try to get the ability from the user, you end up in an infinite loop. | |||
# rubocop:disable Metrics/MethodLength | |||
def self.add_default_participants(collection_type_id) | |||
def add_default_participants(collection_type_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel better if this were in a submodule ClassMethods
Reading this module it would be easy to think it's an instance method. But we prepend it to a singleton class.
end | ||
end | ||
|
||
Hyrax::ThumbnailPathService.singleton_class.send(:prepend, Hyrax::ThumbnailPathServiceDecorator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hyrax::ThumbnailPathService.singleton_class.send(:prepend, Hyrax::ThumbnailPathServiceDecorator) | |
Hyrax::ThumbnailPathService.singleton_class.send(:prepend, Hyrax::WorkThumbnailPathServiceDecorator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the above change? We can get rid of another module and reduce duplication of knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, are you suggesting to remove the app/services/hyrax/thumbnail_path_service_decorator.rb
and put Hyrax::ThumbnailPathService.singleton_class.send(:prepend, Hyrax::WorkThumbnailPathServiceDecorator)
in app/services/hyrax/work_thumbnail_path_service_decorator.rb
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my concern with that is the coupling of both of services. In the super
Hyrax actually has two different returns
WorkThumbnailPathService
=> ActionController::Base.helpers.image_path 'work.png'
and
ThumbnailPathService
=> ActionController::Base.helpers.image_path 'default.png'
If in an event that we want to change Hyku's thumbnail path and work thumbnail path then it would be easier to find and already set up. But I'm not holding on to this idea too strongly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm alright with what you have. But consider, your tests show what super
means and the code looks like duplication (because it is; it just overlays different things, which is the nature of polymorphism/encapsulation)
This commit addresses comments from the review but one thing that is of note is loading the I18n translations in the application.rb file. We needed this because our decorators load prior to I18n loads the locales in our config/locales directory for them to use so we were getting missing translations.
This commit will reconcile the services that are overrides for Hyrax with the Hyrax 5.0.0rc2 version. There were a number of overrides that were changed to the decorator pattern.
Ref: