Skip to content

Try to make send_notifications more reliable#12160

Merged
oliverguenther merged 5 commits intodevfrom
refactor/try_to_make_send_notifications_more_reliable
Feb 28, 2023
Merged

Try to make send_notifications more reliable#12160
oliverguenther merged 5 commits intodevfrom
refactor/try_to_make_send_notifications_more_reliable

Conversation

@cbliard
Copy link
Member

@cbliard cbliard commented Feb 24, 2023

It is a follow-up of #12147 where some comments where made about how the send_notifications was used and forwarded to services.

This makes both `#active?` / `#active=` methods and `#already_set?` / `#already_set=` methods symmetrical. Before it was confusing to have `#active` return a Concurrent::ThreadLocalVar instance while `#active=` should be a boolean (asymmetrical value types).
@cbliard
Copy link
Member Author

cbliard commented Feb 27, 2023

I think this addresses the comments I had in #12147.

Things that could be considered:

  • calling Journal::NotificationConfiguration.with(send_notifications, &) is currently done in Shared::ServiceContext, and is called from BaseServices::BaseContracted#perform by extracting the :send_notifications value from the params hash.
    This could be replaced by a module included with a around_call that would extract the :send_notifications value from params and call Journal::NotificationConfiguration.with(send_notifications, &) directly. This would remove this responsibility from Shared::ServiceContext
  • Journal::NotificationConfiguration could be renamed, as it is also used in non-journal places too.
  • in config/initializers/subscribe_listeners.rb, there is guard clause next unless payload[:send_notifications] in some of the subscriptions. If all notifiers were consistently using Journal::NotificationConfiguration, then the guard clause could be replaced with next unless Journal::NotificationConfiguration.active? (or next if Journal::NotificationConfiguration.inactive? for a more positive world). There is the question of the notification message being present in the payload. Should it then be carried over in
    Journal::NotificationConfiguration?
  • Modify the shared examples 'BaseServices create service' and 'BaseServices update service' to call the service with send_notifications: false and ensure no notification job has been created.

But I'm afraid of going down a rabbit hole if I try to do any of the above propositions.

@cbliard cbliard marked this pull request as ready for review February 27, 2023 15:39
When calling a service with `send_notifications: false`, the
`Journal::NotificationConfiguration.active?` will be set to `false` and
subsequent calls to set it to `true` will have no effect and log a
warning.

For this reason, it's better to use `nil` as default value for
`send_notifications` so that
`Journal::NotificationConfiguration.active?` is changed only when the
value is explicitly `true` or `false`, and ignored when the value is
`nil`.
@cbliard cbliard force-pushed the refactor/try_to_make_send_notifications_more_reliable branch from d7aa5aa to a06e519 Compare February 27, 2023 16:47
Copy link
Member

@oliverguenther oliverguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a lot better already. Thanks 👍

@oliverguenther oliverguenther merged commit d7c0031 into dev Feb 28, 2023
@oliverguenther oliverguenther deleted the refactor/try_to_make_send_notifications_more_reliable branch February 28, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants