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

feat: Add modify function to the GuildMemberRoleManager for adding/removing roles at once. #10355

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

scottbucher
Copy link
Contributor

@scottbucher scottbucher commented Jun 18, 2024

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

The GuildMemberRoleManager class has an add & remove function which, when given multiple roles, use the GuildMemberManager.edit function which utilizes Modify Guild Member endpoint allowing discord.js to add/remove multiple roles in a single api call. However, there is no function for adding and removing multiple roles at once, this PR adds one.

Additionally, the add/remove functions in the GuildMemberRoleManager both used the same logic for resolving a list of roles. This PR moves that logic into a shared function both can use, as well as the new modify function.

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating
  • This PR changes the library's interface (methods or parameters added)

@scottbucher scottbucher requested a review from a team as a code owner June 18, 2024 23:17
Copy link

vercel bot commented Jun 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Jun 21, 2024 11:52pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Jun 21, 2024 11:52pm

@scottbucher scottbucher changed the title Add modify function to the GuildMemberRoleManager for adding/removing roles at once. feat: Add modify function to the GuildMemberRoleManager for adding/removing roles at once. Jun 18, 2024
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
packages/discord.js/src/managers/GuildMemberRoleManager.js Outdated Show resolved Hide resolved
packages/discord.js/src/managers/GuildMemberRoleManager.js Outdated Show resolved Hide resolved
@almostSouji
Copy link
Member

However, there is no function for adding and removing multiple roles at once

Yes, there is: https://discord.js.org/docs/packages/discord.js/14.15.3/GuildMemberRoleManager:Class#set
You supply the set of roles you want to be on the member after the operation.

Expanding on this further for specific use cases in user land is simple enough to do.

@scottbucher
Copy link
Contributor Author

However, there is no function for adding and removing multiple roles at once

Yes, there is: https://discord.js.org/docs/packages/discord.js/14.15.3/GuildMemberRoleManager:Class#set You supply the set of roles you want to be on the member after the operation.

Expanding on this further for specific use cases in user land is simple enough to do.

Yea I did see this, and I had built it for my use case, it just felt like something that should and could easily be built into discordjs

@@ -4393,6 +4393,12 @@ export class GuildStickerManager extends CachedManager<Snowflake, Sticker, Stick
public fetchUser(sticker: StickerResolvable): Promise<User | null>;
}

export interface ModifyGuildMemberRolesOptions {
Copy link
Contributor Author

@scottbucher scottbucher Jun 19, 2024

Choose a reason for hiding this comment

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

Is this the right place to put this? What is the preference for ordering in the this typings file?

packages/discord.js/typings/index.d.ts Outdated Show resolved Hide resolved
/**
* Resolves roles from the input.
* @param {Readonly<RoleResolvable[]> | ReadonlyCollection<Snowflake, Role>} rolesToResolve The roles to resolve
* @returns {Role[]} The resolved roles
Copy link
Contributor

@NotSugden NotSugden Jun 30, 2024

Choose a reason for hiding this comment

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

this is not what is returned by this function.

an array of role ids is returned.

as this is only used to resolve role ids and is private we should keep it as returning role ids as makes more sense. This will require a small change to the modify function as well.

@@ -4408,6 +4414,7 @@ export class GuildMemberRoleManager extends DataManager<Snowflake, Role, RoleRes
roleOrRoles: RoleResolvable | readonly RoleResolvable[] | ReadonlyCollection<Snowflake, Role>,
reason?: string,
): Promise<GuildMember>;
public modify(options: ModifyGuildMemberRolesOptions): GuildMember;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public modify(options: ModifyGuildMemberRolesOptions): GuildMember;
public modify(options: ModifyGuildMemberRolesOptions): Promise<GuildMember>;

/**
* Modifies the roles of the member.
* @param {ModifyGuildMemberRolesOptions} [options] Options for modifying the roles
* @returns {GuildMember}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {GuildMember}
* @returns {Promise<GuildMember>}

* @property {Readonly<RoleResolvable[]> | ReadonlyCollection<Snowflake, Role>} [rolesToAdd] The roles to add
* @property {Readonly<RoleResolvable[]> | ReadonlyCollection<Snowflake, Role>} [rolesToRemove] The roles to remove
* @property {Readonly<RoleResolvable[]> | ReadonlyCollection<Snowflake, Role>} [reason] Reason for modifying
* the roles
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a typedef for ReadonlyCollection and Readonly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in Progress
Development

Successfully merging this pull request may close these issues.

6 participants