[BUG] Params not being forwarded to mailer

[stlucasgarcia]

Params not being forwarded to mailer

Describe the Bug: params are not being sent to the mailer correctly. The only data that is being correctly set are :notification and static values such as:

  deliver_by :email do |config|
    config.mailer = "UserMailer"
    config.method = "new_post"
    config.params = -> { {foo: :bar} }
  end

To Reproduce:

Considering:

• Noticed is installed
• You have a Post model with some data
• You have a User model with some data
• You are using async or solid_queue queue adapters (I tested with only those)

1. Create a new notifier

rails generate noticed:notifier NewCommentNotifier

3. Create user mailer

rails g mailer UserMailer

5. Uncomment email deliver method

# new_comment_notifier.rb
# To deliver this notification:
#
# NewComment.with(record: @post, message: "New post").deliver(User.all)

class NewCommentNotifier < ApplicationNotifier
  # Add your delivery methods
  #
  deliver_by :email do |config|
    config.mailer = "UserMailer"
    config.method = "new_post"
    # config.params = ->(recipient) { {user: recipient} }  If a uncomment this code it throws ArgumentError (wrong number of arguments (given 0, expected 1))
  end
  #
  # bulk_deliver_by :slack do |config|
  #   config.url = -> { Rails.application.credentials.slack_webhook_url }
  # end
  #
  # deliver_by :custom do |config|
  #   config.class = "MyDeliveryMethod"
  # end

  # Add required params
  #
  # required_param :message
end

6. Add new_post to UserMailer

# user_mailer.rb

class UserMailer < ApplicationMailer

  def new_post
    puts params
  end
end

7. Run console and create a new notification

rails c

irb(main):001> NewCommentNotifier.with(record: Post.first).deliver(User.first)

Expected Behavior: The logged params object must contain the following:

{
 :notification => #<NewCommentNotifier::Notification>
 :record => #<Post>,
 :recipient => #<User>
}

Actual Behavior: Params contains only the :notification key with data, while :record and :recipient are nil

{
 :notification => #<NewCommentNotifier::Notification>
 :record => nil,
 :recipient => nil
}

Environment:

• Noticed gem version: 2.1.3
• Ruby version: 3.3.0
• Rails version: 7.1.3
• Operating System: macOS 14.2.1

Related Issues:

365

Labels to Apply: BUG

Checklist:

• [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]

Params does not receive an argument so it should be:

config.params = ->{ {user: recipient} }

[stlucasgarcia]

Thanks for the quick reply @excid3

I have now the following code:

# new_comment_notifier.rb
# To deliver this notification:
#
# NewComment.with(record: @post, message: "New post").deliver(User.all)

class NewCommentNotifier < ApplicationNotifier
  # Add your delivery methods
  #
  deliver_by :email do |config|
    config.mailer = "UserMailer"
    config.method = "new_post"
    config.params = ->{ {user: recipient} }
  end
  #
  # bulk_deliver_by :slack do |config|
  #   config.url = -> { Rails.application.credentials.slack_webhook_url }
  # end
  #
  # deliver_by :custom do |config|
  #   config.class = "MyDeliveryMethod"
  # end

  # Add required params
  #
  # required_param :message
end

I made no changes to the mailer, when I create a new notification I get the following log:

{
 :user => nil,
 :notification => #<NewCommentNotifier::Notification>,
 :record => nil,
 :recipient => nil
}

Am I doing something wrong?

[excid3]

Lemme check our test suite and see if I can reproduce.

[excid3]

I'm not able to reproduce it. Everything is included correctly from my testing.

[stlucasgarcia]

It must be something on my side then. I am going to close this and keep investigating.

Thanks for the support!

[excid3]

Trying to think of what else it could be. I'd check your Gemfile.lock and make sure you're on the latest. I know this was a bug in previous versions and fixed at some point.

[stlucasgarcia]

@excid3 I just found out what it was.

My models had id type of uuid, while I did not change the noticed migration to support uuid.

Is it worth adding a warning or error for these cases? Or have the generator to add theuuid type on the migrations if the initializer g.orm :active_record, primary_key_type: :uuid exists (just some brainstorming)

[stlucasgarcia]

I have seen you just did some fix in the migrations side

[excid3]

Ah yeah! I just merged #402 which matches what Rails does for ActiveStorage migrations so we can do the same thing. Will cut a new release of that shortly.

[rromanchuk]

@excid3 i'm debugging this right now as we speak (this ticket specifically), let me clean up my noisy queue and i can send more details. I didn't see anything obvious in Noticed::DeliveryMethods::Email, so let me clean up the noise

[rromanchuk]

lol actually, looks like i have a general config error, hasn't even gotten to actual delivery methods. I'm guessing my :if blocks are out of scope

[rromanchuk]

This is how i had communication prefs logic split

module Apnable
  extend ActiveSupport::Concern

  included do
    deliver_by :ios do |config|
      # truncated
      config.if = :push_notifications?
    end
  end
end

class FollowedNotifier < Noticed::Event
  include Apnable

  def email_notifications?
    recipient.follower_alerts.include?('email')
  end

  def push_notifications?
    recipient.follower_alerts.include?('push')
  end

  def sms_notifications?
    recipient.follower_alerts.include?('sms')
  end
end

Looks like recipient is yielded and i'm not receiving it so probably explains the

ArgumentError (wrong number of arguments (given 1, expected 0))

Fixed It yields the notification https://github.com/excid3/noticed/blob/main/lib/noticed/delivery_method.rb#L54 now i'm getting actual channel specific delivery failures, which is a good sign and expected in my current env

def email_notifications?(notification)
  notification.recipient.follower_alerts.include?('email')
end