From 7b885b09ce56ab6cab9e8a3da6caaaedf04d9453 Mon Sep 17 00:00:00 2001 From: Kevin Dew Date: Tue, 4 May 2021 23:59:22 +0100 Subject: [PATCH] Use a mixin for common `.call` class interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This creates a new mixin, Callable, which can be used to provide a class with the boilerplate so that it can have a public class method of `.call` which calls `#initialize` with the arguments and then `#call`. The creation of this class is a port of the same approach applied in Content Publisher [1] (and most of this commit message is a port too in case you're feeling a touch of déjà vu). This mixin replaces the inheritance approach used in builders, presenters and services. The change was prompted by a linting violation with one of rubocop's newer rules: `Lint/MissingSuper`, which is an offence when a class inherits but does not call `super` in it's initialize method. This violation seemed to only affect some of GOV.UK's newer code, such as this service pattern, which led to some debate [2] about what the most appropriate means to share this behaviour is, whether our inheritance approach was actually idiomatic and how using super in every `initialize` would be unnecessary boilerplate. The Rubocop thread [3] regarding this rule raised an interesting point "I'm curious of examples where there are good reasons to explicitly not call super.", which led to me considering why we didn't think `super` would be appropriate in these classes. My conclusion to this was that we weren't actually intending to create a subtype with inheritance, we actually just wanted standardisation with the interface, and that the inherited classes have very little behavioural consistency, breaking Liskov substitution principle [4]. When looking at the Ruby standard library a module seems the most appropriate way to implement it, considering similarities with Singleton and Enumerable. [1]: https://github.com/alphagov/content-publisher/commit/5933f87c02403e25e06af6152b9f348d3235b2b2 [2]: https://github.com/alphagov/rubocop-govuk/pull/125 [3]: https://github.com/rubocop/rubocop/issues/8506 [4]: https://thoughtbot.com/blog/back-to-basics-solid#liskov-substitution-principle --- app/builders/application_builder.rb | 7 ------- app/builders/bulk_subscriber_list_email_builder.rb | 4 +++- app/builders/digest_email_builder.rb | 4 +++- app/builders/immediate_email_builder.rb | 4 +++- app/builders/subscriber_auth_email_builder.rb | 4 +++- app/builders/subscription_auth_email_builder.rb | 4 +++- .../subscription_confirmation_email_builder.rb | 4 +++- app/presenters/application_presenter.rb | 7 ------- app/presenters/bulk_email_body_presenter.rb | 4 +++- app/presenters/content_change_presenter.rb | 4 +++- app/presenters/footer_presenter.rb | 4 +++- app/presenters/message_presenter.rb | 4 +++- app/presenters/source_url_presenter.rb | 4 +++- app/services/application_service.rb | 7 ------- app/services/auth_token_generator_service.rb | 4 +++- app/services/content_change_handler_service.rb | 4 +++- app/services/create_subscriber_list_service.rb | 4 +++- app/services/create_subscription_service.rb | 4 +++- app/services/digest_initiator_service.rb | 4 +++- app/services/immediate_email_generation_service.rb | 4 +++- .../matched_content_change_generation_service.rb | 4 +++- app/services/matched_message_generation_service.rb | 4 +++- app/services/message_handler_service.rb | 4 +++- app/services/send_email_service.rb | 4 +++- app/services/unsubscribe_all_service.rb | 4 +++- lib/callable.rb | 14 ++++++++++++++ 26 files changed, 80 insertions(+), 43 deletions(-) delete mode 100644 app/builders/application_builder.rb delete mode 100644 app/presenters/application_presenter.rb delete mode 100644 app/services/application_service.rb create mode 100644 lib/callable.rb diff --git a/app/builders/application_builder.rb b/app/builders/application_builder.rb deleted file mode 100644 index 6f0bb70b8..000000000 --- a/app/builders/application_builder.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ApplicationBuilder - def self.call(*args, **kwargs) - new(*args, **kwargs).call - end - - private_class_method :new -end diff --git a/app/builders/bulk_subscriber_list_email_builder.rb b/app/builders/bulk_subscriber_list_email_builder.rb index 181da5704..88ece03fd 100644 --- a/app/builders/bulk_subscriber_list_email_builder.rb +++ b/app/builders/bulk_subscriber_list_email_builder.rb @@ -1,4 +1,6 @@ -class BulkSubscriberListEmailBuilder < ApplicationBuilder +class BulkSubscriberListEmailBuilder + include Callable + BATCH_SIZE = 5000 def initialize(subject:, body:, subscriber_lists:) diff --git a/app/builders/digest_email_builder.rb b/app/builders/digest_email_builder.rb index 0868856aa..01fa42838 100644 --- a/app/builders/digest_email_builder.rb +++ b/app/builders/digest_email_builder.rb @@ -1,4 +1,6 @@ -class DigestEmailBuilder < ApplicationBuilder +class DigestEmailBuilder + include Callable + def initialize(content:, subscription:) @content = content @subscription = subscription diff --git a/app/builders/immediate_email_builder.rb b/app/builders/immediate_email_builder.rb index 191a8e797..2929e52d7 100644 --- a/app/builders/immediate_email_builder.rb +++ b/app/builders/immediate_email_builder.rb @@ -1,4 +1,6 @@ -class ImmediateEmailBuilder < ApplicationBuilder +class ImmediateEmailBuilder + include Callable + def initialize(content, subscriptions) @content = content @subscriptions = subscriptions diff --git a/app/builders/subscriber_auth_email_builder.rb b/app/builders/subscriber_auth_email_builder.rb index 7dbdf1f4a..20174a77d 100644 --- a/app/builders/subscriber_auth_email_builder.rb +++ b/app/builders/subscriber_auth_email_builder.rb @@ -1,4 +1,6 @@ -class SubscriberAuthEmailBuilder < ApplicationBuilder +class SubscriberAuthEmailBuilder + include Callable + def initialize(subscriber:, destination:, token:) @subscriber = subscriber @destination = destination diff --git a/app/builders/subscription_auth_email_builder.rb b/app/builders/subscription_auth_email_builder.rb index 7da477972..70f89f7e1 100644 --- a/app/builders/subscription_auth_email_builder.rb +++ b/app/builders/subscription_auth_email_builder.rb @@ -1,4 +1,6 @@ -class SubscriptionAuthEmailBuilder < ApplicationBuilder +class SubscriptionAuthEmailBuilder + include Callable + def initialize(address:, token:, subscriber_list:, frequency:) @address = address @token = token diff --git a/app/builders/subscription_confirmation_email_builder.rb b/app/builders/subscription_confirmation_email_builder.rb index 16729d199..f642bda5d 100644 --- a/app/builders/subscription_confirmation_email_builder.rb +++ b/app/builders/subscription_confirmation_email_builder.rb @@ -1,4 +1,6 @@ -class SubscriptionConfirmationEmailBuilder < ApplicationBuilder +class SubscriptionConfirmationEmailBuilder + include Callable + def initialize(subscription:) @subscription = subscription @subscriber = subscription.subscriber diff --git a/app/presenters/application_presenter.rb b/app/presenters/application_presenter.rb deleted file mode 100644 index ee7dbebef..000000000 --- a/app/presenters/application_presenter.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ApplicationPresenter - def self.call(*args, **kwargs) - new(*args, **kwargs).call - end - - private_class_method :new -end diff --git a/app/presenters/bulk_email_body_presenter.rb b/app/presenters/bulk_email_body_presenter.rb index 893430048..24421c461 100644 --- a/app/presenters/bulk_email_body_presenter.rb +++ b/app/presenters/bulk_email_body_presenter.rb @@ -1,4 +1,6 @@ -class BulkEmailBodyPresenter < ApplicationPresenter +class BulkEmailBodyPresenter + include Callable + def initialize(body, subscriber_list) @body = body @subscriber_list = subscriber_list diff --git a/app/presenters/content_change_presenter.rb b/app/presenters/content_change_presenter.rb index 4711dec85..7e43d2586 100644 --- a/app/presenters/content_change_presenter.rb +++ b/app/presenters/content_change_presenter.rb @@ -1,6 +1,8 @@ require "redcarpet/render_strip" -class ContentChangePresenter < ApplicationPresenter +class ContentChangePresenter + include Callable + EMAIL_DATE_FORMAT = "%l:%M%P, %-d %B %Y".freeze def initialize(content_change, subscription) diff --git a/app/presenters/footer_presenter.rb b/app/presenters/footer_presenter.rb index a851c4551..b5a1f5e9a 100644 --- a/app/presenters/footer_presenter.rb +++ b/app/presenters/footer_presenter.rb @@ -1,4 +1,6 @@ -class FooterPresenter < ApplicationPresenter +class FooterPresenter + include Callable + def initialize(subscriber, subscription) @subscription = subscription @subscriber = subscriber diff --git a/app/presenters/message_presenter.rb b/app/presenters/message_presenter.rb index 64ff76366..becef9555 100644 --- a/app/presenters/message_presenter.rb +++ b/app/presenters/message_presenter.rb @@ -1,4 +1,6 @@ -class MessagePresenter < ApplicationPresenter +class MessagePresenter + include Callable + def initialize(message, _subscription = nil) @message = message end diff --git a/app/presenters/source_url_presenter.rb b/app/presenters/source_url_presenter.rb index 28db5a03b..21429e07d 100644 --- a/app/presenters/source_url_presenter.rb +++ b/app/presenters/source_url_presenter.rb @@ -1,4 +1,6 @@ -class SourceUrlPresenter < ApplicationPresenter +class SourceUrlPresenter + include Callable + def initialize(url, utm_source:, utm_content:) @url = url @utm_source = utm_source diff --git a/app/services/application_service.rb b/app/services/application_service.rb deleted file mode 100644 index 2292345d7..000000000 --- a/app/services/application_service.rb +++ /dev/null @@ -1,7 +0,0 @@ -class ApplicationService - def self.call(*args, **kwargs) - new(*args, **kwargs).call - end - - private_class_method :new -end diff --git a/app/services/auth_token_generator_service.rb b/app/services/auth_token_generator_service.rb index b19d9cae2..7a48f68c5 100644 --- a/app/services/auth_token_generator_service.rb +++ b/app/services/auth_token_generator_service.rb @@ -1,4 +1,6 @@ -class AuthTokenGeneratorService < ApplicationService +class AuthTokenGeneratorService + include Callable + CIPHER = "aes-256-gcm".freeze OPTIONS = { cipher: CIPHER, serializer: JSON }.freeze diff --git a/app/services/content_change_handler_service.rb b/app/services/content_change_handler_service.rb index d806e24d0..e2a868c2f 100644 --- a/app/services/content_change_handler_service.rb +++ b/app/services/content_change_handler_service.rb @@ -1,4 +1,6 @@ -class ContentChangeHandlerService < ApplicationService +class ContentChangeHandlerService + include Callable + def initialize(params:, govuk_request_id:, user: nil) @params = params @govuk_request_id = govuk_request_id diff --git a/app/services/create_subscriber_list_service.rb b/app/services/create_subscriber_list_service.rb index 90eed604c..810ff510f 100644 --- a/app/services/create_subscriber_list_service.rb +++ b/app/services/create_subscriber_list_service.rb @@ -1,4 +1,6 @@ -class CreateSubscriberListService < ApplicationService +class CreateSubscriberListService + include Callable + def initialize(title:, url:, matching_criteria:, user:) @title = title @url = url diff --git a/app/services/create_subscription_service.rb b/app/services/create_subscription_service.rb index 4ae79eed5..5f2169f3e 100644 --- a/app/services/create_subscription_service.rb +++ b/app/services/create_subscription_service.rb @@ -1,4 +1,6 @@ -class CreateSubscriptionService < ApplicationService +class CreateSubscriptionService + include Callable + attr_reader :subscriber_list, :subscriber, :frequency, :current_user def initialize(subscriber_list, subscriber, frequency, current_user) diff --git a/app/services/digest_initiator_service.rb b/app/services/digest_initiator_service.rb index 8a646082b..982f94b0f 100644 --- a/app/services/digest_initiator_service.rb +++ b/app/services/digest_initiator_service.rb @@ -1,4 +1,6 @@ -class DigestInitiatorService < ApplicationService +class DigestInitiatorService + include Callable + def initialize(date:, range:) @range = range @date = date diff --git a/app/services/immediate_email_generation_service.rb b/app/services/immediate_email_generation_service.rb index 3ebcd6822..d1b061fd6 100644 --- a/app/services/immediate_email_generation_service.rb +++ b/app/services/immediate_email_generation_service.rb @@ -1,4 +1,6 @@ -class ImmediateEmailGenerationService < ApplicationService +class ImmediateEmailGenerationService + include Callable + BATCH_SIZE = 5000 def initialize(content, **) diff --git a/app/services/matched_content_change_generation_service.rb b/app/services/matched_content_change_generation_service.rb index 8524803b3..72e622a3e 100644 --- a/app/services/matched_content_change_generation_service.rb +++ b/app/services/matched_content_change_generation_service.rb @@ -1,4 +1,6 @@ -class MatchedContentChangeGenerationService < ApplicationService +class MatchedContentChangeGenerationService + include Callable + def initialize(content_change, **) @content_change = content_change end diff --git a/app/services/matched_message_generation_service.rb b/app/services/matched_message_generation_service.rb index 063368816..767972ea7 100644 --- a/app/services/matched_message_generation_service.rb +++ b/app/services/matched_message_generation_service.rb @@ -1,4 +1,6 @@ -class MatchedMessageGenerationService < ApplicationService +class MatchedMessageGenerationService + include Callable + def initialize(message, **) @message = message end diff --git a/app/services/message_handler_service.rb b/app/services/message_handler_service.rb index 34053e871..1b47f1ce8 100644 --- a/app/services/message_handler_service.rb +++ b/app/services/message_handler_service.rb @@ -1,4 +1,6 @@ -class MessageHandlerService < ApplicationService +class MessageHandlerService + include Callable + def initialize(params:, govuk_request_id:, user: nil) @params = params @govuk_request_id = govuk_request_id diff --git a/app/services/send_email_service.rb b/app/services/send_email_service.rb index 5528deb3a..75e9ddd82 100644 --- a/app/services/send_email_service.rb +++ b/app/services/send_email_service.rb @@ -1,4 +1,6 @@ -class SendEmailService < ApplicationService +class SendEmailService + include Callable + class NotifyCommunicationFailure < RuntimeError; end def initialize(email:, metrics: {}) diff --git a/app/services/unsubscribe_all_service.rb b/app/services/unsubscribe_all_service.rb index 2306422d2..7d9dcf8f6 100644 --- a/app/services/unsubscribe_all_service.rb +++ b/app/services/unsubscribe_all_service.rb @@ -1,4 +1,6 @@ -class UnsubscribeAllService < ApplicationService +class UnsubscribeAllService + include Callable + attr_reader :subscriber, :reason def initialize(subscriber, reason, **) diff --git a/lib/callable.rb b/lib/callable.rb new file mode 100644 index 000000000..3dbf45812 --- /dev/null +++ b/lib/callable.rb @@ -0,0 +1,14 @@ +# This mixin is to provide the boilerplate for classes that implement a single +# public class method of `.call` - typically used by objects that are entirely +# to perform a business transaction. +module Callable + extend ActiveSupport::Concern + + included do + def self.call(...) + new(...).call + end + + private_class_method :new + end +end