feat:(bookmarks) added removing bookmarks #916
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
…and channel fetching
…and channel fetching
…and channel fetching
…and channel fetching
Contributor
Reviewer's GuideThis PR enhances bookmark functionality by introducing configurable add/remove emojis and central emoji validation, refactors the reaction handling workflow—splitting add/remove flows, making embed creation async and adding DM reaction options—bolsters user/channel fetch error handling, updates the poll cog to safely handle missing or DM channels, and adds a new standalone utility cog for bookmarks. Sequence diagram for handling add/remove bookmark reactionssequenceDiagram
actor User
participant Bot
participant Channel
participant Message
User->>Bot: Reacts with add/remove bookmark emoji
Bot->>Channel: Fetch channel
Bot->>Message: Fetch message
alt Add bookmark emoji
Bot->>User: Send DM with embed
Bot->>Message: Add remove bookmark reaction
else Remove bookmark emoji (in DM)
Bot->>Message: Delete message (if author is bot)
end
Entity relationship diagram for bookmark emoji configurationerDiagram
CONSTANTS {
string ADD_BOOKMARK
string REMOVE_BOOKMARK
}
BOOKMARKS {
string valid_add_emojis
string valid_remove_emojis
string valid_emojis
}
BOOKMARKS ||--|| CONSTANTS : uses
Class diagram for updated Bookmarks service and constantsclassDiagram
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
Class diagram for new utility Bookmarks cogclassDiagram
class Bookmarks {
- bot: commands.Bot
+ on_raw_reaction_add(payload)
}
class EmbedCreator {
+ create_info_embed(title, description)
}
Bookmarks ..> EmbedCreator : 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}
Contributor
There was a problem hiding this comment.
Hey @cherryl1k - I've reviewed your changes - here's some feedback:
- The new
cogs/utility/bookmarks.pyduplicates much of the logic intux/cogs/services/bookmarks.py—consider consolidating into a single cog to avoid maintenance overhead and conflicting behavior. - The log message in the emoji validation else branch uses profanity—please replace it with a clear, professional error message.
- You’re storing
ADD_BOOKMARKandREMOVE_BOOKMARKas strings but treating them like collections in_is_valid_emoji; consider using lists or sets of emoji names for better clarity and future extensibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `cogs/utility/bookmarks.py` duplicates much of the logic in `tux/cogs/services/bookmarks.py`—consider consolidating into a single cog to avoid maintenance overhead and conflicting behavior.
- The log message in the emoji validation else branch uses profanity—please replace it with a clear, professional error message.
- You’re storing `ADD_BOOKMARK` and `REMOVE_BOOKMARK` as strings but treating them like collections in `_is_valid_emoji`; consider using lists or sets of emoji names for better clarity and future extensibility.
## Individual Comments
### Comment 1
<location> `tux/cogs/services/bookmarks.py:143` </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 potential exceptions from add_reaction with a try/except block to prevent unhandled errors if the bot lacks permissions or DMs are restricted.
</issue_to_address>
### Comment 2
<location> `tux/cogs/services/bookmarks.py:27` </location>
<code_context>
+ self.bot = bot
+
+ @commands.Cog.listener()
+ async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None:
+ if str(payload.emoji) == "🔖":
+ try:
</code_context>
<issue_to_address>
Consider refactoring on_raw_reaction_add to flatten control flow, centralize fetch logic, and make embed creation synchronous.
```markdown
You can flatten `on_raw_reaction_add` and remove unreachable branches by:
1. Dropping the initial `valid_emojis` check and final `else` (it’s never reached once you branch by add/remove).
2. Pulling user/channel get‐then‐fetch logic into small helpers.
3. Making `_create_bookmark_embed` a sync method again (nothing inside is async).
Example refactor:
```python
class Bookmarks(commands.Cog):
def __init__(…):
…
self.add_emojis = set(CONST.ADD_BOOKMARK)
self.remove_emojis = set(CONST.REMOVE_BOOKMARK)
async def on_raw_reaction_add(self, payload):
emoji = payload.emoji.name
if emoji in self.add_emojis:
user = await self._get_user(payload.user_id) or return
channel = await self._get_channel(payload.channel_id) or return
message = await self._safe_fetch(channel.fetch_message, payload.message_id) or return
embed = self._create_bookmark_embed(message)
await self._send_bookmark(user, message, embed)
elif emoji in self.remove_emojis:
user = await self._get_user(payload.user_id) or return
channel = await self._get_channel(payload.channel_id) or return
message = await self._safe_fetch(channel.fetch_message, payload.message_id) or return
await self._delete_bookmark(message)
# no final else needed
async def _get_user(self, user_id):
user = self.bot.get_user(user_id)
if user: return user
return await self._safe_fetch(self.bot.fetch_user, user_id)
async def _get_channel(self, channel_id):
channel = self.bot.get_channel(channel_id)
if channel: return channel
return await self._safe_fetch(self.bot.fetch_channel, channel_id)
async def _safe_fetch(self, fn, *args):
try:
return await fn(*args)
except discord.NotFound:
logger.error(f"Not found: {fn.__name__}({args[0]})")
except (discord.Forbidden, discord.HTTPException) as e:
logger.error(f"Failed {fn.__name__}: {e}")
def _create_bookmark_embed(self, message: discord.Message) -> discord.Embed:
content = message.content
if len(content) > CONST.EMBED_MAX_DESC_LENGTH:
content = content[:CONST.EMBED_MAX_DESC_LENGTH - 3] + "..."
embed = EmbedCreator.create_embed(
bot=self.bot,
embed_type=EmbedCreator.INFO,
title="Message Bookmarked",
description=f"> {content}",
)
embed.add_field("Author", message.author.name, inline=False)
embed.add_field("Jump to Message", f"[Click Here]({message.jump_url})", inline=False)
if message.attachments:
urls = "\n".join(a.url for a in message.attachments)
embed.add_field("Attachments", urls, inline=False)
return embed
```
This removes deep nesting, centralizes fetch-and-log, drops the “how’d you get here?” branch, and keeps embed creation synchronous.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| try: | ||
| await user.send(embed=embed) | ||
| dm_message = await user.send(embed=embed) | ||
| await dm_message.add_reaction(CONST.REMOVE_BOOKMARK) |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Adding a reaction to the DM message may fail if the bot lacks permissions.
Handle potential exceptions from add_reaction with a try/except block to prevent unhandled errors if the bot lacks permissions or DMs are restricted.
| return False | ||
|
|
||
| @commands.Cog.listener() | ||
| async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None: |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring on_raw_reaction_add to flatten control flow, centralize fetch logic, and make embed creation synchronous.
You can flatten `on_raw_reaction_add` and remove unreachable branches by:
1. Dropping the initial `valid_emojis` check and final `else` (it’s never reached once you branch by add/remove).
2. Pulling user/channel get‐then‐fetch logic into small helpers.
3. Making `_create_bookmark_embed` a sync method again (nothing inside is async).
Example refactor:
```python
class Bookmarks(commands.Cog):
def __init__(…):
…
self.add_emojis = set(CONST.ADD_BOOKMARK)
self.remove_emojis = set(CONST.REMOVE_BOOKMARK)
async def on_raw_reaction_add(self, payload):
emoji = payload.emoji.name
if emoji in self.add_emojis:
user = await self._get_user(payload.user_id) or return
channel = await self._get_channel(payload.channel_id) or return
message = await self._safe_fetch(channel.fetch_message, payload.message_id) or return
embed = self._create_bookmark_embed(message)
await self._send_bookmark(user, message, embed)
elif emoji in self.remove_emojis:
user = await self._get_user(payload.user_id) or return
channel = await self._get_channel(payload.channel_id) or return
message = await self._safe_fetch(channel.fetch_message, payload.message_id) or return
await self._delete_bookmark(message)
# no final else needed
async def _get_user(self, user_id):
user = self.bot.get_user(user_id)
if user: return user
return await self._safe_fetch(self.bot.fetch_user, user_id)
async def _get_channel(self, channel_id):
channel = self.bot.get_channel(channel_id)
if channel: return channel
return await self._safe_fetch(self.bot.fetch_channel, channel_id)
async def _safe_fetch(self, fn, *args):
try:
return await fn(*args)
except discord.NotFound:
logger.error(f"Not found: {fn.__name__}({args[0]})")
except (discord.Forbidden, discord.HTTPException) as e:
logger.error(f"Failed {fn.__name__}: {e}")
def _create_bookmark_embed(self, message: discord.Message) -> discord.Embed:
content = message.content
if len(content) > CONST.EMBED_MAX_DESC_LENGTH:
content = content[:CONST.EMBED_MAX_DESC_LENGTH - 3] + "..."
embed = EmbedCreator.create_embed(
bot=self.bot,
embed_type=EmbedCreator.INFO,
title="Message Bookmarked",
description=f"> {content}",
)
embed.add_field("Author", message.author.name, inline=False)
embed.add_field("Jump to Message", f"[Click Here]({message.jump_url})", inline=False)
if message.attachments:
urls = "\n".join(a.url for a in message.attachments)
embed.add_field("Attachments", urls, inline=False)
return embedThis removes deep nesting, centralizes fetch-and-log, drops the “how’d you get here?” branch, and keeps embed creation synchronous.
| self.bot = bot | ||
|
|
||
| @commands.Cog.listener() | ||
| async def on_raw_reaction_add(self, payload: discord.RawReactionActionEvent) -> None: |
Contributor
There was a problem hiding this comment.
issue (code-quality): Add guard clause (last-if-guard)
Comment on lines
+22
to
+24
| if emoji.name in valid_list: # noqa: SIM103 | ||
| return True | ||
| return False |
Contributor
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Lift code into else after jump in control flow (
reintroduce-else) - Replace if statement with if expression (
assign-if-exp) - Simplify boolean if expression (
boolean-if-exp-identity) - Remove unnecessary casts to int, str, float or bool (
remove-unnecessary-cast)
Suggested change
| if emoji.name in valid_list: # noqa: SIM103 | |
| return True | |
| return False | |
| return emoji.name in valid_list |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #916 +/- ##
=====================================
Coverage 8.96% 8.96%
=====================================
Files 121 121
Lines 10223 10222 -1
Branches 1239 1239
=====================================
Hits 916 916
+ Misses 9235 9234 -1
Partials 72 72
*This pull request uses carry forward flags. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Update the Python version in the CONTRIBUTING.md and DOCKER.md files to 3.13.5. This ensures that the documentation reflects the latest stable version of Python being used in the project, which may include important bug fixes and improvements. Keeping the Python version up-to-date helps maintain compatibility and security across development and production environments.
…and channel fetching
Collaborator
Author
|
git is pissing me off please god make it stop |
Collaborator
Author
|
git made like 70 extra commits ill redo this AGAIn tmr |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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:
Enhancements: