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

Permission value can be unlimited #560

Closed
wants to merge 1 commit into from
Closed

Permission value can be unlimited #560

wants to merge 1 commit into from

Conversation

Kaeios
Copy link

@Kaeios Kaeios commented Feb 18, 2019

Values can be negative to set unlimite like max-bans, ...

value can be negative to set unlimited value like max-bans, ...
@Poslovitch
Copy link
Member

Poslovitch commented Feb 18, 2019

I don't understand what the PR's doing. @DarkRails ?

@tastybento Would that actually be an addition? What's the point of the "sanity check" we're doing with the permissions, by the way?

@Kaeios
Copy link
Author

Kaeios commented Feb 18, 2019

in the config you can set a max amount of ban per island & if set to -1. It is supposed to set it to unlimited but the ban limit can also be defined by a permission, the method to get max ban limit for a user is

int banLimit = issuer.getPermissionValue(getPermissionPrefix() + "ban.maxlimit", getIWM().getBanLimit(getWorld()));

default ban limit: -1 (supposed to be unlimited)

but the default value -1 is replaced by 1 by the getPermissionValue method.
So you can ban only one player of your island even if this settings is set to -1 by default

I hope you understand what I mean.

@Poslovitch Poslovitch added Type: Bug Priority: Medium Status: Need review Waiting for a review to be made on this Pull Request labels Feb 20, 2019
@Poslovitch
Copy link
Member

I understand what you mean, however, I'm afraid it may cause bugs. @tastybento, I really need your review on that.

@BONNe
Copy link
Member

BONNe commented Feb 20, 2019

So,
It will not create bugs...
It will just not work... as with homes :)

Btw... is it even possible to define permission with text permission.value.-1 ?

Copy link
Member

@tastybento tastybento left a comment

Choose a reason for hiding this comment

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

This makes sense.

@Poslovitch Poslovitch added Status: Pending Waiting for a developer to start working on this issue. and removed Status: Need review Waiting for a review to be made on this Pull Request labels Feb 20, 2019
Copy link
Member

@tastybento tastybento left a comment

Choose a reason for hiding this comment

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

So, this part is OK, but other places need to be fixed to enable -1 to be valid.

@tastybento
Copy link
Member

@BONNe You can specify -1 in a permission, but like you say, this change alone won't work.

The use case is that the ban limit has been set to say 3 in config.yml, but the admin wants to give some players the ability to have unlimited bans. To do that, they would give the permission bskyblock.ban.maxlimit.-1.

This code change won't do it, so I'll fix it.

tastybento added a commit that referenced this pull request Feb 20, 2019
@tastybento
Copy link
Member

@DarkRails Thanks for the tip. In the end I have to do more coding, but now using -1 should work.

@tastybento tastybento closed this Feb 20, 2019
@tastybento tastybento removed the Status: Pending Waiting for a developer to start working on this issue. label Feb 20, 2019
@tastybento tastybento added this to the 1.3.0 milestone Feb 20, 2019
@Poslovitch Poslovitch added the Status: Done This issue has been completed or answered. This pull request has been merged. label Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Done This issue has been completed or answered. This pull request has been merged. Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants