excid3 / noticed

Notifications for Ruby on Rails applications
MIT License
2.45k stars 173 forks source link

Unable to make Twillio work #134

Closed pacMakaveli closed 1 year ago

pacMakaveli commented 3 years ago

Hi guys,

I do apologise if this has been raised before. I could only find https://github.com/excid3/noticed/issues/26 that relates to my problem, but it doesn't fix it.

Maybe I'm not understanding the documentation

class RefreshErrorNotification < Noticed::Base
  deliver_by :twilio, debug: true

  def message
    t('.message')
  end
end

My understanding is that by creating

en:
  notifications:
    refresh_error_notification:
      message: 'My Message'

Twillio should pick that message. It doesn't. Looking at the code https://github.com/excid3/noticed/blob/master/lib/noticed/delivery_methods/twilio.rb#L17 I can't see where Noticed tries to call that method at all and instead its looking at notification.params[:message]

#<RefreshErrorNotification:0x00007f98ce4dbc28
 @params=
  {:resource=>
    #<Stat::Fortnite::Identity:0x00007f98cdeb7810
     id: 1,
     user_id: 1,
     account_id: nil,
     username: "pacMakaveli90",
     platform: 0,
     created_at: Tue, 29 Jun 2021 09:39:59.389578000 BST +01:00,
     updated_at: Tue, 29 Jun 2021 09:39:59.389578000 BST +01:00,
     authorization_code: nil,
     refresh_token: nil,
     refresh_token_expires_at: nil,
     access_token: nil,
     access_token_expires_at: nil,
     remote_session_id: nil,
     tenure_level: nil,
     older_than_one_month: nil,
     older_than_two_months: nil,
     remote_created_at: nil,
     remote_profile_status: "available",
     refreshed_at: nil>,
   :error=>
    "Sorry the authorization code you supplied was not found. It is possible that it was no longer valid"},
 @recipient=
  #<Partner:0x00007f98c529ac10
   id: 1,
   user_id: 1,
   token: nil,
   domain: "games.directory",
   name: "games.directory",
   description: "Y",
   subdomain: "gd",
   access: "full",
   requests: 0,
   period: "second",
   uuid: "50364aed-b5d2-40e9-a63b-1f16d5631145",
   created_at: Wed, 07 Apr 2021 11:45:29.638000000 BST +01:00,
   updated_at: Mon, 10 May 2021 00:29:50.107000000 BST +01:00,
   accepted_partner_agreement: true,
   accepted_partner_agreement_at: Mon, 10 May 2021 00:29:45.000000000 BST +01:00,
   accepted_acceptable_use_policy: true,
   accepted_acceptable_use_policy_at: Mon, 10 May 2021 00:29:50.000000000 BST +01:00,
   accepted_privacy_policy: false,
   accepted_privacy_policy_at: nil,
   accepted_terms_of_service: false,
   accepted_terms_of_service_at: nil>,
 @record=nil>

I can fix this by doing either:

Body: notification.params[:message] || notification.message

or passing another value to: ::RefreshErrorNotification.with(resource: resource, error: e.to_s).deliver(user.partner) of message: "my twillio message here"

I personally would've thought that Noticed should automatically call the message method first and perhaps fallback to the message param or check if there is a param of message and then fallback to the message.

Am I doing something wrong?

excid3 commented 3 years ago

Correct, Twilio won't look for a message method. It just uses a message in the params. https://github.com/excid3/noticed/blob/master/lib/noticed/delivery_methods/twilio.rb#L10-L20

I like the idea of calling a message method if it's defined and updating the delivery methods to call that and fallback to a message in params. That would be useful for I18n messages.

PRs welcome.

pacMakaveli commented 3 years ago

@excid3 cool! I'll work on it at some point this week.

dpaola2 commented 2 years ago

Chiming in my support for this. When the documented idiomatic way to construct messages for notifications is to have a message method on the Notification, I found this default behavior to be unintuitive.

dpaola2 commented 2 years ago

Easy override for anyone else coming along: use the format option as in:

  deliver_by :twilio, credentials: :get_twilio_credentials, format: :twilio_formatted, debug: true

  def twilio_formatted
    {
      From: get_twilio_credentials[:phone_number],
      To: recipient.phone_number,
      Body: message
    }
  end