Comments (14)
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
from noticed.
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).
from noticed.
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
from noticed.
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.
from noticed.
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.
from noticed.
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.
Line 81 in fbb7742
That's definitely a bug we should fix.
from noticed.
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.
from noticed.
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.
from noticed.
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
from noticed.
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
from noticed.
Thank you @rbague
from noticed.
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.
from noticed.
@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
from noticed.
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?
from noticed.
Related Issues (20)
- Question: Save with custom columns
- Generating noticed:model yields uninitialized constant Noticed::Generators::ModelGenerator::ActiveRecord
- How do we feel about delegating to the instance? HOT 1
- trouble reading from `record` in custom ActionCable formatter HOT 3
- [BUG] Generator did not include `include Noticed::Model` in the Notification model HOT 2
- Mention the User association setup in the docs
- [BUG] Can't use Pathname credentials in FCM delivery method, when defining them in a method
- V2 Delivery Method Config Options Presence Validation? HOT 4
- Not able to get recipient in the mailer method HOT 1
- recipient id vary HOT 5
- Noticed v2 Update Instructions / Sample Code HOT 29
- Twilio messaging will no longer work after their migration to content templates for Whatsapp HOT 1
- [BUG] Rspec running infinitely HOT 5
- [BUG] failing zeitwerk check
- [BUG] FCM Delivery Method errors when config[:json] is a method HOT 1
- [BUG] Inaccurate recipient_type class name for STI model, when delivering the notification
- Rails crashes when not using ActionCable on 2.0.2 and above. HOT 6
- [BUG] Second migration file not necessary when installing noticed on a new app
- [BUG] Updating to ~> 2.1 - Unknown migration version "7.1"
- [BUG] Getting an undefined method gsub error when trying to use the twilio_messaging delivery method upon upgrade to V2 HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from noticed.