When rendering notifications, the notification locale becomes available in the renderer. This makes sense.
However, rendering grouped notifications still passes only one notification record (the last item of the array) to the renderer.

As discussed here, "delivery method" might be a better, more generic name than "pusher".

One possible naming scheme...

• Rename gem (and its top-level namespace) to notification-delivery (NotificationDelivery)??

  • Or leave it as notification-pusher (NotificationPusher)?

• Add a namespace under that for delivery methods to be under: NotificationDelivery::DeliveryMethod

  • Add an abstract base class that defines the interface (basically just an empty call that raises a NotImplementedError).NotificationDelivery::DeliveryMethod::Base?
  • Move concrete delivery methods under this namespace: NotificationDelivery::DeliveryMethod::ActionMailer, etc.

• Rename NotificationPusher::Pusher to NotificationDelivery::DeliveryMethodConfiguration (mentioned here)

• Rename define_pusher to define_delivery_method or register_delivery_method (since the definition really happens in the class itself)

• Rename push method to deliver

I'm certainly open to ideas and discussion!

Is Rails 5.1 still supported with the latest release of this gem (1.2.6)? The current gemspec file specifies rails 5.2.

It seemed surprising to me that NotificationPusher::ActionMailer and the other pushers were trying to push directly from the initialize method.

Seems like it might make it a little easier to test and debug if one were able to instantiate a pusher object without it performing any action, and moving the actual action to another method like call or perform (2 generic methods I've seen before for implementing generic services) ... or push if you want to be more specific.

I would recommend using the generic callable interface, call. Then you can invoke it with the even more generic .(*args), the same way you can invoke a Proc...

SomePusher.new(notification, options).()

Describe the bug
The notification.create is not working for new users and I don't know why

I have two users

User 1:

 #<User id: 1, email: "[email protected]", name: "User Name 0", wallet_hash: nil, created_at: "2018-12-21 11:38:40",updated_at: "2018-12-21 11:38:40", roles: [:user]>
2.5.0 :002 > User.second.attributes
  User Load (0.8ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1 OFFSET $2  [["LIMIT", 1], ["OFFSET", 1]]
 => {"id"=>2, "email"=>"[email protected]", "encrypted_password"=>"$2a$11$bGAWm0orRPJtAwGJCJd3Z.Lqiwp3h/Xnup2CrIuQrCwJlWkRmKMTa", "reset_password_token"=>nil, "reset_password_sent_at"=>nil, "name"=>"User Name 1", "wallet_hash"=>nil, "remember_created_at"=>nil, "sign_in_count"=>15, "current_sign_in_at"=>Wed, 16 Jan 2019 13:55:00 UTC +00:00, "last_sign_in_at"=>Wed, 16 Jan 2019 13:30:27 UTC +00:00, "current_sign_in_ip"=>#<IPAddr: IPv4:127.0.0.1/255.255.255.255>, "last_sign_in_ip"=>#<IPAddr: IPv4:127.0.0.1/255.255.255.255>, "created_at"=>Fri, 21 Dec 2018 11:38:40 UTC +00:00, "updated_at"=>Wed, 16 Jan 2019 13:55:00 UTC +00:00, "invitation_token"=>nil, "invitation_created_at"=>nil, "invitation_sent_at"=>nil, "invitation_accepted_at"=>nil, "invitation_limit"=>nil, "invited_by_type"=>nil, "invited_by_id"=>nil, "invitations_count"=>0, "roles"=>[:user], "confirmation_token"=>nil, "confirmed_at"=>Fri, 21 Dec 2018 11:38:40 UTC +00:00, "confirmation_sent_at"=>nil}

and User 2

#<User id: 23, email: "[email protected]", name: "User 8", wallet_hash: nil, created_at: "2019-01-16 13:28:46", updated_at: "2019-01-16 13:30:13", roles: [:user]>
2.5.0 :004 > User.last.attributes
  User Load (1.1ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT $1  [["LIMIT", 1]]
 => {"id"=>23, "email"=>"[email protected]", "encrypted_password"=>"$2a$11$fdLnWLunn9.XY6f.mxr6A.oq3x7eQxBW49eAn0zUOnkZiHfJHIz/u", "reset_password_token"=>nil, "reset_password_sent_at"=>nil, "name"=>"User 8", "wallet_hash"=>nil, "remember_created_at"=>nil, "sign_in_count"=>1, "current_sign_in_at"=>Wed, 16 Jan 2019 13:30:13 UTC +00:00, "last_sign_in_at"=>Wed, 16 Jan 2019 13:30:13 UTC +00:00, "current_sign_in_ip"=>#<IPAddr: IPv4:127.0.0.1/255.255.255.255>, "last_sign_in_ip"=>#<IPAddr: IPv4:127.0.0.1/255.255.255.255>, "created_at"=>Wed, 16 Jan 2019 13:28:46 UTC +00:00, "updated_at"=>Wed, 16 Jan 2019 13:30:13 UTC +00:00, "invitation_token"=>nil, "invitation_created_at"=>nil, "invitation_sent_at"=>nil, "invitation_accepted_at"=>nil, "invitation_limit"=>nil, "invited_by_type"=>nil, "invited_by_id"=>nil, "invitations_count"=>0, "roles"=>[:user], "confirmation_token"=>"sS-69N43utcZixsYWtwE", "confirmed_at"=>Wed, 16 Jan 2019 13:30:01 UTC +00:00, "confirmation_sent_at"=>Wed, 16 Jan 2019 13:28:46 UTC +00:00}

When I try creating notification with the first user it works

Notification.create!(target: User.find(2), object: Certificate.first, type: 'claim-share')
#<Notification id: 48, target_type: "User", target_id: 2, object_type: "Certificate", object_id: 10, read: false,metadata: {}, created_at: "2019-01-16 15:48:06", updated_at: "2019-01-16 15:48:06", type: "claim-share", subscription_id: nil, category: nil>

But if I try to create notification with the second user it gives an error

Notification.create!(target: User.find(23), object: Certificate.first, type: 'claim-share')
ActiveRecord::RecordNotSaved (Failed to save the record):

The difference between the two users is that the first user is a seed user and was created a long time back and the second user is a new user and was created using an invite link that is why i have pasted the attributes of both the users.

What is the problem here?

When I use the render_notifications helper it renders the container div but not the notifications themselves.

This is the HAML:

.notifications
  %i.fa.fa-bell{class: ("error" if @unread_notifications&.any?)}
  .notification-dropdown.Dropdown__menu
    %ul.Dropdown__menu.Organisation__menu
      = render_notifications @unread_notifications if current_user && @unread_notifications

This is what's in app/assets/views/notifications/push/_index.html.erb:

%li.notification-container
  %h3= notification.metadata[:title]
  %p= notification.metadata[:content]

This is the output:

<div class="notifications">
  <i class="error fa fa-bell"></i>
  
  <div class="notification-dropdown Dropdown__menu" style="display: none;">
    <ul class="Dropdown__menu Organisation__menu" style="display: none;">
      <div class="notification-renderer notifications"></div>
    </ul>
  </div>
</div>

I'm on macOS High Sierra and using Chrome Version 67.0.3396.99.

I'm also certain that @unread_notifications contains some notifications.

• general
• handler
• pusher
• pusher-actionmailer
• pusher-onesignal
• renderer
• settings

I'm not sure what these two additional parameters are intended for, but they should have default values like renderer, or renderer should be placed at last position in the params list. Failing to do so makes render_notifications helper method unusable.

Also these two parameters are not mentioned in README documentation.

Reliably check if constant exists

Something I noticed when I was stepping through this code a debugger...

[11, 20] in notifications-rails/notification-pusher/lib/notification_pusher/pusher.rb
   11:     end
   12: 
   13:     def push(notification, options = {})
   14:       options = @options.merge!(options)
   15: 
=> 16:       return unless defined?(NotificationPusher.const_get(@name))
   17:       instance = NotificationPusher.const_get(@name).new(notification, options)
   18:       @instances << instance
   19:     end
   20: 
(byebug) defined?(NotificationPusher.const_get(@name))
"method"                  

What was the defined? check for? I assume it's supposed to check if the constant exists under NotificationPusher.

The fact that defined? returned "method" made me think it's probably actually doing what it was supposed to be doing. Looks like it doesn't actually perform the method call first, so I assume it's the same as doing defined?(NotificationPusher.const_get) ... which should always be true, I think.

main > defined?(ActiveRecord.const_get)
=> "method"

Can we just use const_defined? for that?

main > ActiveRecord.const_defined?(:Base)
=> true

main > ActiveRecord.const_defined?(:Rase)
=> false

Warning: Doesn't trigger autoloading

I ran into this when I was trying to write my own ActionMailer pusher (#53)...

Unless NotificationPusher::ActionMailer is already defined when you call const_get, it looks like it actually falls back to getting the constant from higher levels. So, until I worked around it by adding:

require 'notification_pusher/action_mailer

NotificationPusher.const_get(@name) was actually returning top-level ::ActionMailer and causing errors when it tried to use it.

I wonder if there's a way we could safely trigger the const to be autoloaded there? Or try to reference it (to trigger const_missing auto-loading; see example below) but rescue in case it couldn't find it. We could look at what other libraries do; I'm not sure what's typical for that...

Or maybe it's safer to just require it to be required like I ended up doing and not try to do anything too automagical. (I know Ruby's autoload is going away in Ruby 3. Not sure about Rails's autoloading...)

Anyway, this is what I observed in the debugger session: When I actually referenced NotificationPusher::ActionMailer, Rails's const_missing magic caused the constant to materialize and the subsequent const_get to get the correct class...

(byebug) NotificationPusher.const_get(@name)
ActionMailer
(byebug) NotificationPusher::ActionMailer
NotificationPusher::ActionMailer
(byebug) NotificationPusher.const_get(@name)
NotificationPusher::ActionMailer

render_notifications is rendering an empty div. render_notification will generate a notification.
Both of them however, are giving me an error if I pass a renderer:
=render_notification notification, 'feed'
I get:
wrong number of arguments (given 2, expected 1)

Rails 5.2
notification-renderer 1.2.6

Thanks!

Describe the solution you'd like

I'd like to change, or have the option to change, the settings, category_settings columns from text type to jsonb.

JSON-serialized data seems to be a better and more standard option than YAML-based serialization. Using a JSONB data type in the database makes it possible to efficiently search, validate, and manipulate the data in those columns at the database level.

For more about using jsonb types in Rails, see:
https://guides.rubyonrails.org/active_record_postgresql.html#json-and-jsonb

Implementation required / options

Change data type

This is something the end user can do if they want — just edit the generated migration to use:

t.jsonb :settings
t.jsonb :category_settings

instead of

t.text :settings
t.text :category_settings

like they do here: https://github.com/paper-trail-gem/paper_trail#postgresql-json-column-type-support

Change/disable the serializer

Currently, we have this in notification-settings/app/models/notification_settings/setting.rb:

    serialize :settings, Hash
    serialize :category_settings, Hash

Unfortunately that doesn't seem to be compatible with jsonb columns. When a class name (Hash) is passed to serialize, it serializes it as a YAML document (see serialize docs).

We need to a way to either have it not call serialize at all... or a way to specify which "coder" to use ... or have it check the column's data type and automatically do the right thing

Since it's just as easy, and (something like this) might even be necessary (in order to allow the hash to still be accessible with symbol keys like the code currently does), I'd recommend just allowing a different
serializer to be specified in the configuration.

This is how I have my app configured currently and is working so far:
(based on HashSerializer example from https://nandovieira.com/using-postgresql-and-jsonb-with-ruby-on-rails)

class JsonColumnHashWithIndifferentAccessSerializer
  def self.dump(hash)
    hash
  end

  def self.load(hash)
    (hash || {}).with_indifferent_access
  end
end

module NotificationSettings
        Setting
  class Setting
    serialize :settings,          JsonColumnHashWithIndifferentAccessSerializer
    serialize :category_settings, JsonColumnHashWithIndifferentAccessSerializer
  end
end

(Note: something like this may also solve #38)

So far, this didn't even require any changes to the gem, since calling serialize :settings again appears to override any previous serialization configuration you may have. But I'd prefer it if the gem officially supported jsonb and provided something like a config.hash_serializer option so end user doesn't have to monkey-patch the gem. (Better yet, configure itself automatically as mentioned below.)

Challenge: Remaining compatible with databases other than Postgres

I assume it wouldn't be an acceptable solution to require use of PostgreSQL... so the question is, can we take advantage of jsonb columns if the schema uses them but still make it work for databases that don't have them? (MySQL actually has a JSON data type too but I don't know much about it.)

I think we can. The changes suggested above don't break compatibility with the old way; they just allow users to use the much nicer jsonb type if they want to (I sure do).

Can we automatically detect data type and do the right thing?

Probably, but not sure how yet...

Then we wouldn't have to add any new config options. (And it could detect it on a per-column basis so folks could use jsonb for whichever hash columns they want; wouldn't force you to use it for all of them. There's also serialize :metadata, Hash in notification-handler.)

This is what PaperTrail does. Unfortunately, I don't think it's safe to check columns_hash in the model's class definition where we're calling serialize. PaperTrail gets away with it because it handles the deserialization itself when the object/object_changes attribute readers are called (object_deserialized method, actually) and doesn't call serialize in the class definition like we're doing. Hmm.

• rspec
• rubocop

Is a target record required to create a Notification?

Apparently it is because I got this error when I tried to create one without a target:

     NoMethodError:
       undefined method `notification_setting' for nil:NilClass
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:32:in `can_create?'
     # in `can_create?'

• before_create :validate_create calls
• can_create? calls
• target.notification_setting.present?

Should we require a target more directly and explicitly?

Like add a validates :target, presence: true?

It doesn't look like there are any validations on models yet... I'm curious why not.

I guess users would have to actually check notification.errors (including the one returned from user.notify) if they want to know why it didn't get created.

Currently the other reason a notification might not get created is that before_create :validate_create throws :abort. Is there any way to check why it wasn't able to be created in that case, in case I wanted to log or report whether the notification was sent out and why not (was it because the recipient's status was in a do_not_notify_status? was it because they have this pusher turned off for this category? was it because they have this pusher turned off in general? etc.)?

Would it be worth considering just adding validation errors instead of using a before_create? Then a user could simply check notification.errors to learn the reason it couldn't be created...

Is your feature request related to a problem? Please describe.

Currently settings_allow_create? (and its sibling category_settings_allow_create?) return false if the notification_setting has not been explicitly enabled:

      def settings_allow_create?
        target.notification_setting.settings.dig(:enabled)
      end

... which means Notification#can_create? will return false if a NotificationSettings::Setting exists for the user but doesn't have a settings[:enabled] key, as can be seen in this example that uses logic similar to can_create?:

main > user = User.last
main > setting = user.notification_setting
=> #<NotificationSettings::Setting:0x0000557cb9086fc0
 id: nil,
 object_type: "User",
 ...
main > setting.settings.dig(:enabled)   
=> nil
main > false unless setting.settings.dig(:enabled)   
=> false

Describe the solution you'd like

I'd like notifications to be considered enabled for a user unless specifically turned off.

This would also make it more consistent with the behavior for when target.notification_setting is not even present (is nil):

      def can_create?
        if target.notification_setting.present?
          ...
        true
      end

If the implicit value for can_create? is true if there isn't any setting record, then it seems the implicit value should also be true if there is a setting record but no settings in said setting record.

Possible implementation

Change settings_allow_create? (and its sibling category_settings_allow_create?) to specifically check for nil and return true instead of nil:

      def settings_allow_create?
        enabled = target.notification_setting.settings.dig(:enabled)
        enabled.nil? ? true : enabled
      end

or simply:

      def settings_allow_create?
        target.notification_setting.settings.fetch(:enabled) { true }
      end

Describe alternatives you've considered

If some applications actually need notifications to be disabled by default, perhaps the default should be a configuration option.

NotificationSettings.configure do |config|
  config.enabled_default = true

  # If certain categories of notifications should be enabled/disabled by default..
  config.enabled_default_by_category[:notifications] = true
  config.enabled_default_by_category[:mentions] = false
end

I noticed that even if can_create? returns false, Notification.create still returns a Notification object — it's just not persisted.

But what's stopping the push from happening if notifications are completely disabled for this target user ({enabled: false} in their settings record) if you do something like this?:

notification = Notification.create(target: User.first, object: Recipe.first)
notification.push(:CustomPusher, option_one: 'value_two')

I haven't tried it to be certain, but if I'm reading the code correctly, this would still push (deliver) the notification — even though it couldn't be "created".

In other words, it doesn't appear that being "created" (saved/persisted) is a requisite state for a push to be allowed — the object need only be initialized.

Should there be an additional check for can_create? inside of can_push?? Or is it up to the end user to handle this, something like:

notification = Notification.create(target: User.first, object: Recipe.first)
if notification.can_create?
  notification.push(:CustomPusher, option_one: 'value_two')
end

... except that can_create? is currently marked private so you can't do that. Should we make it public, to allow for such checks as this?

Hello and thank you for maintaining this gem 👋
Trying to create and push a notification following your instructions but getting an error NoMethodError (private method initialize_pusher' called for #Notification:0x00007fb23539a578)`

Expected behavior

it should not return an error and send a notification to the user

Actual behavior

NoMethodError (private method 'initialize_pusher' called for #<Notification:0x00007fb23539a578>)

Steps to reproduce

Pusher definition:

NotificationPusher.configure do |config|
    config.define_pusher :ActionMailer, email: '[email protected]'
end

irb(main):001:0> notification = Notification.create target: User.first, object: Recipe.first
=> #<Notification id: 3, target_type: "User", target_id: 4, object_type: "Recipe", object_id: 316669, read: false, metadata: {}, created_at: "2018-04-09 14:51:53", updated_at: "2018-04-09 14:51:53", type: nil, subscription_id: nil, category: nil>
irb(main):002:0> notification.push :ActionMailer
Traceback (most recent call last):
        1: from (irb):2
NoMethodError (private method 'initialize_pusher' called for #<Notification:0x00007fb23539a578>)

System configuration

Ruby version:
ruby "2.5.0"

Rails version:
gem "rails", "5.1.4"

I have an app that already has a bunch of different mailers that are used to send different types of notifications. So far, email notifications are the only kind it has, but I want to (1) add the ability to unsubscribe (opt out) from different notifications/categories (which notification-settings may help with) and (2) start adding a framework that will allow sending notifications via different methods eventually.

I looked briefly at using notification-pusher-actionmailer but decided I don't really like how it would make me restructure/throw away all my mailers/templates if I wanted to use it.

At first glance anyway, the provided ActionMailer pusher seems like an unnecessary additional abstraction that actually prevents you from accessing most of the ActionMailer API. It only gives you a subset of functionality—a "least common denominator": the ability to supply a template, and specify to, from, and layout.

But ActionMailer already provides excellent abstractions and DSL for specifying those and so many other things. Why not just use it directly? A couple of things that you can do through the normal mailer API that you apparently can't do with a notification sent through notification-pusher-actionmailer include:

• specify multiple to recipients
• specify cc/bcc recipients
• multipart emails
• attachments
• group related messages together in different mailers (each of which may have different defaults (from:, etc.))

notification-pusher-actionmailer also depends on notification-renderer ... which might have some advantages (like using the same system for emails that you use for rendering notifications on a web page), but I haven't needed/added that dependency yet, and it seems like an unnecessarily different (and less flexible) way of doing things for those used to just using ActionMailer::Base directly... and it would require a fair bit of work to migrate...

If I wanted to migrate from my existing mailers to use the provided ActionMailer pusher, it looks like these are some of the changes I'd have to make:

• create a new notification type for each mailer action that I wanted to use as a notification
• remove the mailer action from the mailer class it's in now; potentially remove the mailers too if they become empty
• move and rename the template to _actionmailer.html.erb under the
• pass data to the template via notification.metadata instead of via instance variables in the mailer action

I think that approach makes a lot of sense for really short notifications that could be sent via any pusher. Some delivery methods (like SMS and push notifications) really only work with short messages, after all. And maybe I'll come back around to that option once I actually add notification methods other than email...

But if you already have long, "rich" HTML mails that you use for notifications, I wonder if it's worth switching everything to notification-renderer right off the bat.

Maybe I'm abusing the term "notification" and trying to us this system for something it wasn't designed for...?

In any case, I started exploring the option of creating an alternative ActionMailer pusher that actually lets you specify an existing mailer/action to use "in situ".

I figure it should be possible to create a thin adapter between the Pusher API and existing mailers, and provide a way to sort of "point" or "link" a notification to an existing mailer action that can be used to deliver it.

I guess there's nothing stopping me from writing my own pusher that does this. Just wondering if anyone else had this need, and what the best approach would be...

Questions:

How to send a single notification that gets sent to multiple people via a single email message?

A few of my existing mailer actions currently send a single email to multiple recipients (so that all the recipients can see each other's names in the To: line of the email). Is there any way to make that work?

It seems like the current Notification structure limits you to having only one target record per notification.
(See #52.)

What to call this new pusher?

I'd kind of like to just call it ActionMailer like the one included with notifications-rails ... esp. since the pusher name is used as a key in the settings hash. Maybe I should avoid using the same name as the provided mail pusher. But if I were to use some custom-sounding name like ExistingMailerAdapter, that would make a poor key for the settings hash and might prevent me from swapping/switching to a different mail pusher later (#51) (without doing a data migration).

Potential advantages:

• easier to integrate with an existing "legacy" system
• less of a barrier to entry for potential users considering using this project
• could use as an intermediate step before fully transitioning to a more pusher-agnostic approach
• allows richer, multi-part, etc. emails

Is your feature request related to a problem? Please describe.

The Readme has this example:

s = User.first.notification_setting
s.settings[:enabled] = false

But when I tried this example after adding notification-settings to an existing project, I got this error instead:

main > s = User.first.notification_setting
=> nil

main > s.settings[:enabled] = false
NoMethodError: undefined method `settings' for nil:NilClass

Describe the solution you'd like

It would be nice if a settings target/object could safely assume that it always has an associated notification_setting.

It looks like this callback was added in order to help ensure that a notification_setting always exists for new users:

before_create :create_notification_setting

... but that doesn't help in the case of pre-existing user records.

I propose (instead of that) overriding notification_setting such that it builds a new setting if one doesn't already exist:

      def notification_setting
        super || build_notification_setting
      end

I've tried it locally and it works fine...

Describe alternatives you've considered

Update the migration generator to create settings records for any existing [user] records.

Describe the bug

As soon as I added notification-pusher to my project (which previously only had notification-handler, notification-settings), the code where I was creating a Notification record started having this error:

     ArgumentError:
       wrong number of arguments (given 0, expected 1..2)
     # notifications-rails/notification-pusher/lib/notification_pusher/notification_library.rb:20:in `push'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:71:in `can_push?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:59:in `initialize_pusher'
     # notifications-rails/notification-handler/lib/notification_handler/target.rb:22:in `notify'

Looking at the code, I see that notification-pusher adds this callback to Notification:

after_create_commit :initialize_pusher

which calls the initialize_pusher from notification-settings/lib/notification_settings/notification_library.rb:

      def initialize_pusher
        return unless can_push?
        super
      end

which (before calling super to pass control on to the "main" initialize_pusher in notification-pusher/lib/notification_pusher/notification_library.rb) checks:

      def can_push?
        if target.notification_setting.present?
          return false unless status_allows_push?

          if push.is_a?(Array)
            return false unless can_use_pushers?(push)
          else
            return false unless can_use_pusher?(push)
          end
        end
        true
      end

The only place Notification#push is defined is in notification-pusher/lib/notification_pusher/notification_library.rb:

      def push(name, options = {})
        self.pusher = name
        self.pusher_options = options
        initialize_pusher
      end

That obviously isn't what it was intended to call, esp. since that would in turn call initialize_pusher again and cause an infinite loop.

I assume what was intended was to refer to pusher rather than push. pusher appears to also be defined in notification-pusher/lib/notification_pusher/notification_library.rb:

attr_accessor :pusher

Next error: undefined method to_sym' for nil:NilClass`

After I made the changes mentioned above, I ran into another error...

     NoMethodError:
       undefined method `to_sym' for nil:NilClass
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:107:in `local_settings_allow_push?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:103:in `local_pusher_settings?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:98:in `settings_allow_push?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:86:in `can_use_pusher?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:72:in `can_push?'
     # notifications-rails/notification-settings/lib/notification_settings/notification_library.rb:60:in `initialize_pusher'

from here:

      def local_settings_allow_push?(pusher)
        target.notification_setting.settings.dig(pusher.to_sym)
      end

which was called from here:

      def can_push?
        if target.notification_setting.present?
          return false unless status_allows_push?

          if pusher.is_a?(Array)
            return false unless can_use_pushers?(pusher)
          else
            return false unless can_use_pusher?(pusher)
          end
        end
        true
      end

But of course pusher was nil. Aren't we allowed to create a notification without passing a pusher? The official examples suggest that this should be allowed:

notification = Notification.create(target: User.first, object: Recipe.first)
notification.push(:CustomPusher, option_one: 'value_two')

Next issue: pushers disabled by default?

I found the suggestion in #4 to try passing push: :ActionMailer. I wasn't sure how push: would work since push isn't an attribute (how was that ever working?), so I tried pusher: :ActionMailer instead:

notification = user.notify(object: self, category: 'weekly_report_mailer.individual_contribution_link', pusher: :ActionMailer) 

... which resolved the error, but didn't actually call the pusher, because global_settings_allow_push? returned nil:

[105, 114] in notifications-rails/notification-settings/lib/notification_settings/notification_library.rb
   105:       def local_settings_allow_push?(pusher)
   106:         target.notification_setting.settings.dig(pusher.to_sym)
   107:       end
   108: 
   109:       def global_settings_allow_push?
=> 110:         target.notification_setting.settings.dig(:index)
   111:       end

Wat? Where is it documented that there needs to be a truthy :index setting in order for pushes to be considered enabled? https://github.com/jonhue/notifications-rails/tree/master/notification-settings talks about setting s.settings[:enabled] = false to disable notifications ("globally"? for a user), but says nothing about :index. What's the difference? What does :index even mean or refer to? (I see "index" mentioned in https://github.com/jonhue/notifications-rails/tree/master/notification-renderer, but I'm not using notification-renderer and it doesn't mention anything about an :index setting.)

Should this be called something else? :push_enabled (to be different from :enabled setting used by settings_allow_create?), perhaps?

I thought things were enabled by default? Should we make another change like #37 (which made settings_allow_create? true by default) to make global_settings_allow_push? true by default?

Or are we really expected to go through all our users and set {index: true} in each of their settings? (Or am I missing something?)

Conclusion

Is anyone actually using this gem?

I'm frustrated by how much this project overpromises, and how much I've had to fight just to get these gems working at all.

The examples in each gem are nice but they're too isolated. They don't really give the big picture of how you're expected to use several of the notifications-rails gems together. A full example app that demonstrates how these gems could really be put to use would be nice to see...

Some automated tests would be really helpful in inspiring confidence that things actually work in intended ways, and continue to stay working (#7)...

Additional context

Using latest version from git.

Describe the bug
I am trying to subscribe a user to an appointment, but I get the following error:

undefined method 'create!' for nil:NilClass

I've tracked the underlying problem which is the following:

1. The Subscriber model (User) in my case when calling current_user.subscribe(appointment) creates a NotificationSettings::Subscription object though the notification_subscriptions relationship.
2. The NotificationSettings::Subscription then has a after_create_commit :create_notification_setting callback which calls. notification_setting.create! which caused the error because notification_setting doesn't exist for this particular subscription yet.

To Reproduce
Steps to reproduce the behavior:

1. Call subscribe on a notification target and pass a notification object as parameter.

Expected behavior
It creates a subscription between the target and the object.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: macOS Mojave
• Browser Firefox
• Version 66.0.1

Additional context
The user was an existing user in the system, before I installed the notifications-rails gem

... in order for dynamic scopes like Notification.notification_category to work.

undefined method default_category' for #NotificationRenderer::Configuration:0x00007fdac6ed3930`

Occurs when attempting to use the render_notifications method. Seems to just be a case of an incorrect method call. default_category is a method for NotificationSettings.configuration but not for NotificationRenderer.configuration.

Even though its gemspec has notification-handler as a dependency, it's not clear from reading the notification-settings documentation alone how this gem depends on / interrelates with other gems.

So if you follow the installation directions as-is:

Now run the generator:
$ rails g notification_settings:install

you end up running into this:

== ... NotificationSettingsMigration: migrating ====================
-- add_column(:notifications, :subscription_id, :bigint, {:index=>true})
rake aborted!
StandardError: An error has occurred, this and all later migrations canceled:

PG::UndefinedTable: ERROR:  relation "notifications" does not exist
: ALTER TABLE "notifications" ADD "subscription_id" bigint

Make this version compatible with Rails >= 5

This means refactoring notifications-rails/notification-settings/lib/notification_settings/notification_library.rb to expect notification_setting.category_settings[x] to resolve to something other than nil.

Follow up to #35

Does the existing NotificationPusher::ActionMailer even work?

I noticed that in notification-pusher/notification-pusher-actionmailer/lib/notification_pusher/action_mailer.rb

    def initialize(notification, options = {})
      ::NotificationPusher::ActionMailer::NotificationMailer.push(
        notification, options
      )
    end

, it calls the mailer action (NotificationMailer.push) to create the message but it never calls deliver (or deliver_now or deliver_later) on it... Maybe I'm just missing something, but what actually triggers the message to get delivered?

Currently, when you define a pusher, you give it a single name, which is used both as a key (in the settings hash, for example) and to look up a constant under NotificationPusher to get the class that implements/provides that pusher:

NotificationPusher.configure do |config|
  config.define_pusher name, options
end

I think it would be helpful to make a separation between the key/name used to identify the pusher and the class that provides that ability.

This would let you use a more generic name like :email as they key. This would in turn make things more swappable, so you could later change to a different pusher that provides pushing to "email" and things would keep working.

Currently I believe that would not be the case because it looks up / stores the actual class name, like ActionMailer, in the settings hash. But really, a user only cares whether notifications are enabled or disabled for "email" or "SMS" or whatever generic delivery method. The specific implementation used for that delivery type shouldn't be stored in that hash.

As an inspiration/example for this, consider Rails's Attributes API:

# config/initializers/types.rb
ActiveRecord::Type.register(:money, MoneyType)

Describe the bug
When I call scheduled_event.notify_subscribers

I get the following error:

ActiveRecord::HasManyThroughAssociationPolymorphicSourceError (Cannot have a has_many :through association 'ScheduledEvent#notification_subscribers' on the polymorphic object 'Subscriber#subscriber' without 'source_type'. Try adding 'source_type: "NotificationSubscriber"' to 'has_many :through' definition.)

When I modify the code and add source_type: ''User (since that's the target class I am looking for it's subscribers), it works.

To Reproduce
Steps to reproduce the behavior:

1. Call notify_subscribers on any object with the notification_object declaration.
2. See error

Expected behavior
Be able to create a notification for all of the subscribers (targets, or users in my case) to the object.

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

• OS: macOS Mojave
• Browser Firefox
• Version 66.0.1

Additional context
Ruby Version: 2.6.1p33
Rails Version: 5.2.2.1
notification-rails: 1.2.6

Add any other context about the problem here.

I'm trying to use notification-settings to help make a Notification Preferences page where users can opt in/out of different notifications/mailings—like StackOverflow (https://stackoverflow.com/users/email/settings/41234), GitHub (https://github.com/settings/notifications), and many other sites have. I found this gem when searching for a gem that helped to create such a page. I couldn't find any gems that helped directly with creating a Notification Preferences page, so I chose one instead that at least provided a nice backend. I thought it might make it easier overall.... now I'm having some doubts.

The category_settings on NotificationSettings::Setting seems to provide a decent solution for storing an "enabled" (opted out) setting for different notifications/mailings.

However, the fact that category_settings is itself a Hash, with a Hash value (like {enabled: true}) for each key within it is making it harder to build a form for it than it otherwise might be (can't do form_for @settings.category_settings, for example).

If I had just made a simple NotificationPreferences model instead, with a boolean attribute for each mailing, then it would be as simple as adding a f.check_box for each notification option ("category").

Is there an example app anywhere that demonstrates how to create a simple form for a user to edit their NotificationSettings?

Would you consider adding an example of how to use NotificationSettings::Setting in a form to the Readme?

I suppose the subscribe/unsubscribe API wouldn't even need a form because they would just be triggered by a simple button on a resource page, but it seems like apps that use category_settings would want some kind of page/form for editing those settings. An example of this for users and would-be users could be most helpful. :)

Describe the bug

1. push sets pusher and then uses it (initiates a push) immediately
2. If that happens before the transaction is committed, then when after_create_commit gets called, pusher will still be set, causing pusher to get called again.

To Reproduce
Steps to reproduce the behavior:

User.transaction do
  notification = user.notify()
  notification.push :PusherName # <-- `initialize_pusher` gets triggered here, which calls `initiate_push`
end  # <-- after_create_commit :initialize_pusher` gets triggered here, which calls `initiate_push` *again*

Expected behavior

Expect the pusher to only push the notification once!

That would be an embarrassing bug if you sent the same notification twice via the same delivery method.

Fix

Fix and tests will be pushed up shortly (no pun intended).

Describe the bug
undefined method default_category' for #NotificationRenderer::Configuration:0x00007fdac6ed3930`

Occurs when attempting to create notification.

Example

notification = Notification.create target: User.first, object: Certificate.first, category: 'notification'

This started ever since I installed the notification_settings using

rails g notification_settings:install
rails db:migrate

In the documentation, it has this example:

NotificationHandler.configure do |config|
  config.define_group :subscribers, User.where(subscriber: true) + Admin.all
end

But it seems like at the time the configure block is run (when the app boots) is not when User.where(subscriber: true) + Admin.all should be evaluated. It should not be evaluated until you actually go to send a message to that group, because the set of records in the relation may have changed since the app booted up. (Also, it's inefficient to run that query unnecessarily.)

Just like Rails started requiring a callable to be passed to scope in Rails 4, we should require a callable to be passed to define_group:

NotificationHandler.configure do |config|
  config.define_group :subscribers, -> { User.where(subscriber: true) + Admin.all }
end

(Moved out of #41 so we don't distract from the main focus of that issue.)

Having successfully changed the columns to jsonb in my app, I was exploring integrating with the attr_json gem.

This would make things more structured and give us more control over the data type of the elements inside the settings/category_settings/metadata attributes. A plain nested Ruby Hash is kind of a terrible data type, if you think about it:

• You access things with hash[:key] instead of object.key like every other Ruby object (you can't just send it key or key= messages).
• You have to decide whether keys should be strings or symbols (or allow both, and use HashWithIndifferentAccess).
• You don't have much control over defaults (#38), especially for nested hashes.

Can we do better than that?

Here's a possible way to use attr_json so you can see what it looks like...

module NotificationSettings
        Setting
  class Setting
    class Settings
      include AttrJson::Model

      attr_json :enabled,      :boolean, default: true
      attr_json :ActionMailer, :boolean, default: true
      # ...
    end

    include AttrJson::Record

    ['some_category',
     'other_category'
    ].each do |category|
      attr_json category, Settings.to_type, container_attribute: :category_settings, store_key: category
      # Initialize with a default setting so you don't have to worry about it being nil
      define_method category do
        super() || public_send(:"#{category}=", Settings.new)
      end
    end
  end
end

Then you can use it like this...

main > user=User.first; s=user.notification_setting

main > s.some_category.enabled = false
=> false

main > s.save
main > s.reload.category_settings
=> {"some_category"=>#<NotificationSettings::Setting::Settings:0x000055cfa50a9040 @attributes={"enabled"=>false, "ActionMailer"=>true}>}

What I was really wanting to do was this...

s.category_settings.some_category.enabled = false

rather than adding methods like some_category to the top level of the model. Seems a bit cleaner to have them namespaced rather than "unnesting" them like this. But on second thought, being able to simply check setting.some_category.enabled (shorter) is kind of nice, as long as there's no chance of name conflicts...

(If we wanted them namespaced, we could probably do it... Either just prefix the method name, or build our own category_settings proxy object. Or add that feature to attr_json...)

Compatibility (maintaining the old API)

Still allows access like a hash to a limited extent...

main > s.category_settings['some_category'].enabled

main > s.category_settings = {some_category: {:enabled=>false}}
main > s.save
main > s.reload.category_settings
=> {"some_category"=>#<NotificationSettings::Setting::Settings:0x000055d39bc4e4c8 @attributes={"enabled"=>false, "ActionMailer"=>true}>, ...

but unfortunately breaks things in some incompatible ways:

main > s.category_settings['some_category'][:enabled]
NoMethodError: undefined method `[]' for #<NotificationSettings::Setting::Settings:0x000055d39ddd5bf0>
from (pry):18:in `__pry__'

and

main > s.category_settings['some_category'] = {enabled: false}
=> {:enabled=>false}

main > s.save
NoMethodError: undefined method `valid?' for [:enabled, false]:Array
from ~/.gem/ruby/2.4.5/gems/activerecord-5.1.6/lib/active_record/validations/associated.rb:13:in `valid_object?'

Too bad.

Advantages

• Nicer API
• Don't even need to use serialize :category_settings; use this instead of serialize

Potential challenges/downsides

• Can only use this if database column has jsonb (or json?) type, so this feature would probably have to be an optional enhancement... which is far less exciting since we'd still have to keep compatible with the least common denominator and internal code would have to keep accessing those columns as Hashes and not benefit from this change...

• We may not know all the categories ahead of time, so how can we list them in the Settings type? (It may allow the type to be dynamically modified at run-time, I don't know.)

• Causes it to store in the hash with string keys (since JSON doesn't have concept of symbols vs. strings). We can probably work around that by override a method somewhere and calling .with_indifferent_access so that references by symbol (which this gem's internals use) continue to work...

Questions

• Does this belong in this gem or in the app (so they can list their own pushers, categories, default enabled state), etc.?

• Is there a better name for that than Setting::Settings? That seems a redundant.

• What should the default values be for all categories? and should that be configurable by the app?

Conclusion

May not be worth it with all the potential downsides... Still, I'd love to use something more structured for settings/category_settings than an nested hash, so maybe this will inspire some thought towards improving that?

Describe the bug
render_notifications not working even though render_notification is working

my notification/claim_shares/_index.html.erb

  <%= link_to certificate_path(notification.object.id), class: "text-dark media py-2 push" do %>
    <div class="mx-3">
      <i class="fa fa-fw fa-certificate text-success"></i>
    </div>
    <div class="media-body font-size-sm pr-2">
      <div class="font-w600">Claim your share</div>
      <div class="text-muted font-italic"><%= distance_of_time_in_words(notification.object.created_at, DateTime.now) + " ago" %></div>
    </div>
  <% end %>

This is where I'm calling the notifications

 <ul class="nav-items my-2">
              <% if current_user.notifications.count > 0 %>
                <%= render_notification current_user.notifications.last %>
              <% else %>
                <li>
                  <a class="text-dark media py-2" href="#">
                    <div class="mx-3">
                      <i class="fa fa-meh text-secondary"></i>
                    </div>
                    <div class="media-body font-size-sm pr-2">
                      <div class="font-w600">You have no notifications.</div>
                    </div>
                  </a>
                </li>
              <% end %>
            </ul>`

The above method works but when i try using render_notifications it returns an empty div

It's possible to run into errors such as this if you forget to add an initializer that calls NotificationSettings.configure:

     NoMethodError:
       undefined method `do_not_notify_statuses' for nil:NilClass
     # notification-settings/lib/notification_settings/notification_library.rb:126:in `do_not_notify_statuses'
     # notification-settings/lib/notification_settings/notification_library.rb:39:in `status_allows_create?'
     # notification-settings/lib/notification_settings/notification_library.rb:33:in `can_create?'
     # notification-settings/lib/notification_settings/notification_library.rb:28:in `validate_create'

Current implementation doesn't initialize configuration unless and until you call NotificationSettings.configure:

module NotificationSettings
  class << self
    attr_accessor :configuration
  end

  def self.configure
    self.configuration ||= Configuration.new
    yield configuration
  end

Should we unconditionally configure NotificationSettings (and the other gems) with the default config so people don't have to call NotificationSettings.configure if they just want the defaults? (I think that's the way RSpec.configure and most other gems work. We could check how they do it...)

• Add a unit test that it returns a default Configuration object even when NotificationSettings.configure not called

Remove the notification-pusher-actioncable package until it is implemented.

As a follow-up thought to #50, I wonder if instead of requiring target to always be present, I wonder if we should allow it to be omitted, and just fix notification-settings to not break if target is missing...

`notification_setting' for nil:NilClass

The specific use case I'm considering currently is that I have some notifications that should go out to a group of people (multiple recipients) in a single email (so that all the recipients can see each other's names in the To: line of the email.

I don't see an easy way to do that currently.

I see that you can actually specify a group for a notification instead of a target — but it looks like that (before_validation :create_for_group) actually just "flattens" your group, duplicates the notification, and creates a copy of the same notification with target set to each member of the group. So yes, it provides a facade of sending to a group, but ultimately the "send to group" request gets translated into a bunch of "send to individual" operations.

I don't think that would work for my use case, because (I think) it would try to push an individual/separate email for each one.

Options that come to mind...

• don't go through / create a Notification for these emails at all for these — just "special case" them in the app
• create a special GroupNotification class/subclass that knows how to send a single notification/email to a gorup. Have this single notification record represent/handle sending itself to a "group".
• have multiple discrete notifications, but group/aggregate these together somehow in the pusher and not allow the individual notifications to get pushed individually
• create a Group record that we can use as the target

The other reason why it seems the current "group" feature wouldn't work for my case is that the groups are dynamic. The groups I need are more like a combination of a role + a group/company where they have that role — for example, one group is everyone matching {role: :manager, at_company: Company.find(3)} and another group is {role: :member, at_company: Company.find(4)}. There could be any number of them; they can't really be enumerated in the configuration file...

config.define_group :members_company_4, User.with_role(:member, Company.find(4))

Since I can't use the existing group feature, it seems like I need to build another way of sending to groups... which led me to realize that having target be strictly required could actually be a problem.

If only there were a database record for each group that I needed to send to... Then I could simply set the polymorphic target to that Group or Company record and it would just work. But if I need to be more specific and say "send to all {role} people at {company}", then I don't know how to get that to work.

One crazy idea I just had would be to add a new serialized group_spec column that let you specify any arbitrary data structure as the group target. (Can't use target_type/target_id columns for that since they're strongly typed as just a type + id.) Then you could store something like {role: :member, at_company: 4} and be able to interpret that at "push" time.

Or, if you could safely serialize an actual ActiveRecord::Relation (store in a new target_relation column?), that might be an even better option...

I don't want to make things crazy complicated though.

I'll have to keep thinking about this...