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

Update Invite to match Discord changes #2500

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

My-Name-Is-Jeff
Copy link

@My-Name-Is-Jeff My-Name-Is-Jeff commented Jul 15, 2023

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

Discord made all the previous expanded Invite properties (max_uses, temporary, creation, etc...) except for uses accessible without authentication, so remove the expanded property and deprecate usages

Additionally, update docs to remove the outdated notice that certain fields require expansion

Adds a new method getTimeExpires to return the expires_at field of an invite, which is null if the invite never expires

Add the FRIEND InviteType and a way to parse it

Change EntityBuilder to use the type id to determine invite type

Discord made all the properties here except for uses accessible without authentication
Update docs to remove the expansion requirement
@My-Name-Is-Jeff My-Name-Is-Jeff marked this pull request as draft July 15, 2023 00:53
@My-Name-Is-Jeff My-Name-Is-Jeff changed the title Update Invite to accommodate Discord making only the uses field require expansion Update Invite to match Discord changes Jul 15, 2023
@My-Name-Is-Jeff My-Name-Is-Jeff marked this pull request as ready for review July 15, 2023 02:32
@freya022
Copy link
Contributor

Could you please link the appropriate Discord API docs PR/commit that documents this change?

@My-Name-Is-Jeff
Copy link
Author

My-Name-Is-Jeff commented Jul 15, 2023

Could you please link the appropriate Discord API docs PR/commit that documents this change?

There doesn't seem to be any docs changes, noticed the changes after exceptions were thrown when trying to resolve guild invites

Probably should have opened a GH Issue but thought it was redundant

@MinnDevelopment
Copy link
Member

Looks like there is still a compilation issue here with the constructor usage. Have you tested whether the usage of with_expiration=true makes a difference? It doesn't seem like we use it at all, currently.

@My-Name-Is-Jeff
Copy link
Author

Looks like there is still a compilation issue here with the constructor usage. Have you tested whether the usage of with_expiration=true makes a difference? It doesn't seem like we use it at all, currently.

from what I recall the with_expiration query makes no difference on the response body whether or not it its true or false

@My-Name-Is-Jeff
Copy link
Author

Should compile now, missed an internal usage

@MinnDevelopment MinnDevelopment added the status: freezer this is currently put on hold label Jul 28, 2023
@MinnDevelopment
Copy link
Member

Gonna hold off on merging this, still waiting for some info regarding the API stability here.

@MinnDevelopment
Copy link
Member

I've been told that the changes to expanding invites will need to be reverted. The API will not continue to return the expanded object on the resolve endpoint in the future.

@My-Name-Is-Jeff
Copy link
Author

I've been told that the changes to expanding invites will need to be reverted. The API will not continue to return the expanded object on the resolve endpoint in the future.

Seems like Discord. I'll check after they revert the changes and see if there's anything left.

@My-Name-Is-Jeff
Copy link
Author

Change to max_age field noted here: discord/discord-api-docs#6233

@MinnDevelopment MinnDevelopment removed the status: freezer this is currently put on hold label Aug 14, 2023
@MinnDevelopment MinnDevelopment marked this pull request as draft August 14, 2023 10:16
@MinnDevelopment
Copy link
Member

I've gone ahead and converted this PR into a draft, until it has been adjusted. If you don't want to continue working on it, feel free to close it and open an issue instead.

@My-Name-Is-Jeff My-Name-Is-Jeff marked this pull request as ready for review September 1, 2023 02:20
@@ -67,14 +69,23 @@ public InviteActionImpl deadline(long timestamp)
return (InviteActionImpl) super.deadline(timestamp);
}

@Override
public boolean isRestricted()
Copy link
Member

Choose a reason for hiding this comment

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

The name "restricted" is too vague for what it represents

Co-authored-by: Florian Spieß <business@minn.dev>
@My-Name-Is-Jeff My-Name-Is-Jeff marked this pull request as draft September 15, 2023 23:46
@My-Name-Is-Jeff My-Name-Is-Jeff marked this pull request as ready for review September 24, 2023 21:58
* @throws java.lang.IllegalStateException
* if this is not an expanded invite
* @return The max uses of this invite or {@code 0} if there is no limit
*/
Copy link
Member

Choose a reason for hiding this comment

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

The docs changes here seem unrelated to the rest of this PR, please revert them.

@@ -312,13 +316,12 @@ default String getUrl()
/**
* Whether this Invite grants only temporary access or not.
*
* <p>This works only for expanded invites and will throw a {@link IllegalStateException} otherwise!
* <p>This works only for expanded invites and will throw a {@link IllegalStateException} otherwise!</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* <p>This works only for expanded invites and will throw a {@link IllegalStateException} otherwise!</p>
* <p>This works only for expanded invites and will throw a {@link IllegalStateException} otherwise!

/**
* The maximum age, 30 days, in seconds, for a restricted invite.
*
* <p>A restricted invite is an invite that is created in a non-community server</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* <p>A restricted invite is an invite that is created in a non-community server</p>
* <p>A restricted invite is an invite that is created in a non-community server

*
* @see #MAX_RESTRICTED_AGE
*/
boolean hasCommunityRestrictions();
Copy link
Member

Choose a reason for hiding this comment

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

Missing @return docs

/**
* Sets the max age in seconds for the invite. Set this to {@code 0} if the invite should never expire. Default is {@code 86400} (24 hours).
* {@code null} will reset this to the default value.
*
* <p>Invites in non-community servers are not allowed to be permanent, with an allowed range of {@code 1} (1 second) to {@code 2592000} (30 days).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* <p>Invites in non-community servers are not allowed to be permanent, with an allowed range of {@code 1} (1 second) to {@code 2592000} (30 days).
* <p>Invites in {@link #hasCommunityRestrictions() non-community} servers are not allowed to be permanent, with an allowed range of {@code 1} (1 second) to {@code 2592000} (30 days).

if (nonCommunityRestricted)
{
Checks.check(maxAge != null && maxAge >= 1 && maxAge <= MAX_RESTRICTED_AGE,
"maxAge for invites in non-community guilds must be between 1 second and 30 days!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"maxAge for invites in non-community guilds must be between 1 second and 30 days!");
"The maximum age for invites in non-community guilds must be between 1 second and 30 days!");

Also, the API Docs say 7 days is the maximum, are the docs wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Seems like they haven't merged the API doc changes yet? Not sure if the changes are live

Copy link
Member

Choose a reason for hiding this comment

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

The client also limits it to 7 days:

image

Copy link
Author

Choose a reason for hiding this comment

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

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.

None yet

3 participants