Merge branch 'dont-unblock-on-remove-0.8' into telepathy-gabble-0.8

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

5 files changed, 514 insertions, 74 deletions

diff --git a/src/roster-channel.c b/src/roster-channel.c
index 8a5b7f137..c588b48f6 100644
--- a/src/roster-channel.c
+++ b/src/roster-channel.c
@@ -1,7 +1,7 @@
 /*
- * gabble-roster-channel.c - Source for GabbleRosterChannel
- * Copyright (C) 2005, 2006 Collabora Ltd.
- * Copyright (C) 2005 Nokia Corporation
+ * roster-channel.c - Source for GabbleRosterChannel
+ * Copyright © 2005, 2006, 2008, 2010 Collabora Ltd.
+ * Copyright © 2005, 2008, 2010 Nokia Corporation
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -450,6 +450,7 @@ _gabble_roster_channel_add_member_cb (GObject *obj,
                                       GError **error)
 {
   GabbleRosterChannelPrivate *priv;
+  GabbleRoster *roster;
   gboolean ret = FALSE;
 #ifdef ENABLE_DEBUG
   TpHandleRepoIface *handle_repo;
@@ -458,6 +459,7 @@ _gabble_roster_channel_add_member_cb (GObject *obj,
   const gchar *contact_id;
 
   priv = GABBLE_ROSTER_CHANNEL (obj)->priv;
+  roster = priv->conn->roster;
 
 #ifdef ENABLE_DEBUG
   handle_repo = tp_base_connection_get_handles ((TpBaseConnection *) priv->conn,
@@ -474,7 +476,7 @@ _gabble_roster_channel_add_member_cb (GObject *obj,
 
   if (TP_HANDLE_TYPE_GROUP == priv->handle_type)
     {
-      ret = gabble_roster_handle_add_to_group (priv->conn->roster,
+      ret = gabble_roster_handle_add_to_group (roster,
           handle, priv->handle, error);
     }
   else if (TP_HANDLE_TYPE_LIST != priv->handle_type)
@@ -486,19 +488,12 @@ _gabble_roster_channel_add_member_cb (GObject *obj,
   /* publish list */
   else if (GABBLE_LIST_HANDLE_PUBLISH == priv->handle)
     {
-      /* send <presence type="subscribed"> */
-      ret = gabble_connection_send_presence (priv->conn,
-          LM_MESSAGE_SUB_TYPE_SUBSCRIBED, contact_id, message, error);
+      ret = gabble_roster_handle_subscribed (roster, handle, message, error);
     }
   /* subscribe list */
   else if (GABBLE_LIST_HANDLE_SUBSCRIBE == priv->handle)
     {
-      /* add item to the roster (GTalk depends on this, clearing the H flag) */
-      gabble_roster_handle_add (priv->conn->roster, handle, NULL);
-
-      /* send <presence type="subscribe"> */
-      ret = gabble_connection_send_presence (priv->conn,
-          LM_MESSAGE_SUB_TYPE_SUBSCRIBE, contact_id, message, error);
+      ret = gabble_roster_handle_subscribe (roster, handle, message, error);
     }
   /* deny list */
   else if (GABBLE_LIST_HANDLE_DENY == priv->handle)
@@ -528,6 +523,7 @@ _gabble_roster_channel_remove_member_cb (GObject *obj,
                                          GError **error)
 {
   GabbleRosterChannelPrivate *priv;
+  GabbleRoster *roster;
   TpBaseConnection *conn;
   TpHandleRepoIface *contact_repo;
 #ifdef ENABLE_DEBUG
@@ -537,6 +533,7 @@ _gabble_roster_channel_remove_member_cb (GObject *obj,
   const gchar *contact_id;
 
   priv = GABBLE_ROSTER_CHANNEL (obj)->priv;
+  roster = priv->conn->roster;
   conn = (TpBaseConnection *) priv->conn;
 #ifdef ENABLE_DEBUG
   handle_repo = tp_base_connection_get_handles (conn, priv->handle_type);
@@ -551,7 +548,7 @@ _gabble_roster_channel_remove_member_cb (GObject *obj,
 
   if (TP_HANDLE_TYPE_GROUP == priv->handle_type)
     {
-      ret = gabble_roster_handle_remove_from_group (priv->conn->roster,
+      ret = gabble_roster_handle_remove_from_group (roster,
           handle, priv->handle, error);
     }
   else if (TP_HANDLE_TYPE_LIST != priv->handle_type)
@@ -563,42 +560,24 @@ _gabble_roster_channel_remove_member_cb (GObject *obj,
   /* publish list */
   else if (GABBLE_LIST_HANDLE_PUBLISH == priv->handle)
     {
-      /* send <presence type="unsubscribed"> */
-      ret = gabble_connection_send_presence (priv->conn,
-          LM_MESSAGE_SUB_TYPE_UNSUBSCRIBED, contact_id, message, error);
-
-      /* remove it from local_pending here, because roster callback doesn't
-         know if it can (subscription='none' is used both during request and
-         when it's rejected) */
-      if (tp_handle_set_is_member (GABBLE_ROSTER_CHANNEL (obj)->group.local_pending, handle))
-        {
-          TpIntSet *rem = tp_intset_new ();
-
-          tp_intset_add (rem, handle);
-          tp_group_mixin_change_members (obj, "", NULL, rem, NULL, NULL,
-              0, 0);
-
-          tp_intset_destroy (rem);
-        }
+      ret = gabble_roster_handle_unsubscribed (roster, handle, message, error);
     }
   /* subscribe list */
   else if (GABBLE_LIST_HANDLE_SUBSCRIBE == priv->handle)
     {
-      /* send <presence type="unsubscribe"> */
-      ret = gabble_connection_send_presence (priv->conn,
-          LM_MESSAGE_SUB_TYPE_UNSUBSCRIBE, contact_id, message, error);
+      ret = gabble_roster_handle_unsubscribe (roster, handle, message, error);
     }
   /* stored list */
   else if (GABBLE_LIST_HANDLE_STORED == priv->handle)
     {
       /* send roster subscription=remove IQ */
-      ret = gabble_roster_handle_remove (priv->conn->roster, handle, error);
+      ret = gabble_roster_handle_remove (roster, handle, error);
     }
   /* deny list */
   else if (GABBLE_LIST_HANDLE_DENY == priv->handle)
     {
       /* unblock contact */
-      ret = gabble_roster_handle_set_blocked (priv->conn->roster, handle,
+      ret = gabble_roster_handle_set_blocked (roster, handle,
           FALSE, error);
     }
   else
diff --git a/src/roster-channel.h b/src/roster-channel.h
index bf254780d..ea36ea861 100644
--- a/src/roster-channel.h
+++ b/src/roster-channel.h
@@ -1,7 +1,7 @@
 /*
- * gabble-roster-channel.h - Header for GabbleRosterChannel
- * Copyright (C) 2005 Collabora Ltd.
- * Copyright (C) 2005 Nokia Corporation
+ * roster-channel.h - Header for GabbleRosterChannel
+ * Copyright © 2005, 2008 Collabora Ltd.
+ * Copyright © 2005, 2008 Nokia Corporation
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
diff --git a/src/roster.c b/src/roster.c
index dd301af3a..bac7f9606 100644
--- a/src/roster.c
+++ b/src/roster.c
@@ -1,8 +1,8 @@
 /*
  * roster.c - Source for Gabble roster helper
  *
- * Copyright (C) 2006 Collabora Ltd.
- * Copyright (C) 2006 Nokia Corporation
+ * Copyright © 2006–2010 Collabora Ltd.
+ * Copyright © 2006–2010 Nokia Corporation
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -2043,6 +2043,34 @@ static LmHandlerResult roster_edited_cb (GabbleConnection *conn,
                                          GObject *roster_obj,
                                          gpointer user_data);
 
+/*
+ * Cancel any subscriptions on @item by sending unsubscribe and/or
+ * unsubscribed, as appropriate.
+ */
+static gboolean
+roster_item_cancel_subscriptions (
+    GabbleRoster *roster,
+    TpHandle contact,
+    GabbleRosterItem *item,
+    GError **error)
+{
+  gboolean ret = TRUE;
+
+  if (item->subscription & GABBLE_ROSTER_SUBSCRIPTION_FROM)
+    {
+      DEBUG ("sending unsubscribed");
+      ret = gabble_roster_handle_unsubscribed (roster, contact, NULL, error);
+    }
+
+  if (ret && (item->subscription & GABBLE_ROSTER_SUBSCRIPTION_TO))
+    {
+      DEBUG ("sending unsubscribe");
+      ret = gabble_roster_handle_unsubscribe (roster, contact, NULL, error);
+    }
+
+  return ret;
+}
+
 /* Apply the unsent edits to the given roster item.
  *
  * \param roster The roster
@@ -2054,7 +2082,7 @@ roster_item_apply_edits (GabbleRoster *roster,
                          TpHandle contact,
                          GabbleRosterItem *item)
 {
-  gboolean altered = FALSE, ret;
+  gboolean altered = FALSE, ret = TRUE;
   GabbleRosterItem edited_item;
   TpIntSet *intset;
   GabbleRosterPrivate *priv = roster->priv;
@@ -2080,13 +2108,42 @@ roster_item_apply_edits (GabbleRoster *roster,
     }
 #endif
 
+  if (edits->new_google_type != GOOGLE_ITEM_TYPE_INVALID
+      && edits->new_google_type != item->google_type)
+    {
+      DEBUG ("Changing Google type from %d to %d", item->google_type,
+             edits->new_google_type);
+      altered = TRUE;
+      edited_item.google_type = edits->new_google_type;
+    }
+
   if (edits->new_subscription != GABBLE_ROSTER_SUBSCRIPTION_INVALID
       && edits->new_subscription != item->subscription)
     {
-      DEBUG ("Changing subscription from %d to %d",
-             item->subscription, edits->new_subscription);
-      altered = TRUE;
-      edited_item.subscription = edits->new_subscription;
+      /* Here we check the google_type of the *edited* item (as patched in the
+       * block above) to deal correctly with a batch of edits containing both
+       * (un)block and remove.
+       */
+      if (edits->new_subscription == GABBLE_ROSTER_SUBSCRIPTION_REMOVE &&
+          edited_item.google_type == GOOGLE_ITEM_TYPE_BLOCKED)
+        {
+          /* If they're blocked, we can't just remove them from the roster,
+           * because that would unblock them! So instead, we cancel both
+           * subscription directions.
+           */
+          DEBUG ("contact is blocked; not removing");
+          ret = roster_item_cancel_subscriptions (roster, contact, item, NULL);
+          /* deliberately not setting altered: we haven't altered the roster
+           * directly.
+           */
+        }
+      else
+        {
+          DEBUG ("Changing subscription from %d to %d",
+              item->subscription, edits->new_subscription);
+          altered = TRUE;
+          edited_item.subscription = edits->new_subscription;
+        }
     }
 
   if (edits->new_name != NULL && tp_strdiff (item->name, edits->new_name))
@@ -2096,15 +2153,6 @@ roster_item_apply_edits (GabbleRoster *roster,
       edited_item.name = edits->new_name;
     }
 
-  if (edits->new_google_type != GOOGLE_ITEM_TYPE_INVALID
-      && edits->new_google_type != item->google_type)
-    {
-      DEBUG ("Changing Google type from %d to %d", item->google_type,
-             edits->new_google_type);
-      altered = TRUE;
-      edited_item.google_type = edits->new_google_type;
-    }
-
   if (edits->add_to_groups || edits->remove_from_groups)
     {
 #ifdef ENABLE_DEBUG
@@ -2187,7 +2235,7 @@ roster_item_apply_edits (GabbleRoster *roster,
       &edited_item);
   ret = _gabble_connection_send_with_reply (priv->conn,
       message, roster_edited_cb, G_OBJECT (roster),
-      GUINT_TO_POINTER(contact), NULL);
+      GUINT_TO_POINTER(contact), NULL) && ret;
   if (ret)
     {
       /* assume everything will be OK */
@@ -2453,6 +2501,15 @@ gabble_roster_handle_remove (GabbleRoster *roster,
       item->unsent_edits->new_subscription = GABBLE_ROSTER_SUBSCRIPTION_REMOVE;
       return TRUE;
     }
+  else if (item->google_type == GOOGLE_ITEM_TYPE_BLOCKED)
+    {
+      /* If they're blocked, we can't just remove them from the roster,
+       * because that would unblock them! So instead, we cancel both
+       * subscription directions.
+       */
+      DEBUG ("contact#%u is blocked; not removing", handle);
+      return roster_item_cancel_subscriptions (roster, handle, item, error);
+    }
   else
     {
       DEBUG ("immediate edit to contact#%u - change subscription to REMOVE",
@@ -2666,6 +2723,93 @@ gabble_roster_handle_remove_from_group (GabbleRoster *roster,
   return ret;
 }
 
+gboolean
+gabble_roster_handle_subscribe (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error)
+{
+  GabbleRosterPrivate *priv = roster->priv;
+  TpHandleRepoIface *contact_repo = tp_base_connection_get_handles (
+      (TpBaseConnection *) priv->conn, TP_HANDLE_TYPE_CONTACT);
+  const gchar *contact_id = tp_handle_inspect (contact_repo, handle);
+
+
+  /* add item to the roster (GTalk depends on this, clearing the H flag) */
+  gabble_roster_handle_add (priv->conn->roster, handle, NULL);
+
+  /* send <presence type="subscribe"> */
+  return gabble_connection_send_presence (priv->conn,
+      LM_MESSAGE_SUB_TYPE_SUBSCRIBE, contact_id, message, error);
+}
+
+gboolean
+gabble_roster_handle_unsubscribe (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error)
+{
+  TpHandleRepoIface *contact_repo = tp_base_connection_get_handles (
+      (TpBaseConnection *) roster->priv->conn, TP_HANDLE_TYPE_CONTACT);
+  const gchar *contact_id = tp_handle_inspect (contact_repo, handle);
+
+  /* send <presence type="unsubscribe"> */
+  return gabble_connection_send_presence (roster->priv->conn,
+      LM_MESSAGE_SUB_TYPE_UNSUBSCRIBE, contact_id, message, error);
+}
+
+gboolean
+gabble_roster_handle_subscribed (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error)
+{
+  TpHandleRepoIface *contact_repo = tp_base_connection_get_handles (
+      (TpBaseConnection *) roster->priv->conn, TP_HANDLE_TYPE_CONTACT);
+  const gchar *contact_id = tp_handle_inspect (contact_repo, handle);
+
+  /* send <presence type="subscribed"> */
+  return gabble_connection_send_presence (roster->priv->conn,
+      LM_MESSAGE_SUB_TYPE_SUBSCRIBED, contact_id, message, error);
+}
+
+gboolean
+gabble_roster_handle_unsubscribed (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error)
+{
+  TpHandleRepoIface *contact_repo = tp_base_connection_get_handles (
+      (TpBaseConnection *) roster->priv->conn, TP_HANDLE_TYPE_CONTACT);
+  const gchar *contact_id = tp_handle_inspect (contact_repo, handle);
+  GabbleRosterChannel *publish = _gabble_roster_get_channel (roster,
+      TP_HANDLE_TYPE_LIST, GABBLE_LIST_HANDLE_PUBLISH, NULL, NULL);
+  gboolean ret;
+
+  /* send <presence type="unsubscribed"> */
+  ret = gabble_connection_send_presence (roster->priv->conn,
+      LM_MESSAGE_SUB_TYPE_UNSUBSCRIBED, contact_id, message, error);
+
+  /* remove it from publish:local_pending here, because roster callback doesn't
+     know if it can (subscription='none' is used both during request and
+     when it's rejected) */
+  if (tp_handle_set_is_member (publish->group.local_pending, handle))
+    {
+      TpIntSet *rem = tp_intset_new ();
+
+      tp_intset_add (rem, handle);
+      tp_group_mixin_change_members (G_OBJECT (publish), "", NULL, rem, NULL,
+          NULL, 0, 0);
+
+      tp_intset_destroy (rem);
+    }
+
+  return ret;
+}
 
 static const gchar * const list_channel_fixed_properties[] = {
     TP_IFACE_CHANNEL ".ChannelType",
diff --git a/src/roster.h b/src/roster.h
index ee6991f96..b201e67de 100644
--- a/src/roster.h
+++ b/src/roster.h
@@ -1,8 +1,8 @@
 /*
  * roster.h - Headers for Gabble roster helper
  *
- * Copyright (C) 2006 Collabora Ltd.
- * Copyright (C) 2006 Nokia Corporation
+ * Copyright © 2006–2010 Collabora Ltd.
+ * Copyright © 2006–2010 Nokia Corporation
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -79,6 +79,7 @@ gboolean gabble_roster_handle_set_name (GabbleRoster *, TpHandle,
 gboolean gabble_roster_handle_remove (GabbleRoster *, TpHandle, GError **);
 gboolean gabble_roster_handle_add (GabbleRoster *, TpHandle, GError **);
 gboolean gabble_roster_handle_has_entry (GabbleRoster *, TpHandle);
+
 gboolean gabble_roster_handle_add_to_group (GabbleRoster *roster,
                                             TpHandle handle,
                                             TpHandle group,
@@ -88,6 +89,28 @@ gboolean gabble_roster_handle_remove_from_group (GabbleRoster *roster,
                                                  TpHandle group,
                                                  GError **error);
 
+gboolean gabble_roster_handle_subscribe (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error);
+gboolean gabble_roster_handle_unsubscribe (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error);
+
+gboolean gabble_roster_handle_subscribed (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error);
+gboolean gabble_roster_handle_unsubscribed (
+    GabbleRoster *roster,
+    TpHandle handle,
+    const gchar *message,
+    GError **error);
+
 G_END_DECLS
 
 #endif /* __ROSTER_H__ */
diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py
index 8c4f05837..8dbf270de 100644
--- a/tests/twisted/roster/test-google-roster.py
+++ b/tests/twisted/roster/test-google-roster.py
@@ -1,3 +1,4 @@
+# vim: set fileencoding=utf-8 :
 """
 Test workarounds for gtalk
 """
@@ -9,8 +10,8 @@ from gabbletest import (
     expect_list_channel
     )
 from servicetest import (
-    sync_dbus, EventPattern, wrap_channel,
-    assertLength, assertEquals, assertContains,
+    call_async, sync_dbus, EventPattern,
+    assertLength, assertEquals, assertContains, assertDoesNotContain,
     )
 import constants as cs
 import ns
@@ -18,11 +19,11 @@ import ns
 from twisted.words.protocols.jabber.client import IQ
 from twisted.words.xish import domish
 
-def make_set_roster_iq(stream, user, contact, state, ask):
+def make_set_roster_iq(stream, user, contact, state, ask, attrs={}):
     iq = IQ(stream, 'set')
     query = iq.addElement((ns.ROSTER, 'query'))
     add_gr_attributes(query)
-    add_roster_item(query, contact, state, ask)
+    add_roster_item(query, contact, state, ask, attrs=attrs)
     return iq
 
 def add_gr_attributes(query):
@@ -42,8 +43,23 @@ def add_roster_item(query, contact, state, ask, attrs={}):
 
     return item
 
-def test(q, bus, conn, stream):
-    conn.Connect()
+def is_stored(event):
+    return event.path.endswith('/stored')
+
+def is_subscribe(event):
+    return event.path.endswith('/subscribe')
+
+def is_publish(event):
+    return event.path.endswith('/publish')
+
+def is_deny(event):
+    return event.path.endswith('/deny')
+
+def test_inital_roster(q, bus, conn, stream):
+    """
+    This part of the test checks that Gabble correctly alters on which lists
+    contacts appear based on the google:roster attributes and special-cases.
+    """
 
     event = q.expect('stream-iq', query_ns=ns.ROSTER)
     query = event.query
@@ -69,6 +85,16 @@ def test(q, bus, conn, stream):
     add_roster_item(query, 'this-is-a-jid@badger.com', 'none', True)
     add_roster_item(query, 'lp-bug-298293@gmail.com', 'both', False,
         {'gr:autosub': 'true'})
+    # These contacts are blocked but we're subscribed to them, so they should
+    # show up in all of the lists.
+    add_roster_item(query, 'blocked-but-subscribed@boards.ca', 'both', False,
+        {'gr:t': 'B'})
+    add_roster_item(query, 'music-is-math@boards.ca', 'both', False,
+        {'gr:t': 'B'})
+    # This contact is blocked, and we have no other subscription to them; so,
+    # they should not show up in 'stored'.
+    add_roster_item(query, 'blocked-and-no-sub@boards.ca', 'none', False,
+        {'gr:t': 'B'})
 
     # Send back the roster
     stream.send(result)
@@ -79,14 +105,30 @@ def test(q, bus, conn, stream):
     # <https://bugs.launchpad.net/ubuntu/+source/telepathy-gabble/+bug/398293>,
     # where Gabble was incorrectly hiding valid contacts.
 
-    expected_contacts = ['lp-bug-298293@gmail.com']
+    mutually_subscribed_contacts = ['lp-bug-298293@gmail.com',
+        'blocked-but-subscribed@boards.ca',
+        'music-is-math@boards.ca']
     rp_contacts = ['this-is-a-jid@badger.com']
+    blocked_contacts = ['blocked-but-subscribed@boards.ca',
+        'blocked-and-no-sub@boards.ca',
+        'music-is-math@boards.ca']
 
-    publish = expect_list_channel(q, bus, conn, 'publish', expected_contacts)
+    publish = expect_list_channel(q, bus, conn, 'publish',
+        mutually_subscribed_contacts)
     subscribe = expect_list_channel(q, bus, conn, 'subscribe',
-        expected_contacts, rp_contacts=rp_contacts)
+        mutually_subscribed_contacts, rp_contacts=rp_contacts)
     stored = expect_list_channel(q, bus, conn, 'stored',
-        expected_contacts+rp_contacts)
+        mutually_subscribed_contacts+rp_contacts)
+    deny = expect_list_channel(q, bus, conn, 'deny',
+        blocked_contacts)
+
+    return (publish, subscribe, stored, deny)
+
+def test_flickering(q, bus, conn, stream, subscribe):
+    """
+    Google's server is buggy, and subscription state transitions "flicker"
+    sometimes. Here, we test that Gabble is suppressing the flickers.
+    """
 
     contact = 'bob@foo.com'
     handle = conn.RequestHandles(cs.HT_CONTACT, ['bob@foo.com'])[0]
@@ -124,12 +166,6 @@ def test(q, bus, conn, stream):
             "none", True)
     stream.send(iq)
 
-    def is_stored(event):
-        return event.path.endswith('/stored')
-
-    def is_subscribe(event):
-        return event.path.endswith('/subscribe')
-
     # Gabble should report this update to the UI.
     q.expect_many(
         EventPattern('dbus-signal', signal='MembersChanged',
@@ -209,5 +245,263 @@ def test(q, bus, conn, stream):
     sync_dbus(bus, q, conn)
     q.unforbid_events(change_events)
 
+# This event is forbidden in all of the deny tests!
+remove_events = [
+    EventPattern('stream-iq', query_ns=ns.ROSTER,
+        predicate=(lambda event:
+            event.query.firstChildElement()['subscription'] == 'remove'),
+        )
+    ]
+
+def test_deny_simple(q, bus, conn, stream, stored, deny):
+    """
+    If we remove a blocked contact from 'stored', they shouldn't actually be
+    removed from the roster: rather, we should cancel both subscription
+    directions, at which point they will vanish from 'stored', while
+    remaining on 'deny'.
+    """
+
+    contact = 'blocked-but-subscribed@boards.ca'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0]
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    stored.Group.RemoveMembers([handle], "")
+
+    q.forbid_events(remove_events)
+
+    q.expect_many(
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribe'),
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribed'),
+        )
+
+    # Our server sends roster pushes in response to our unsubscribe and
+    # unsubscribed commands.
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "from", False, attrs={'gr:t': 'B'}))
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False, attrs={'gr:t': 'B'}))
+
+    # As a result they should drop off all three non-deny lists, but not fall
+    # off deny:
+    q.expect_many(
+        *[ EventPattern('dbus-signal', signal='MembersChanged',
+                        args=['', [], [handle], [], [], 0, cs.GC_REASON_NONE],
+                        predicate=p)
+           for p in [is_stored, is_subscribe, is_publish]
+         ])
+
+    assertContains(handle,
+        deny.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.unforbid_events(remove_events)
+
+def test_deny_overlap_one(q, bus, conn, stream, subscribe, stored, deny):
+    """
+    Here's a tricker case: blocking a contact, and then removing them before
+    the server's responded to the block request.
+    """
+
+    # As we saw in test_flickering(), we have a subscription to Bob,
+    # everything's peachy.
+    contact = 'bob@foo.com'
+    handle = conn.RequestHandles(cs.HT_CONTACT, ['bob@foo.com'])[0]
+
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        subscribe.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.forbid_events(remove_events)
+
+    # But then we have a falling out. In a blind rage, I block Bob:
+    call_async(q, deny.Group, 'AddMembers', [handle], "")
+    event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    item = event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('B', item[(ns.GOOGLE_ROSTER, 't')])
+
+    # Then — *before the server has replied* — I remove him from stored.
+    call_async(q, stored.Group, 'RemoveMembers', [handle], "")
+
+    # subscription='remove' is still forbidden from above. So we sync to ensure
+    # that Gabble's received RemoveMembers, and if it's going to send us a
+    # remove (or premature <presence type='unsubscribe'/>) we catch it.
+    sync_dbus(bus, q, conn)
+    sync_stream(q, stream)
+
+    # So now we send a roster push and reply for the block request.
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+        'to', False, attrs={ 'gr:t': 'B' }))
+    acknowledge_iq(stream, event.stanza)
+
+    # At which point, Bob should appear on 'deny', and Gabble should send an
+    # unsubscribe, but *not* an unsubscribe*d* because Bob wasn't subscribed to
+    # us!
+    unsubscribed_events = [
+        EventPattern('stream-presence', presence_type='unsubscribed')
+        ]
+    q.forbid_events(unsubscribed_events)
+
+    q.expect_many(
+        EventPattern('dbus-signal', signal='MembersChanged', predicate=is_deny,
+            args=["", [handle], [], [], [], 0, 0]),
+        EventPattern('stream-presence', to=contact,
+            presence_type='unsubscribe'),
+        )
+
+    # And our server sends us a roster push in response to unsubscribe:
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False, attrs={'gr:t': 'B'}))
+
+    # As a result, Gabble makes Bob fall off subscribe and stored.
+    q.expect_many(
+        EventPattern('dbus-signal', signal='MembersChanged',
+            predicate=is_subscribe,
+            args=["", [], [handle], [], [], 0, 0]),
+        EventPattern('dbus-signal', signal='MembersChanged',
+            predicate=is_stored,
+            args=["", [], [handle], [], [], 0, 0]),
+        )
+
+    # And he should definitely still be on deny. That rascal.
+    assertContains(handle,
+        deny.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    q.unforbid_events(unsubscribed_events)
+    q.unforbid_events(remove_events)
+
+def test_deny_overlap_two(q, bus, conn, stream,
+                          subscribe, publish, stored, deny):
+    """
+    Here's another tricky case: editing a contact (setting an alias, say), and
+    then while that edit's in flight, blocking and remove the contact.
+    """
+
+    # This contact was on our roster when we started.
+    contact = 'lp-bug-298293@gmail.com'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0]
+
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        subscribe.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        publish.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    # Once again, at no point in this test should anyone be removed outright.
+    q.forbid_events(remove_events)
+
+    # First up, we edit the contact's alias, triggering a roster update from
+    # the client.
+    conn.Aliasing.SetAliases({handle: 'oh! the huge manatee!'})
+    event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    item = event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('oh! the huge manatee!', item['name'])
+
+    # Before the server responds, we block and remove the contact. The edits
+    # should be queued...
+    patterns = [
+        EventPattern('stream-iq', query_ns=ns.ROSTER),
+        EventPattern('stream-presence', presence_type='unsubscribed'),
+        EventPattern('stream-presence', presence_type='unsubscribe'),
+        ]
+    q.forbid_events(patterns)
+
+    deny.Group.AddMembers([handle], '')
+    stored.Group.RemoveMembers([handle], '')
+
+    # Make sure if the edits are sent prematurely, we've got them.
+    sync_stream(q, stream)
+    q.unforbid_events(patterns)
+
+    # Okay, now we respond to the alias update. At this point we expect an
+    # update to gr:t=B, leaving subscription=both intact, and subscription
+    # cancellations.
+    acknowledge_iq(stream, event.stanza)
+    roster_event, _, _ = q.expect_many(*patterns)
+
+    item = roster_event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertEquals('B', item[(ns.GOOGLE_ROSTER, 't')])
+
+    # And we're done. Clean up.
+    q.unforbid_events(remove_events)
+
+def test_deny_unblock_remove(q, bus, conn, stream, stored, deny):
+    """
+    Test unblocking a contact, and, while that request is pending, deleting
+    them.
+    """
+
+    # This contact was on our roster, blocked and subscribed, when we started.
+    contact = 'music-is-math@boards.ca'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0]
+
+    # They're blocked, and we have a bidi subscription, so they should be on
+    # deny and stored. (We already checked this earlier, but we've been messing
+    # with the roster so let's be sure the preconditions are okay...)
+    assertContains(handle,
+        deny.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+    assertContains(handle,
+        stored.Properties.Get(cs.CHANNEL_IFACE_GROUP, "Members"))
+
+    # Unblock them.
+    deny.Group.RemoveMembers([handle], '')
+
+    roster_event = q.expect('stream-iq', query_ns=ns.ROSTER)
+    item = roster_event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+    assertDoesNotContain((ns.GOOGLE_ROSTER, 't'), item.attributes)
+
+    # If we now remove them from stored, the edit shouldn't be sent until the
+    # unblock event has had a reply.
+    q.forbid_events(remove_events)
+    stored.Group.RemoveMembers([handle], '')
+
+    # Make sure if the remove is sent prematurely, we catch it.
+    sync_stream(q, stream)
+    q.unforbid_events(remove_events)
+
+    # So now we send a roster push and reply for the unblock request.
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+        'both', False, attrs={}))
+    acknowledge_iq(stream, roster_event.stanza)
+
+    # And on receiving the push and reply, Gabble should show them being
+    # removed from deny, and send a remove.
+
+    _, roster_event = q.expect_many(
+        EventPattern('dbus-signal', signal='MembersChanged',
+            args=['', [], [handle], [], [], 0, cs.GC_REASON_NONE],
+            predicate=is_deny),
+        remove_events[0],
+        )
+    item = roster_event.query.firstChildElement()
+    assertEquals(contact, item['jid'])
+
+    stream.send(make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+        'remove', False, attrs={}))
+    acknowledge_iq(stream, roster_event.stanza)
+
+    q.expect('dbus-signal', signal='MembersChanged',
+        args=['', [], [handle], [], [], 0, cs.GC_REASON_NONE],
+        predicate=is_stored)
+
+
+def test(q, bus, conn, stream):
+    conn.Connect()
+
+    publish, subscribe, stored, deny = test_inital_roster(q, bus, conn, stream)
+
+    test_flickering(q, bus, conn, stream, subscribe)
+    test_deny_simple(q, bus, conn, stream, stored, deny)
+    test_deny_overlap_one(q, bus, conn, stream, subscribe, stored, deny)
+    test_deny_overlap_two(q, bus, conn, stream,
+        subscribe, publish, stored, deny)
+    test_deny_unblock_remove(q, bus, conn, stream, stored, deny)
+
 if __name__ == '__main__':
     exec_test(test, protocol=GoogleXmlStream)