Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(GuildChannel): Fix manageable method for voice-channels #6447

Merged
merged 5 commits into from Aug 29, 2021

Conversation

DraftProducts
Copy link
Contributor

@DraftProducts DraftProducts commented Aug 15, 2021

Please describe the changes this PR makes and why it should be merged:

Resolves #6446

<GuildChannel>.manageable is not efficient with voice channels because <GuildChannel>.viewable is not checked on voice channels.

if (VoiceBasedChannelTypes.includes(this.type)) { 
   // connect permission check
 } else if (!this.viewable) { 
   return false;
 }

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating

@DraftProducts DraftProducts changed the title Fix manageable method for voice-channels fix(GuildChannel): Fix manageable method for voice-channels Aug 15, 2021
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
src/structures/GuildChannel.js Outdated Show resolved Hide resolved
Co-authored-by: Sugden <28943913+NotSugden@users.noreply.github.com>
@ckohen
Copy link
Member

ckohen commented Aug 15, 2021

This is incorrect, you can manage channels without permission to view them, the error in the original issue is caused because the bot did not have permission to view the channel, and thus could not change other roles permission on it.

@DraftProducts
Copy link
Contributor Author

So you can manage voice-channel without view permission but for text channels, it is needed?

@ckohen
Copy link
Member

ckohen commented Aug 15, 2021

Correct, I should have specified voice, but the permission bit to manage the channel is actually CONNECT on voice channels, regardless of the VIEW_CHANNEL bit. The error actually explains it better, you get Missing Access when you can't change overwrites on the channel, and Missing Permissions when you can't change one of the permissions you are trying to change

Copy link
Member

@kyranet kyranet left a comment

Choose a reason for hiding this comment

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

Can't we just do this?

// Owner always can manage:
if (this.client.user.id === this.guild.ownerId) return true;

// Get the permissions, if we couldn't resolve to a member, return false:
const permissions = this.permissionsFor(this.client.user);
if (!permissions) return false;

// Get the bitfield and see if we fulfill it:
const bitfield = VoiceBasedChannelTypes.includes(this.type)
  ? Permissions.FLAGS.VIEW_CHANNEL | Permissions.FLAGS.MANAGE_CHANNELS | Permissions.FLAGS.CONNECT
  : Permissions.FLAGS.VIEW_CHANNEL | Permissions.FLAGS.MANAGE_CHANNELS;
return permissions.has(bitfield, false);

It's far more efficient, resolves MemberResolvable and creates a Permissions instance only once (as opposed to twice).

@iCrawl
Copy link
Member

iCrawl commented Aug 25, 2021

Are we moving forward with kyras suggestion here, or whats the plan @DraftProducts

@DraftProducts
Copy link
Contributor Author

We can go with Kyras suggestion 👍

Co-authored-by: Kyra <kyranet@users.noreply.github.com>
Co-authored-by: ckohen <chaikohen@gmail.com>
@iCrawl iCrawl merged commit 9301c9b into discordjs:main Aug 29, 2021
@DraftProducts DraftProducts deleted the fix/manageable-voice-channels branch January 24, 2022 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Permissions on channel update after all needed tests
7 participants