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

Resolve [Bug]/#24 #26

Closed
wants to merge 11 commits into from
Closed

Conversation

csm-kb
Copy link
Contributor

@csm-kb csm-kb commented Dec 1, 2023

Resolves #24

@Awakened-Redstone @kaydenvg
When executing the whitelist register command in Discord, we take the member's guild's getPublicRole() (which isn't returned in the list from getRoles() because it is documented as implicit):

The @everyone role; synonymous with the guild's ID.

and add it explicitly to the roles we're checking on the member.

This allows users to configure the mod using their guild's @everyone role, and have it behave as expected.

@csm-kb
Copy link
Contributor Author

csm-kb commented Dec 1, 2023

Thought that signed... going to do an interactive rebase to have that sign

@csm-kb
Copy link
Contributor Author

csm-kb commented Dec 1, 2023

done! should be good to go @Awakened-Redstone

@Awakened-Redstone Awakened-Redstone changed the base branch from master to mc/1.20.2 December 1, 2023 02:46
@Awakened-Redstone
Copy link
Owner

Awakened-Redstone commented Dec 1, 2023

Seems good, I just need to check it a bit more later as the mod runs periodic checks, so may need to add that line to some other parts of the code, like CoreEvents:80 and DiscordDataProcessor:59

I'm sorry for the confusing branches, I'm working on some changes to the repo as AutoWhitelist Lite has been deprecated

@Awakened-Redstone Awakened-Redstone self-assigned this Dec 1, 2023
@Awakened-Redstone
Copy link
Owner

I forgot to mention, the major branch currently is mc/1.20.2
Once I have more time I'll remove unused branches and rename master to archive/lite

@Awakened-Redstone Awakened-Redstone self-requested a review December 2, 2023 20:46
Copy link
Owner

@Awakened-Redstone Awakened-Redstone left a comment

Choose a reason for hiding this comment

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

The roles list is immutable, it has to either be copied to a mutable list or use another method to verify for the @everyone role.
The user can use a bot to give all users a role, but supporting @everyone is a nice QoL, also needs to add the extra code to other places that may change the user whitelist, as mentioned on the last comment

@csm-kb
Copy link
Contributor Author

csm-kb commented Dec 8, 2023

mutable list

my bad, it's been years since I touched java... damn ArrayList 😂

@csm-kb csm-kb marked this pull request as draft December 8, 2023 19:08
@csm-kb
Copy link
Contributor Author

csm-kb commented Dec 8, 2023

This should be better, was fighting Gradle to use my jdk-17 path (little gremlin)

  • I pulled the get roles logic into its own helper function and modified the other files making use of .getRoles() in that capacity to use this
  • I did a rebase onto mc/1.20.2 to catch back up

Please take a look!

@csm-kb csm-kb marked this pull request as ready for review December 8, 2023 21:05
@csm-kb csm-kb closed this Dec 8, 2023
@csm-kb csm-kb deleted the master branch December 8, 2023 21:10
@csm-kb csm-kb restored the master branch December 8, 2023 21:12
@csm-kb csm-kb deleted the master branch December 8, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-triage T-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: @everyone role does not work for whitelisting
2 participants