1146093 - Add some system messages for when an occupant leaves a XMPP MUC room

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

Add an appropriate system message telling the user what happened (banned or removed or kicked from MUC room) using codes when when the user is no longer an occupant in XMPP MUC room.

http://www.xmpp.org/extensions/xep-0045.html
http://xmpp.org/registrar/mucstatus.html

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - handle system messages when user is no longer an occupant

Comment 2

[aleth [:aleth]]

Comment on attachment 8639597 [details] [diff] [review]
rev 1 - handle system messages when user is no longer an occupant

Review of attachment 8639597 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, there were a lot of permutations here!

::: chat/locales/en-US/xmpp.properties
@@ +101,5 @@
>  
> +# LOCALIZATION NOTE (conversation.message.parted.*):
> +#   These are displayed as a system message when a participant parts a room.
> +#   %S is the part message supplied by the user.
> +conversation.message.parted.you=You have left this room.

this -> the (just for consistency with IRC)

@@ +102,5 @@
> +# LOCALIZATION NOTE (conversation.message.parted.*):
> +#   These are displayed as a system message when a participant parts a room.
> +#   %S is the part message supplied by the user.
> +conversation.message.parted.you=You have left this room.
> +conversation.message.parted.you.reason=You have left this room: %S

this -> the

@@ +156,5 @@
> +
> +# LOCALIZATION NOTE (conversation.message.MUCShutdown):
> +#   These are displayed as a system message when a participant is removed from
> +#   a room because of a system shutdown.
> +conversation.message.mucShutdown =You have been removed from this room because of a system shutdown.

nit: space after mucShutdown

::: chat/protocols/xmpp/xmpp.jsm
@@ +205,5 @@
>          participant.name = item.attributes["nick"];
>          return;
>        }
>        if (item && item.attributes["role"] == "none") {
>          // XEP-0045: the user is no longer an occupant.

This comment needs updating as it's not always the user who's leaving
// XEP-0045: an occupant has left the room.

@@ +209,5 @@
>          // XEP-0045: the user is no longer an occupant.
>          this.removeParticipant(nick);
> +
> +        let actor = item.getElement(["actor"]);
> +        let actorNick = actor ? actor.attributes["nick"] : "";

Does it make sense for isActor to be != "" if actorNick == ""? It seems like that would mess up the system message.

@@ +210,5 @@
>          this.removeParticipant(nick);
> +
> +        let actor = item.getElement(["actor"]);
> +        let actorNick = actor ? actor.attributes["nick"] : "";
> +        let isActor = actor ? ".actor" : "";

Add a comment above this block
// Who caused the participant to leave the room.
and a blank line after.

@@ +212,5 @@
> +        let actor = item.getElement(["actor"]);
> +        let actorNick = actor ? actor.attributes["nick"] : "";
> +        let isActor = actor ? ".actor" : "";
> +        let reasonNode = item.getElement(["reason"]);
> +        let reason = reasonNode ? reason.innerText : "";

Same question here: if reason == "" does it make sense for isReason to be != ""?

@@ +213,5 @@
> +        let actorNick = actor ? actor.attributes["nick"] : "";
> +        let isActor = actor ? ".actor" : "";
> +        let reasonNode = item.getElement(["reason"]);
> +        let reason = reasonNode ? reason.innerText : "";
> +        let isReason = reasonNode ? ".reason" : "";

Comment above
// Why the participant left.
blank line after.

@@ +218,5 @@
> +        let isYou = "";
> +        if (nick == this.nick) {
> +          this.left = true;
> +          isYou = ".you";
> +        }

To keep with the previous style,
let isYou = nick == this.nick ? ".you" : "";
if (isYou)
  this.left = true;

@@ +219,5 @@
> +        if (nick == this.nick) {
> +          this.left = true;
> +          isYou = ".you";
> +        }
> +        let message;

Move this var down above the if block where it is used.

@@ +231,5 @@
> +            retMsg = _(messageID, actorNick);
> +          else if (isYou && isReason)
> +            retMsg = _(messageID, reason);
> +          else if (isYou)
> +            retMsg = _(messageID);

Is it easier to read if you nest it? like
if (isYou) {
  if (isActor) {
    if (isReason) ...
    else ...
  }
  else if (isReason) ...
  else ...
}
else if (isActor) {
...
}
else ...

I'm not sure. Seems worth a try.

@@ +241,5 @@
> +            retMsg = _(messageID, nick, reason);
> +          else
> +            retMsg = _(messageID, nick);
> +          return retMsg;
> +        }

Have you considered instead
function __(aMsg) {
  let messageID = aMsg + isYou + isActor + isReason;
  let params = [actorNick, nick, reason].filter(s => s);
  return _(messageID, ...params); 
}

@@ +247,5 @@
>          if (codes.indexOf("110") != -1) {
>            // XEP-0045 7.14: Self-presence.
>            // This presence refers to this account.
> +          message = __("conversation.message.parted");
> +          this.WARN(message)

Why the WARN?

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - handle system messages when user is no longer an occupant

(In reply to aleth [:aleth] from comment #2)
> Does it make sense for isActor to be != "" if actorNick == ""? It seems like
> that would mess up the system message.

> Same question here: if reason == "" does it make sense for isReason to be !=
> ""?

you are right about this case I think it's handled now.

> Why the WARN?

I just used it for testing something and now it's removed.

Comment 4

[aleth [:aleth]]

Comment on attachment 8645745 [details] [diff] [review]
rev 2 - handle system messages when user is no longer an occupant

Review of attachment 8645745 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/locales/en-US/xmpp.properties
@@ +154,5 @@
> +conversation.message.removedNonMember=%1$S has removed from the room because its configuration has been changed to members-only.
> +conversation.message.removedNonMember.actor=%1$S has removed from the room because %2$S has changed it to members-only.
> +conversation.message.removedNonMember.you=You have been removed from the room because its configuration has been changed to members-only.
> +#   %1$S is the person who changed the room configuration.
> +conversation.message.removedNonMember.you.actor=You have been removed you from the room because %1$S has changed it to members-only.

Typos (double you)

::: chat/protocols/xmpp/xmpp.jsm
@@ +219,5 @@
> +        let reason = reasonNode ? reasonNode.innerText : "";
> +        let isReason = reason ? ".reason" : "";
> +
> +        let isYou = nick == this.nick ? ".you" : "";
> +        let useNick = isYou ? "" : nick;

How about affectedNick? (I wasn't sure from the name what useNick meant.)

@@ +234,4 @@
>          if (codes.indexOf("110") != -1) {
>            // XEP-0045 7.14: Self-presence.
>            // This presence refers to this account.
> +          message = __("conversation.message.parted");

If you do message = "conversation.message.parted" etc, then you can do without the __( ) helper function and just put that code in the if(message) clause at the end, as it is only ever called once.

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - handle system messages when user is no longer an occupant

Comment 6

[aleth [:aleth]]

Comment on attachment 8646267 [details] [diff] [review]
rev 3 - handle system messages when user is no longer an occupant

Review of attachment 8646267 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing those reason edge cases.

When someone /parts the room (other than the user), there is no message. (That would be conversation.message.parted)

Which bug adds the join messages (for when someone joins the room)?

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #6)
> When someone /parts the room (other than the user), there is no message.
> (That would be conversation.message.parted)
> 
> Which bug adds the join messages (for when someone joins the room)?

Actually, there is another bug for that bug 1172355

Comment 8

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/73749d05e2cc590eafe66decb177d1ca4a78c6ae
changeset:  73749d05e2cc590eafe66decb177d1ca4a78c6ae
user:       Abdelrhman Ahmed <[e-mail]>
date:       Tue Aug 11 03:09:00 2015 +0200
description:
Bug 1146093 - Add some system messages for when an occupant leaves a XMPP MUC room. r=aleth

Comment 9

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #7)
> (In reply to aleth [:aleth] from comment #6)
> > When someone /parts the room (other than the user), there is no message.
> > (That would be conversation.message.parted)
> > 
> > Which bug adds the join messages (for when someone joins the room)?
> 
> Actually, there is another bug for that bug 1172355

OK, in that case those two system messages should be next, because the current state after landing this is a bit confusing to the user ;)

Comment 10

[aleth [:aleth]]

Note: some comments wordwrapped on checkin.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Missing "been" at https://hg.mozilla.org/comm-central/rev/73749d05e2cc#l1.64 and the following line?

Comment 12

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/67e6d83976095509136b840ddd71bf49fc75e0eb
changeset:  67e6d83976095509136b840ddd71bf49fc75e0eb
user:       aleth <[e-mail]>
date:       Tue Aug 11 14:46:00 2015 +0200
description:
Bug 1146093 - Followup to fix two string typos. rs=aleth

Comment 13

[aleth [:aleth]]

(In reply to Aryx [:archaeopteryx][:aryx] from comment #11)
> Missing "been" at https://hg.mozilla.org/comm-central/rev/73749d05e2cc#l1.64
> and the following line?

Thanks, good catch!