smogon / pokemon-showdown

Pokémon battle simulator.
https://pokemonshowdown.com
MIT License
4.82k stars 2.81k forks source link

Add a way to flag 'tour' or 'suspect' accounts #5646

Open scheibo opened 5 years ago

scheibo commented 5 years ago

I'm not against adding a tour bit to accounts - one use case for flagging accounts that came up recently is that we could disable /offertie for ladder tours in the same way we do on Smogtours/room tours. Similarly, being able to flag an account as participating in a suspect test would also make some Tier Leaders happy, given there was concern over how they would treat users who /offertie.

Originally posted by @scheibo in https://github.com/Zarel/Pokemon-Showdown-Client/pull/1326/threads/MDIzOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTg2MTM5OTc2OnYy/unresolve

Such a bit would also allow us to disable /ionext in OLT (policy pending)

thejetou commented 5 years ago

How would we do this in a reasonable way?

One way would be for upper staff to be able to mark account that start with a certain string as laddertour / suspect (or just straight up hard-coded with the suspect test PRs / commits, though that would also require a PR / commit for every ladder tour..), and in the User object have a type property which could be regular, laddertour, or suspect, depending on if the userid starts with the laddertour or suspect values, otherwise, it's a regular account.

Then from there, we would use standard logic to enable / disable certain features.

We would also need to display it in the UI, perhaps in /cmd userdetails?

There's probably a better approach though.

scheibo commented 5 years ago

How would we do this in a reasonable way?

I don't have a solution, I probably should have made that clear with the 'discussion' label :)

mark account that start with a certain string

In an ideal design, I'd imagine this to just be a toggle users could set, and it wouldnt need to affect their name at all. However, encoding it in the name as we currently do today seems to be the most obvious implementation which would result in the least changes to code/protocol.

thejetou commented 5 years ago

In an ideal design, I'd imagine this to just be a toggle users could set, and it wouldnt need to affect their name at all.

I'm not sure I agree with users being able to make any of their accounts a 'suspect' / 'ladder' account; that'd be really confusing to new users if falsely flagged.

scheibo commented 5 years ago

Current feeling: add a Config.prefixlist, something like:

Config.prefixlist = {
   // OLT Qualifier #2
   'lt62hg': ['ionext', 'modjoin', 'hideroom', 'offertie'],
   // UU Suspect
   'uumt':  ['offertie'],
   // Balanced Hackmons Ladder Tour
   'bhltt':  ['ionext', 'modjoin', 'hideroom', 'offertie'],
};

And then disabling/modifying various commands based on the configuration (simply disabling commands for the user isn't enough - for example, a room between a regular username and one with a prefix banning 'ionext' would not be allowed to be invite only either).

This solution is still not completely elegant, and requires privileged users to make config changes, but is slightly more 'designed' than hardcoding specific checks everywhere and scales beyond just OLT (though unlikely we'd really want to bother making updates for every single ladder tour).

Interplay with prefixed and non-prefixed players is the challenge here, but being able to preserve a normal ladder experience (no changes to commands/options available) for regs who dont encounter OLT players seems like something desirable to shoot for

QuiteQuiet commented 5 years ago

I randomize the ladder prefix for each OLT cycle as I post them so that there's no way to know what account to ladder in advance on, and most suspect tests do something similar. Putting this in the config or otherwise locations that has a non-negligible amount of delay (and a need to bother someone to update the list) to this going live unless the prefixes were prepared in advance, something I'm not too fond of since it would be quite easy to figure out what prefix had been selected for the later cycles through trial and error.

Zarel commented 5 years ago

I randomize the ladder prefix for each OLT cycle as I post them so that there's no way to know what account to ladder in advance on

Wouldn't it be easier just to require the account be made after a specific date? It's not like account creation date is private.

thejetou commented 5 years ago

Wouldn't it be easier just to require the account be made after a specific date? It's not like account creation date is private.

Players can keep their accounts unregistered and register it when the cycle has started.

QuiteQuiet commented 5 years ago

Basically what @TheJetOU said. There's nothing that stops players from starting to ladder on LT63## today and then registering it as the cycle begins, essentially giving some 2-4 times the length to ladder on if they did this. Randomizing 2 letters makes up 351 potential alts, which is more than enough that nobody is going to ladder on all of them in advance. It's still not a too-large number that it couldn't be checked if it were stored somewhere on PS in advance, though.

scheibo commented 5 years ago

Can we not just track the date of the first battle for an account as well, and make sure it only occurs after a suspect/OLT period begins?

Zarel commented 5 years ago

We could also just not keep ladder records for unregistered users.

scheibo commented 5 years ago

We could also just not keep ladder records for unregistered users.

What does that entail? I imagine they'd still keep their rating though, so once they registered they'd be a different place in the ladder than players who didn't (I might be misinterpreting you though. Reseting ratings on registration would be an option to prevent this, but unlikely to be a popular change)

Zarel commented 5 years ago

We could make it so unregistered users could only ladder unrated.

scheibo commented 5 years ago

That's fine by me.

However, just so we're on the same page, that solution ('only accounts registered after the tournament starts are eligible') solves the potential issue with cheating, but the prefix (or some other bit) is still required to both immediately reveal who is in the tournament (for example, for filtering the ladder to see what rank you are in comparison to the other competitors) and to be able to apply special constraints like I proposed earlier.

scheibo commented 5 years ago

Bump. Itd be great to make progress here before OLT ends this year

Zarel commented 5 years ago

The side issue is blocked by #5648. The main issue could use implementation but doesn't seem to be blocked on any decisionmaking?

scheibo commented 5 years ago

Wrong issue? which side issue are you talking about (was this intended for the blitz PR?)? And is #5648 approved?

EDIT: nvm, you mean:

We could make it so unregistered users could only ladder unrated.

Zarel commented 5 years ago

I'm confused. As far as I can tell, this isn't a PR, this is an issue ticket suggesting we add a way to flag tour/suspect accounts. I support the general idea of adding a way to flag such accounts, but no details/UX have been given in this thread for me to have further comment on.

scheibo commented 5 years ago

but no details/UX have been given in this thread for me to have further comment on.

See https://github.com/Zarel/Pokemon-Showdown/issues/5646#issuecomment-514666798 ???

Zarel commented 5 years ago

Oh, Config.prefixlist. Not a huge fan but I won't stand in the way of it; it's better than nothing.

scheibo commented 5 years ago

Yeah, I originally wanted something more engineered, but Config.prefixlist is the 'obvious' and naive way to do it, and I couldnt come up with an alternative that wasnt really complicated (plus, people are already really used to prefixes).

It should also be simple enough that id be easy to rip out if we had a better way of implementing it in the future

scheibo commented 5 years ago

@Zarel - this and #5648 are going to take longer to hash out and implement than the time remaining before the last OLT qualifier starts. What are your thoughts on turning /ionext, /modjoin and /hideroom off for the entire OU ladder for the remainder of the OLT qualifying period per the TDs requests?

Zarel commented 5 years ago

We could just hardcode those based on prefix?

scheibo commented 5 years ago

Instead of coordinating, I wonder if just adding ['LT61', 'LT62', 'LT63', 'LT64'] (or even ['LT6']) to the forced public prefix list during OLT is a better choice - yes, there are some false positives because the official has two extra chars, but i don't know how much we care? It also sidesteps the problem of leaking the real prefix?

@QuiteQuiet

QuiteQuiet commented 5 years ago

For us that would work fine too. Maybe better since there's no need to continually update it once per cycle.

The smaller the prefix gets the more worried I am of false positives (LT6 may be too short to be safe). Depending on how rare false positives are that might be good enough though.

scheibo commented 5 years ago

The bulk of this issue is done, but I believe the remaining work to be:

1) Make it so unregistered users could only ladder unrated. (blocked on #5648) - this would let us set the prefixes ahead of time, but coordination hasn't proven to be too difficult. 2) Hide joins by default Smogtours-style

thejetou commented 5 years ago

Hide joins by default Smogtours-style

I don't think that's reasonable? Why would we hide the join messages in battles? It seems more sensible to just fix /hidejoins (perhaps even add it in "Battle Options") for the people that don't want to see the joins rather than vice versa.

scheibo commented 5 years ago

Zarel is allergic to having hidejoins hide joins. Also, this is only for forced public battles

thejetou commented 5 years ago

Zarel is allergic to having hidejoins hide joins.

Does he just want it bugged till the end of time?

Also, this is only for forced public battles

What does the room being forced public have to do with hiding joins? Fixing /hidejoins and https://github.com/Zarel/Pokemon-Showdown/commit/03c642b7d62b436024eb78b7604cb6905ea32a07 seem much more reasonable solutions.

I'd more agree with hiding joins in all battles unilaterally under the premise of "it's unnecessary and takes up space" than just hiding it in battles.

Zarel commented 5 years ago

I don't think that's reasonable? Why would we hide the join messages in battles? It seems more sensible to just fix /hidejoins (perhaps even add it in "Battle Options") for the people that don't want to see the joins rather than vice versa.

For the record, the plan there is to default joins to off in OLT (forced-public) games, and on in other games. Which is a reasonably sensible default.

I generally prefer "smart defaults" rather than "force users to choose between one bad setting and another bad setting".

Does he just want it bugged till the end of time?

No, I agree that /hidejoins should hide joins. But we do need to 1. make sure people don't accidentally hide joins everywhere if it's not their intent, and 2. make it possible to change back to the default.

I'd more agree with hiding joins in all battles unilaterally under the premise of "it's unnecessary and takes up space" than just hiding it in battles.

My suspicion is that players generally want to know when someone joins their game, they just don't want it to spam up the battle log if 50 people join to watch at the same time.

Maybe the chatroom-style battle collapsing is a good trick here, actually.

scheibo commented 5 years ago

The problem we're trying to solve with turning off joins for OLT (and why they're turned off on Smogtours) is partly log spam, but also because knowing that 50 people are watching gives some players anxiety. Chatroom-style battle collapsing does not solve the latter.

Zarel commented 5 years ago

Chatroom-style battle collapsing does not solve the latter.

Changing /hidejoins solves the latter, though. I support making both changes.