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(Guild): add api methods for membership screening #5144

Closed
wants to merge 13 commits into from

Conversation

ckohen
Copy link
Member

@ckohen ckohen commented Dec 22, 2020

Please describe the changes this PR makes and why it should be merged:
Ref: discord/discord-api-docs#2396
A similar PR is coming to do the same with the Guild Welcome Screen

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
Comment on lines 1519 to 1531
let formFields = [];
for (const field of res.form_fields) {
formFields.push({
fieldType: field.field_type,
label: field.label,
values: field.values,
required: field.required,
});
}
return {
enabled: this.membershipScreeningEnabled,
description: res.description,
formFields: formFields,
Copy link
Contributor

Choose a reason for hiding this comment

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

seems this code is duplicated, in any case the same as before this can use the map function

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed that there was a few instances of this necessary, would it be desirable to have a private function that transforms this data and call that in each location instead?

typings/index.d.ts Outdated Show resolved Hide resolved
src/structures/Guild.js Show resolved Hide resolved
@tipakA tipakA mentioned this pull request Dec 26, 2020
5 tasks
src/structures/Guild.js Outdated Show resolved Hide resolved
src/structures/Guild.js Show resolved Hide resolved
@almostSouji
Copy link
Member

update: upstream discord/discord-api-docs#2396 has been merged and finalized

src/structures/Guild.js Outdated Show resolved Hide resolved
src/util/Constants.js Outdated Show resolved Hide resolved
src/structures/Guild.js Outdated Show resolved Hide resolved
* remove utility method
* clarify types
* add error message
* better example
- advatih
if (!this.features.includes('COMMUNITY')) {
throw new Error('COMMUNITY');
}
const data = await this.client.api.guilds(this.id)['member-verification'].get();
Copy link
Member

Choose a reason for hiding this comment

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

Discord responds with a 204 if membership screening is not yet set up.
(data will be an empty buffer then, causing an error to be thrown when trying to map the undefined form_fields)

Copy link
Member Author

Choose a reason for hiding this comment

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

This may be a bug with how the API is currently handling this, see discord/discord-api-docs#2530
A check is still a good idea though, will add.

@@ -1422,6 +1470,45 @@ class Guild extends Base {
.then(() => this);
}

/**
* A `Partial` object is a representation of any existing object.
* This object contains between 1 and all of the original objects parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Since you actually can edit with an empty object.

Suggested change
* This object contains between 1 and all of the original objects parameters.
* This object contains between 0 and all of the original objects parameters.

@ckohen
Copy link
Member Author

ckohen commented Jan 29, 2021

Looks like the associated API methods for this are being changed, I'll draft this PR until we get the updated ones.
Ref: discord/discord-api-docs#2547

@ckohen ckohen marked this pull request as draft January 29, 2021 21:51
@ckohen ckohen mentioned this pull request Mar 19, 2021
5 tasks
@iCrawl
Copy link
Member

iCrawl commented Apr 30, 2021

If this is still wanted (upstream PR has been merged) this should be undrafted and rebased.

@advaith1
Copy link
Contributor

advaith1 commented Apr 30, 2021

@iCrawl the docs were removed pending breaking changes (see previous comment), and Discord hasn't given any information since then

@iCrawl
Copy link
Member

iCrawl commented Apr 30, 2021

Right, I didn't check, I assumed that would be the "new" PR. Thats fine then.

if (!this.features.includes('COMMUNITY')) {
throw new Error('COMMUNITY');
}
const data = await this.client.api.guilds(this.id)['member-verification'].get();
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
const data = await this.client.api.guilds(this.id)['member-verification'].get();
const data = await this.client.api.guilds(this.id, 'member-verification').get();

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll address this when I can resume work on this PR.

@iCrawl iCrawl modified the milestone: Version 13 Jun 27, 2021
@ckohen ckohen closed this Jul 4, 2021
@ckohen ckohen deleted the guild-membership-screening branch July 4, 2021 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants