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 an optional X-Audit-Log-Reason header to auditable actions #296

Closed
maddin77 opened this issue Mar 21, 2018 · 6 comments
Closed

Add an optional X-Audit-Log-Reason header to auditable actions #296

maddin77 opened this issue Mar 21, 2018 · 6 comments
Labels
discord feature Related to Discord's functionality. enhancement An improvement to Serenity.

Comments

@maddin77
Copy link

No description provided.

@arqunis arqunis added enhancement An improvement to Serenity. discord feature Related to Discord's functionality. labels Apr 21, 2018
@xSke
Copy link
Contributor

xSke commented Mar 1, 2019

I can tackle this. Been meaning to contribute, and it seems like low-hanging fruit.

My plan is to add a Option<&str> parameter to every method in http/raw.rs executing an auditable action. This will create major breaking changes to users of that module. The same goes for the models module. Alternatively, I can make two variants of every method, one named x_with_reason taking an Option<&str>, the other staying the same signature and delegating to the reason-taking function with None. Which variant would be preferred?

@Erk-
Copy link
Member

Erk- commented Mar 1, 2019

If you did the latter version it could be added to the current branch without breaking changes, then we can look at how we should structure it on the v0.6.x branch afterwards.

@LeSeulArtichaut
Copy link
Contributor

Because it has been left for long, I'd like to work on this.
I'll be adding with_reason methods like suggested by @xSke to be compatible with the current version of Serenity. However, to support UTF-8 reasons, I'd need to do percent-encoding. Is it ok to have an additional dependency or should I write my own implementation for this?

@Erk-
Copy link
Member

Erk- commented Dec 2, 2019

I would say that it is better to add a single dependency, https://crates.io/crates/percent-encoding, (which does not depend on any other crate) than to roll it ourselves.

@LeSeulArtichaut
Copy link
Contributor

I think I'll give up the idea of having two variants of the same method. There already are a lot of them, and I think having two methods for 30 routes or so, just in the raw Http struct, is a lot, but there are much more methods from all the model structs.

@kangalio
Copy link
Collaborator

kangalio commented Jan 2, 2022

Does this issue still need to be open?

@arqunis arqunis closed this as completed May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discord feature Related to Discord's functionality. enhancement An improvement to Serenity.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants