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

Make local blacklist and local whitelist mutually exclusive #4002

Open
wants to merge 4 commits into
base: V3/develop
Choose a base branch
from

Conversation

NeuroAssassin
Copy link
Contributor

Type

  • Bugfix
  • Enhancement
  • New feature

Description of the changes

Prevent setting local blacklist/whitelist when the opposite is not empty. Fixes #4001.

@NeuroAssassin NeuroAssassin requested a review from tekulvw as a code owner June 21, 2020 21:37
@Jackenmen Jackenmen self-assigned this Jun 21, 2020
@Jackenmen Jackenmen added this to the Backlog milestone Jun 21, 2020
@Jackenmen Jackenmen added QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. QA: Needed and removed QA: Needed QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. labels Jun 29, 2020
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

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

I haven't tested these changes yet (although it seems fine from the code), but there are two things that I noticed:

  • the error messages should be worded differently - they should say something more along these lines (but not necessarily exactly like this - this is just a rough idea on what it should say):
    Users can't be added to local whitelist while local blacklist is in use. You can use `{prefix}localblacklist clear` if you want to clear the local blacklist and use the local whitelist instead.
    
    The important part that needs to be communicated is that the other list is currently in use/non-empty; the information about how to clear it is less important and is more of a "tip".
  • this should probably also have same notice for bot-wide whitelist/blacklist - while it's a lot less important because bot owners are the only people that can change it (and they're not affected by it), it would still be good if they knew that their action might not work as intended

@Jackenmen Jackenmen added QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. and removed QA: Needed labels Jul 7, 2020
@Jackenmen
Copy link
Member

There are also some conflicts here, but you shouldn't worry about those until #3910 is merged (unless you want to deal with the conflict resolution twice :P)

@Jackenmen Jackenmen modified the milestones: Backlog, 3.3.10 Jul 7, 2020
@Jackenmen
Copy link
Member

For your information, #3910 is now merged

@Kowlin Kowlin modified the milestones: 3.3.10, 3.3.11 Jul 9, 2020
@Jackenmen Jackenmen modified the milestones: 3.3.11, Backlog Jul 9, 2020
@Jackenmen Jackenmen removed the QA: Changes Requested Used by few QA members. Awaiting changes requested by maintainers or QA. label Oct 22, 2020
@Jackenmen Jackenmen added Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing. and removed Type: Fix labels Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Unexpected behavior, result, or exception. In case of PRs, it is a fix for the foregoing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

localwhitelist and localblacklist can both be non-empty at the same time
4 participants