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

TrustTown command #6276

Merged
merged 7 commits into from
Nov 14, 2022
Merged

TrustTown command #6276

merged 7 commits into from
Nov 14, 2022

Conversation

Kali0033
Copy link
Contributor

@Kali0033 Kali0033 commented Nov 1, 2022

Description:

This pull request adds a new command to town commands titled trusttown, this allows a town member with appropriate permissions the ability to trust an entire town at once. This is envisioned to greatly supplement servers which use war modes which have true conquering of towns, as nation level permissions can often become messy.


New Nodes/Commands/ConfigOptions:

New permission nodes:
towny.command.town.trusttown
towny.command.townyadmin.town.trusttown

New commands:
/t trusttown add *townname*
/t trusttown remove *townname*
/t trusttown list

/ta *townname* trusttown add *townname*
/ta *townname* trusttown remove *townname*
/ta *townname* trusttown list


Relevant Towny Issue ticket:

N/A


  • I have tested this pull request for defects on a server.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.

Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

The new column needs to be added to the SQLSchema before the changes to TownySQLSource will work.

I didn't spot anything else on the first pass though.

src/com/palmergames/bukkit/towny/command/TownCommand.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/command/TownCommand.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/command/TownCommand.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
@LlmDl LlmDl added this to the 0.98.5.0 milestone Nov 1, 2022
Copy link

@nicosammito nicosammito left a comment

Choose a reason for hiding this comment

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

minor code changes for better code quality

src/com/palmergames/bukkit/towny/command/TownCommand.java Outdated Show resolved Hide resolved
src/com/palmergames/bukkit/towny/db/TownySQLSource.java Outdated Show resolved Hide resolved
@Kali0033
Copy link
Contributor Author

Kali0033 commented Nov 2, 2022

Sorry about the review (relatively new to Github, not quite sure if that review even contains latest commits) but have made a new commit which addresses all requested/suggested changes.

Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

Something which has to be discussed is how dangerous this command would be:

  • Mayor A is basically giving another mayor and a group of assistants the ability to allow anyone to be trusted in Mayor A's town.
  • This could be a disaster if a trusted town is Open or becomes Open after becoming trusted.
  • For this reason, the adding of trusted towns needs to be locked behind a Confirmation that makes the mayor aware of the danger they're putting their town in when they add another town as trusted.

The rest of the PR is coming together nicely, I'll be able to review it again after the Confirmation is added and the other small requests are addressed.

resources/lang/en-US.yml Outdated Show resolved Hide resolved
resources/lang/en-US.yml Outdated Show resolved Hide resolved
@nicosammito
Copy link

Just to give you a little hint, you don't need to manually commit these suggestions, you can simply accept these over GitHub

@LlmDl
Copy link
Member

LlmDl commented Nov 3, 2022

The suggestions requesting swapping args[0] and the subcommands "add" aren't necessary. If we cared about that we'd be refactoring most of the command classes.

You can accept the stream/foreach stuff if you want.

@nicosammito
Copy link

The suggestions requesting swapping args[0] and the subcommands "add" aren't necessary. If we cared about that we'd be refactoring most of the command classes.

You can accept the stream/foreach stuff if you want.

I kind of understand that, but we can now start to change this MR by MR. Swapping this increases the readability and also makes more sense because we check whether the specific string is equal of some param and not the other way around

@LlmDl
Copy link
Member

LlmDl commented Nov 3, 2022

The suggestions requesting swapping args[0] and the subcommands "add" aren't necessary. If we cared about that we'd be refactoring most of the command classes.
You can accept the stream/foreach stuff if you want.

I kind of understand that, but we can now start to change this MR by MR. Swapping this increases the readability and also makes more sense because we check whether the specific string is equal of some param and not the other way around

I don't really care for it and I don't see a point in changing this now.

Copy link
Member

@LlmDl LlmDl left a comment

Choose a reason for hiding this comment

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

Found one more snag, the only thing left to do is create add/remove events for the API.

src/com/palmergames/bukkit/towny/object/Town.java Outdated Show resolved Hide resolved
@Kali0033
Copy link
Contributor Author

Kali0033 commented Nov 4, 2022

I got a bit ahead of myself and implemented all requests before scrolling to the bottom lol, I can create a new commit again undoing the changes regarding swapping args[0] and the subcommands if wanted.

@nicosammito
Copy link

I got a bit ahead of myself and implemented all requests before scrolling to the bottom lol, I can create a new commit again undoing the changes regarding swapping args[0] and the subcommands if wanted.

Yep this would be a goo idea. You can than simply apply my suggestions here on the website, rather than committing than manually.

@LlmDl
Copy link
Member

LlmDl commented Nov 4, 2022

I'm not entirely certain who Nico is, whether they're on the discord or not. They aren't a Towny org member though.

@Kali0033
Copy link
Contributor Author

Kali0033 commented Nov 4, 2022

Appreciate the advice Nico regarding adding commits directly on site, saw that button but wasn't sure if they would format correctly, I take it being Github though they have probably factored that into the design and works as intended.

Otherwise have made a new commit addressing current issues laid out regarding reverting some formatting changes, events and the issue with removeAllTrustedTowns()
Also unsure what the bug is with regards to auto-compiler integrated onto site, I have compiled on local and have a working build that I have tested.

@nicosammito
Copy link

I'm not entirely certain who Nico is, whether they're on the discord or not. They aren't a Towny org member though.

Nope, im currently not in discord, just found this nice project and wanted to contribute

@nicosammito
Copy link

Appreciate the advice Nico regarding adding commits directly on site, saw that button but wasn't sure if they would format correctly, I take it being Github though they have probably factored that into the design and works as intended.

Otherwise have made a new commit addressing current issues laid out regarding reverting some formatting changes, events and the issue with removeAllTrustedTowns()

Formatting should work fine, never experienced any bugs with it

This commit adds a new feature where you can trust a town in it's entirety. This is envisioned to supplement war modes that feature true conquest greatly.
Remove commented code and remove stray line.
Changes requested/suggested by LimDL and nicosammito.
This commit adds a confirmation box, accompanying text and renames an incorrectly named method.
@LlmDl LlmDl merged commit f5d6396 into TownyAdvanced:master Nov 14, 2022
LlmDl added a commit that referenced this pull request Nov 14, 2022
    - It is now possible for a town to add another town as trusted.
    - This means that anyone in the 2nd town can do any
build/destroy/switch/itemuse as if they were a mayor in the first town.
    - Obviously this should only be given out under very limited
circumstances, anyone added to the 2nd town after trust is given will
also have full plot perm rights in the first town.
    - There is a confirmation warning the player that uses the /t
trusttown add command.
  - New Command: /ta [townname] trusttown
    - Requires towny.command.townyadmin.town.trusttown permission node.
    - Subcommands:
      - add *townname*
      - remove *townname*
      - list
  - New Command: /t trusttown
    - Requires towny.command.town.trusttown permission node.
    - Subcommands:
      - add [townname]
      - remove [townname]
      - list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants