search

novuhq / novu

Open-Source Notification Platform. Embeddable Notification Center, E-mail, Push and Slack Integrations.
https://novu.co
Other
35.36k stars 3.92k forks source link

[NV-1472] More than one digest step is not working properly #2699

Open jainpawan21 opened 1 year ago

jainpawan21 commented 1 year ago

When having 2 digests in a workflow, the digest is not behaving as expected.

Here first digest is of 60 seconds and second is of 60 minutes, both are of regular type.

To reproduce:

1. Fire 3 events in quick succession 2. After a minute, receive in-app notification 3. Digest begins for the email notification 4. Fire 5 events in quick succession 5. Wait a minute (and longer), no in-app notification for the second in-app digest

Short term workaround for this is to disable second digest step in workflow

From SyncLinear.com | NV-1472

scopsy commented 1 year ago

@oba2311 could you elaborate on why this is blocking NV-1593 ? I don't see any reason for it to block the other ticket.

oba2311 commented 1 year ago

@scopsy - how common it is that we have more than 1 digest node? sounds odd to me and seems like we have been working on this for quite some time.

scopsy commented 1 year ago

Seems like we have 3 different people talked about this (see attached conversations by Pawan). Right now we should either fix it, or disable the ability to add a second digest node.

scopsy commented 1 year ago

This is usefull when you want to: Daily Emails, but hourly in apps, I think the use case makes sense to me. We can further improve it with NV-1593 as it might be related and users trying to achieve this with this pattern

oba2311 commented 1 year ago

Oh in that case if it helps with daily hourly - then def worth it.

tsssdev commented 1 year ago

I have fixed this as part of #2727 , created draft PR, please have look at it. the code committed working perfectly for multiple digest steps.

oba2311 commented 1 year ago

@p-fernandez @scopsy LMK WDYT as I'm looking to wrap up the project this ticket is part of 😃

scopsy commented 1 year ago

this one still not done, Unfortuently we can't merged #2727 as is. We would need to either restructure it in smaller PR's or have @p-fernandez work on it as a standalone. Until then we won't be able to close this issue

oba2311 commented 1 year ago

Thanks @scopsy .

I had a chat with @p-fernandez who explained the same. I'd suggest that we sync and decide on what's next after Pablo is done with the E2E testing (we need to stop for a sec to estimate ROI on this).