search

wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

Remove Simperium #4760

Closed daniloercoli closed 7 years ago

daniloercoli commented 7 years ago

Title has it ;)

This PR is the sum up of the following PR's, which have been reviewed and tested separately:

Closes #4661

Thanks to @roundhill for the initial starting branch! 🙇

Test Plan

Scenario A: Logout

  1. Log into a dotcom account
  2. Verify that the notifications do show up in the Notes tab
  3. Logout, and log into a self hosted site
  4. Verify that the Notifications tab is empty

Scenario B: Unseen Badge

  1. Log into the app, and send it to background
  2. Receive a Push Notification, and tap on it
  3. Tap the back button. The app should take you on the notifications list 4 Receive another Push Notification, and verify that the badge is orange.
  4. Scroll a bit the list and the orange badge should disappear.

Scenario C: New notes float button Badge

  1. Log into the app, and go to the Notifications tab
  2. Scroll the list down
  3. Receive a Push Notification
  4. The app should show the New Notes available button
  5. Tap on the button and the app should scroll to the top 5b. Repeat the steps above, but this time scroll the list instead of tapping the button. The app should dismiss the button and the badge.

Scenario D: Unread Notes

  1. Log into a dotcom account
  2. Pick an unread note, and tap on it
  3. Hit back
  4. Verify that the note is not marked as unread anymore
  5. Verify on dotcom // another device, that the "Mark as Read" OP was propagated.

Scenario E: Quick Actions (Like)

  1. Log into a dotcom account, and send the app to BG
  2. Receive a Push Notification
  3. Expand the note view and like the comment
  4. Verify on the browser that the comment was liked

Scenario F: Quick Actions (Approve)

  1. Log into a dotcom account, and send the app to BG
  2. Receive a Push Notification
  3. Expand the note view and approve the comment
  4. Verify on the browser that the comment was approved

Scenario G: Quick Actions (2FA)

  1. Log into a dotcom account, and send the app to BG
  2. Open a new browser and log into your 2FA protected account. Choose to verify the login with the app
  3. Receive a 2FA Push Notification
  4. Tap on approve
  5. Verify the browser that the login was approved

Scenario H: Quick Actions (Like)

  1. Log into a dotcom account, enable the PIN Lock, and send the app to BG
  2. Receive a Push Notification
  3. Expand the note view and check that quick actions are not there.

Scenario I: Background Synch

  1. Log into a dotcom account using a device
  2. Go to the Me Tab (or any other tabs except Notifications)
  3. Send the app to BG
  4. Trigger any event that would trigger a new notification (receive a like, comment, etc)
  5. After few secs bring the app to foreground without tapping on the notification item in the notification center
  6. Verify that the badge is ON
  7. Tap on the Notifications tab and verify that the new notification is already available in the list
  8. Verify that the badge is OFF
  9. Tap on the new Note
  10. Go back to the notifications list and verify the note is marked as read.

--

Note: There are still some edge cases that are not correctly handled both by this branch and by develop (current version of notifications with Simperium).

mzorz commented 7 years ago

Ok, here's my feedback after testing the functionality on this branch

For case A I have this problem:

  1. First I delete the app from my handset, then run the develop version, to try to test the DB version change. Sign in to my wpcom.
  2. Compile and run the remove/simperium-master branch, it works OK.
  3. logout.

Now, starting the actual case A:

  1. I try to login with my wpcom account and verify there’s the Notifications in there.
  2. logout
  3. Try to login with a self hosted site, apparently it does, but 2 things happen: a) a toast is shown, saying Notifications couldn’t be loaded b) a dialog appears with an error “Connection error: incorrect username or password”. Video here: https://cloudup.com/c-t8oaW2WYo (please download it and play it locally - I couldn’t make it play within the browser).

Didn’t investigate further, but might be something with the DB? Because even with that error, I can dismiss the dialog by pressing CANCEL and then got to the Notifications tab and I can see the list of notifications for the previously logged in user (that is, my wpcom user).

Scenario B: when I tap the back button on step 3, the orange dot is gone. If I receive another PN, the badge turns into its orange dot version, but there’s no count as per what the test describes (I think I know what you mean, but it does;t fit the description - please forget this comment if you know this is working and it’s just a matter of wording/language for the test).

Scenario C: When I’m on the notifications list and scroll down to the middle of the list, then have a PN received, in order to make the floating button appear, the floating button appears then quickly disappears. See video here https://cloudup.com/cDxc7V6Tq2n

Scenario D: couldn’t test as per problem in test C

Scenario E: OK Scenario F: OK Scenario G: OK Scenario H: Not ok done things slightly differently and found an issue here:

  1. app on the foreground, with PIN lock set, but app left in the foreground
  2. have a PN be sent
  3. the PN is received, but it has no quick actions in it (should be there). See demo https://cloudup.com/cZmfJqoe-FC

I’ll try to tackle the issues with scenario H tomorrow as time allows (we’ll see!)

Scenario I: OK

daniloercoli commented 7 years ago

Scenario B: when I tap the back button on step 3, the orange dot is gone. If I receive another PN, the badge turns into its orange dot version, but there’s no count as per what the test describes (I think I know what you mean, but it does;t fit the description - please forget this comment if you know this is working and it’s just a matter of wording/language for the test).

I've updated the description of the test, since the count marker isn't there. Marking this as working.

daniloercoli commented 7 years ago

Now, starting the actual case A:

  1. I try to login with my wpcom account and verify there’s the Notifications in there.
  2. logout
  3. Try to login with a self hosted site, apparently it does, but 2 things happen: a) a toast is shown, saying Notifications couldn’t be loaded b) a dialog appears with an error “Connection error: incorrect username or password”. Video here: https://cloudup.com/c-t8oaW2WYo (please download it and play it locally - I couldn’t make it play within the browser).

Didn’t investigate further, but might be something with the DB? Because even with that error, I can dismiss the dialog by pressing CANCEL and then got to the Notifications tab and I can see the list of notifications for the previously logged in user (that is, my wpcom user).

Sorry, forgot to push my latest changes yesterday about .org blogs :face_with_head_bandage: It should be fixed now.

daniloercoli commented 7 years ago

Scenario H: Not ok done things slightly differently and found an issue here:

  1. app on the foreground, with PIN lock set, but app left in the foreground
  2. have a PN be sent
  3. the PN is received, but it has no quick actions in it (should be there). See demo https://cloudup.com/cZmfJqoe-FC

I see there are problems with PNs received when the app is in FG. I bet we have the same problems in develop, since most of the PNs handling code is the same. Basically I tested the app by opening it and going to the Reader. Kept the app in FG, and made comments on a blog of mine. PNs are received on the device, but the sound is not always played. Notifications occasionally appear in the notifications center for a quick while, sometime they don't appear at all.

And I can reproduce that action buttons are missing if PinLock is enabled.

daniloercoli commented 7 years ago

@mzorz About Scenario H - I've just tested develop and there are almost the same problems you reported above in that branch. Since they're not strictly related to remove-simperium, would you mind to open a new ticket for them, and fix them next week when you're back from the meetup?

(Btw, if changes are really easy we can squeeze them into this PR).

daniloercoli commented 7 years ago

Scenario C: When I’m on the notifications list and scroll down to the middle of the list, then have a PN received, in order to make the floating button appear, the floating button appears then quickly disappears. See video here https://cloudup.com/cDxc7V6Tq2n

Fixed!

This is ready for another pass!

daniloercoli commented 7 years ago

@roundhill @kwonye Would you mind to take a look at this, so we can try to get it merged before EOW?

mzorz commented 7 years ago

Here's my results for this second pass:

Scenario A: OK Scenario B: OK Scenario C: OK Scenario D: Can see the unread appearance changes in the Android app, but couldn’t see the comment being marked Read on the web notifications list.

Remaining things at this point are D and H. So check scenario D, I'll take scenario H - I'll open another PR if I see it takes too long for me to fix.

Then finally let's make a full pass again once we have these 2 ready, and I'll check the code.

daniloercoli commented 7 years ago

Scenario D: Can see the unread appearance changes in the Android app, but couldn’t see the comment being marked Read on the web notifications list.

I will check this shortly, but we should check the note on the DB or by requesting it command line, since it could be an issue on the web client. Another way to check if it was marked as read, is to manually start a PTR. When the loading is completed you've a fresh list of notes from the remote server. That note should be marked as Read.

mzorz commented 7 years ago

Another way to check if it was marked as read it to start a PTR. When the loading is completed you've a fresh list of notes from the remote server.

you mean Pull to refresh, right? If that's really refreshing the list then yes, it seems to be working alright and there must be some problem in the server for the mark as read sync. We should address it though (separately ofc)

daniloercoli commented 7 years ago

Yes, Pull to Refresh completely erases the notes table, and re-populates it with fresh data just retrieved from the REST API. Just checked point D and works for me. Note is marked as read on the server around 20/30 secs after I opened it on the mobile app.

mzorz commented 7 years ago

Ok, just pushed some very basic commits addressing

All of these have been addressed/tested on quick actions:

TEST SCENARIOS run for quick actions: a) app on the foreground, get PN -> show quick actions b) app in the background, get PN -> show quick actions c) device locked, app on the fg, get PN -> hide quick actions d) device locked, app in the bg, get PN -> hide quick actions

PIN LOCK ON states (hide quick actions on all cases): e) app on the foreground, get PN -> hide quick actions f) app in the background, get PN -> hide quick actions g) device locked, app on the fg, get PN -> hide quick actions h) device locked, app in the bg, get PN -> hide quick actions i) app fg, get a PN, then turn PIN LOCK OFF -> show quick actions j) app fg, get a PN with PIN LOCK OFF (quick actions shown), then turn PIN LOCK ON -> hide quick actions

All of them were OK for me - mind testing on your side @daniloercoli ? So I can start reviewing the code and also we can then have @roundhill and @kwonye give it a try

daniloercoli commented 7 years ago

I'm sorry Mario, but still experiencing issues with Quick Actions and the notifications rebuilding code when the app is Foreground, Both on this PR that on develop.

No problem if the app is in BG.

Test Scenario

I see the following errors in the log:

11-10 19:26:17.412 21317-25409/? I/NotifUtils: Showing notification with unreadCount of 1 and unseenCount of 1
11-10 19:26:17.757 12337-25411/? V/WordPress-NOTIFS: Received Message

------>>11-10 19:26:17.772 4800-4800/? E/NotificationService: Suppressing notification from package by user request.**

11-10 19:26:17.781 17015-24992/org.wordpress.android V/WordPress-NOTIFS: Received Message
11-10 19:26:17.794 3668-6529/? D/NuPlayerDriver: stop(0xeb5abb00)
11-10 19:26:17.794 3668-6529/? D/NuPlayerDriver: notifyListener_l(0xeb5abb00), (8, 0, 0), loop setting(0, 0)

---->  11-10 19:26:17.795 6508-6525/? W/MessageQueue: Handler (android.media.MediaPlayer$EventHandler) {6d1e7b3} sending message to a Handler on a dead thread
                                               java.lang.IllegalStateException: Handler (android.media.MediaPlayer$EventHandler) {6d1e7b3} sending message to a Handler on a dead thread
                                                   at android.os.MessageQueue.enqueueMessage(MessageQueue.java:543)
                                                   at android.os.Handler.enqueueMessage(Handler.java:643)
                                                   at android.os.Handler.sendMessageAtTime(Handler.java:612)
                                                   at android.os.Handler.sendMessageDelayed(Handler.java:582)
                                                   at android.os.Handler.sendMessage(Handler.java:519)
                                                   at android.media.MediaPlayer.postEventFromNative(MediaPlayer.java:3059)
11-10 19:26:17.795 3668-6561/? D/NuPlayerDriver: reset(0xeb5abb00) at state 8
11-10 19:26:17.808 3668-25382/? W/AMessage: failed to post message as target looper for handler 0 is gone.
11-10 19:26:17.810 3668-25382/? D/NuPlayerDriver: notifyResetComplete(0xeb5abb00)
11-10 19:26:18.342 19645-19794/? I/Icing: Indexing 8AB0FA9C63D18999CFF555337451D1917E4FE63F from com.google.android.gm
11-10 19:26:18.436 19645-19794/? I/Icing: Indexing done 8AB0FA9C63D18999CFF555337451D1917E4FE63F
11-10 19:26:18.558 21317-25409/? I/NotifUtils: Account: -808687737 vibrate: false
11-10 19:26:18.558 21317-25409/? I/NotifUtils: New email in -808687737 vibrateWhen: false, playing notification: content://settings/system/notification_sound
11-10 19:26:18.565 21317-25409/? I/NotifUtils: notifying summary notification id: 2020805896
11-10 19:26:18.576 21317-25409/? I/NotifUtils: notifying conversation notification 1437152477

I suggest you to open a new ticket for them, and fix these new issues on Quick Actions out of this PR next week. Depends if this PR get merged before the code freeze or not.

mzorz commented 7 years ago

oh thank you @daniloercoli , I see it now, checking it in. It won't be a blocker for this PR, I promise

kwonye commented 7 years ago

I suggest you to open a new ticket for them, and fix these new issues on Quick Actions out of this PR next week. Depends if this PR get merged before the code freeze or not.

Totally agree @daniloercoli, especially since @mzorz is unavailable the rest of this week.

kwonye commented 7 years ago

@daniloercoli there are a few straggling references to simperium

  1. In gradle.properties-example both wp.simperium.app* keys
  2. In res/xml/backup_scheme.xml there's <include domain="sharedpref" path="simperium.xml">
  3. In both CommentDetailFragment.java and NotificationsDetailListFragment.java there are comments referencing Simperium
  4. in AppLog.java in the utils library, SIMPERIUM is one of the tags.
mzorz commented 7 years ago

Several points to add to my review @daniloercoli :

PNs are received on the device, but the sound is not always played. Notifications occasionally appear in the notifications center for a quick while, sometime they don't appear at all.

https://github.com/wordpress-mobile/WordPress-Android/pull/4760#issuecomment-259770230

Weird thing, I can't repro the case for the "disappearing" notification on remove/simperium-master with the steps provided - did you git pull? just asking because I've seen this only with the "remove" code that I deleted in this commit https://github.com/wordpress-mobile/WordPress-Android/pull/4760/commits/700b3954135e4c747352c6b83322483d034b1fc5 and it's not happening on develop for me either.

I managed to reproduce a similar behaviour with the code in that commit reverted in place (that is, removing notifications in the onResume method for NotificationsListFragment).

Also, the sound is being played for me every time.

And I can reproduce that action buttons are missing if PinLock is enabled.

It’s OK for them to not be there at all while PinLock is enabled (even when the app is on the foreground, that's how it's coded).

More feedback

EventBus.getDefault().post(new NotificationEvents.NotificationsChanged(sActiveNotificationsMap.size() > 0));
daniloercoli commented 7 years ago

One thing that I’ve noticed though, is the orange dot seems to go away after a minute or two and the device locks it screen and then I unlock it. (the notifications are there in the status bar though).

The orange dot is removed onResume, since you're seeing the notifications list. The notifications remains in the status bar since you've removed the code that cleans them in https://github.com/wordpress-mobile/WordPress-Android/pull/4760/commits/700b3954135e4c747352c6b83322483d034b1fc5

Not sure how to solve this since since onResume is correctly called when the fragmed is resumed, but also on startup since the fragment is inside the tabbed interface.

mzorz commented 7 years ago

Not sure how to solve this since since onResume is correctly called when the fragmed is resumed, but also on startup since the fragment is inside the tabbed interface.

Let's leave it for now, it's probably a minor issue anyway (if an issue at all). Given the notifications remain accessible in the status bar, this is probably not worth it pursuing any further.

daniloercoli commented 7 years ago

Another weird thing, if you get 2 notifications (they’re grouped), and tap on the notification, you’re taken to the Notifications tab, but the “new notifications” button appears, even if I’m already being shown the top scroll position. Also the button does not go away with PTR (it should). See video here https://cloudup.com/cgBKJ9Loe4A

Wasn't able to reproduce the issue, but added some other checks on the button.

daniloercoli commented 7 years ago

@mzorz Also fixed the issue with the badge reset notifications. Added EventBus.getDefault().post(new NotificationEvents.NotificationsChanged(sActiveNotificationsMap.size() > 0)); in handleBadgeResetPN .

Side note: Don't know why, but i'm getting a lot of badge reset PNs. A lot. I haven't wpcom open on any device, nor the browser; I receive the PNs on my Android phone but they disappear after a while (Issue reported above hrs ago, also seeing it on develop). I set a breakpoint in the app, thinking that was the app marking them as read, instead the code that mark a note as read is not executed. More, since I'm seeing the same issue on develop it's not this branch that has the problem. Chrome leaked? I had to disable handleBadgeResetPN locally to keep testing this PR.

roundhill commented 7 years ago

Are we doing anything to refresh a note when the detail view opens?

Scenario:

  1. Receive an additional like on a post (say it went from 2 to 3 likes)
  2. Open push notification, you are taken to the detail view.
  3. Observe that the new liker doesn't show in the list.
daniloercoli commented 7 years ago

Are we doing anything to refresh a note when the detail view opens?

  • The single note obj is updated each time you receive a PN. With data available in the payload or requesting data with REST if the payload is not available. If you've received PNs for those likes, you should have the fresh version of the note.

@roundhill Have you received PNs?

Those 2 points above should be suffice IMO, however the details view doesn't refresh the note. Will take a quick look this morning to see if there is a smart way to check note "freshness".

roundhill commented 7 years ago

@roundhill Have you received PNs?

Yes, I'm getting pushes. I think you're right about the detail view note not refreshing.

I'm also seeing strange behavior for a specific 'like' note. It doesn't seem to ever mark as read, and when I open it and go back to the list, the note duplicates itself in the list:

screenshot_20161111-083121

mzorz commented 7 years ago

I had to disable handleBadgeResetPN locally to keep testing this PR.

yeah it's not the Android branch, it's something in the server that is triggering these badge-reset PNs. Have to say, I noticed this 2 days ago but not now anymore. Is it still happening to you? We'll need to check the server-side code to look for the root cause I think

daniloercoli commented 7 years ago

@mzorz - I tested Like notifications this morning on more than one blog, on more than one post, and with 3 tests account without having the ability to replicate the issue @roundhill is experiencing on that note.

@roundhill I've updated the code a bit, would you mind trying it again? Also, PTR could help on this case to see if note gets duplicated or are actually 2 distinct notes.

roundhill commented 7 years ago

I'm also seeing strange behavior for a specific 'like' note. It doesn't seem to ever mark as read, and when I open it and go back to the list, the note duplicates itself in the list:

I think I was running the wrong branch, nevermind ;)

roundhill commented 7 years ago

Nice, I think this is looking really good. @daniloercoli and @mzorz are you good to go for 🚢 ?

mzorz commented 7 years ago

👍 :shipit: for me 🔥

daniloercoli commented 7 years ago

:shipit:

jleandroperez commented 7 years ago

Wooooooo congratz gentlemen, epic PR here!!!

roundhill commented 7 years ago

:shipit: 🎉