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

Implement /unblacklistall command #2800

Merged
merged 1 commit into from Sep 30, 2016
Merged

Implement /unblacklistall command #2800

merged 1 commit into from Sep 30, 2016

Conversation

panpawn
Copy link
Contributor

@panpawn panpawn commented Sep 29, 2016

This also introduces Punishments#roomUnblacklistAll to do the heavy lifting of this command.

@panpawn panpawn force-pushed the patch-182 branch 2 times, most recently from 0cdbdde to 1629127 Compare September 29, 2016 21:52
@Zarel
Copy link
Member

Zarel commented Sep 29, 2016

...who asked for this? I don't think this is a good idea... especially if used accidentally.

if (!target) {
user.lastCommand = '/unblacklistall';
this.errorReply("THIS WILL UNBLACKLIST ALL BLACKLISTED USERS.");
this.errorReply("To confirm, use: /unblacklistall confirm");
Copy link
Contributor Author

@panpawn panpawn Sep 29, 2016

Choose a reason for hiding this comment

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

I don't think this is a good idea... especially if used accidentally.

This kind of prevents the "if used accidentally" scenario, for the most part

@panpawn
Copy link
Contributor Author

panpawn commented Sep 29, 2016

Also, I just think it might be helpful with rooms where the blacklists are rather large and if a room should want to remove them; same purpose for /deroomvoiceall.

Also, I thought we were making roomban-type punishments closer to global ban punishments... There is an /unbanall command, so I thought with that in mind, mirroring that functionality with blacklists wouldn't be a horrible idea.

@Asheviere
Copy link
Contributor

Asheviere commented Sep 29, 2016 via email

});
if (!success) return false;
Punishments.savePunishments();
return success;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably return the list of previously blacklisted users, which should be logged.
With that, the command becomes revertible and safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Well, it's not completely revertible, unless you log IPs/alts, and even then there's not a good way to add IPs/alts to a blacklist.

@panpawn panpawn force-pushed the patch-182 branch 2 times, most recently from 59b5331 to 07f3a63 Compare September 30, 2016 02:04
@panpawn
Copy link
Contributor Author

panpawn commented Sep 30, 2016

Updated


let unblacklisted = Punishments.roomUnblacklistAll(room);
if (!unblacklisted) return this.errorReply("No users are currently blacklisted in this room to unblacklist.");
this.addModCommand("All blacklists in this room have been lifted by " + user.name + ". This has unblacklisted: " + unblacklisted.join(', '));
Copy link
Contributor

Choose a reason for hiding this comment

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

Like this it will spam the room though.

Just log it with this.logEntry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@panpawn
Copy link
Contributor Author

panpawn commented Sep 30, 2016

Updated

if (!unblacklisted) return this.errorReply("No users are currently blacklisted in this room to unblacklist.");
let reply = `All blacklists in this room have been lifted by ${user.name}.`;
room.add(`|c|${user.getIdentity()}|/log ${reply}`).update();
this.logEntry(`${reply} This has unblacklisted: ${unblacklisted.join(', ')}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two spaces here, but it's not a big deal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny you mention that actually, I was taught to always put two spaces in-between sentences in school when I was learning how to type. I'll remove this though, now that I'm remembering this isn't a standard for PS stuff.

@panpawn
Copy link
Contributor Author

panpawn commented Sep 30, 2016

Updated

if (!unblacklisted) return this.errorReply("No users are currently blacklisted in this room to unblacklist.");
let reply = `All blacklists in this room have been lifted by ${user.name}.`;
room.add(`|c|${user.getIdentity()}|/log ${reply}`).update();
this.logEntry(`${reply} This has unblacklisted: ${unblacklisted.join(', ')}`);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is redundant with the room.add above. I think the ${reply} part can be safely removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, how are we mod-logging this then?

Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we modlogging this?

Copy link
Member

Choose a reason for hiding this comment

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

The code I recommend here is:

this.addModCommand(`All blacklists in this room have been lifted by ${user.name}.`);
this.logEntry(`Unblacklisted users: ${unblacklisted.join(', ')}`);

The .update() is unnecessary; commands automatically update rooms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (!unblacklisted) return this.errorReply("No users are currently blacklisted in this room to unblacklist.");
let reply = `All blacklists in this room have been lifted by ${user.name}.`;
room.add(`|c|${user.getIdentity()}|/log ${reply}`).update();
this.logEntry(`${reply} This has unblacklisted: ${unblacklisted.join(', ')}`);
Copy link
Member

Choose a reason for hiding this comment

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

The code I recommend here is:

this.addModCommand(`All blacklists in this room have been lifted by ${user.name}.`);
this.logEntry(`Unblacklisted users: ${unblacklisted.join(', ')}`);

The .update() is unnecessary; commands automatically update rooms.

This also introduces Punishments#roomUnblacklistAll to do the heavy lifting of this command.
@panpawn
Copy link
Contributor Author

panpawn commented Sep 30, 2016

Updated

@Zarel Zarel merged commit b30d471 into smogon:master Sep 30, 2016
@Zarel
Copy link
Member

Zarel commented Sep 30, 2016

I still think this is going to cause more problems than it solves, but I guess we can try it out and see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants