feat:(bookmarks) added removing bookmarks #904
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR extends the bookmark feature to support removable emojis configurable via settings, implements deletion of bookmarked messages in DMs, and hardens raw reaction handlers (including the poll handler) with fallback user/channel fetching and error logging. Sequence diagram for bookmark add and remove via emoji reactionsequenceDiagram
actor User
participant Tux
participant Discord
participant Settings
User->>Discord: Reacts with add or remove emoji
Discord->>Tux: on_raw_reaction_add event
Tux->>Settings: Get valid emojis
alt Add emoji
Tux->>Discord: Fetch message, user, channel
Tux->>User: Send bookmark DM
Tux->>User: Add remove emoji to DM
else Remove emoji (in DM)
Tux->>Discord: Fetch message, user, channel
Tux->>Discord: Delete DM message
end
Entity relationship diagram for bookmark emoji configurationerDiagram
CONSTANTS {
string ADD_BOOKMARK
string REMOVE_BOOKMARK
}
BOOKMARKS ||--|| CONSTANTS : uses
Updated class diagram for Bookmarks service with bookmark removalclassDiagram
class Bookmarks {
- bot: Tux
- valid_add_emojis
- valid_remove_emojis
- valid_emojis
+ _is_valid_emoji(emoji, valid_list) bool
+ on_raw_reaction_add(payload)
+ _create_bookmark_embed(message) Embed
+ _delete_bookmark(message, user)
+ _send_bookmark(user, message, embed, emoji)
}
class Constants {
+ ADD_BOOKMARK
+ REMOVE_BOOKMARK
}
Bookmarks --> Constants : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cherryl1k - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tux/cogs/services/bookmarks.py:45` </location>
<code_context>
- if str(payload.emoji) != "🔖":
+ # Get the user who reacted to the message
+ user = self.bot.get_user(payload.user_id)
+ if user is None:
+ logger.error(f"User not found for ID: {payload.user_id}")
</code_context>
<issue_to_address>
get_user may return None for users not in cache.
Consider using await self.bot.fetch_user(payload.user_id) as a fallback to reliably retrieve the user if they are not in the cache.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
# Get the user who reacted to the message
user = self.bot.get_user(payload.user_id)
if user is None:
logger.error(f"User not found for ID: {payload.user_id}")
return
=======
# Get the user who reacted to the message
user = self.bot.get_user(payload.user_id)
if user is None:
try:
user = await self.bot.fetch_user(payload.user_id)
except discord.NotFound:
logger.error(f"User not found for ID: {payload.user_id}")
return
except discord.HTTPException as fetch_exc:
logger.error(f"HTTPException while fetching user {payload.user_id}: {fetch_exc}")
return
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tux/cogs/services/bookmarks.py:41` </location>
<code_context>
-------
None
"""
+ if not self._is_valid_emoji(payload.emoji, self.valid_emojis):
+ return
</code_context>
<issue_to_address>
Consider replacing multiple emoji checks and if/elif branches with a single emoji-to-handler dictionary lookup for cleaner dispatch.
Here’s one way to collapse both of your `if/elif` emoji‐checks into a single lookup. You build one small dict mapping each emoji → a handler (and whether that handler needs you to build an embed), then in your listener you do one lookup and dispatch:
```python
class Bookmarks(commands.Cog):
def __init__(self, bot: Tux) -> None:
self.bot = bot
# map emoji (ID or name) → (handler, needs_embed)
self.emoji_actions: dict[int|str, tuple[Callable, bool]] = {
**{e: (self._send_bookmark, True) for e in CONFIG.ADD_BOOKMARK},
**{e: (self._delete_bookmark, False) for e in CONFIG.REMOVE_BOOKMARK},
}
@commands.Cog.listener()
async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None:
# Fetch user/channel/message first…
user = self.bot.get_user(payload.user_id)
if not user:
logger.error(f"User not found: {payload.user_id}")
return
channel = self.bot.get_channel(payload.channel_id)
if not channel:
logger.error(f"Channel not found: {payload.channel_id}")
return
channel = cast(discord.TextChannel|discord.Thread, channel)
try:
message = await channel.fetch_message(payload.message_id)
except discord.NotFound:
logger.error(f"Message not found: {payload.message_id}")
return
except (discord.Forbidden, discord.HTTPException) as e:
logger.error(f"Failed fetch: {e}")
return
# single lookup
key = payload.emoji.id or payload.emoji.name
action = self.emoji_actions.get(key)
if not action:
return
handler, needs_embed = action
if needs_embed:
embed = await self._create_bookmark_embed(message)
await handler(user, message, embed, payload.emoji)
else:
await handler(message, user)
```
And then keep your two small handlers untouched:
```python
async def _delete_bookmark(self, message: discord.Message, user: discord.User) -> None:
if message.author is not self.bot.user:
return
await message.delete()
@staticmethod
async def _send_bookmark(
user: discord.User,
message: discord.Message,
embed: discord.Embed,
emoji: discord.PartialEmoji
) -> None:
…
```
This:
- Drops all repeated `_is_valid_emoji` checks
- Collapses the `if/elif` into one `dict.get`
- Keeps both add/remove routines in tiny focused methods.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
not sure why its getting giving an error for config,py with the tests |
|
don't merge yet im trying to figure out what happened with git and why it was having a stroke |
|
it should be good now |
cherryl1k
changed the title
|
Please use the conventional commit styling for commit messages, and use rebase to pull from main instead of creating a merge commit every time you update the branch. |
|
definitely going to have to squash merge this to keep the git history looking somewhat reasonable |
|
this is hell |
|
im going to close this cause this is a mess and i should be using a feature branch for this |
cherryl1k
changed the title
|
reopening this apologies about the messy commit history and the lack of a proper branch my other branch is completely wack |
There was a problem hiding this comment.
Hey @cherryl1k - I've reviewed your changes - here's some feedback:
- Consider using a list or set for valid_add_emojis/valid_remove_emojis instead of a single string so emoji lookups aren’t matching substrings of the emoji characters.
- Extract the repeated fetch‐or‐get logic for users and channels into a shared helper to avoid duplicating error handling in both cogs.
- Verify that
_delete_bookmarkdeletes the DM bookmark message (sent by the bot) rather than the original source message to avoid accidentally removing content from the server.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a list or set for valid_add_emojis/valid_remove_emojis instead of a single string so emoji lookups aren’t matching substrings of the emoji characters.
- Extract the repeated fetch‐or‐get logic for users and channels into a shared helper to avoid duplicating error handling in both cogs.
- Verify that `_delete_bookmark` deletes the DM bookmark message (sent by the bot) rather than the original source message to avoid accidentally removing content from the server.
## Individual Comments
### Comment 1
<location> `tux/cogs/services/bookmarks.py:21` </location>
<code_context>
+ self.valid_emojis = CONST.ADD_BOOKMARK + CONST.REMOVE_BOOKMARK
+
+ # The linter wants to change this but it breaks when it does that
+ def _is_valid_emoji(self, emoji: discord.PartialEmoji, valid_list: str) -> bool:
+ if emoji.name in valid_list: # noqa: SIM103
+ return True
+ return False
</code_context>
<issue_to_address>
The use of 'in' with a string for emoji validation may lead to false positives.
Checking 'in' on a string can match substrings, not just exact emoji names. Use a list or tuple and check for equality to ensure only valid emojis are matched.
</issue_to_address>
### Comment 2
<location> `tux/cogs/services/bookmarks.py:16` </location>
<code_context>
def __init__(self, bot: Tux) -> None:
self.bot = bot
+ self.valid_add_emojis = CONST.ADD_BOOKMARK
+ self.valid_remove_emojis = CONST.REMOVE_BOOKMARK
+ self.valid_emojis = CONST.ADD_BOOKMARK + CONST.REMOVE_BOOKMARK
+
+ # The linter wants to change this but it breaks when it does that
</code_context>
<issue_to_address>
Storing valid emojis as concatenated strings may cause ambiguity.
Concatenating emojis into a string can lead to incorrect substring matches and iteration issues, particularly with multi-codepoint emojis. Consider using a list or tuple to store valid emojis for more reliable handling.
Suggested implementation:
```python
self.valid_add_emojis = list(CONST.ADD_BOOKMARK)
self.valid_remove_emojis = list(CONST.REMOVE_BOOKMARK)
self.valid_emojis = self.valid_add_emojis + self.valid_remove_emojis
```
```python
def _is_valid_emoji(self, emoji: discord.PartialEmoji, valid_list: list[str]) -> bool:
return emoji.name in valid_list
```
If `CONST.ADD_BOOKMARK` and `CONST.REMOVE_BOOKMARK` are not already sequences (e.g., lists or tuples), but are strings of emojis, this change will treat each emoji as a single Unicode codepoint. If your emojis are multi-codepoint (e.g., emojis with skin tone modifiers), you may need to explicitly define them as lists of emoji strings in the constants file for full correctness.
</issue_to_address>
### Comment 3
<location> `tux/cogs/services/bookmarks.py:111` </location>
<code_context>
-
return embed
+ async def _delete_bookmark(self, message: discord.Message, user: discord.User) -> None:
+ if message.author is not self.bot.user:
+ return
+ await message.delete()
</code_context>
<issue_to_address>
Using 'is not' for user comparison may not be reliable.
Compare user IDs instead of object instances to ensure correct user matching.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai dismiss |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cherryl1k - I've reviewed your changes - here's some feedback:
- Deleting the original message in
_delete_bookmarkremoves the source message rather than the DM bookmark—consider tracking and deleting the DM message object instead. - User/channel fetch and error-handling code is duplicated in both cogs—extract a shared helper to DRY this logic.
- The
_is_valid_emojisignature and linter workaround suggest its input type could be clearer; consider accepting an iterable of emoji names and simplifying toreturn emoji.name in valid_emojis.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Deleting the original message in `_delete_bookmark` removes the source message rather than the DM bookmark—consider tracking and deleting the DM message object instead.
- User/channel fetch and error-handling code is duplicated in both cogs—extract a shared helper to DRY this logic.
- The `_is_valid_emoji` signature and linter workaround suggest its input type could be clearer; consider accepting an iterable of emoji names and simplifying to `return emoji.name in valid_emojis`.
## Individual Comments
### Comment 1
<location> `tux/cogs/services/bookmarks.py:111` </location>
<code_context>
-
return embed
+ async def _delete_bookmark(self, message: discord.Message, user: discord.User) -> None:
+ if message.author is not self.bot.user:
+ return
+ await message.delete()
</code_context>
<issue_to_address>
Using 'is not' for author comparison may not be reliable.
Object identity isn't guaranteed; compare author IDs instead for accuracy.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if message.author is not self.bot.user:
return
=======
if message.author.id != self.bot.user.id:
return
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tux/cogs/services/bookmarks.py:139` </location>
<code_context>
try:
- await user.send(embed=embed)
+ dm_message = await user.send(embed=embed)
+ await dm_message.add_reaction(CONST.REMOVE_BOOKMARK)
except (discord.Forbidden, discord.HTTPException) as dm_error:
</code_context>
<issue_to_address>
Adding a reaction to the DM message may fail if the bot lacks permissions.
Handle or document the potential exception if adding a reaction to a DM fails due to permission issues.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+112
to
+113
| if message.author is not self.bot.user: | ||
| return |
There was a problem hiding this comment.
suggestion (bug_risk): Using 'is not' for author comparison may not be reliable.
Object identity isn't guaranteed; compare author IDs instead for accuracy.
Suggested change
| if message.author is not self.bot.user: | |
| return | |
| if message.author.id != self.bot.user.id: | |
| return |
Comment on lines
+139
to
+140
| dm_message = await user.send(embed=embed) | ||
| await dm_message.add_reaction(CONST.REMOVE_BOOKMARK) |
There was a problem hiding this comment.
suggestion: Adding a reaction to the DM message may fail if the bot lacks permissions.
Handle or document the potential exception if adding a reaction to a DM fails due to permission issues.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added config options for bookmarks in the settings.yml including custom emojis and added the ability for the user to delete bookmarks in their DMs
Guidelines
My code follows the style guidelines of this project (formatted with Ruff)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if needed
My changes generate no new warnings
I have tested this change
Any dependent changes have been merged and published in downstream modules
I have added all appropriate labels to this PR
I have followed all of these guidelines.
How Has This Been Tested? (if applicable)
Tested by reacting to messages both inside and outside of DMs both from tux and from the user all works correctly and it only removes Tux messages in DMs
Screenshots (if applicable)
Additional Information
closed my previous pull request because it was a mess. change to poll.py is ensuring it doesn't try to read reactions in dms
also sorry that some of the commits are messy and non-standard
Summary by Sourcery
Enable bookmark removal via emoji reactions and enhance bookmark reaction handling with configurable emojis, robust fetching, and error logging.
New Features:
Bug Fixes:
Summary by Sourcery
Enable bookmark removal via emoji reaction, expose bookmark emojis as constants, improve reaction handling with fallback fetches and error logging, and fix the poll reaction handler to fetch uncached channels
New Features:
Bug Fixes:
Enhancements: