Skip to content

Added command&system to purge all messages up to given message#1055

Merged
MarkKoz merged 4 commits into
masterfrom
Senjan/clean_message
Jul 20, 2020
Merged

Added command&system to purge all messages up to given message#1055
MarkKoz merged 4 commits into
masterfrom
Senjan/clean_message

Conversation

@Senjan21
Copy link
Copy Markdown
Contributor

This PR aims to close issue #1024 by implementing purging system that purges messages up until certain message specified by moderator.
The command input supports:

  • Message ID (Will only work if message is in the channel in which command was invoked)
  • ChannelID-MessageID (obtainable via shift-clicking on “Copy ID”)
  • jump url to the message

However, the system of deletion is completely different from the original one, due to the fact that TextChannel.purge wouldn't otherwise support that system well enough.
I'd like to hear opinions on the PR.

@Senjan21 Senjan21 requested a review from a team as a code owner July 15, 2020 16:38
@Senjan21 Senjan21 requested review from jb3 and tagptroll1 and removed request for a team July 15, 2020 16:38
@ghost ghost added the needs 2 approvals label Jul 15, 2020
@MarkKoz MarkKoz added a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 2 - normal Normal Priority t: feature New feature or request labels Jul 15, 2020
Comment thread bot/cogs/clean.py
# thus we need to paginate the amount to always be <= 100
await channel.delete_messages(messages[i:i + 100])
else:
messages += await channel.purge(limit=amount, check=predicate)
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.

Since you've essentially written all the code needed, can we get rid of purge? It redundantly iterates message history. We already have message objects from our own history search, so let's use those. That being said, one strange thing purge does is use a single delete strategy if messages are older than 2 weeks. I don't know if that's important.

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.

Reason why it does single deletes is because you can't delete messages older than 14 days via bulk delete endpoint, I didn't include that edge case in the code as (at least to me) it appears unlikely for moderators to attempt and purge messages older than that.
That being said attempts of deleting the messages older than 14 days via delete_messages will raise following error:

discord.errors.HTTPException: 400 Bad Request (error code: 50034): You can only bulk delete messages that are under 14 days old.

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.

Do you think we can do away with purge though?

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.

I don't think purge is necessary considering we iterate the history either way, I am all up to rewriting current system so it doesn't use purge in separate PR as it isn't exactly the point of this PR

Comment thread bot/cogs/clean.py Outdated
Comment on lines +141 to +144
if message.created_at <= until_message.created_at:
# means we have found the message until which we were supposed to be deleting.
message_ids.append(message.id)
break
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.

If the current message is older than until_message, then the current message should not be appended and deleted, as it's too far back. Something like this, but maybe you can come up with something more elegant:

if message.created_at < until_message.created_at:
    break

messages.append(message)

if message.id == until_message.id:
    message_ids.append(message.id)
    break

@ghost ghost added s: waiting for author Waiting for author to address a review or respond to a comment and removed needs 2 approvals labels Jul 15, 2020
@ghost ghost added needs 2 approvals and removed s: waiting for author Waiting for author to address a review or respond to a comment labels Jul 16, 2020
Copy link
Copy Markdown
Contributor

@tagptroll1 tagptroll1 left a comment

Choose a reason for hiding this comment

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

Lgtm

@ghost ghost removed the needs 1 approval label Jul 20, 2020
@MarkKoz MarkKoz merged commit fb6be07 into master Jul 20, 2020
@MarkKoz MarkKoz deleted the Senjan/clean_message branch July 20, 2020 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 2 - normal Normal Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants