Async notifications: Post error / success notifications (part II): #6763

mzorz · 2017-10-20T20:32:16Z

Continues #6756 (this one should be merged into that other one first!)
Also adds the functionality requested for in #6407 (adding the button to the Posts list should come in another PR)

This one implements Post error notifications (and snackbars) on top of the Post success notifications work done in #6756

Notes:

snackbars have been enhanced
the duration for these is 5 seconds now in this PR
we implemented the RETRY functionality for Aztec only (i.e. if you're using the Visual editor, then a failed upload won't make the RETRY button appear in your error notification)
as in Post sucess notifications (part 1) #6756 , didn't implement the CLOSE button (see my comment there)
changing the color for the quick actions only didn't seem to work for me right away, (i.e. the accent color is changed as well for the notification icon as can be seen here below)
in the notification, we are adding the granular reason in the BigStyle text so the user knows why something happened (for example, "Lost connection to the server" or whatever other reason we already know about and were informing in the current release version)

also, note we are keeping the title as it was, as we're using the Post title if one post has failed

To test:

CASE A:

start a new draft
include a media item (try to make it a large enough one so to be able to continue uploading while exiting the editor)
exit the editor
turn airplane mode ON
observe the error notification appears and reads “1 post, 1 files not uploaded - RETRY”

CASE B:

start a new draft
include 1 media item (try to make it a large enough one so to be able to continue uploading while exiting the editor)
while the media item above approaches half of it being uploaded, include a new media item there
exit the editor
check the notification until it changes from “1/2 media items” to “2/2 media items” (wording of these is changed in another PR Upload foreground notification wording #6748)
turn airplane mode ON
observe the error notification appears, and reads “1 post, 1 files not uploaded (1 file successfully uploaded) - RETRY”

CASE C: AFTER THIS LAST CASE ABOVE:
8. turn airplane mode OFF
9. tap on the notification’s RERTY quick action
10. verify the post and media items are re-uploaded and finish successfully

CASE D:

repeat CASE B.
now turn airplane mode OFF
before attempting a retry, please delete one of the media items that FAILED (the second one you added to the Post) from the Post (enter the editor, place the cursor to the right of the picture, then hit backspace to delete), and exit the editor.
now, tap on the notification’s RETRY quick action
You should see the app retries to upload the remaining media item within the Post and updates the Post gracefully as well.

cc @nbradbury

…-success-notifications

… having a check of which Posts have been already counted as having ended their transfer, either successfully or with error

…try notification color

… to avoid letting the user RETRY on a Post that is currently being edited

…ment

… file so it cannot be retried

…ailed media/posts

… added PUBLISH quick action for uploaded drafts

…s media to be retried along with it

mzorz · 2017-10-24T01:19:13Z

This PR is now ready for review @nbradbury - please note Media-only success and error (i.e. final) notification and Retry implementation is to be addressed on a separate PR 🙇

…-error-success-notifications

nbradbury · 2017-10-24T12:34:28Z

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

+    public void onEventMainThread(UploadService.UploadErrorEvent event) {
+        SiteModel site = getSelectedSite();
+        if (site != null) {
+            if (event.post.getLocalSiteId() == site.getId()) {


Do we need to null check event.post here?

uh, nice! addressed in 0ad299b

nbradbury · 2017-10-24T12:43:50Z

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

@@ -515,7 +530,8 @@ public void onPostUploaded(PostStore.OnPostUploaded event) {
            if (site != null) {
                if (event.post.getLocalSiteId() == site.getId()) {
                    UploadUtils.onPostUploadedSnackbarHandler(getActivity(),
-                            getActivity().findViewById(R.id.coordinator), event, site, mDispatcher);
+                            getActivity().findViewById(R.id.coordinator),


We can drop final PostModel post = event.post; at the top of this method.

addressed in fcfa5c2

nbradbury · 2017-10-24T12:47:16Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostUtils.java

@@ -326,4 +326,9 @@ static boolean updatePostContentIfDifferent(PostModel post, String newContent) {
        }
        return false;
    }
+
+    public static boolean isFirstTimePublish(PostModel post) {
+        return PostStatus.fromPost(post) == PostStatus.DRAFT


EditPostActivity has a similar method here whose logic is different than this.

That's correct - EditPostActivity can determine things differently given it holds an mOriginalPost and can eventually tell if there are any differences. It is different for the UploadService or, fwiw, anywhere else other than the Editor where changes might happen anytime.

nbradbury · 2017-10-24T12:55:58Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java

-    }
-
-    void incrementUploadedPostCountFromForegroundNotification(@NonNull PostModel post) {
+    void incrementUploadedPostCountFromForegroundNotificationOrFinish(@NonNull PostModel post) {


I'm confused by orFinish being part of this method name. Any chance we can get rid of it, especially since the method name is so long?

Sure thing! got it back in 6aa5620 and also renamed the Media method counterpart to keep it the same way

…method

nbradbury · 2017-10-24T14:56:16Z

This is likely unrelated to this PR, but the post list was (correctly) showing that a post's media failed to upload yet the upload progress remained visible.

nbradbury · 2017-10-24T14:57:45Z

I ran into a situation where I received an "Upload failed" notification yet the "Uploading" notification didn't go away.

nbradbury · 2017-10-24T15:42:27Z

I'm guessing GENERIC_ERROR is coming from the backend? If so, we should detect that error message and not display it to the user.

mzorz · 2017-10-24T20:21:18Z

but the post list was (correctly) showing that a post's media failed to upload yet the upload progress remained visible.

Nice catch Nick! , addressed in a different PR #6782

mzorz · 2017-10-24T20:32:43Z

I'm guessing GENERIC_ERROR is coming from the backend? If so, we should detect that error message and not display it to the user.

It's defined in the network layer (a.k.a FluxC) - we've seen that before, and IIRC we decided to let it all the way up to the user to make it easier for us to understand what's going on when people reaching out to us through Helpshift. That said, we can come up with something nicer that still is "unique", but I'd need some way to reproduce. Do you remember what you did to make it fail this way?

…romForegroundNotification

mzorz · 2017-10-24T20:36:29Z

I ran into a situation where I received an "Upload failed" notification yet the "Uploading" notification didn't go away.

#6763 (comment)
addressed in b2b49d6

nbradbury · 2017-10-25T14:42:26Z

Do you remember what you did to make it fail this way?

I simply enabled airplane mode during upload, but other times I did that I received a different error.

nbradbury · 2017-10-25T14:59:10Z

mzorz added 13 commits October 20, 2017 10:32

changed error notification icon and color

8bead6e

added new error message wording

b82858a

Merge branch 'mzorz/post-success-notifications' into mzorz/post-error…

19a095f

…-success-notifications

fixed multiline error message setting in bigText in bigStyle

940d60b

removed old updateNotificationIcon method

b119d6e

removed unused import

e3a30ad

renamed method to match the incrementer for media as well

88101b4

fix the post count on error occurrences for media within that Post by…

a09209f

… having a check of which Posts have been already counted as having ended their transfer, either successfully or with error

fixed successful upload count

99403fc

made snackbars work for upload Errors as well

3a37bae

set snackbar duration to 5 seconds as specificed in design

751b601

implemented retry

ab402f9

changed snackbar to hold only the defined summarizing message, and re…

a296301

…try notification color

mzorz mentioned this pull request Oct 20, 2017

Async media: foreground notifications master thread #6735

Closed

13 tasks

mzorz added 8 commits October 20, 2017 17:39

made Retry functionality available from the Snackbar

9b86d53

removed unused parameter in UploadService.getUploadPostServiceIntent

c6d5a41

refactored isFirstTimePublish code into a PostUtils tool

a0f1db8

dismissing the final notification when opening the corresponding Post…

dcd4b7d

… to avoid letting the user RETRY on a Post that is currently being edited

showing Toast if RETRY is pressed but Aztec is not enabled at that mo…

c61cfd0

…ment

added Toast in case RETRY was attempted but there's any missing media…

cc48757

… file so it cannot be retried

only dispatch event if payload is not empty

2c99ae0

making sure to re-tag media as UPLOADING before starting a RETRY on f…

c34561e

…ailed media/posts

mzorz referenced this pull request Oct 22, 2017

simplified post upload success and error notification id handling and…

08113cc

… added PUBLISH quick action for uploaded drafts

mzorz added 2 commits October 22, 2017 10:34

fixed merge conflicts

78a2e35

added analytics tracking for RETRY for first publish posts

738fa31

mzorz added the [Status] Not Ready for Review label Oct 22, 2017

mzorz added 4 commits October 23, 2017 08:48

making sure we register the PostModel on retry of a Post that contain…

198092e

…s media to be retried along with it

fixed merge conflict

5633411

fixed merge conflict

bc7403c

fixed argument type for getPagesAndOrPostsString call

309c3d6

mzorz added 2 commits October 23, 2017 16:58

adding counter adjustment method for retry

66b372b

changed fluxc branch

4b37119

mzorz mentioned this pull request Oct 24, 2017

resets the UploadState for a Post that is being registered wordpress-mobile/WordPress-FluxC-Android#594

Merged

mzorz changed the base branch from mzorz/post-success-notifications to issue/6389-upload-service-notification October 24, 2017 01:01

mzorz removed the [Status] Not Ready for Review label Oct 24, 2017

Merge branch 'issue/6389-upload-service-notification' into mzorz/post…

8458cfa

…-error-success-notifications

nbradbury self-assigned this Oct 24, 2017

nbradbury reviewed Oct 24, 2017

View reviewed changes

updated FluxC hash

e579fab

nbradbury reviewed Oct 24, 2017

View reviewed changes

mzorz added 3 commits October 24, 2017 09:56

added null guard

0ad299b

dropped unused var

fcfa5c2

renamed incrementUploadedPostCountFromForegroundNotificationOrFinish …

6aa5620

…method

This was referenced Oct 24, 2017

Async notifications: media-only error notifications (part III) #6780

Merged

Async media: Post list showing both progress and an error #6781

Closed

added missing call to mPostUploadNotifier.incrementUploadedPostCountF…

b2b49d6

…romForegroundNotification

fixed merge conflict

158f6b8

nbradbury merged commit 02bd896 into issue/6389-upload-service-notification Oct 25, 2017

mzorz deleted the mzorz/post-error-success-notifications branch November 6, 2017 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async notifications: Post error / success notifications (part II): #6763

Async notifications: Post error / success notifications (part II): #6763

mzorz commented Oct 20, 2017 •

edited

Loading

mzorz commented Oct 24, 2017

nbradbury Oct 24, 2017

mzorz Oct 24, 2017

nbradbury Oct 24, 2017

mzorz Oct 24, 2017

nbradbury Oct 24, 2017

mzorz Oct 24, 2017

nbradbury Oct 24, 2017

mzorz Oct 24, 2017

nbradbury commented Oct 24, 2017

nbradbury commented Oct 24, 2017

nbradbury commented Oct 24, 2017

mzorz commented Oct 24, 2017

mzorz commented Oct 24, 2017

mzorz commented Oct 24, 2017

nbradbury commented Oct 25, 2017

nbradbury commented Oct 25, 2017

Async notifications: Post error / success notifications (part II): #6763

Async notifications: Post error / success notifications (part II): #6763

Conversation

mzorz commented Oct 20, 2017 • edited Loading

mzorz commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbradbury commented Oct 24, 2017

nbradbury commented Oct 24, 2017

nbradbury commented Oct 24, 2017

mzorz commented Oct 24, 2017

mzorz commented Oct 24, 2017

mzorz commented Oct 24, 2017

nbradbury commented Oct 25, 2017

nbradbury commented Oct 25, 2017

mzorz commented Oct 20, 2017 •

edited

Loading