Add delay to entire notification

[excid3]

We just added delays for individual delivery methods which is great (thanks @rbague!).

@gathuku mentioned it would be great to delay the entire notification: https://github.com/excid3/noticed/pull/61#issuecomment-724941484

In my use case am using a custom delivery to send SMS, I wanted a way where you can schedule an SMS to be sent later(at a specific dateTime). This means you will pick dateTime when you want the SMS to be delivered. What I tried is to get the chosen DateTime difference with Time.now in minutes pass it to my notification instance as params and finally set it as delay in the delivery method.

I think this makes sense to do and think we could probably allow similar syntax as ActiveJob:

Job.set(wait: 1.week).perform_later
Job.set(wait_until: Date.tomorrow.noon).perform_later

We should support .set(**options) syntax and simply pass those along on a deliver_later call. This would keep the interface nicely similar to ActiveJob and flexible.

If we collect all the options and simply pass them along to ActiveJob, we can safely support all future features/changes that ActiveJob might have.

[excid3]

Currently we have a single method for delivering:

        # Always perfrom later if a delay is present
        if (delay = delivery_method.dig(:options, :delay))
          method.set(wait: delay).perform_later(args)
        elsif enqueue
          method.perform_later(args)
        else
          method.perform_now(args)
        end

We could combine the delivery method delay with a set(wait:) in order to support notifications with multiple delivery methods.

For example:

class NewComment < Noticed::Base
  deliver_by :database
  deliver_by :slack
  deliver_by :email, delay: 5.minutes
end

NewComment.with(comment: @comment).set(wait: 15.minutes).deliver_later

The database delivery method should run immediately (so the other delivery methods can access the database record). The slack delivery method should run in 15 minutes. The email delivery method should run in 20 minutes.

We should also support wait_until:

NewComment.with(comment: @comment).set(wait_until: 15.minutes.from_now).deliver_later

Question: What happens if set(wait: 5.minutes, wait_until: 10.minutes.from_now) is used? Does one take precedence in ActiveJob? Answer: wait_until takes precedence

[rbague]

Answering your question, yes, wait_until takes precedence over wait as you can see here.

But setting a delay on the notification brings an issue with the database delivery, which I encountered while working on #61. The database delivery must be executed immediately, it can not be enqueued, otherwise, the database record won't be available for the other deliveries, as run_delivery_method here would return the scheduled job instead of the database record.

I am working on adding documentation, as well as adding some tests for it since I found the issue. I hope to have a PR soon.

[excid3]

Thanks for checking on wait_until 👍

We do have an exception for database delivery so it runs immediately no matter what. I'll update my example to use a different delivery method to clarify that.

[excid3]

It looks like we might be running the database delivery method twice though because it's not removed from the delivery methods on line 81. https://github.com/excid3/noticed/blob/fbb7742baf525cd8a6648131f747bc4c39f01b4d/lib/noticed/base.rb#L81

That's definitely a bug we should fix.

[rbague]

Sorry, my bad. What I meant here is that the database delivery can't be delayed as it won't return the database record, I realized this after merging the PR. That is what I was working on

But setting a delay on the notification brings an issue with the database delivery, which I encountered while working on #61. The database delivery must be executed immediately, it can not be enqueued, otherwise, the database record won't be available for the other deliveries, as run_delivery_method here would return the scheduled job instead of the database record.

And regarding the issue you just commented, it seems to be working right in my machine, the database delivery is only executed once and removed from the delivery_methods array.

[excid3]

Oh yeah, I just glossed over the delete line there. It does remove it.

That's fine if database delivery is the exception. It always will be for that reason.

[gathuku]

Hey @excid3 , I gave this a try but am having problems.

What i implemented

module Noticed
class Base
   class_attribute :set_options, instance_writer: true, default: {}
   
   class << self 
   end

   def set(options)
     set_options = options
     self
   end

end
end

This allowed to set the options required , which is stored in class_attribute set_options

CommentNofication.with(foo: :bar).set(wait: 3.minutes, wait_until: Date.tomorrow.noon, priority: 10)

when i come to delivering i added another elseif which checks if set_options has wait_until and set it in method.

        # Always perfrom later if a delay is present
        if (delay = delivery_method.dig(:options, :delay))
          method.set(wait: delay).perform_later(args)
        elsif(wait_until = set_options.dig(:wait_until))
            method.set(wait_until: wait_until).perform_later(args)
        elsif enqueue
          method.perform_later(args)
        else
          method.perform_now(args)
        end

This implementation results to ActiveJob::SerializationError: Unsupported argument type: Noticed::DeliveryMethods::Database on delivering notification . ref

ExampleNotification.with(foo: :bar).set(wait_until: Date.tomorrow.noon).deliver(user)

cc @rbague

[rbague]

Hi @gathuku. The error you are seeing is because the database delivery is being enqueued instead of being executed right away. Thus making this line return the enqueued job (Noticed::DeliveryMethods::Database - which can't be serialized by active_job) instead of the database record, as the code expects. There should be a failing test to ensure this doesn't happen.

The database delivery should always be executed with perform_now regardless of which options are passed to ensure that the database record is always accessible by the other delivery methods

[gathuku]

Thank you @rbague

[dvodvo]

Chiming in late here (possibly). wait_until seems to reference a delay. Does/can it apply to a set datetime?

An existing workflow would be to launch a whenever schedule action and call the mailer, Twilio issuance engine, whatever. If I am not mistaken, with noticed I would have to generate a Custom Delivery Method and hook it into some scheduling mechanism. Which is possibly more complex than the existing workflow. This is then compounded by the fact that other cases are much simpler with noticed thus leading to establishing both mechanisms, which is unduly complex.

[gathuku]

I would like to see this feature implemented. Right now you can delay each delivery method with delay option. @excid3 can we have something like delay_until: Date.tomorrow.noon, but i would like this be set when sending the notifications so it can delay all the notifications methods.

Currently trying to implement something like this , i think it will be a bit easier if there was delay_until

  def perform
    Reminder.for_each do |reminder| 
      # to_be_sent_at less that 1 hour
      if reminder.to_be_sent_at < 1.hour.from_now 
        delay = (reminder.to_be_sent_at - Time.current).minutes
        PaymentReminder.with(borrowing: reminder.borrowing, delay: delay).deliver(reminder.borrowing.user)
      end
    end
  end

[ykamin-booth]

I guess I didn't realize this feature had not been implemented. I had been using the standard ActiveJobs delay method with DelayedJobs with no issues: MessageNotification.with(message: message).delay(run_at: Time.now+15.seconds).deliver(recipients)

However, now that I moved to Sidekiq I'm getting a NoMethod error when I try to do this (despite enabling the delay method for Sidekiq in my initializer). NoMethodError: undefined method 'delay' for #<MessageNotification:0x00007f1930f7be58>

Is this expected? Why would there be different behavior here?

My use case for delaying the entire notification is in a chat where if the other person is in the chat and reads the message, there shouldn't be a notification at all (both in terms of cluttering up the list of notifications on the index page and in terms of seeing the new notification come in over actioncable).

[gathuku]

@ykamin-booth i think delay is an delayed_job specific method so when you switch to sidekiq you can't use the method. I think you want to achieve fallback notifications https://github.com/excid3/noticed#fallback-notifications

[ykamin-booth]

Because I set Sidekiq::Extensions.enable_delay! in my sidekiq initializer, I should still be able to use the method (https://github.com/mperham/sidekiq/wiki/Delayed-extensions). I have tested this with other classes and instance methods and was able to use it--it's only in Noticed notifications that I'm getting this NoMethod error.

Fallback notifications wouldn't work in this use case unless I just had some kind of wrapper notification that was never visible to its recipients, because the point is that I don't want the notification to even hit the database if the message is read: deliver_by :database, unless: :message_read?