Skip to content

Conversation

@aforcier
Copy link
Contributor

Adds some notification support for async post publishing (#5880). #5880 should be merged first, and then the base branch of this PR updated.

This is intended to be temporary, to give us some visual feedback on what's happening (post waiting for media to finish --> post itself publishing), and also to show an error notification when a post upload is cancelled because it had failed media. It's temporary because the upload UI is getting a major overhaul as part of the async media project, and that will supersede these changes.

To test:
Run the same cases as in #5880. You should get an immediate notification when pressing 'Publish', and you should get an error notification if a media item fails while the post is queued to upload.

cc @mzorz @daniloercoli

@aforcier aforcier force-pushed the feature/async-media-temporary-notifications branch from 397039b to 46fca06 Compare May 31, 2017 18:40
@mzorz
Copy link
Contributor

mzorz commented Jun 2, 2017

Good job @aforcier! Generally speaking the code looks good, I only found these things that might be improved:

  • if you write a post and get out of the editor while leaving some media upload in progress, you get a notification. Let it finish and you can see the notification updates until the final state notification "Post updated".
    If you then open that same post again for editing, then add a new media item and start uploading, then leave the editor, you end up having that "Post updated" notification which should be cancelled in favor of the most recent event notification happening (that is, the new notification that tells you some media is being uploaded and then the post is being updated). Otherwise you end up seeing a "Post updated" along with a "Media uploading/Post uploading" notification for the same post.
    (Note that notifications for different Posts should remain on the system dashboard if the user chooses not to dismiss them).

  • If you write a new draft, add an image, leave the editor. Wait for everything to finish. Then open the draft again, now tap on the settings wheel and change the status from Draft to Publish. Come back from the settings screen to the editor (tapping the back arrow) and add another media item. Now leave the editor tapping back.
    --> The Toast "Post saved on device / SYNC" is shown, and then no progress notifications are shown, even if the media is being uploaded (checked this by debugging). Also, in this case only, the post doesn't seem to be uploaded in the end (I guess this is correct as I didn't hit UPDATE or PUBLISH but only changed the Post settings and then hit the back arrow). Also, it lets me get in the editor while some media is uploading, potentially causing some problem there too.

aforcier added 2 commits June 7, 2017 14:59
…dia-temporary-notifications

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/posts/services/PostUploadService.java
@aforcier
Copy link
Contributor Author

aforcier commented Jun 7, 2017

if you write a post and get out of the editor while leaving some media upload in progress, you get a notification. Let it finish and you can see the notification updates until the final state notification "Post updated".
If you then open that same post again for editing, then add a new media item and start uploading, then leave the editor, you end up having that "Post updated" notification which should be cancelled in favor of the most recent event notification happening (that is, the new notification that tells you some media is being uploaded and then the post is being updated). Otherwise you end up seeing a "Post updated" along with a "Media uploading/Post uploading" notification for the same post.
(Note that notifications for different Posts should remain on the system dashboard if the user chooses not to dismiss them).

As discussed we should deal with this in a separate PR (and after we know the rest of the design requirements for notifications).

For context, this isn't completely straightforward since the in-progress notification and the 'post uploaded' notification are very different. The in-progress notification is an ongoing notification pinning the service to the foreground to prevent the system from killing the service. Once the upload completes we don't need to pin the service to the foreground anymore (and it should be stopped if there's nothing left to upload), so that one is just a normal notification.

We should rework this so the 'uploaded' notification is dismissed if we're uploading the same post again (but, as mentioned, at a later point).

@aforcier
Copy link
Contributor Author

aforcier commented Jun 7, 2017

If you write a new draft, add an image, leave the editor. Wait for everything to finish. Then open the draft again, now tap on the settings wheel and change the status from Draft to Publish. Come back from the settings screen to the editor (tapping the back arrow) and add another media item. Now leave the editor tapping back.
--> The Toast "Post saved on device / SYNC" is shown, and then no progress notifications are shown, even if the media is being uploaded (checked this by debugging). Also, in this case only, the post doesn't seem to be uploaded in the end (I guess this is correct as I didn't hit UPDATE or PUBLISH but only changed the Post settings and then hit the back arrow). Also, it lets me get in the editor while some media is uploading, potentially causing some problem there too.

So there are a few things going on here, I'll separate them out.

the post doesn't seem to be uploaded in the end (I guess this is correct as I didn't hit UPDATE or PUBLISH but only changed the Post settings and then hit the back arrow)

Changing the status from Draft to Publish makes this a 'published' post, and as you say since we didn't actually hit UPDATE, uploading the post automatically when exiting the editor would publish a draft, which is probably not what the user intended (or if it is we probably should not assume it). So it's correct not to queue up a post upload, and instead just save locally and update the post model in the db (from the MediaUploadService) when its media uploads complete.

Also, it lets me get in the editor while some media is uploading, potentially causing some problem there too.

I may have missed your meaning here - isn't this expected behavior? At least for Aztec, where we have reattachment. For the visual editor, we'll take care of this when we disable opening any post that has uploading media in general.

no progress notifications are shown

We're currently only showing progress notifications if the post is supposed to be uploading - if we're just updating the post locally, and the media uploads are proceeding (and you can re-open the post right away and see them still uploading), I'm not sure any messaging is needed in this case. We'll bring this up in the i2 design discussion with @bysl though.

@aforcier
Copy link
Contributor Author

aforcier commented Jun 7, 2017

PR updated with changes to the base branch (had to make some changes to accommodate notification error messaging for media cancellation).

@aforcier aforcier changed the base branch from feature/async-media-plus-publish to feature/async-media June 8, 2017 12:31
@mzorz
Copy link
Contributor

mzorz commented Jun 8, 2017

I may have missed your meaning here - isn't this expected behavior? At least for Aztec, where we have reattachment. For the visual editor, we'll take care of this when we disable opening any post that has uploading media in general.

Agh you're right! my bad - sorry

@mzorz
Copy link
Contributor

mzorz commented Jun 8, 2017

Thanks for the important notes and clarifications here @aforcier ! Will re-test and get back with comments if I should make any 👍

@mzorz
Copy link
Contributor

mzorz commented Jun 8, 2017

🔥 :shipit:

@mzorz mzorz merged commit 33d86a3 into feature/async-media Jun 8, 2017
@aforcier aforcier deleted the feature/async-media-temporary-notifications branch June 8, 2017 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants