Skip to content

Unpin all messages when help channel goes dormant#1936

Merged
ChrisLovering merged 7 commits into
mainfrom
feature/1903/fix-pin-inconsistency
Jun 7, 2022
Merged

Unpin all messages when help channel goes dormant#1936
ChrisLovering merged 7 commits into
mainfrom
feature/1903/fix-pin-inconsistency

Conversation

@dementati
Copy link
Copy Markdown
Contributor

@dementati dementati commented Nov 5, 2021

As described in #1903, unpinning a help channel question will occasionally fail when a channel goes dormant. This issue was previously fixed in #1909 by querying and unpinning all pinned messages in the help channel when it moves from dormant to available. This likely fixes the issue, but the current unpinning solution is still unnecessarily complex.

Unpinning the question when the channel goes dormant is currently solved by storing the question message ID when the channel is first claimed and retrieving it from there to be unpinned at the appropriate time. An alternative and simpler solution, implemented in this PR, is to query all pinned messages and unpin those found, like what happens when the channel becomes available. This allows us to get rid of the message cache with no known disadvantages.

@dementati dementati marked this pull request as ready for review November 5, 2021 14:00
@MarkKoz
Copy link
Copy Markdown
Contributor

MarkKoz commented Nov 5, 2021

If they're unpinned when the channel becomes available, then why even bother unpinning when it goes dormant?

This allows us to get rid of the message cache with no known disadvantages.

Getting pinned messages requires an API call to Discord. With the cache, this is avoided. That is basically the only advantage. That being said, this sort of optimisation is not necessary; it was nice to have but it's not worth keeping if it's causing consistency issues.

@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: help channels Related to the help channel system p: 2 - normal Normal Priority s: needs review Author is waiting for someone to review and approve t: enhancement Changes or improvements to existing features labels Nov 7, 2021
@dementati
Copy link
Copy Markdown
Contributor Author

If they're unpinned when the channel becomes available, then why even bother unpinning when it goes dormant?

Apparently, it's the API itself that bugs out occasionally, and making two passes will decrease the likelihood that there are pins remaining by the time the channel becomes available.

Getting pinned messages requires an API call to Discord. With the cache, this is avoided. That is basically the only advantage.

That is true.

it was nice to have but it's not worth keeping if it's causing consistency issues.

As stated above, it's allegedly the API and not the cache that caused the issue. I just prefer the solution in this PR because it reduces code complexity.

@dementati dementati mentioned this pull request Nov 7, 2021
@Xithrius Xithrius requested a review from Bluenix2 November 26, 2021 23:01
Copy link
Copy Markdown
Contributor

@Bluenix2 Bluenix2 left a comment

Choose a reason for hiding this comment

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

Perfect, works much better than the previous behaviour! Another bug squashed..
image

Comment thread bot/exts/help_channels/_message.py Outdated
Comment thread bot/exts/help_channels/_message.py Outdated
Comment thread bot/exts/help_channels/_message.py Outdated
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

This will allow a discord.Message to be passed to pin_wrapper instead of a message ID. Thus, the wrapper will be able to call the discord.Message.pin and discord.Message.unpin functions rather than using the undocumented methods on HTTPClient. Would you mind updating the wrapper function too @dementati?

@ChrisLovering ChrisLovering added s: waiting for author Waiting for author to address a review or respond to a comment and removed s: needs review Author is waiting for someone to review and approve labels Apr 3, 2022
@Xithrius
Copy link
Copy Markdown
Contributor

@dementati What's your status on this PR?

@MarkKoz MarkKoz self-assigned this Jun 7, 2022
@MarkKoz MarkKoz removed s: waiting for author Waiting for author to address a review or respond to a comment a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) labels Jun 7, 2022
@ChrisLovering ChrisLovering merged commit 5ae61fc into main Jun 7, 2022
@ChrisLovering ChrisLovering deleted the feature/1903/fix-pin-inconsistency branch June 7, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: help channels Related to the help channel system p: 2 - normal Normal Priority t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants