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

[Bug]: @everyone role does not work for whitelisting #24

Closed
kaydenvg opened this issue Nov 15, 2023 · 8 comments · Fixed by #27
Closed

[Bug]: @everyone role does not work for whitelisting #24

kaydenvg opened this issue Nov 15, 2023 · 8 comments · Fixed by #27
Assignees
Labels
T-bug Something isn't working

Comments

@kaydenvg
Copy link

kaydenvg commented Nov 15, 2023

Version information

1.0.0 Alpha 8

Version type

Full

Expected Behavior

users with no role/@everyone are able to use np!register to whitelist themselves

Actual Behavior

"Sorry, but I couldn't accept your request
It seems that you don't have the required subscription/member level or don't have your Twitch/Youtube account linked to your Discord account."

Reproduction Steps

  1. Be a normal @everyone user
  2. Type np!register
  3. response is:
    "Sorry, but I couldn't accept your request
    It seems that you don't have the required subscription/member level or don't have your Twitch/Youtube account linked to your Discord account."

Java version

N/A, issue with bot

Additional information

Whitelisting self works with any role other than the default @everyone. Made a new role with no special permissions/not admin and it works, but I'd prefer if anyone joining the server with the default role could whitelist themselves.

@kaydenvg kaydenvg added S-needs-triage T-bug Something isn't working labels Nov 15, 2023
@Awakened-Redstone Awakened-Redstone self-assigned this Nov 16, 2023
@csm-kb
Copy link
Contributor

csm-kb commented Nov 30, 2023

yo! I'm w/ @kaydenvg , there's two ways I think could be good for solving this (ref

boolean accepted = !Collections.disjoint(roles.stream().map(Role::getId).toList(), new ArrayList<>(whitelistDataMap.keySet()));
):

  1. have the bot recognize when a guild's @everyone role is in the config list
  2. have a new config setting akin to allowEveryone: boolean, and have it bypass the whitelist check if that is true

thoughts? would be a fast PR

@Awakened-Redstone
Copy link
Owner

Yeah, it shouldn't be complicated. I have been pretty busy recently, so I haven't had time to work on the issue. I'll try working on it this weekend.

@csm-kb
Copy link
Contributor

csm-kb commented Nov 30, 2023

I ask because we'd be happy to make the change and make the PR, given which way you want to take it! 🙇‍♂️

@Awakened-Redstone
Copy link
Owner

You are always free to make a PR

@csm-kb
Copy link
Contributor

csm-kb commented Nov 30, 2023

Would the config option be the preferred method here?

@Awakened-Redstone
Copy link
Owner

I have considered adding a small additional line for processing @everyone (from server id) into a role for the cached list or process something like id -1 into the role, the reason I don't look for the presence of the role or add an option for that is because I want to allow the users to also use roles along @everyone

I want to later work on some QoL for the mod and push it into beta or release when I have more time, feel free to make a PR with a workaround, it can be improved later, the mod is currently on alpha state.

@Awakened-Redstone Awakened-Redstone pinned this issue Dec 1, 2023
@csm-kb
Copy link
Contributor

csm-kb commented Dec 1, 2023

Question: given master is the default branch but I note dev/full, which should I make this PR against?
disregard, going to fork and PR a master

  • to this end, you may want to add contribution how-to notes in the readme! 😌

@csm-kb csm-kb mentioned this issue Dec 1, 2023
@csm-kb csm-kb mentioned this issue Dec 8, 2023
@Awakened-Redstone Awakened-Redstone linked a pull request Dec 8, 2023 that will close this issue
Awakened-Redstone pushed a commit that referenced this issue Dec 9, 2023
* Discord: add guild public role ID check for @everyone
* Modify member get roles for better @everyone support
@Awakened-Redstone
Copy link
Owner

Fixed by #27, patch released in 1.0.0-alpha.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants