Wordpress-android: Push Notifications not working for new logins

[oguzkocer]

Steps to reproduce:

1. Install a fresh copy of the app. The tested commit (latest develop) is: 7788b2228c666806946c3a33a6e6e1fcdf6dd7ad
2. Login with a WordPress.com account
3. Trigger a notification from your web browser. Commenting on a post, liking etc.
4. Verify that you don't get a push notification
5. Re-launch the app
6. Trigger another notification
7. Verify that you get the push notification
8. Test this again with logging out and logging in as well.

I believe this issue has been present for a long time and likely reason is that we are not registering the token when the user signs in, but instead during each launch when we get it from Google.

[oguzkocer]

It looks like @mzorz attempted to fix this issue in #7876 but unfortunately that solution will not work at all times. If you login with password, the WPMainActivity will not be present, so the WPMainActivity.onAuthenticationChanged will not be triggered. We either have to do the same thing in some of the other onAuthenticationChanged callbacks, which I really don't like, or we have to ensure that the onAuthenticationChanged callback we have this on is present at all times. This is probably not the only issue about observing login/logout events.

[mzorz]

Good catch @oguzkocer , I wonder if we can have something like EventBus Sticky Events of the sorts for FluxC? FluxC already uses EventBus at its core, so it shouldn't be much of a pain to make it configurable as to emit sticky events on certain events such as this - or even do by default on this kind of events only and make the consumer of FluxC aware of this. What do you think?

[daniloercoli]

If you login with password, the WPMainActivity will not be present, so the WPMainActivity.onAuthenticationChanged will not be triggered.

Weird, I tested it by logging me in with username and password, and that callback was called correctly.

[daniloercoli]

I quickly investigated the cause of the issue ^, since I was seeing the callback called correctly.
It was because I followed the test steps, and did a logout before trying to login again. Android have WPMainActivity still in memory and executes the callback and register for PNs.
If you start with empty app from scratch the callback is not called as reported in this issue.

[oguzkocer]

I wonder if we can have something like EventBus Sticky Events of the sorts for FluxC?

@mzorz Even though using a sticky event fixes this issue, it also brings in another mechanic to keep in mind while working with FluxC events. Currently, when we handle the event, we have the assumption that the event was triggered just now. If we do the sticky event, it breaks this assumption and we might start seeing weird things happening in the app, for example, because an old event was handled by an Activity created long after it's dispatched. Even if that's unlikely to happen, I don't think we should make it harder for ourselves to understand what will happen when a FluxC event is dispatched.

As far as I understand it, the correct way to handle such events (not just this one) is to have an object observing for them at all times. I guess that'll mean a Service in Android. I'll look into this a bit more today and see what we can do.

[mzorz]

Currently, when we handle the event, we have the assumption that the event was triggered just now.

Well as per all FluxC standards, when you receive the event you should actually query FluxC again for the latest and real value of the thing that changed. You shouldn't care about reading an "old" value.

[oguzkocer]

Well as per all FluxC standards, when you receive the event you should actually query FluxC again for the latest and real value of the thing that changed. You shouldn't care about reading an "old" value.

I don't think I said anything about reading any values. Not all FluxC events are about fetching and reading a value and not all FluxC events even has a DB backing it.

[mzorz]

I don't think I said anything about reading any values.

You didn't, I did. I meant to say, that it is common (and recommended) practice in FluxC-consumers to use events to be notified that something changed only, and then it is expected that the consumer reads FluxC to obtain the latest value where applicable.

You're right about not all FluxC events being backed by a DB entity, but that, in any case, is an implementation detail belonging to FluxC, which may as well implement the same behavior with a sticky event, preferences or anything (not necessary for it to implement a SQLite table just to "remember" a value, for example).

Simplifying: the consumer sends a request to FluxC to know something -> it at some point (immediately or not) gets notified by FluxC about it -> it should query FluxC for details.

Services and observables may sound robust and the way to go for a more complete and more general approach, but the simplicity of keeping a value that may not get updated too often makes exploring other options sound, at first at least, fitter to me.

Hope it clarified where I was coming from!

[oguzkocer]

Services and observables may sound robust and the way to go for a more complete and more general approach, but the simplicity of keeping a value that may not get updated too often makes exploring other options sound, at first at least, fitter to me.

I understand where you're coming from, but the thing is, if we are not going for the "correct" implementation, there are just better options to fix this issue then introduce a concept that might complicate things for other developers. I say "correct" between quotation marks, because I don't even know if that's the best approach yet, I simply need to look into it further.

I am very well aware of the FluxC practices, I've been pushing to make them better for some time now, including the single source of truth principle which you mentioned. I am very happy to hear that you're aware of them and try to follow them, but unfortunately that's not the case for everyone. One of the most important things for me when I am working on libraries, or code that would be used by others, is to make sure it's as easy to grasp as possible without any gotchas to think about, if possible. Unfortunately FluxC fails on that regard and it being an event based approach doesn't help much.

Keep in mind that even though you clearly understand how FluxC events worked, your initial solution didn't address the problem fully. And that's definitely not a blame, it's just such an easy mistake to make, because it requires you to know how and when the WPMainActivity is created/destroyed. That shouldn't be the case. If we had a class/service that handles all of this, and whenever there is a change needs to be made, we point people to that class/service, that'll be a very powerful thing for us. A lot less exploration and definitely a lot less knowledge required. Most developers would try to find the correct instance of OnAuthenticationChanged for this issue and add the token implementation there, which is mostly the correct thing to do. What we should make sure is that there is a "correct" instance of OnAuthenticationChanged for people to add these type of stuff in.

Having sticky events introduce a similar problem to the one mentioned above. It just requires people to know more and think about more. If we want a simple and quick fix, we can already do so as shown in the PR. If we want a proper fix, I think there is just a better option.

Hope that makes sense!