notification_methods not working when used on ApplicationNotifier

[janpeterka]

Bug Report

Describe the Bug:
When using notification_methods on my notifier, it works:

class NewAnnouncementNotifier < ApplicationNotifier
  notification_methods do
    def message
      params[:message]
    end
  end
end

When using on ApplicationNotifier (generated automatically) like this:

class ApplicationNotifier < Noticed::Event
  notification_methods do
    def message
      params[:message]
    end
  end
end

it doesn't, saying:

undefined method `message' for #<NewAnnouncementNotifier::Notification id: 1, type: "NewAnnouncementNotifier::Notification", event_id: 1, recipient_type: "Member", recipient_id: 31, read_at: "2024-02-14 16:21:57.000000000 +0100", seen_at: nil, created_at: "2024-02-14 16:42:06.654227000 +0100", updated_at: "2024-02-14 16:21:57.694708000 +0100">

To Reproduce:

1. generate Notifier using rails generate noticed:notifier NewCommentNotifier
2. add notification_methods to ApplicationNotifier

Environment:

• Noticed gem version: 2.1.3

• Ruby version: 3.2.2

• Rails version: 7.1.3

• Operating System: Ubuntu 23.10

• [x ] I have searched for similar issues and couldn't find any

• [ x] I have checked the documentation for relevant information

• [ x] I have included all the required information

[excid3]

Yeah, that's expected since it applies that block immediately to the Notification class under the Notifier. Don't know that there's a good way of doing that but since we didn't intend this I wouldn't consider it a bug. We may want to add support for it, but it's more of a feature add.

[janpeterka]

Thanks, I understand. It seems bit counterintuitive to me, so maybe it might be mentioned in docs?

[excid3]

I'd like to explore supporting that, but not sure when I'll have time.

Presumably, this would need to store the block in a variable on ApplicationNotifier and then evaluate it when we define the subclass Notification constants. It may not be very hard to add.

[ibrahima]

I just wanted to mention that this is surprising to me as well. What then is the purpose of ApplicationNotifier being the base class if we can't define inherited methods there? Though I guess the Notifier is different from the Notification so it makes sense that it wouldn't work. Using a module for this seems okay but I feel like it would be simpler if we could just define methods in a class hierarchy.

[madhums]

Umm I have some issues with notification_methods. Here's my Notifier class

# NewPostNotifier
#
class NewPostNotifier < Noticed::Event
  validates :record, presence: true

  deliver_by :email do |config|
    config.mailer = "NotificationMailer"
    config.method = :notify_new_post
    config.if = :email_notifications?
  end

  required_params :discussion, :story

  notification_methods do
    def message
      t(".message", discussion_title: params[:discussion].title)
    end

    def url
      story_discussion_path(params[:story], params[:discussion])
    end
  end

  # @todo save to db if email_notification?

  # we send notifications to all participants who have set their preference
  # and also to the discussion creator if preference is set
  def email_notifications?(notification)
    recipient = notification.recipient
    # skip when recipient is the post creator
    return false if recipient.id == record.user.id
    if recipient.id == discussion.user.id # discussion owner
      recipient.preference&.notify_new_post_on_discussion
    else # any participants in post
      recipient.preference&.notify_any_post_in_discussion
    end
  end

  def discussion
    params[:discussion]
  end

  def story
    params[:story]
  end
end

1. It does not seem to obey default_url_options which I've set in application_controller.rb

   def default_url_options
     {lang: I18n.locale}
   end

   Also tried adding the below to config/environments/development.rb but doesn't work

   # Default url options for locale based routing
   config.action_controller.default_url_options = {lang: I18n.locale}

   I get an error when I render notifications listing saying "No route matches story_discussion_path,

2. notification_methods block does not seem to have access to its class methods. I would like to use discussion and story but these don't seem to be available. Any clue how to make this work? otherwise it's quite repetitive.

[excid3]

We already do. You must set your default url options correctly. Either in your Rails environment config like normal, or in the notifiers.

Rails.application.configure do
  routes.default_url_options[:host] = 'localhost:3000'
end

See #49 for previous discussion.

[madhums]

Nope! I think that's not working after the upgrade. I have this

# config/environments/development.rb
Rails.application.configure do
  config.action_controller.default_url_options = routes.default_url_options = {lang: I18n.locale, host: ENV["DOMAIN_NAME"]}
end

Now I expect lang and host to be always substituted to the default options provided but instead I get this

No route matches {:action=>"show", :controller=>"discussions", :lang=>#<Story id: 47, title: {"en"=>"testing"}, description: {"en"=>"<div>asd</div>"}, outcomes: {}, source: {}, country: "AL", start_date: nil, end_date: nil, user_id: 6, updater_id: 6, created_at: "2024-02-20 09:06:59.139069000 +0000", updated_at: "2024-02-20 09:06:59.139069000 +0000", discarded_at: nil, category_id: 1>, :story_id=>#<Discussion id: 93, title: "asd", description: "<div>asdasd</div>", story_id: 47, user_id: 6, updater_id: 6, discarded_at: nil, created_at: "2024-02-22 14:41:28.844998000 +0000", updated_at: "2024-02-22 14:41:28.844998000 +0000">}, missing required keys: [:id]

    def url
      story_discussion_path(event.story, event.discussion)
    end

Although passing lang param to story_discussion_path or defining a default_url_options method within the notifications_method block works.

In v1, I had this and it worked quite nicely

# config/initializers/noticed.rb
module Noticed
  class Base
    def default_url_options
      {lang: I18n.locale}
    end
  end
end

[excid3]

Definitely works. See the latest commit where I confirmed it works.

[madhums]

I checked the tests, indeed it works. I am at loss why default_url_options hash is empty within the notification_methods block in my application. I just tried debugging it with pry. Do you have any pointers?

[madhums]

So I ended up with something like this and it seems to work now. (in case anyone else faces this issue)

# app/notifiers/concerns/notification_extensions.rb
module NotificationExtensions
  extend ActiveSupport::Concern

  included do
    def default_url_options
      Rails.application.routes.default_url_options
    end
  end
end

# config/environments/development.rb
  routes.default_url_options = {lang: I18n.locale, host: '...'}

# config/application.rb
module YOUR_APP
  class Application < Rails::Application 
    # ...
    config.to_prepare do
      Noticed::Notification.include NotificationExtensions
    end
    # ...
  end
end

I am still not sure what's the reason default_url_options isn't being included by default or why it doesn't inherit from config.action_controller.default_url_options.