-
-
Notifications
You must be signed in to change notification settings - Fork 734
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
Allow null user_id and provide partial Webhooks #675
Conversation
added webhooks to AuditLogEntry
@@ -27,6 +27,8 @@ | |||
import java.util.Collections; | |||
import java.util.List; | |||
import java.util.Map; | |||
import net.dv8tion.jda.core.entities.Webhook; | |||
import net.dv8tion.jda.core.entities.impl.WebhookImpl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these imports should be sorted properly.
|
||
return new WebhookImpl(channel, id) | ||
.setToken(token) | ||
.setOwner(owner==null ? null : channel.getGuild().getMember(owner)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: give the operators such as ==
some space
@@ -1375,7 +1383,8 @@ public AuditLogEntry createAuditLogEntry(GuildImpl guild, JSONObject entryJson, | |||
final JSONObject options = entryJson.isNull("options") ? null : entryJson.getJSONObject("options"); | |||
final String reason = entryJson.optString("reason", null); | |||
|
|||
final UserImpl user = (UserImpl) createFakeUser(userJson, false); | |||
final UserImpl user = userJson == null ? null : (UserImpl) createFakeUser(userJson, false); | |||
final WebhookImpl webhook = webhookJson == null ? null : (WebhookImpl) createWebhook(webhookJson); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since these two methods always return the specific implementation it would make more sense to just change the return type rather than doing a typecast
@@ -155,6 +155,9 @@ public WebhookManager getManager() | |||
@Override | |||
public WebhookClientBuilder newClient() | |||
{ | |||
if (token == null) | |||
throw new IllegalStateException("Webhooks without known tokens (such as those retrieved from Audit Logs) " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add this to the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getManager()
should fail in the same way.
Edit: Maybe it makes sense to use the route without token? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should always throw an exception for a fake webhook for this method. I would suggest a WebhookClientBuilder#newClient(String token)
but that would be confusing for non-fake webhooks.
@@ -108,7 +122,7 @@ public Guild getGuild() | |||
* The {@link net.dv8tion.jda.core.entities.User User} responsible | |||
* for this action. | |||
* | |||
* @return The User instance | |||
* @return Possibly-null User instance | |||
*/ | |||
public User getUser() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add an @Nullable
to these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addition of the @Nullable
annotation breaks pre-existing kotlin code that assumes not-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad code to assume things it shouldn't be assuming.
modified some return types added documentation for Webhook#newClient()
added more docs modified getURL() method to account for lack of token
@@ -57,11 +58,12 @@ | |||
TextChannel getChannel(); | |||
|
|||
/** | |||
* The owner of this Webhook. | |||
* The owner of this Webhook. This will be null for partial Webhooks, such as those retrieved from Audit Logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mark Webhook
as IFakable
for this scenario. Fake webhooks would not be able to use newClient()
or getManager()
.
@@ -155,6 +155,9 @@ public WebhookManager getManager() | |||
@Override | |||
public WebhookClientBuilder newClient() | |||
{ | |||
if (token == null) | |||
throw new IllegalStateException("Webhooks without known tokens (such as those retrieved from Audit Logs) " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getManager()
should fail in the same way.
Edit: Maybe it makes sense to use the route without token? What do you think?
Pull Request Etiquette
There are several guidelines you should follow in order for your
Pull Request to be merged.
Description
This PR fixes an issue where JDA required the user_id field in audit log entries to be non-null, despite Discord allowing null values for items such as webhook self-deletion. This PR also adds a getWebhook() method to AuditLogEntry to get a partial Webhook object that are the target of some Audit Log Entries. This fulfills #662.