Presence: show better error status messages.

When you get <presence type='error'>, any <status/> element in the
presence is an echo of a stanza you sent out. So the message in that is
not the contact's presence: it's either your status, or maybe the
message you sent with a subscription request, depending on whether we
got the <presence type='error'/> for a roster contact or someone we
tried to subscribe to.

So instead, I think we should use the text of the error (if any) as the
status for such contacts, falling back to the error element name if the
server is unkind and doesn't include an error message. Yes, this means
strings will show up in the UI in the server's locale, but this is
hardly news.

https://bugs.freedesktop.org/show_bug.cgi?id=45491
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

3 files changed, 91 insertions, 0 deletions

diff --git a/src/presence-cache.c b/src/presence-cache.c
index bf5f87bf9..4b73104d3 100644
--- a/src/presence-cache.c
+++ b/src/presence-cache.c
@@ -43,6 +43,7 @@
 #include <wocky/wocky-utils.h>
 #include <wocky/wocky-namespaces.h>
 #include <wocky/wocky-data-form.h>
+#include <wocky/wocky-xmpp-error-enumtypes.h>
 
 #define DEBUG_FLAG GABBLE_DEBUG_PRESENCE
 
@@ -1837,11 +1838,29 @@ gabble_presence_parse_presence_message (
       return TRUE;
 
     case WOCKY_STANZA_SUB_TYPE_ERROR:
+    {
+      GError *error = NULL;
+      gboolean ret;
+
       NODE_DEBUG (presence_node, "Received error presence");
+
+      ret = wocky_stanza_extract_errors (message, NULL, &error, NULL, NULL);
+      g_assert (ret);
+
+      /* If there's a <status/> in this presence, it's our own echoed back at
+       * us. So we don't want to use that. Instead, we use the <error><text> if
+       * there is any, or the name of the error condition if not. */
+      if (tp_str_empty (error->message))
+        status_message = wocky_enum_to_nick (WOCKY_TYPE_XMPP_ERROR,
+            error->code);
+      else
+        status_message = error->message;
+
       gabble_presence_cache_update (cache, handle, resource,
           GABBLE_PRESENCE_ERROR, status_message, priority);
 
       return TRUE;
+    }
 
     case WOCKY_STANZA_SUB_TYPE_UNAVAILABLE:
       if (gabble_roster_handle_sends_presence_to_us (priv->conn->roster,
diff --git a/tests/twisted/Makefile.am b/tests/twisted/Makefile.am
index 7c756ff28..03638bb6c 100644
--- a/tests/twisted/Makefile.am
+++ b/tests/twisted/Makefile.am
@@ -64,6 +64,7 @@ TWISTED_TESTS = \
 	plugin-channel-managers.py \
 	power-save.py \
 	presence/decloak.py \
+	presence/error.py \
 	presence/initial-contact-presence.py \
 	presence/initial-presence.py \
 	presence/invisible_xep_0126.py \
diff --git a/tests/twisted/presence/error.py b/tests/twisted/presence/error.py
new file mode 100644
index 000000000..53f3703bc
--- /dev/null
+++ b/tests/twisted/presence/error.py
@@ -0,0 +1,71 @@
+"""
+Tests that Gabble provides at least some useful information from error
+presences.
+"""
+
+from gabbletest import exec_test, make_presence, elem
+from servicetest import assertEquals
+import ns
+import constants as cs
+
+def test(q, bus, conn, stream):
+    jids = ['gregory@unreachable.example.com',
+            'thehawk@unreachable.example.net',
+           ]
+    gregory, hawk = jids
+    gregory_handle, hawk_handle = conn.RequestHandles(cs.HT_CONTACT, jids)
+
+    event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    event.stanza['type'] = 'result'
+    for jid in jids:
+        item = event.query.addElement('item')
+        item['jid'] = jid
+        item['subscription'] = 'both'
+
+    stream.send(event.stanza)
+    q.expect('dbus-signal', signal='PresencesChanged',
+        args=[{gregory_handle: (cs.PRESENCE_OFFLINE, 'offline', ''),
+               hawk_handle:    (cs.PRESENCE_OFFLINE, 'offline', ''),
+              }
+             ])
+
+    # Our server can't resolve unreachable.example.com so it sends us an error
+    # presence for Gregory. (This is what Prosody actually does.)
+    presence = make_presence(gregory, type='error')
+    error_text = u'Connection failed: DNS resolution failed'
+    presence.addChild(
+        elem('error', type='cancel')(
+          elem(ns.STANZA, 'remote-server-not-found'),
+          elem(ns.STANZA, 'text')(
+            error_text
+          )
+        ))
+
+    stream.send(presence)
+
+    e = q.expect('dbus-signal', signal='PresencesChanged')
+    presences, = e.args
+    type_, status, message = presences[gregory_handle]
+    assertEquals(cs.PRESENCE_ERROR, type_)
+    assertEquals('error', status)
+    assertEquals(error_text, message)
+
+    # How about maybe the hawk's server is busted?
+    presence = make_presence(hawk, type='error')
+    presence.addChild(
+        elem('error', type='cancel')(
+          elem(ns.STANZA, 'internal-server-error'),
+        ))
+    stream.send(presence)
+
+    e = q.expect('dbus-signal', signal='PresencesChanged')
+    presences, = e.args
+    type_, status, message = presences[hawk_handle]
+    assertEquals(cs.PRESENCE_ERROR, type_)
+    assertEquals('error', status)
+    # FIXME: It might be less user-hostile to give some kind of readable
+    # description of the error in future.
+    assertEquals('internal-server-error', message)
+
+if __name__ == '__main__':
+    exec_test(test)