Paths

Table of Contentst

Clientside lobby visitor role support (muting clients)
ClosedPublic
Actions

Authored by elexis on Apr 16 2017, 6:38 AM.

Details

Reviewers

scythetwirler
user1
vladislavbelov

Commits

rP19514: Clientside Lobby support for muted players (visitor role).

Summary

An until recently unused moderatoration measure against annoying lobby clients is to mute them, which can be accomplished by setting their role to 'visitor'.
In accordance with the specs https://xmpp.org/extensions/xep-0045.html, clients that have the visitor role can't post chat messages anymore.
The role is lost when logging out and in again, so it is only a temporary measure.

This patch prints a chat message if a user was muted or unmuted or if a client became a moderator or lost the moderation access.
It also hides the chat input, so that clients don't get an error message when failing to send chat, but don't even have that text form anymore.

scythetwirler also has an XPartaMupp patch in the making that persists that role by letting the bot remember the role locally and automatically
setting it back to visitor upon login.
Therefore the patch doesn't print those muted messages to other users if a muted user logs in.

A gloox patch could be implemented to make those 2 C++ lines a tid nicer (we can observe affiliation changes with gloox but not role changes), if someone wants to.

(screenshot doesn't show the input hiding)

Test Plan

Every newly added string should be tested, so 3 accounts and 2 windows of 0ad are needed.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Apr 16 2017, 6:38 AM

Harbormaster completed remote builds in B1134: Diff 1271.Apr 16 2017, 6:38 AM

elexis edited the summary of this revision. (Show Details)Apr 16 2017, 6:41 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/780/ for more details.

Harbormaster completed remote builds in B1134: Diff 1271.Apr 16 2017, 7:28 AM

vladislavbelov requested changes to this revision.Apr 16 2017, 8:18 AM

vladislavbelov added a subscriber: vladislavbelov.

vladislavbelov added inline comments.

binaries/data/mods/public/gui/lobby/lobby.js
166 ↗	(On Diff #1271)	Shouldn't this TODO be in the `SendGetRatingList` function?
185 ↗	(On Diff #1271)	Why `we` and not `me`?
188 ↗	(On Diff #1271)	`5000` should be a named constant. Looks like a hack, shouldn't we send a role with `join`?
273 ↗	(On Diff #1271)	Why it duplicates L157? Shouldn't it be set only on join, if it's the joined time?
source/lobby/XmppClient.cpp
784 ↗	(On Diff #1271)	How I do like commented lines without description.

This revision now requires changes to proceed.Apr 16 2017, 8:18 AM

elexis added inline comments.Apr 17 2017, 12:19 AM

binaries/data/mods/public/gui/lobby/lobby.js
166 ↗	(On Diff #1271)	Oh right, forgot removing that TODO plus the call, as I discussed this with scythetwirler and the call is deprecated as the bot now decides when to send that rating list. should become a separate commit
185 ↗	(On Diff #1271)	I'm used to that replacement xD Can rename, yes
188 ↗	(On Diff #1271)	Can do a global. See the summary. When we join, our role is User, but then _some time_ after the join the role is changed by XPartaMupp to visitor. We would have to change XMPP specifications and implement them in ejabberd if we don't use scythetwirlers hack. So you are right, it is a hack. Perhaps we should grow up and try to fix the specs :s (We also need to update the gloox OSX and windows bundle and implement D87 properly)
273 ↗	(On Diff #1271)	Not dupe, we never receive our own join. All join messages before our join is complete are suppressed in XmppClient.cpp
source/lobby/XmppClient.cpp
784 ↗	(On Diff #1271)	yep, can be removed too

scythetwirler also has an XPartaMupp patch in the making that persists that role by letting the bot remember the role locally and automatically
setting it back to visitor upon login.

Change the affiliation of the users in question to none (from member).

In D339#13839, @leper wrote:

scythetwirler also has an XPartaMupp patch in the making that persists that role by letting the bot remember the role locally and automatically
setting it back to visitor upon login.

Change the affiliation of the users in question to none (from member).

All clients that register have the unaffiliated affiliation, not member.
Only visitors are muted afaik.
Visitor state is reset upon logout, independent of the affiliation.

In D339#13841, @elexis wrote:

In D339#13839, @leper wrote:

scythetwirler also has an XPartaMupp patch in the making that persists that role by letting the bot remember the role locally and automatically
setting it back to visitor upon login.

Change the affiliation of the users in question to none (from member).

All clients that register have the unaffiliated affiliation, not member.

Then change that.

In D339#13842, @leper wrote:

In D339#13841, @elexis wrote:

In D339#13839, @leper wrote:

scythetwirler also has an XPartaMupp patch in the making that persists that role by letting the bot remember the role locally and automatically
setting it back to visitor upon login.

Change the affiliation of the users in question to none (from member).

All clients that register have the unaffiliated affiliation, not member.

Then change that.

and then mute unaffiliated clients in the 0AD client, not having a solution that is in sync with what ejabberd does?

In D339#13845, @elexis wrote:

and then mute unaffiliated clients in the 0AD client, not having a solution that is in sync with what ejabberd does?

You can either make the bot do something that should be split out, or you can add a bot (or something else) that gives member to newly joining jids, which can then be removed thereby resulting in persistent devoicing. (Might even work, but you are aware that rooms can be configured, right?)

In D339#14089, @leper wrote:

In D339#13845, @elexis wrote:

and then mute unaffiliated clients in the 0AD client, not having a solution that is in sync with what ejabberd does?

You can either make the bot do something that should be split out, or you can add a bot (or something else) that gives member to newly joining jids, which can then be removed thereby resulting in persistent devoicing. (Might even work, but you are aware that rooms can be configured, right?)

https://xmpp.org/extensions/xep-0045.html#roles-priv
Participant -> Send Messages to All && not configuration settings MAY modify this privilege

Only visitors are muted afaik

How could we configure a room to devoice clients if not using the visitor affiliation?

elexis mentioned this in D323: Add lobby help command and unify lobby commands.Apr 27 2017, 12:18 PM

Unless leper can point out a configuration option which I couldn't find in the linked XMPP specs, the visitor role will be inevitable if we don't change the XMPP specs.

binaries/data/mods/public/gui/lobby/lobby.js
166 ↗	(On Diff #1271)	It's #4151 in fact, fixed by rP19463 now

Remove that TODO as it was addressed in rP19463.
Make that 5 second limit a global.
Rename we to me.
Remove one unrelated commented out loc in XmppClient.cpp

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/898/ for more details.

Harbormaster completed remote builds in B1301: Diff 1502.Apr 27 2017, 4:18 PM

In D339#15799, @elexis wrote:

Unless leper can point out a configuration option which I couldn't find in the linked XMPP specs, the visitor role will be inevitable if we don't change the XMPP specs.

The whole point was about keeping the state somewhere that isn't the lobby bot, not not using that role.

vladislavbelov requested changes to this revision.Apr 28 2017, 11:35 AM

vladislavbelov added inline comments.

binaries/data/mods/public/gui/lobby/lobby.js
187 ↗	(On Diff #1502)	@elexis have noticed bugs, when a user already connected and `g_JoinedTime[msg.text]` is undefined.
191 ↗	(On Diff #1502)	I think the name could be more explanatory, like `roleChangedFormat`. Because it's not exactly the text.
source/lobby/XmppClient.cpp
817 ↗	(On Diff #1502)	I think we should use named constants instead of numbers.

This revision now requires changes to proceed.Apr 28 2017, 11:35 AM

In D339#15871, @leper wrote:

In D339#15799, @elexis wrote:

Unless leper can point out a configuration option which I couldn't find in the linked XMPP specs, the visitor role will be inevitable if we don't change the XMPP specs.

The whole point was about keeping the state somewhere that isn't the lobby bot, not not using that role.

Ok I think I get it now x)
When someone registers, his role should be changed from unaffiliated to member, then the bot should immediately change the role to visitor.

The specs state that

Member

A user who is on the "whitelist" for a members-only room or who is registered with an open room. A member has an affiliation of "member".

So I guess it is meant that we can attach custom policies to the different states if we don't use a member-only room.

There are some (potential) issues with that though:

(We might still need a lobby.js time constant if the affiliation of registered users is changed)
(Not sure whether gloox supports affiliation change subscriptions or whether we get a presence update in case)
If we mute someone temporarily with the visitor role, the lobby.js code will not show that change
If we settle on non-members being muted, we can't use the member state for anything else anymore. We could for example use the member state for
- email-address registered users or otherwise whitelisted ones
- users who are allowed to have ratings
- could disallow logging in non-whitelisted users in case of a trollwave
We already have a state that saves exactly that information in accordance with the specs.

One could also argue that @mute is just an incomplete implementation of /ban :-)
Just displaying "You have been muted" or "You are muted" upon each login isn't an actual issue to begin with, so no need to do the wololo

We should also have some icon or profile detail info for muted users. In some earlier version it displayed "Visitor" without translation, so having translate("Muted Player") might be something. Can't find an ascii icon or sprite that fits well with the playerlist, especially since we have buddy state indicators there.

binaries/data/mods/public/gui/lobby/lobby.js
117 ↗	(On Diff #1502)	(Notice the @mute can be and already had been patched independently of the release, so the timeout workaround is not useless)
191 ↗	(On Diff #1502)	If you mean msg.text, yep, thats quite annoying and to be done in #4482. If you mean txt, that name should be good becaues it contains the actual text besides the nickname
source/lobby/XmppClient.cpp
817 ↗	(On Diff #1502)	(and CreateGUIMessage should be removed entirely too, see #4482, but that is not related to implementing basic functionality here, we should write a simple ticket for that)

Remove the contested muting of mute messages on login, thereby not having to fix the bug of the workaround, nor needing the workaround, nor needing globals, nor needing a different workaround.

Harbormaster completed remote builds in B1325: Diff 1525.Apr 29 2017, 7:03 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/919/ for more details.

Harbormaster completed remote builds in B1325: Diff 1525.Apr 29 2017, 9:14 AM

elexis mentioned this in D396: Lobby GUI cleanup and disconnect enhancements.Apr 29 2017, 11:33 PM

Seems good and it tested well for me.

In D339#16980, @user1 wrote:

Seems good and it tested well for me.

(We discussed the patch for more than 2 hours even and judged it from all angles. You mentioned that it is important to you that WFGBot (or WFGModBot or whatever) won't send that chat message anymore once we have this one in place and you weren't fully convinced of the member state either right? Also PM'd the lobby admin who didn't really have time to get into the argument about that member state.)

Thanks for the testing in the svn lobby, consideration of features (sjw phenomenon, bot sent chat messages and member state alternative) and code style checks, i.e. review, user1!