471004 - Tag support for Fastpath chat sessions

Status: VERIFIED FIXED

(support.mozilla.org Graveyard :: Chat, enhancement)

Description

[Matthew Middleton (:zzxc)]

It should be possible to apply tags to chat sessions, both in Spark and in the administration panel.  The main purpose of this will be to store which bugs or behaviors are associated with a specific chat session.  I will be adding these tags when reviewing live chat logs and while monitoring chats.  It should also be possible for anyone to add tags while chatting or monitoring, using a textual command.  People helping with a chat that needs investigation might add a "helpwanted" tag, for example.  Tags, combined with data from CSAT, will allow for more automated live chat metrics.

Initially, there won't be support in Spark for this, so a textual interface will need to be used.  Some examples of how this might work:

/tag bug434403 (adds the bug434403 tag to the current chat)
/tag -bug434403 (removes the tag)
/tag bug434403, crash (adds both the crash and bug434403 tags to the current chat)

Additionally, support for adding tags is needed on the View Transcript page, and an interface for uploading bulk edits of tags is needed.  These bulk edits will be uploaded by me, based on automated metrics I collect.  In the future, a script on the tools server may make bulk tag edits automatically.

Most of the time tagging will be done by me, either by reading the chat log or by using a script.  I will bulk apply certain tags when specific KB articles are linked to in chats, for example.  These tags can then be queried to see how many cases we have of a specific issue.

Comment 1

[Matthew Middleton (:zzxc)]

Attachment: Implement tag support via a chatroom bot

This implements tag support by adding a new fpSessionTags table to the database.  The database upgrade is automatic for MySQL, but the included upgrade script needs to be translated to other supported databases (postgres, ms sql, etc) before this could be accepted upstream.  The DB update also adds a configurable groupchat room (eg. Contributors) and the maxMaxChats setting from bug 458793.

Agents use the /tag command to add tags to a chat, with an optional ? or ! prefix on each tag.  A command such as /tag one,+two, -three,?four,!five is supported.  Typing /tag with no arguments prints the list of tags, which are saved to the DB when the chat is closed.

The "RoomBot" also implements a similar /invite command for inviting users to the current chat without clicking through the Fastpath UI.  It can be used to invite a full JID, a workgroup name, a username, or a nickname.  So, commands like /invite zzxc and /invite support are supported.

Comment 2

[Laura Thomson :laura]

Comment on attachment 366196 [details] [diff] [review]
Implement tag support via a chatroom bot

Pushing review to Austin as his JavaFu >> mine.

Comment 3

[Matthew Middleton (:zzxc)]

Instructions for testing this are at https://wiki.mozilla.org/Support/Live_Chat/Tags

Comment 4

[Austin King [:ozten]]

Comment on attachment 366196 [details] [diff] [review]
Implement tag support via a chatroom bot

Good patch, good error detection and checking for null values. I am approving this patch.
I've not compiled or tested the code. The following are suggestions:

src/plugins/fastpath/src/java/org/jivesoftware/xmpp/workgroup/roombot/RoomBot.java 
 Indentation is off around listToString method. Please indent or add braces around if/else if.
 I didn't test the code but it seems off.
if (builder.length() > 0)
    builder.append(delimiter);
else if ((prefix != null) && (prefix.length() > 0))
    builder.append(prefix);
builder.append((String)myElement);

The cmdInvite method is very long consider splitting it up by each peice where you try to detect the user.
All of the user detection could become a several methods and cmdInvite would pick up at
/*Send an invitation inside an IQ to the workgroup


Forgive my ignorance of this XMPP library... but is throwing a PacketRejectedException the right way to control execution flow for normal code paths?
Probably a design issue with the library and not your patch, but this seems incorrect. Can you call a different method, or just return here?

Since your  going to be adding support for lots of commands, consider reducing duplicate code at line 338 by making an array of "good" commands
["/tag", "/invite"] and then testing if arguments[0] is a member of the array, if so doing
		replyFrom = new JID(to.getNode(), to.getDomain(), workgroup.getWorkgroupName());
		replyTo = from;
		cmdInvite(arguments, replyFrom, replyTo, to, userRequest, workgroup);
		throw new PacketRejectedException();
Then as you add support for new commands, it will be a new string in the list with no new block of code below.

src/plugins/fastpath/src/java/org/jivesoftware/xmpp/workgroup/request/UserRequest.java
Add to documentation that tags that are longer than 253 chars are ignored and will fail silently

Line 521 same indentation if no braces comment. The indentation here is very confusing with a while, try, and switch on same indentation.

If the \0 only used in the database? I find this tag flag confusing during the processing. Could this be similifed to look at
/tag, /+tag, /!tag, /?tag
and then only use \0 during the insert into the database?

Comment 5

[Matthew Middleton (:zzxc)]

Attachment: Address review comments (for check-in)

(In reply to comment #4)
Thanks for the suggestions, see the revised patch and my replies below.

> src/plugins/fastpath/src/java/org/jivesoftware/xmpp/workgroup/roombot/RoomBot.java 
>  Indentation is off around listToString method. Please indent or add braces
> around if/else if.
>  I didn't test the code but it seems off.

The indentation looks wrong in the patch because tab spaces are used.  When the preceding + is stripped, it looks normal.

 
> The cmdInvite method is very long consider splitting it up by each peice where
> you try to detect the user.

I separated this out into a reusable findNode function.

> Forgive my ignorance of this XMPP library... but is throwing a
> PacketRejectedException the right way to control execution flow for normal code
> paths?
> Probably a design issue with the library and not your patch, but this seems
> incorrect. Can you call a different method, or just return here?

Throwing a PacketRejectedException is the only way to throw out a packet.  I checked, and other plugins use this to filter out packets as well.

> Since your  going to be adding support for lots of commands, consider reducing
> duplicate code at line 338 by making an array of "good" commands
> ["/tag", "/invite"] and then testing if arguments[0] is a member of the array,
> if so doing

done.

> src/plugins/fastpath/src/java/org/jivesoftware/xmpp/workgroup/request/UserRequest.java
> Add to documentation that tags that are longer than 253 chars are ignored and
> will fail silently

done.

> Line 521 same indentation if no braces comment. The indentation here is very
> confusing with a while, try, and switch on same indentation.

Again, it looks correct with the +/- removed.

> If the \0 only used in the database? I find this tag flag confusing during the
> processing. Could this be similifed to look at
> /tag, /+tag, /!tag, /?tag
> and then only use \0 during the insert into the database?

\0 is used to specify 'no flag'.  In this case, 'null' gets written to the database instead of a null character.  \0 is used during processing to find tags with no flag.

? and ! are the only defined flags, + and - are used for adding/removing.  Each tag only exists once, regardless of any flag that is applied.  More tags may be defined in the future.

Comment 6

[Matthew Middleton (:zzxc)]

Checked in r24216