Skip to content

Fix false error message when deleting reminders with duplicate IDs#3500

Open
oliveman-au wants to merge 1 commit intopython-discord:mainfrom
oliveman-au:fix/reminder-delete-duplicate-id-false-error
Open

Fix false error message when deleting reminders with duplicate IDs#3500
oliveman-au wants to merge 1 commit intopython-discord:mainfrom
oliveman-au:fix/reminder-delete-duplicate-id-false-error

Conversation

@oliveman-au
Copy link
Copy Markdown

When running !remind delete with duplicate IDs (e.g. !remind delete 123 123 123), the reminder is deleted successfully but the bot incorrectly shows "The other reminder(s) could not be deleted as they're either locked, belong to someone else, or don't exist."

This happens because the check compares len(deleted_ids) against len(ids), but the loop iterates over set(ids) to deduplicate. So passing 3 identical IDs results in 1 deletion but len(ids) is still 3, making it look like 2 failed.

Fixed by comparing against len(set(ids)) instead, which matches what was actually attempted.

@oliveman-au
Copy link
Copy Markdown
Author

I Reproduced The Bug In The Image

Screenshot 2026-04-25 205620

Copy link
Copy Markdown
Member

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one-liner fix, code looks good, you've proven it's a real bug, even added a screenshot of the bug being reproduced - nice! Good PR. Thanks for the contribution! 👍🏼

Copy link
Copy Markdown
Member

@jb3 jb3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should reuse the already created set from earlier in the command, that's going to be a cleaner solution here and avoids a set creation twice for the same list (and solves other issues like how passing the same reminder ID in would go above the 5 reminder limit, even if it's the same reminder).

I suggest instead you do the set conversion earlier in the command (pretty much the first thing) and replace subsequent set(ids) invocations with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants