Bug 40518

http://cgit.collabora.com/git/user/bari/telepathy-gabble.git/log/?h=local-pending-flicker prevents Gabble from removing local pending contacts from the stored list. It's a trick similar to what we already to in the case of remote pending contacts.

+    # We add Alice
+    presence = domish.Element(('jabber:client', 'presence'))
+    presence['from'] = contact
+    presence['type'] = 'subscribe'
+    stream.send(presence)

This comment is wrong. What's happening here is that Alice is trying to subscribe to *us*.

I think it's worth explicitly saying in either the code comment or the test that Google's not striiiictly speaking buggy:subscription='none' on the roster item is not technically a change if they weren't on the roster before, it just means that they're now explicitly on the roster with subscription='none', as opposed to not on the roster (implicitly with subscription='none').

+    When somebody asks to subscribe to us, Google sends the subscription
+    request and then a roster update saying there is no subscription.

So here, for instance: the roster update doesn't technically contradict the request.

We should test that receiving <presence type='unsubscribe'/> followed by a roster push like the one Google sends is correctly treated as notification that Alice has cancelled her subscription request: http://xmpp.org/rfcs/rfc6121.html#sub-cancel-inbound

It may already be tested, but I'm tempted to say it should be tested in this test case, too, to guarantee that ignoring the roster push doesn't have knock-on effects.

After a chat with Will on IRC, we agreed that the RFC section he suggested to test is 3.3.3 and not 3.2.3

http://xmpp.org/rfcs/rfc6121.html#sub-unsub-inbound

in the scenario for which Alice sends a un-subscription request right after her subscription request and before any approval/rejection by the user.

[Bugzilla is complaining the file I'm attaching is empty, workaround:]

http://pastebin.com/9YW4VkLB has the dbus logs for the extented test which do:
- unsubscribe
- expect ContactChanged
- send the maybe-redundant iq
- actually sees MembersChanged raised for the handle removed from storedm, which should be ignored

My commit with the extended test:
http://cgit.collabora.com/git/user/kalfa/telepathy-gabble.git/commit/?id=c32031008828651720a5661452b0e0a998ab8685

What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2), while the handle should've been in ASK state according to the last signal.
Since Marco's fix is based on the ASK state, it might be it.

-    # We add Alice
+    # Alice asks to subscribes to us

This should be a separate patch.

-    self_handle = conn.GetSelfHandle()

As should this.

+    iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact,
+            "none", False)
+    stream.send(iq)

You should move the comment

    # Now we send the spurious roster update with subscribe="none" and verify
    # that nothing happens in reaction to that

up to before you test this new case.

(In reply to comment #4)
> What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2),
> while the handle should've been in ASK state according to the last signal.
> Since Marco's fix is based on the ASK state, it might be it.

I don't think it's changing from No to Removed_Remotely.


+    q.expect('dbus-signal', signal='ContactsChanged',
+            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
+                cs.SUBSCRIPTION_STATE_REMOVED_REMOTELY, '')}, []])

The first two fields of the tuple mean:

• We are not subscribing to the contact's presence;
• The contact removed their request to see our presence.

The previous state change, in response to their subscription request, was:

    q.expect('dbus-signal', signal='ContactsChanged',
            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
                cs.SUBSCRIPTION_STATE_ASK, '')}, []])

which means:

• We are not subscribing to the contact's presence;
• The contact has asked to see our presence.

So I think this test looks right.

-if test $sleep != 0; then
+if [ "$sleep" -ne 0 ]; then

What's this for?

(In reply to comment #5)
> -    # We add Alice
> +    # Alice asks to subscribes to us
> 
> This should be a separate patch.
> 
> -    self_handle = conn.GetSelfHandle()
> As should this.

OK.
This test has been introduced by Marco, so I guess that it means it will be committed as two separate patch sets, one from Marco and the second from me atop Marco's work.



> +    iq = make_set_roster_iq(stream, 'test@localhost/Resource', contact,
> +            "none", False)
> +    stream.send(iq)
> 
> You should move the comment
> 
>     # Now we send the spurious roster update with subscribe="none" and verify
>     # that nothing happens in reaction to that
> 
> up to before you test this new case.
> 
> (In reply to comment #4)
> > What I seems odd is the ContactsChanged with NO -> REMOVED_REMOTELY (1->2),
> > while the handle should've been in ASK state according to the last signal.
> > Since Marco's fix is based on the ASK state, it might be it.
> 
> I don't think it's changing from No to Removed_Remotely.
> 
> 
> +    q.expect('dbus-signal', signal='ContactsChanged',
> +            args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
> +                cs.SUBSCRIPTION_STATE_REMOVED_REMOTELY, '')}, []])
> 
> The first two fields of the tuple mean:
> 
> • We are not subscribing to the contact's presence;
> • The contact removed their request to see our presence.
> 
> The previous state change, in response to their subscription request, was:
> 
>     q.expect('dbus-signal', signal='ContactsChanged',
>             args=[{handle: (cs.SUBSCRIPTION_STATE_NO,
>                 cs.SUBSCRIPTION_STATE_ASK, '')}, []])
> 
> which means:
> 
> • We are not subscribing to the contact's presence;
> • The contact has asked to see our presence.

Right, the first field is the sate of "subscribe" and the second of "publish"

http://telepathy.freedesktop.org/spec/Connection_Interface_Contact_List.html#Struct:Contact_Subscriptions

it does not represent a transition. my bad.

> So I think this test looks right.

It the test looks correct, the fact that it fails means that we need to look a way to fix it in gabble.

I can look at it, if Marco agrees (he's quite busy AFAIK).

> -if test $sleep != 0; then
> +if [ "$sleep" -ne 0 ]; then
> 
> What's this for?

The shell complained and I had to fix to be able to run the test.

It should be in a different commit as well, if the patch is OK.

(In reply to comment #6)
> (In reply to comment #5)
> > So I think this test looks right.
> 
> It the test looks correct, the fact that it fails means that we need to look a
> way to fix it in gabble.

I found an error in the test, and fixed it: http://cgit.collabora.com/git/user/wjt/telepathy-gabble-wjt.git/commit/?h=local-pending-flicker&id=69e9f76b6c736e9e1bed148c23eb45a583862559

Created attachment 53048 [details] [review]
remove unused symbol

Created attachment 53049 [details] [review]
Alice is requesting a subscription

Created attachment 53050 [details] [review]
expect both signals and forbid them only for publish

First part of Will's fix, related to Marco's part

Created attachment 53051 [details] [review]
test subscription cancellation

My part of the test + Will's fixes

Created attachment 53052 [details] [review]
Marco's patch, first part of the test case, without Will's fix

this is actually the first one fo the serie, Marco's first patch.

then there are the others, in the attached order.

these patches look good now.

Merged to 0.12 (will be in 0.12.8), 0.14 (will be in 0.14.1) and master (will be in 0.15.0).