Fixed missing tracks for error notifications #12184

mzorz · 2020-06-12T15:26:30Z

Fixes #12063

For a reference, check original PR in #10679

When a post/page upload or media upload resulted in an error notification being shown, these notifications were not being properly tracked.
This PR fixes that, by checking the proper ARG_NOTIFICATION_TYPE in onNewIntent in the related activities (it was being set properly in the upload service and related classes, but not used when intended).

- While at it, also fixed an issue in b739754: tracking code was only being called for new instances of WPMainActivity, when it should really have been the case for new intents to always get tracked. reverted in cef1846

To test:

CASE A: Post list

start writing a Post
add a local video
exit the editor while it's uploading
send the app to the background so the notification will appear when needed (see next steps)
turn airplane mode ON
the error notification that the upload couldn't be made appears
tap on the error notification
observe the call to trackTappedNotification gets executed

Variant: try creating the post from the Post list or from the Main activity, results should be the same (special case is when the Posts list it still instantiated, so don't go back to the Main activity in step 4 to let it sit there).

CASE B: Pages
Follow the same steps above but create a Page instead.

CASE C: Media

go to the Media section of the site / app
tap on the + button to create a media item and upload
select choose video from device
tap on a video
while the video is uploading, exit the activity
now send the app to the background
turn airplane mode ON
wait for the error notification to appear
tap on it
observe the call to trackTappedNotification gets executed

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2020-06-12T15:26:36Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2020-06-12T15:32:22Z

You can test the changes on this Pull Request by downloading the APK here.

…new intent brings this WPMainActivity to the foreground

oguzkocer · 2020-06-15T18:04:24Z

Hi there! 15.1 is being frozen, so this PR is being bumped to 15.2. If these changes have to be shipped with 15.1, please target release/15.1 and cc me in the PR so I can cut a new beta release. Also if you can keep the release dates in mind and bump the PRs & issues you are working on before the code freeze date, we'd really appreciate it.

This is a standard release version bump message. Let me know if you have any feedback.

develric

Good catch with this @mzorz 👍 !

I did a first pass and left a couple of notes/doubts I have to get your feedback. Let me know wdyt, thanks 🙇‍♂️

WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java

WordPress/src/main/java/org/wordpress/android/ui/pages/PagesActivity.kt

oguzkocer · 2020-06-29T18:00:20Z

Hi there! 15.2 is being frozen, so this PR is being bumped to 15.3. If these changes have to be shipped with 15.2, please target release/15.2 and cc me in the PR so I can cut a new beta release. Also if you can keep the release dates in mind and bump the PRs & issues you are working on before the code freeze date, we'd really appreciate it.

oguzkocer · 2020-07-13T06:13:35Z

@mzorz @develric This issue is being bumped a second time without any updates. Could you please either merge it until the next code freeze or update the milestone manually and document the blocker for it? Thank you!

Hi there! 15.3 is being frozen, so this PR is being bumped to 15.4. If these changes have to be shipped with 15.3, please target release/15.3 and cc me in the PR so I can cut a new beta release. Also if you can keep the release dates in mind and bump the PRs & issues you are working on before the code freeze date, we'd really appreciate it.

This is a standard release version bump message. Let me know if you have any feedback.

mzorz · 2020-07-13T13:33:56Z

@mzorz @develric This issue is being bumped a second time without any updates. Could you please either merge it until the next code freeze or update the milestone manually and document the blocker for it? Thank you!

Hi @oguzkocer ! This PR was a side contribution made in the last bit of HACK week and I was planning on following up with @develric's comments, but got busy with stories alpha, hence deprioritized this one in my ToDo list since this is not even a blocker. Should have made a comment about that here, getting back at it now 👍

oguzkocer · 2020-07-13T13:37:45Z

@mzorz No worries about keeping a PR open but not getting to work on it. We just need to document our intentions in the PR for others, in this case release managers, to see. For example, it'd be totally fine to remove the milestone, convert the PR to a draft and add a comment saying "This is not a priority right now, so will work on it when I can". Hope that makes sense!

mzorz · 2020-07-13T13:42:39Z

Your point about communication is clear @oguzkocer 👍

… time a new intent brings this WPMainActivity to the foreground" This reverts commit b739754.

…tantiated

mzorz · 2020-07-13T19:20:25Z

Ready for another round @develric - this time keeping the PR constrained to fixing what the issue describes 👍 🙇

develric

Hi @mzorz 👋! Checked this again and LGTM 👍. I tested the behavior as per reported steps and it works as expected. Only left a comment related to a small np, otherwise I'm fine to merge it. Let me know and thanks 🙇‍♂️.

WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.kt

develric

Thanks for the np update 🙇‍♂️ ! LGTM

mzorz added 4 commits June 12, 2020 11:02

track notification tap event when intent is on PostsListActivity

697c257

guard against trying to deserialize inexistent extra

020cbf9

track notification tap event when intent is on PagesActivity

e3702b1

track notification tap event when intent is on MediaBrowserActivity

d3f3ba0

mzorz added [Type] Bug Notifications Media labels Jun 12, 2020

mzorz added this to the 15.1 milestone Jun 12, 2020

mzorz requested a review from planarvoid June 12, 2020 15:26

mzorz requested a review from develric June 12, 2020 15:26

mzorz added 3 commits June 12, 2020 12:53

moved tracker code to onNewIntent to make sure to track every time a …

b739754

…new intent brings this WPMainActivity to the foreground

removed unused import

7de0c80

Merge branch 'develop' into issue/12063-missing-tracks

5d102f2

oguzkocer modified the milestones: 15.1, 15.2 Jun 15, 2020

develric self-assigned this Jun 16, 2020

develric reviewed Jun 16, 2020

View reviewed changes

oguzkocer modified the milestones: 15.2 ❄️, 15.3 Jun 29, 2020

oguzkocer modified the milestones: 15.3, 15.4 Jul 13, 2020

mzorz marked this pull request as draft July 13, 2020 13:44

mzorz removed this from the 15.4 milestone Jul 13, 2020

mzorz added 3 commits July 13, 2020 12:41

fixed merge conflict

f21acb1

Revert "moved tracker code to onNewIntent to make sure to track every…

cef1846

… time a new intent brings this WPMainActivity to the foreground" This reverts commit b739754.

registering PagesActivity for injection, needed for tracker to be ins…

c23e3bf

…tantiated

mzorz marked this pull request as ready for review July 13, 2020 19:20

mzorz added this to the 15.4 milestone Jul 13, 2020

planarvoid removed their request for review July 16, 2020 12:41

develric mentioned this pull request Jul 17, 2020

Misleading snackbar message on page load fail retry #12477

Open

develric reviewed Jul 17, 2020

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.kt Outdated Show resolved Hide resolved

mzorz added 2 commits July 19, 2020 20:40

added unadvertedly removed empty line back

9602501

removed spaces

c240b65

develric approved these changes Jul 20, 2020

View reviewed changes

develric merged commit 143172e into develop Jul 20, 2020

develric deleted the issue/12063-missing-tracks branch July 20, 2020 08:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed missing tracks for error notifications #12184

Fixed missing tracks for error notifications #12184

mzorz commented Jun 12, 2020 •

edited

peril-wordpress-mobile bot commented Jun 12, 2020 •

edited

peril-wordpress-mobile bot commented Jun 12, 2020 •

edited

oguzkocer commented Jun 15, 2020

develric left a comment

oguzkocer commented Jun 29, 2020

oguzkocer commented Jul 13, 2020

mzorz commented Jul 13, 2020

oguzkocer commented Jul 13, 2020

mzorz commented Jul 13, 2020

mzorz commented Jul 13, 2020

develric left a comment

develric left a comment

Fixed missing tracks for error notifications #12184

Fixed missing tracks for error notifications #12184

Conversation

mzorz commented Jun 12, 2020 • edited

To test:

peril-wordpress-mobile bot commented Jun 12, 2020 • edited

peril-wordpress-mobile bot commented Jun 12, 2020 • edited

oguzkocer commented Jun 15, 2020

develric left a comment

Choose a reason for hiding this comment

oguzkocer commented Jun 29, 2020

oguzkocer commented Jul 13, 2020

mzorz commented Jul 13, 2020

oguzkocer commented Jul 13, 2020

mzorz commented Jul 13, 2020

mzorz commented Jul 13, 2020

develric left a comment

Choose a reason for hiding this comment

develric left a comment

Choose a reason for hiding this comment

mzorz commented Jun 12, 2020 •

edited

peril-wordpress-mobile bot commented Jun 12, 2020 •

edited

peril-wordpress-mobile bot commented Jun 12, 2020 •

edited