Fixes Missing Reminders From Empty Caches by HassanAbouelela · Pull Request #1919 · python-discord/bot

HassanAbouelela · 2021-10-28T23:33:46Z

Blocked by python-discord/site#618.
See #1916 for more info. This PR shouldn't close that issue.

I updated the reminders cog to try and get around the issue with missing users in the cache during startup. To try and resolve that issue, I switched to fetching as a fallback if the user wasn't found. To avoid accidentally hammering the discord API with fetch requests at startup, I removed the validity checks from the init function, instead opting to only run the check during the send function. It doesn't cost much to fail the task later.

Duplicates the `get_or_fetch_member` utility for users.

Refractors the reminders cog to not check for users in cache during startup, when they are most likely to be missing. Tries to fetch from the discord API if user could not be found in cache. Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>

onerandomusername · 2021-12-26T05:48:56Z

This is no longer blocked by python-discord/site#618 as its been merged!

jchristgit

Hassan my beloved

After Mark's comments are addressed, this looks good to me.

Xithrius · 2022-02-15T00:50:55Z

Bumping this PR, would anyone like to take over? ~~This is still in high priority.~~

Keeping ensure_valid_reminder separate wasn't improving readability; it required a complex return type.

Inactive reminders aren't used anywhere, so they may as well just be deleted.

It's only used for a mention, which can trivially be created manually.

Both try_send_reminder and send_reminder delete the reminder, so allowing them both to run simultaneously for the same reminder could lead to a race condition. Admittedly, it'd be quite rare, since it'd require the reminder to become invalid after send_reminder has been called.

The exact time isn't used, so it's simpler to just used a boolean to determine whether the reminder is late.

MarkKoz · 2022-05-29T00:10:02Z

In hindsight, the DB should have been modified to store the timestamp of the last send attempt rather than a count of failures. The latter approach is flawed because if the bot restarts immediately after failing, it will instantly try again rather than waiting.

wookie184 · 2022-05-29T13:27:15Z

Hmmm. Given #2080 should have fixed the original issue, I'm not sure what the benefit of this retry approach is any more.

MarkKoz · 2022-05-29T16:51:05Z

Hmmm. Given #2080 should have fixed the original issue, I'm not sure what the benefit of this retry approach is any more.

It fixed it by doing away with the author check. This means it still sends the reminder even if the user left the server.

wookie184 · 2022-05-29T17:03:38Z

Hmmm. Given #2080 should have fixed the original issue, I'm not sure what the benefit of this retry approach is any more.

It fixed it by doing away with the author check. This means it still sends the reminder even if the user left the server.

Doesn't fetch_user still get the user if they're not on the server, so this would do the same? If we use get_or_fetch_member then it should only (and always as it isn't reliant on cache) return the member if they're on the server, so the retry logic wouldn't be necessary.

MarkKoz · 2022-05-29T17:08:14Z

@wookie184 Yes, actually. I guess I don't know what the point is. @HassanAbouelela ?

HassanAbouelela · 2022-05-29T17:13:34Z

This PR was written before that one and opted not to do away with the checks. If we don't care about the check anymore, then this is redundant.

MarkKoz · 2022-05-29T18:31:42Z

I'm going to close this then. I don't see any benefit to checking the user if it can't reliably know whether they're in the guild.

HassanAbouelela changed the title ~~Adds Get-Fetch User Utility~~ Fixes Missing Reminders From Empty Caches Oct 29, 2021

HassanAbouelela added review: do not merge The PR can be reviewed but cannot be merged now and removed review: do not merge The PR can be reviewed but cannot be merged now labels Oct 29, 2021

HassanAbouelela added 2 commits October 30, 2021 19:49

Adds Get-Fetch User Utility

e560b42

Duplicates the `get_or_fetch_member` utility for users.

HassanAbouelela force-pushed the reminders-fetch branch from 2dae405 to 5debba4 Compare October 30, 2021 15:50

HassanAbouelela requested review from jb3 and ks129 as code owners October 30, 2021 15:50

ChrisLovering removed the review: do not merge The PR can be reviewed but cannot be merged now label Nov 7, 2021

Xithrius added the s: needs review Author is waiting for someone to review and approve label Dec 15, 2021

Xithrius requested review from ChrisLovering and jchristgit December 15, 2021 14:10

MarkKoz requested changes Dec 25, 2021

View reviewed changes

Comment thread bot/exts/utils/reminders.py Outdated

Comment thread bot/exts/utils/reminders.py Outdated

Comment thread bot/exts/utils/reminders.py Outdated

Comment thread bot/exts/utils/reminders.py Outdated

Comment thread bot/exts/utils/reminders.py Outdated

jchristgit approved these changes Jan 8, 2022

View reviewed changes

wookie184 added s: waiting for author Waiting for author to address a review or respond to a comment and removed s: needs review Author is waiting for someone to review and approve labels Jan 17, 2022

Xithrius added up for grabs Available for anyone to work on and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jan 21, 2022

mbaruh mentioned this pull request Feb 11, 2022

Don't validate reminder author #2080

Merged

Xithrius requested review from ChrisLovering, kosayoda and scragly and removed request for ChrisLovering and ks129 February 15, 2022 00:49

Xithrius added p: 2 - normal Normal Priority and removed p: 1 - high High Priority labels May 27, 2022

MarkKoz added 6 commits May 28, 2022 16:04

Reminders: merge validation into send_reminder

4553893

Keeping ensure_valid_reminder separate wasn't improving readability; it required a complex return type.

Reminders: delete reminder instead of deactivating it

e8e4e40

Inactive reminders aren't used anywhere, so they may as well just be deleted.

Use global bot instance instead of a bot parameter

50b8a0c

Reminders: refactor function names

664a0c8

Reminders: remove redundant user arg from send_reminder

e826113

It's only used for a mention, which can trivially be created manually.

MarkKoz self-assigned this May 28, 2022

Merge main into reminders-fetch

04c35cb

MarkKoz removed the up for grabs Available for anyone to work on label May 28, 2022

MarkKoz added 3 commits May 28, 2022 16:42

Reminders: replace expected_time param with boolean

f6f55d6

The exact time isn't used, so it's simpler to just used a boolean to determine whether the reminder is late.

Reminders: update docstrings

969b90b

Reminders: use constants for max retries and retry time

b863783

Reminders: consider the reminder late if sent after retrying

b462add

MarkKoz closed this May 29, 2022

ChrisLovering deleted the reminders-fetch branch August 16, 2023 09:50

Uh oh!

Conversation

HassanAbouelela commented Oct 28, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

onerandomusername commented Dec 26, 2021

Uh oh!

jchristgit left a comment

Choose a reason for hiding this comment

Uh oh!

Xithrius commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MarkKoz commented May 29, 2022

Uh oh!

wookie184 commented May 29, 2022

Uh oh!

MarkKoz commented May 29, 2022

Uh oh!

wookie184 commented May 29, 2022

Uh oh!

MarkKoz commented May 29, 2022

Uh oh!

HassanAbouelela commented May 29, 2022

Uh oh!

MarkKoz commented May 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Xithrius commented Feb 15, 2022 •

edited

Loading