Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix upload notification not shown on retry #9956

Merged
merged 6 commits into from Jun 7, 2019

Conversation

@malinajirka
Copy link
Contributor

commented May 29, 2019

Fixes #9951

This PR takes care of promoting the UploadService to the foreground (shows an upload in progress notification) when retrying upload.

I've removed this line
mPostUploadNotifier.removePostInfoFromForegroundNotificationData(post, mediaToRetry);
but tbh I'm not sure what was the initial purpose of the line. I haven't find any reason why we need to invoke it on this place. If I keep it there, the upload notification will always show "1 post remaining" even when I invoke retry on three posts.
Imagine we have 3 posts

  1. First post increases sNotificationData.mTotalPostItems to 1
  2. Second post invokes mPostUploadNotifier.removePostInfoFromForegroundNotificationData(post, mediaToRetry); -> decreases sNotificationData.mTotalPostItems to 0 -> and increases it back to 1
  3. Third post also decreases it to 0 and back to 1
  4. The notification shows "1 post remaining" even when there are actually 3 posts being uploaded

To test:
retry-notification-2

  1. Turn on airplane mode
  2. Create a local draft
  3. Turn off airplane mode and notice the upload notification is being shown

  1. Turn on airplane mode
  2. Create 3 local drafts
  3. Turn off airplane mode and notice the upload notification (3 posts remaining) is being shown

retry-notification

  1. Open editor
  2. Enter title
  3. Press back and turn on airplane mode asap
  4. Wait until the upload fails
  5. Turn off airplane mode and notice the upload notification is shown

  1. Open editor and enter title
  2. Press back and turn on airplane mode asap
  3. Open editor and enter title
  4. Turn off airplane mode, press back and turn on airplane mode (you might want to set network type to GPRS in emulator settings so the draft from step 2 doesn't get uploaded)
  5. Wait until the upload fails
  6. Turn off airplane mode and notice the upload notification (2 posts remaining) is shown

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

Before/After gifs
missing-retry-notif
fixed-missing-retry-notif

@malinajirka malinajirka added this to the 12.6 milestone May 29, 2019

@malinajirka malinajirka requested a review from mzorz May 29, 2019

@mzorz
Copy link
Contributor

left a comment

Thanks for diving into this @malinajirka 🙇
Good call on adding what was missing in order to make the notification being shown 👍

While reviewing this, I found another couple problems, which are not strictly related to this PR but may be quick to solve.

Problem A: with Posts with media (in particular, Aztec posts with media) - it is indeed a bit tricky, as also some things changed since the gradual introduction of Gutenberg that made things slip a bit.

So, the problem actually started with the introduction of hardcoding isAztecEnabled and isGutenbergEnabled to true in #9208, given the following condition would never be met:

                    boolean processWithAztec =
                            AppPrefs.isAztecEditorEnabled() && !AppPrefs.isGutenbergEditorEnabled()
                            && !postHasGutenbergBlocks;

(given both return true, you can't have isAztecEnabled() and !isGutenbergEnabled() at the same time).

This in turn made the flag being passed to retryUpload() to be false, and thus this part of code wouldn't be run: https://github.com/wordpress-mobile/WordPress-Android/blob/develop/WordPress/src/main/java/org/wordpress/android/ui/uploads/UploadService.java#L751-L753

This effectively means the Post wouldn't get registered for the upload, and media (if the Post contained failed media) wouldn't get retried.

The solution to this would be to just remove the isAztecEnabled() and isGutenbergEnabled calls and replace the flag check with this when calling retryUpload():

                    boolean postHasGutenbergBlocks = PostUtils.contentContainsGutenbergBlocks(post.getContent());
                    retryUpload(post, !postHasGutenbergBlocks);

Re: the following:

I've removed this line
mPostUploadNotifier.removePostInfoFromForegroundNotificationData(post, mediaToRetry);
but tbh I'm not sure what was the initial purpose of the line. I haven't find any reason why we need to invoke it on this place. If I keep it there, the upload notification will always show "1 post remaining" even when I invoke retry on three posts.

I think the reason why it was there is because the retryUpload() method was mostly there to implement the "RETRY" button on a Notification (or on the Posts list itself). Apparently we haven't tested retrying more than one Post at once, hence the defect of assuming we could call mPostUploadNotifier.removePostInfoFromForegroundNotificationData() there in that place.
I've locally tried and made a change in PostUploadHandler and added a proper call to removePostInfoFromForegroundNotification to adjust the sNotificationData data in the place where an error is captured, as that's a more proper place to handle it, and it seemed to work well.

If you're ok with these changes, feel free to apply them - I prepared a patch where you can see them / you can also download the file and apply it to your branch if you think they're good (use git apply mario.patch on the command line to make the modifs happen): https://gist.github.com/mzorz/33899b7293e796098d317d3b7ecd1eca

Following is a list of other two things that I found but don't have a suggestion for solving (we may open new issues)

Problem B:

  1. Open the editor
  2. don't write nothing (no title, no content), but just send the app to the background (this will make the Post be saved locally! even if empty)
  3. bring the app back to the foreground
  4. observe the UploadService gets triggered and wants to upload the empty post (which is wrong), and thus generates an annoying error notification.

Also, if you go back to the Posts list the Post will cease to exist, and so the Notification's RETRY button will do nothing (if you tap on the Notification, a "Post no longer exists" message will be shown).

Problem C:

  1. turn airplane mode ON
  2. start a draft, give it a title, some content
  3. exit the editor and observe it's saved as Local Drafts
  4. open it again
  5. edit the title or content
  6. tap on SAVE (navigation bar)
  7. observe the "Ready to publish? Keep editing / Publish immediately" dialog is shown --> it should only be saving then update here (probably should also say UPDATE instead of SAVE up there)

Again, thanks for this PR, and let me know about the other issues (don't know if they're under someone's radar already)

@@ -784,6 +783,7 @@ private void retryUpload(PostModel post, boolean processWithAztec) {
// send event so Editors can handle clearing Failed statuses properly if Post is being edited right now
EventBus.getDefault().post(new UploadService.UploadMediaRetryEvent(mediaToRetry));
} else {
mPostUploadNotifier.addPostInfoToForegroundNotification(post, null);

This comment has been minimized.

Copy link
@mzorz

mzorz May 30, 2019

Contributor

nice catch here - it definitely seems the right place where to add it

@mzorz mzorz added this to In Review in Offline Support: Posting [Android] via automation May 30, 2019

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

Woow, thank you so much for the thorough review and for the great suggestions!

Problem A

I've applied your patch. Thanks!!

Problem B:

This was recently fixed in #9962.

Problem C:

I've created a new issue - #9966.

Thanks @mzorz! I have just applied your patch and tested the app and it LGTM! It's ready for another round/merge. 🙇

@loremattei loremattei modified the milestones: 12.6 ❄️, 12.7 Jun 3, 2019

@loremattei

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

Hey! I'm moving this to 12.7 since 12.6 has been cut. If you want it to land 12.6, please feel free to move it back, target the release branch and ping me to build a new beta.

@nbradbury

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Just a friendly reminder to please make sure to manually merge develop into this branch before you merge this PR, as develop now contains the AndroidX migration. The automatic merge can succeed but still break the build. Better to find out now before it breaks develop. Thanks!

@mzorz

mzorz approved these changes Jun 5, 2019

Copy link
Contributor

left a comment

Awesome! Just looked into it @malinajirka , LGTM, approving now, feel free to make the develop merge into this branch first 👍

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Thanks @mzorz !;) I've merged develop into the branch + I verified everything is migrated to AndroidX (no changes were required). I've also updated release notes - moved the note about this fix into 12.7.

@malinajirka

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

I'm merging this as the PR has been approved. 🚢

@malinajirka malinajirka merged commit 05e19d9 into develop Jun 7, 2019

4 checks passed

Peril All green. Nice work.
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

Offline Support: Posting [Android] automation moved this from In Review to Done (PRs) Jun 7, 2019

@malinajirka malinajirka deleted the issue/9951-upload-notif-on-retry branch Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.