From 8fcf944c1afbcd98a5cbf582e33d88d877a6980f Mon Sep 17 00:00:00 2001 From: Marco Barisione Date: Wed, 31 Aug 2011 17:31:24 +0100 Subject: Roster: don't hide local pending contacts from stored --- src/roster.c | 13 +++++++--- tests/twisted/roster/test-google-roster.py | 41 ++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/src/roster.c b/src/roster.c index 5656662a5..4fd9c74af 100644 --- a/src/roster.c +++ b/src/roster.c @@ -1233,12 +1233,17 @@ process_roster ( case GABBLE_ROSTER_SUBSCRIPTION_FROM: case GABBLE_ROSTER_SUBSCRIPTION_BOTH: if (google_roster && - /* Don't hide contacts from stored if they're remote pending. - * This works around Google Talk flickering ask="subscribe" - * when you try to subscribe to someone; see - * test-google-roster.py. + /* Don't hide contacts from stored if they're pending. + * This works around two Google Talk issues: + * - When you try to subscribe to someone, you get a flickering + * ask="subscribe"; + * - When somebody tries to subscribe to you, you get a presence + * with type="subscribe" followed by a roster update with + * subscribe="none". + * See test-google-roster.py for more details. */ item->subscribe != TP_SUBSCRIPTION_STATE_ASK && + item->publish != TP_SUBSCRIPTION_STATE_ASK && !_google_roster_item_should_keep (jid, item)) { tp_handle_set_remove (changed, handle); diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py index 630e3c314..790501c88 100644 --- a/tests/twisted/roster/test-google-roster.py +++ b/tests/twisted/roster/test-google-roster.py @@ -131,8 +131,9 @@ def test_inital_roster(q, bus, conn, stream): 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. + Google's server is buggy; when asking to subscribe to somebody, the + subscription state transitions "flicker" sometimes. Here, we test that + Gabble is suppressing the flickers. """ self_handle = conn.GetSelfHandle() @@ -264,6 +265,41 @@ def test_flickering(q, bus, conn, stream, subscribe): sync_dbus(bus, q, conn) q.unforbid_events(change_events) +def test_local_pending(q, bus, conn, stream, subscribe): + """ + When somebody asks to subscribe to us, Google sends the subscription + request and then a roster update saying there is no subscription. + This causes the contact to appear in local pending and then disappear. + Here, we test that Gabble is suppressing the flickers. + """ + + self_handle = conn.GetSelfHandle() + contact = 'alice@foo.com' + handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0] + + # We add Alice + presence = domish.Element(('jabber:client', 'presence')) + presence['from'] = contact + presence['type'] = 'subscribe' + stream.send(presence) + + q.expect('dbus-signal', signal='ContactsChanged', + args=[{handle: (cs.SUBSCRIPTION_STATE_NO, + cs.SUBSCRIPTION_STATE_ASK, '')}, []]) + + # Now we send the spurious roster update with subscribe="none" and verify + # that nothing happens in reaction to that + change_event = EventPattern('dbus-signal', signal='MembersChanged') + q.forbid_events([change_event]) + + iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact, + "none", False) + stream.send(iq) + + sync_stream(q, stream) + sync_dbus(bus, q, conn) + q.unforbid_events([change_event]) + # This event is forbidden in all of the deny tests! remove_events = [ EventPattern('stream-iq', query_ns=ns.ROSTER, @@ -520,6 +556,7 @@ def test(q, bus, conn, stream): publish, subscribe, stored, deny = test_inital_roster(q, bus, conn, stream) test_flickering(q, bus, conn, stream, subscribe) + test_local_pending(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, -- cgit v1.2.1 From 273654bfb07e5ff593b162dd455f739f697b71b8 Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Wed, 2 Nov 2011 11:39:21 +0000 Subject: Remove useless symbol in test_local_pending --- tests/twisted/roster/test-google-roster.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py index 790501c88..38ee55780 100644 --- a/tests/twisted/roster/test-google-roster.py +++ b/tests/twisted/roster/test-google-roster.py @@ -273,7 +273,6 @@ def test_local_pending(q, bus, conn, stream, subscribe): Here, we test that Gabble is suppressing the flickers. """ - self_handle = conn.GetSelfHandle() contact = 'alice@foo.com' handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0] -- cgit v1.2.1 From 284779859a67e2dfceb4d9930df81e452d5a4b3a Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Wed, 2 Nov 2011 11:39:46 +0000 Subject: fix comment: Alice is just requesting subscription --- tests/twisted/roster/test-google-roster.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py index 38ee55780..3b8d8f6c3 100644 --- a/tests/twisted/roster/test-google-roster.py +++ b/tests/twisted/roster/test-google-roster.py @@ -276,7 +276,7 @@ def test_local_pending(q, bus, conn, stream, subscribe): contact = 'alice@foo.com' handle = conn.RequestHandles(cs.HT_CONTACT, [contact])[0] - # We add Alice + # Alice asks to subscribes to us presence = domish.Element(('jabber:client', 'presence')) presence['from'] = contact presence['type'] = 'subscribe' -- cgit v1.2.1 From 31c2cd55912c72c82cba28202f220a80c8b4faca Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Wed, 2 Nov 2011 13:20:15 +0000 Subject: expect MembersChanged AND ContactsChanged on sub-request We should explicitly expect the relevant state transition on the publish channel; and when forbidding events, we just forbid them from occurring on the publish channel. (fix by Will Thompson, thanks Will) --- tests/twisted/roster/test-google-roster.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py index 3b8d8f6c3..b419cd9e8 100644 --- a/tests/twisted/roster/test-google-roster.py +++ b/tests/twisted/roster/test-google-roster.py @@ -282,13 +282,19 @@ def test_local_pending(q, bus, conn, stream, subscribe): presence['type'] = 'subscribe' stream.send(presence) - q.expect('dbus-signal', signal='ContactsChanged', + q.expect_many( + EventPattern('dbus-signal', signal='MembersChanged', + args=['', [], [], [handle], [], handle, cs.GC_REASON_NONE], + predicate=is_publish), + EventPattern('dbus-signal', signal='ContactsChanged', args=[{handle: (cs.SUBSCRIPTION_STATE_NO, - cs.SUBSCRIPTION_STATE_ASK, '')}, []]) + cs.SUBSCRIPTION_STATE_ASK, '')}, []]), + ) # Now we send the spurious roster update with subscribe="none" and verify - # that nothing happens in reaction to that - change_event = EventPattern('dbus-signal', signal='MembersChanged') + # that nothing happens to her publish state in reaction to that + change_event = EventPattern('dbus-signal', signal='MembersChanged', + predicate=is_publish) q.forbid_events([change_event]) iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact, -- cgit v1.2.1 From 6cb2b5d0f9e4a164537e3861f787e603d02bb8e4 Mon Sep 17 00:00:00 2001 From: Cosimo Alfarano Date: Wed, 2 Nov 2011 13:24:09 +0000 Subject: add case for "cancel subscription" to test_local_pending --- tests/twisted/roster/test-google-roster.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/twisted/roster/test-google-roster.py b/tests/twisted/roster/test-google-roster.py index b419cd9e8..3773377df 100644 --- a/tests/twisted/roster/test-google-roster.py +++ b/tests/twisted/roster/test-google-roster.py @@ -305,6 +305,35 @@ def test_local_pending(q, bus, conn, stream, subscribe): sync_dbus(bus, q, conn) q.unforbid_events([change_event]) + # Now we cancel alice's subscription request and verify that if the + # redundant IQ is sent again, it's safely handled + presence = domish.Element(('jabber:client', 'presence')) + presence['from'] = contact + presence['type'] = 'unsubscribe' + stream.send(presence) + + q.expect_many( + EventPattern('dbus-signal', signal='MembersChanged', + args=['', [], [handle], [], [], handle, cs.GC_REASON_NONE], + predicate=is_publish), + EventPattern('dbus-signal', signal='ContactsChanged', + args=[{handle: (cs.SUBSCRIPTION_STATE_NO, + cs.SUBSCRIPTION_STATE_REMOVED_REMOTELY, '')}, []]), + ) + + # Now we send a roster roster update with subscribe="none" again (which + # doesn't change anything, it just confirms what we already knew) and + # verify that nothing happens to her publish state in reaction to that. + q.forbid_events([change_event]) + + iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact, + "none", False) + stream.send(iq) + + sync_stream(q, stream) + sync_dbus(bus, q, conn) + q.unforbid_events([change_event]) + # This event is forbidden in all of the deny tests! remove_events = [ EventPattern('stream-iq', query_ns=ns.ROSTER, -- cgit v1.2.1