Skip to content

Handle off-topic channel names that are invalid for servers in Server Discovery#2520

Closed
TizzySaurus wants to merge 17 commits into
mainfrom
fix-2500
Closed

Handle off-topic channel names that are invalid for servers in Server Discovery#2520
TizzySaurus wants to merge 17 commits into
mainfrom
fix-2500

Conversation

@TizzySaurus
Copy link
Copy Markdown
Contributor

@TizzySaurus TizzySaurus commented Apr 7, 2023

Related Issues

Closes #2282
Closes #2500

What

This PR adds re-roll logic for when attempting to rename an off-topic channel to a name that's not allowed for servers in Server Discovery (such as ot0-welcome-and-fuck-you).

It will attempt to reroll a maximum of MAX_RENAME_ATTEMPTS per off-topic channel, and will send a message detailing any failed renames for a given off-topic channel to the internal mod-meta channel.

The code also deactivates names that are invalid, triggering a log.info, and adds logic for handling when the name pool isn't big enough (which triggers a log.warn).

Testing

As for testing, I found that the following works:

class Temp:
    def __init__(self, status, reason):
        self.status = status
        self.reason = reason

if (not channel_indx) or attempt < channel_indx:  # change as appropriate
    raise HTTPException(Temp("status", "reason"), {"code": 50035})
await channel.edit(name=name_after)

...and produces the following example message in the mod-meta channel:
Screenshot 2023-08-28 at 22 18 55
To forcefully run the coroutine you can use the int eval command, with the following code:

# Reload Cog
await bot.reload_extension("bot.exts.fun.off_topic_names")

# Activate any deactivated off-topic names
names = await self.bot.api_client.get("bot/off-topic-channel-names", params={"active": "false"})
for name in names:
    await self.bot.api_client.patch(f"bot/off-topic-channel-names/{name}", data={"active": True})
    print(f"Activated otn name {name}")

# Call `update_names()` coroutine
cog = bot.get_cog("OffTopicNames")
await cog.update_names()

@TizzySaurus TizzySaurus added t: bug Something isn't working a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) p: 2 - normal Normal Priority a: fun Related to non-serious, "fun" features (duck pond, off topic channel names) s: needs review Author is waiting for someone to review and approve labels Apr 7, 2023
@TizzySaurus TizzySaurus requested a review from ks129 as a code owner April 7, 2023 17:22
@Xithrius Xithrius requested review from RohanJnr and Xithrius May 2, 2023 02:01
Copy link
Copy Markdown
Contributor

@RohanJnr RohanJnr left a comment

Choose a reason for hiding this comment

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

Tested and works fine. Gj

Comment thread bot/exts/fun/off_topic_names.py Outdated
@RohanJnr
Copy link
Copy Markdown
Contributor

RohanJnr commented May 7, 2023

Also, the reroll command does not work if the channel name is not found in the database. This can be a super rare edge case but we can handle it. Under the re_roll_command function, you can catch the Exception on the await self.de_activate_ot_name(ctx, old_ot_name) and log it. After that the function will continue to rename the channel.

@TizzySaurus
Copy link
Copy Markdown
Contributor Author

Also, the reroll command does not work if the channel name is not found in the database. This can be a super rare edge case but we can handle it. Under the re_roll_command function, you can catch the Exception on the await self.de_activate_ot_name(ctx, old_ot_name) and log it. After that the function will continue to rename the channel.

That seems unrelated to this work, so should probably go in a separate PR?

@TizzySaurus TizzySaurus requested a review from RohanJnr May 7, 2023 20:53
@RohanJnr
Copy link
Copy Markdown
Contributor

RohanJnr commented May 8, 2023

separate

Ye sure, I'll open an issue for that later

Comment thread bot/exts/fun/off_topic_names.py Outdated
@TizzySaurus TizzySaurus requested a review from RohanJnr May 9, 2023 19:54
Comment thread bot/exts/fun/off_topic_names.py Outdated
Comment thread bot/exts/fun/off_topic_names.py Outdated
new_channel_names: list = await self.bot.api_client.get(
"bot/off-topic-channel-names", params={"random_items": num_names_to_fetch}
)
if (num_fetched := len(new_channel_names)) < num_names_to_fetch:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IS this even possible ? We have a ton of channel names and we only rename 3.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is strictly possible, however unlikely, so figured it should be accounted for - especially since the handling is relatively simple.

Copy link
Copy Markdown
Contributor Author

@TizzySaurus TizzySaurus Aug 27, 2023

Choose a reason for hiding this comment

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

The handle for this has changed with the rewrite, but is still there here. Also figured it made more sense to send a message to mod_meta than a log.warn, so updated to reflect that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with keeping the handle since it could save some of my brain cells when, say, I'm running my own instance of the bot in a test server.

Comment thread bot/exts/fun/off_topic_names.py
Comment thread bot/exts/fun/off_topic_names.py Outdated
failed_renames = defaultdict(list)
successfully_renamed = set()
for attempt in range(MAX_RENAME_ATTEMPTS):
# Get the necessary amount of new channel names
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think that all these comments are necessary, comments should exist to document things that don't jump straight to the eye.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's what I'm saying, that the code can be understood by simply reading it, as the logic is kinda straightforward.

I personally don't think it's complex, we'll see what others have to say about it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in hindsight I agree -- these comments were removed as part of the rewrite.

Comment thread bot/exts/fun/off_topic_names.py Outdated
channels = [await get_or_fetch_channel(channel_id) for channel_id in CHANNELS]
failed_renames = defaultdict(list)
successfully_renamed = set()
for attempt in range(MAX_RENAME_ATTEMPTS):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the approach can be enhanced a bit.
We can fetch 3 names (number of attempts) for each channel.
We iterate over each channel, and try to rename it three times, then exit, that way we can increase the chance of a successful rename OP.

number_of_names_to_fetch = MAX_ATTEMPS * len(CHANNELS)
channels = [get_or_fetch_channel(channel) for channel in CHANNELS]
names = iter(api_client.fetch_names(params={"random_items": number_of_names_to_fetch}))

renamed = set()
failed = set()

for channel in channels:
    attempt = 0
    while attempt < MAX_ATTEMPS:
        attempt += 1
        try:
            old_name = channel.name
            name = next(names)
            channel.update(name=name)
            renamed.add(channel.id)
            break
        except:
            # Catch your code here & deactivate the name
            # Reraise if not the right exception
            failed.add(f"- {name}\n") # Leverage the new markdown support to dsplay as a bullet list
            ...

# Simplify the handling of the failed renames.

if failed:
    unchanged = [channel.name for channel in channels if channel.id not in renamed]
    message = f"Couldn't rename the following off-topic channels: {', '.join(unchanged)}" \
              f"The following off-topic names were deactivated:\n{failed}", 

Copy link
Copy Markdown
Contributor Author

@TizzySaurus TizzySaurus Aug 27, 2023

Choose a reason for hiding this comment

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

Rewritten in 2f738ad (have made changes since, so see the "Files Changed" for modern version of the logic).

Currently untested, but lmk what you think :)

@TizzySaurus TizzySaurus requested a review from shtlrs August 27, 2023 20:10
Comment thread bot/exts/fun/off_topic_names.py Outdated
@TizzySaurus TizzySaurus requested a review from shtlrs August 27, 2023 21:49
@Xithrius Xithrius added up for grabs Available for anyone to work on and removed s: needs review Author is waiting for someone to review and approve up for grabs Available for anyone to work on labels Jan 28, 2024
@Xithrius
Copy link
Copy Markdown
Contributor

Closing in favor of recreating within a new PR.

@Xithrius Xithrius closed this Jan 28, 2024
@ChrisLovering ChrisLovering deleted the fix-2500 branch October 31, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: fun Related to non-serious, "fun" features (duck pond, off topic channel names) p: 2 - normal Normal Priority t: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle failure to edit OT channel names Handle Discord's rejection of off-topic channel names due to disallowed words

5 participants