Vexed01 / Vex-Cogs

My cogs for Red.
https://cogdocs.vexcodes.com
GNU General Public License v3.0
27 stars 20 forks source link

[Birthday] Burthday notifications aren't persisting over restart! #108

Closed cool-aid-man closed 1 year ago

cool-aid-man commented 1 year ago

What cog is this bug report for?

Burthday

What versions are you running?

Versions                      
                ╷         ╷         ╷              
                │ Current │ Latest  │ Up to date?  
╶───────────────┼─────────┼─────────┼─────────────╴
  This Cog      │ 1.2.0   │ 1.2.0   │ 🟢           
  Bundled Utils │ b980728 │ b980728 │ 🟢           
  Red           │ 3.4.18  │ 3.4.18  │ 🟢           
                ╵         ╵         ╵

Describe the bug

Hey So, I have been using the birthday cog and noticed the birthday wishes/notifications aren't getting in if you restart the bot after setting up the cog. No error whatsoever.

It ONLY sends the notification (in the pre-set channel) if you set the cog and bdset time <time> is set at a time that is before you restart the bot. If you restart after you set the cog (and everything) it doesn't send any notifications.

Was wondering what could be the reason for this behavior! Does it not store the data!! I know it stores the birthdays but what about pinging time!

Thanks

If there's an error, paste it here

None
Vexed01 commented 1 year ago

When did you first install the cog?

cool-aid-man commented 1 year ago

Oh, I don't have an exact date but a pretty long time (around 7-8 months) Though I do update everything from time to time 🤔

Vexed01 commented 1 year ago

I have taken a look and I think I see the issue. I'll get around to taking a more detailed look and fixing it in the next few days. Thanks for reporting!

cool-aid-man commented 1 year ago

Anytime Thanks vex

Vexed01 commented 1 year ago

I think the issue here could be that due to how timing code works. I'm unable to reproduce this when the time is set for later in the day and the bot is restarted before the set hour, or the time is within the set hour in which case I get messages as soon as the bot starts up. However the bot will not catch up if it is over an hour late, meaning after xx:59.

One of the initial reasons no messages are sent too late is to prevent double messages but a later update made messages only send if the user didn't have the birthday role.

While I could allow the bot to send messages late, there is a compromise needed as sometimes the roles can't be applied properly and this would result in repeated birthday messages (see #107 for an example of role failures). Ordinarily a role assignment failure wouldn't mean there are repeated messages, but a restart in this case within the hour would send a repeat! Which is the opposite of the issue here.

Is the behaviour I mentioned in the first paragraph matching what you see? The messages can theoretically be delayed by up to an hour so, I think, there is only an issue if downtime is over one hour. So I guess I'm asking how long are these restarts? Also do you have dev mode ([p]debug]) enabled?

is set at a time that is before you restart the bot.

This should work if its less than an hour behind, but if the set time is over an hour in the past then nothing will be sent.

I pull timing data direct from config so it should be fully persistent and not require any special treatment on cog load.

I will certainly admit that this is one drawback of using Discord role assignment as a kind of database to determine if the notification has been sent. Long term I might consider storing this internally, then there would be no issues with sending messages late. It would also make it easier to not have a birthday role but still have notifications for servers that want that setup.

cool-aid-man commented 1 year ago

Vex so I was trying to reproduce it again but the issue is not there anymore (I did a complete re-install), I have also checked the previous issues when that happened and (you were right) a few times the timing was, in fact, more than 59 minutes, which is not ideal and shouldn't happen under normal scenarios let alone to any normal user.

So I don't see any reason why the current behavior should be changed.

And yea role assignment as DB is probably not the best way but if it works it works! 😇 At least it saves us from repetitive notifications. Thanks again and really sorry to reply to you this late

Vexed01 commented 1 year ago

I would lean towards downtime of over an hour being exceedingly unlikely but sever issues could cause it. I'm going to close this issue but if you see unintended behaviour please do make a new one.

If/when I do any significant work on the cog I'll look into a more robust system for notifications.

cool-aid-man commented 1 year ago

Yes I absolutely will! Thanks vex