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

Add audit log reason support to Http #1354

Merged
merged 5 commits into from
Jul 2, 2021
Merged

Conversation

kangalio
Copy link
Collaborator

@kangalio kangalio commented May 21, 2021

A second attempt at #296.

This PR only adds audit log reason support to the Http client. Model methods like Member::unban or GuildChannel::edit still cannot be provided with an audit log reason. This was done because:

  • it's unclear how the API for audit log reasons in the model type methods should be designed as to not be a hassle to 90% of use cases where no audit log reason is provided
    • add a Option<&str> parameter to all the methods? That's a big breaking change and requires users to litter None function arguments all over the place
  • adding support to the Http client is the most important thing to open the door for audit log reason support; model types are just extra convenience that can be added later

I tested this by making a small test bot that invokes roughly half of all modified Http functions. The audit log in the server settings shows the correct reason strings. I couldn't test the other half of Http functions because I couldn't really figure out how to invoke them easily.

Unfortunately, Discord apparently doesn't properly document which HTTP routes support an audit log reason and which don't. I was using this table as a guideline, but probably there are some API routes where I falsely added or falsely omitted audit log support.

@@ -508,7 +513,7 @@ impl Member {
/// [Ban Members]: Permissions::BAN_MEMBERS
#[inline]
pub async fn unban(&self, http: impl AsRef<Http>) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you add support to the reason message here (and in others structures above)? Is it in WIP?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This PR only adds audit log support to the Http client for simplicity. I will expand the explanation for this in the PR description, hopefully that will clear it up

@arqunis arqunis added enhancement An improvement to Serenity. http Related to the `http` module. model Related to the `model` module. labels May 22, 2021
@arqunis arqunis force-pushed the next branch 3 times, most recently from 7a56bbd to d1d436e Compare June 9, 2021 20:22
@kangalio kangalio force-pushed the audit-log branch 2 times, most recently from eda97c9 to 80fd30f Compare June 9, 2021 20:29
Licenser and others added 4 commits June 9, 2021 22:37
This adds support for parsing json with the `simd-json` crate for increased performance.
However, it must be enabled with the `simd-json` feature flag, as not all processors support
all SIMD instructions. In addition, the minimum-supported Rust version has been increased to 1.49.
Mainly the delete_XX methods because those apparently don't support audit log reasons (undocumented). And also in ban, because the ban reason overwrites any audit log reason
@HarmoGlace
Copy link
Contributor

Can you resolve conflicts? Otherwise lgtm

@arqunis arqunis merged commit 52bee05 into serenity-rs:next Jul 2, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jul 2, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jul 2, 2021
HarmoGlace pushed a commit to HarmoGlace/serenity that referenced this pull request Jul 4, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jul 11, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Aug 9, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Sep 8, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Sep 25, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Sep 30, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 29, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Nov 15, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Nov 15, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Dec 22, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Dec 23, 2021
arqunis pushed a commit to arqunis/serenity that referenced this pull request Jan 23, 2022
arqunis pushed a commit to arqunis/serenity that referenced this pull request Mar 1, 2022
arqunis pushed a commit to arqunis/serenity that referenced this pull request Mar 15, 2022
arqunis pushed a commit to arqunis/serenity that referenced this pull request Apr 2, 2022
arqunis pushed a commit to arqunis/serenity that referenced this pull request Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An improvement to Serenity. http Related to the `http` module. model Related to the `model` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants