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.

Sign up for GitHub

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

Jump to bottom

Add new infraction filters for the infraction rescheduler #510

Merged
merged 8 commits into from
Jun 4, 2021
Merged

Add new infraction filters for the infraction rescheduler #510

merged 8 commits into from
Jun 4, 2021

Conversation

bast0006
Copy link
Contributor

Add permanent, types, expires_before, and expires_before as filter options for the bot/infractions endpoint. This let's us filter for only in-flight infractions to expire, and also lets us put limits on how far in the future we reschedule infractions if we desire to.

@bast0006 bast0006 requested a review from MarkKoz as a code owner May 22, 2021 20:20
This was referenced May 22, 2021
Merged
Closed
@bast0006
Copy link
Contributor Author

Ok. I'll add tests for those validation failures.

@coveralls
Copy link

coveralls commented May 22, 2021 •
edited
Loading

Coverage Status

Coverage remained the same at 100.0% when pulling b076394 on bast0006:bast0006-new-infraction-filters into 03c787b on python-discord:main.

@bast0006 bast0006 added area: API Related to or causes API changes language: python Involves Python code python Used by dependabot type: feature New feature or request labels May 23, 2021
kosayoda
kosayoda reviewed May 27, 2021
View reviewed changes
Copy link
Member

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

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

From an initial glance, should we have to worry about both the type and types parameters being used together?

@bast0006
Copy link
Contributor Author

bast0006 commented May 28, 2021 •
edited
Loading

From an initial glance, should we have to worry about both the type and types parameters being used together?

I'm not sure, but I don't think so. At the moment, I'm 99% sure doing so behaves mostly as someone would expect. If you specify both type and types, and type is in types, then it will return only type results. If you specify a type that is not in your specified types you will get no results.

We could add an error condition for mixing of parameters like that, but if we do, we might want to do so for the expires_before and expires_after to ensure that they don't overlay badly?

@kosayoda
Copy link
Member

I would personally prefer strict parameter validation, although I'm not sure about others. It would go something like:

  • type and types should be mutually exclusive (or maybe just have one parameter for filtering by a type and multiple types?)
  • permanent and expires_* should be mutually exclusive
  • expires_after should be before expires_before

@bast0006
Raise ValidationError for new bot/infractions filter if the types are…
b9f2589 
… invalid

If the before time is after the after time, or if both `type` and `types` are specified
@bast0006 bast0006 force-pushed the bast0006-new-infraction-filters branch from e651d96 to b9f2589 Compare May 30, 2021 22:42
@bast0006
Fix bot/infractions after and before filter check being inverted
45dfba5 
The check was backwards, enforcing that no results must ever be returned
Relevant tests have been added that actually fetch an infraction with the right times so that I'm sure it's not still broken
@bast0006
Ensure bot/infractions does not accept both expires and permanent fil…
7a5a032 
…ters

Expires and permanent=false are permitted and tested for. Expires_before also filters the database for permanent=false explicitly
@bast0006
Copy link
Contributor Author

Since types and type do a different query (IN vs =) I figured I'd leave them and make them exclusive to each other.

Otherwise, permanent and expires_ are now exclusive and expires_before must now be after expires_after.

kosayoda
kosayoda approved these changes Jun 3, 2021
View reviewed changes
Copy link
Member

@kosayoda kosayoda left a comment

Choose a reason for hiding this comment

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

Looks/tests good to me! Any issues I've missed should surface when we implement the bot side of things.

@Xithrius Xithrius added the priority: 0 - critical Needs to be addressed ASAP label Jun 4, 2021
ChrisLovering
ChrisLovering approved these changes Jun 4, 2021
View reviewed changes
Copy link
Member

@ChrisLovering ChrisLovering left a comment

Choose a reason for hiding this comment

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

Tested and all looks good to me!

Backwards compatible too, so can be merged before the bot PR

@ChrisLovering ChrisLovering merged commit 7d81829 into python-discord:main Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisLovering ChrisLovering ChrisLovering approved these changes

@kosayoda kosayoda kosayoda approved these changes

@MarkKoz MarkKoz Awaiting requested review from MarkKoz MarkKoz is a code owner

Assignees
No one assigned
Labels
area: API Related to or causes API changes language: python Involves Python code priority: 0 - critical Needs to be addressed ASAP python Used by dependabot type: feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@bast0006 @coveralls @kosayoda @ChrisLovering @Xithrius