Cleanup polls code #67

cswhite2000 · 2018-01-07T21:38:01Z

No description provided.

Electroid

Awesome job! It will be nice to have refactored polls so more features can implement them. Mostly minor fixes and changes.

Electroid · 2018-01-07T21:48:56Z

PGM/src/main/java/tc/oc/pgm/polls/Poll.java


-    public Poll(PollManager pollManager, Server server, String initiator) {
+    protected final PollManager pollManager;
+    protected final String initiator;


I would suggest changing the initiator to be a PlayerId so that you can fix the bug where nicked players are exposed if they create a poll.

that feels like it would be a bit restrictive, if other systems want to use polls (say the veto system for tournaments) forcing it into a player id may not be that good

Tournaments are an entirely different system. Besides, if you were to refactor this for teams, it would just make more complicated. It's a reasonable assumption that a poll is initiated by a person and not something else.

Electroid · 2018-01-07T21:51:58Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+        }
+        if (sender instanceof Player) {
+            Player player = (Player) sender;
+            if (TokenUtil.getUser(player).maptokens() < 1) {


I would add a util method for TokenUtil.getUser(CommandSender)

Electroid · 2018-01-07T21:53:42Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+            throw new TranslatableCommandException("poll.map.notallowed");
+        }
+
+        if (PGM.get().getServer().getOnlinePlayers().size() * 4 / 5 > nextMap.getDocument().max_players() && !sender.hasPermission("poll.next.override")) {


Instead of PGM.get()... inject OnlinePlayers and use its methods.

Electroid · 2018-01-07T21:54:31Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+            throw new TranslatableCommandException("poll.map.notallowed");
+        }
+
+        if (PGM.get().getServer().getOnlinePlayers().size() * 4 / 5 > nextMap.getDocument().max_players() && !sender.hasPermission("poll.next.override")) {


Why the 4/5ths?

It accounts for some observers being afk.

Electroid · 2018-01-07T21:55:20Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+
+        if (args.getSuggestionContext() != null) {
+            Collection<Mutation> availableMutations = Sets.newHashSet(Mutation.values());
+            availableMutations.removeAll(mutationQueue.mutations());


I would add a convenience method for mutationsAvailable

Electroid · 2018-01-07T21:57:03Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+        Mutation mutation = StringUtils.bestFuzzyMatch(mutationString, Sets.newHashSet(Mutation.values()), 0.9);
+        if(mutation == null) {
+            throw new TranslatableCommandException("command.mutation.error.find", mutationString);
+        } else if(MutationCommands.getInstance().getMutationQueue().mutations().contains(mutation)) {


Avoid statically referencing other injected commands like MutationsCommands. All you need is the MutationQueue object, which you already have as a local variable in mutationQueue

Electroid · 2018-01-07T21:58:35Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollCustom.java

+        PollCustom create(String text, CommandSender initiator);
+    }
+
+    private String text;


Instead of passing a text object, can that just be a method?

getReasonString() or getPollString()

Where would you put the method? PollCustom needs it to be able to display the message they player used.

Electroid · 2018-01-07T22:00:40Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollKick.java

+
+    @Override
+    public void executeAction() {
+        if(player != null) player.kickPlayer(ChatColor.DARK_RED + "You were poll-kicked.");


There needs to be a better explanation for the kick. Also, I would implement this via an off-record punishment so they get all the formatting.

Although, you may need to make sure off-record punishments, instead of saying ...appeal here... they say ...punishment does not count... or something.

PotatoStealer

That's a job well done on its own, I only have some little things here and there, but thanks for picking up after me :)

058c37a272bed3464d47f0b01038a16a · 2018-01-10T12:30:34Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollCustom.java

+
+    private String text;
+
+    @AssistedInject PollCustom(@Assisted String text, @Assisted CommandSender initiator, PollManager pollManager, Audiences audiences) {


The Poll class already defines an @Injected constructor, I believe @AssistedInject is correct here

PotatoStealer · 2018-01-10T10:53:36Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollKick.java

@@ -21,33 +21,32 @@
        PollKick create(PlayerId initiator, Player player);
    }

-    private final Player player;
+    private final PlayerId player;


This should be named playerId

PotatoStealer · 2018-01-10T10:54:11Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollKick.java

    }

    @Override
    public void executeAction() {
-        PlayerId kicked = userStore.playerId(player);
-        punishmentCreator.create(null, kicked, "The poll to kick " + kicked.username() + " succeeded", PunishmentDoc.Type.KICK, null, false, false, true);
+        punishmentCreator.create(null, player, "The poll to kick " + player.username() + " succeeded", PunishmentDoc.Type.KICK, null, false, false, true);


Maybe make this translatable instead of a hardcoded string

Well, It doesn't really make sense to translate a punishment message, as it cannot be translated per user.

what about translations?

PotatoStealer · 2018-01-10T10:54:39Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollKick.java

        audiences.localServer().sendMessage(new Component(ChatColor.DARK_AQUA).translate("poll.kick.success", new PlayerComponent(identityProvider.currentIdentity(player))));
    }

    @Override
    public String getActionString() {
-        return normalize + "Kick: " + boldAqua + this.player;
+        return normalize + "Kick: " + boldAqua + this.player.username();


Same here, would prefer a translatable component.

PotatoStealer · 2018-01-10T10:59:14Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

@@ -94,34 +105,31 @@ public void pollKick(CommandContext args, CommandSender sender) throws CommandEx
            return CommandUtils.completeMapName(mapName);
        }

-        if (!Config.Poll.enabled()) {
+        if(!Config.Poll.enabled()) {


Can you let the poll package have its own configuration class? I think it would be better than referencing it from the Config class.

058c37a272bed3464d47f0b01038a16a · 2018-01-10T12:30:34Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollCustom.java

+
+    private String text;
+
+    @AssistedInject PollCustom(@Assisted String text, @Assisted CommandSender initiator, PollManager pollManager, Audiences audiences) {


The Poll class already defines an @Injected constructor, I believe @AssistedInject is correct here

PotatoStealer · 2018-01-14T09:19:22Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollCommands.java

+
+import javax.inject.Inject;
+
+public class PollCommands implements Commands {


Use a CommandBinder

PotatoStealer · 2018-01-14T09:20:53Z

Commons/core/src/main/i18n/templates/pgm/PGMErrors.properties

+huddle.globalChatDisabled = Global chat is disabled during team huddle
+
+poll.vote.value.invalid = Accepted values: yes|no


poll.vote.invalidValue

PotatoStealer · 2018-01-14T09:22:26Z

PGM/src/main/java/tc/oc/pgm/polls/Poll.java


    public static String boldAqua = ChatColor.BOLD + "" + ChatColor.AQUA;
    public static String normalize = ChatColor.RESET + "" + ChatColor.DARK_AQUA;
-    public static String seperator = ChatColor.RESET + " | ";
+    public static String separator = ChatColor.RESET + " | ";


PotatoStealer · 2018-01-14T09:23:13Z

PGM/src/main/java/tc/oc/pgm/polls/PollManifest.java

+        final PluginFacetBinder facets = new PluginFacetBinder(binder());
+        facets.register(PollCommands.class);
+        facets.register(PollManager.class);
+        facets.register(PollListener.class);


bindListener

PotatoStealer · 2018-01-14T09:24:54Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollCommands.java

+        PlayerId voter = CommandUtils.senderToMatchPlayer(sender).getPlayerId();
+        Poll currentPoll = pollManager.getPoll();
+        if(currentPoll != null) {
+            if(args.getString(0).equalsIgnoreCase("yes")) {


Maybe have a cleaner way to deal with values. Perhaps use an enum to hold the possible values of a vote.

I think the check is fine. Polls, at least for now, are either yes or no.

PotatoStealer · 2018-01-14T09:25:46Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollCommands.java

+    public void veto(CommandContext args, CommandSender sender) throws CommandException {
+        if(pollManager.isPollRunning()) {
+            pollManager.endPoll(PollEndReason.Cancelled);
+            audiences.localServer().sendMessage(new Component(ChatColor.RED).translate("poll.veto", new PlayerComponent(identityProvider.currentIdentity(sender))));


preferably make this 2 lines instead of one for visibility

PotatoStealer · 2018-01-14T09:26:35Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+        Player initiator = tc.oc.commons.bukkit.commands.CommandUtils.senderToPlayer(sender);
+        Player player = tc.oc.commons.bukkit.commands.CommandUtils.findOnlinePlayer(args, sender, 0);
+
+        if(player.hasPermission("pgm.poll.kick.exempt") && !initiator.hasPermission("pgm.poll.kick.override")) {


Declare the permission strings as a constant at the top of the class, just so other classes may use it for whatever purposes

PotatoStealer · 2018-01-14T09:27:59Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+            return StringUtils.complete(args.getSuggestionContext().getPrefix(), mutationQueue.mutationsAvailable().stream().map(mutation -> mutation.name().toLowerCase()));
+        }
+
+        if(!Config.Poll.enabled()) {


have a class to hold the configuration values, i.e. a specific PollConfiguration

Electroid · 2018-02-27T20:34:06Z

Commons/bukkit/src/main/java/tc/oc/commons/bukkit/punishment/PunishmentFormatter.java

-        parts.add(new Component(new TranslatableComponent("punishment.screen.rules", Links.rulesLink())));
-
-        parts.add(new Component(new TranslatableComponent("punishment.screen.appeal", Links.appealLink())));
+        if (!punishment.off_record()) {


if(punishment.off_record()) { // parts.add("off_record"); } else { // parts.add("others"); }

Electroid · 2018-02-27T20:36:27Z

PGM/src/main/java/tc/oc/pgm/polls/PollBlacklist.java

+        if (lines == null) return;
+        ImmutableList.Builder<PGMMap> maps = ImmutableList.builder();
+        for(String line : lines) {
+            if (line.contains("#")) {


What is this for?

Electroid · 2018-02-27T20:38:14Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollCommands.java

+        PlayerId voter = CommandUtils.senderToMatchPlayer(sender).getPlayerId();
+        Poll currentPoll = pollManager.getPoll();
+        if(currentPoll != null) {
+            if(args.getString(0).equalsIgnoreCase("yes")) {


I think the check is fine. Polls, at least for now, are either yes or no.

Electroid · 2018-02-27T20:38:38Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+        this.pollConfig = pollConfig;
+    }
+
+


Extra new lines here

Electroid · 2018-02-27T20:39:44Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+    )
+    @CommandPermissions("poll.kick")
+    public void pollKick(CommandContext args, CommandSender sender) throws CommandException {
+        Player initiator = tc.oc.commons.bukkit.commands.CommandUtils.senderToPlayer(sender);


Might want to consider statically importing the sendToPlayer method since you use it frequently.

Electroid · 2018-02-27T20:40:48Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+    @CommandPermissions("poll.next")
+    public List<String> pollNext(CommandContext args, CommandSender sender) throws CommandException {
+        final String mapName = args.argsLength() > 0 ? args.getJoinedStrings(0) : "";
+        if(args.getSuggestionContext() != null) {


All of these single-if checks can just be put as one-liners.

if(args.getSuggestionContext() != null) return CommandUtils.completeMapName(mapName); if(!pollConfig.enabled()) throw new TranslatableCommandException("poll.disabled"); // etc...

Electroid · 2018-02-27T20:41:35Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+            throw new TranslatableCommandException("poll.map.notallowed");
+        }
+
+        if(onlinePlayers.count() * 4 / 5 > nextMap.getDocument().max_players() && !sender.hasPermission("poll.next.override")) {


What's the reasoning behind the 4/5ths? Is there a standard place we can put this logic? If a match is too full?

Electroid · 2018-02-27T20:42:22Z

PGM/src/main/java/tc/oc/pgm/polls/commands/PollSubCommands.java

+        }
+
+        if(onlinePlayers.count() * 4 / 5 > nextMap.getDocument().max_players() && !sender.hasPermission("poll.next.override")) {
+            throw new TranslatableCommandException("poll.map.toomanyplayers");


For translation keys, use camel case for multi-words:
poll.map.tooManyPlayers

Electroid · 2018-02-27T20:43:51Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollCustom.java

+    public String getDescriptionMessage() {
+        return boldAqua + text;
+    }
+}


Electroid · 2018-02-27T20:45:20Z

PGM/src/main/java/tc/oc/pgm/polls/types/PollNextMap.java


 public class PollNextMap extends Poll {
+
+    public interface Factory {
+        PollNextMap create(CommandSender sender, PGMMap nextMap, PlayerId playerId);


If the CommandSender is the same as the PlayerId, maybe just provide the sender and derive the player? or the other way around?

I say that because when instantiating this factory, the senderToPlayer method is invoked frequently, and might just be more useful to be put inside the construction.

This partially fixes StratusNetwork/issues#248

Adds buy permission Actually mounts the player Disallows broken versions Removes entity on disconnect/dismount

Electroid suggested changes Jan 7, 2018

View reviewed changes

StratusNetwork deleted a comment from Pablete1234 Jan 7, 2018

StratusNetwork deleted a comment from Electroid Jan 7, 2018

PotatoStealer reviewed Jan 10, 2018

View reviewed changes

058c37a272bed3464d47f0b01038a16a reviewed Jan 10, 2018

View reviewed changes

PotatoStealer reviewed Jan 14, 2018

View reviewed changes

cswhite2000 added 5 commits January 25, 2018 00:46

Cleanup polls code

ac68036

More cleanup of the polls code

7081fe7

Make PollKick use an off record punishment

97fa414

fix compilation error

a38db72

Fix name not showing up correctly in PollKick

3c74a30

cswhite2000 force-pushed the rewrite-polls branch from 2359476 to f5c8438 Compare January 25, 2018 09:32

Improve polls some more

01173c4

cswhite2000 force-pushed the rewrite-polls branch from f5c8438 to 01173c4 Compare January 25, 2018 18:43

Electroid suggested changes Feb 27, 2018

View reviewed changes

Electroid and others added 15 commits July 28, 2018 15:45

Reduce max players on the bungee

88b1b43

Fix players moving lobby minecarts

fd60b83

Minor legacy flag fixes and fix extra quotes in chat

d6f82e1

Warn players if there are no games

72b6e3b

This partially fixes StratusNetwork/issues#248

Fix 1 day holidays

ecfa7e1

Make poll votes clickable

aabd320

Fix poll status messages not showing

4a5fdac

Fix portal spam in lobbies

d55c8b0

Fix staff in observers not being able to read team chat

f1fb481

Make some perms default to false

2b97f9d

Fix headless horseman

293429f

Adds buy permission Actually mounts the player Disallows broken versions Removes entity on disconnect/dismount

Add RepeatingRunnable

7c710de

Add vector rotation utils

a01e595

Add ghost gizmo

4d39fff

Generify HalloweenGizmo, add global purchase perm

6ac16cc

PotatoStealer and others added 7 commits September 24, 2018 17:25

Temporary fix missed assumption in gizmo permissions check

eefbc94

Temporarily fix double jump

3338afa

Remove zombie hitboxes due to buggy behavior

a2249f9

Disable token mutation GUI when polls are diabled

e37fef4

Merge remote-tracking branch 'upstream/master'

4d53d4b

Update polls cleanup

54026c7

Update polls cleanup

ed5f267

AustinLMayes added the testing label Nov 21, 2018

AustinLMayes assigned cswhite2000 Nov 21, 2018

AustinLMayes mentioned this pull request Nov 21, 2018

fixes polls exposing player realname #51

Closed


		private String text;

		@AssistedInject PollCustom(@Assisted String text, @Assisted CommandSender initiator, PollManager pollManager, Audiences audiences) {


		import javax.inject.Inject;

		public class PollCommands implements Commands {

		huddle.globalChatDisabled = Global chat is disabled during team huddle

		poll.vote.value.invalid = Accepted values: yes\|no

Cleanup polls code #67

Are you sure you want to change the base?

Cleanup polls code #67

Conversation

cswhite2000 commented Jan 7, 2018

Electroid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PotatoStealer left a comment

Choose a reason for hiding this comment

058c37a272bed3464d47f0b01038a16a Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

058c37a272bed3464d47f0b01038a16a Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

058c37a272bed3464d47f0b01038a16a Jan 10, 2018 •

edited

Loading

058c37a272bed3464d47f0b01038a16a Jan 10, 2018 •

edited

Loading