{"action":"created","issue":{"url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/533","repository_url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android","labels_url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/533/labels{/name}","comments_url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/533/comments","events_url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/533/events","html_url":"https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533","id":250011701,"number":533,"title":"Add UploadStore","user":{"login":"aforcier","id":9613966,"avatar_url":"https://avatars3.githubusercontent.com/u/9613966?v=4","gravatar_id":"","url":"https://api.github.com/users/aforcier","html_url":"https://github.com/aforcier","followers_url":"https://api.github.com/users/aforcier/followers","following_url":"https://api.github.com/users/aforcier/following{/other_user}","gists_url":"https://api.github.com/users/aforcier/gists{/gist_id}","starred_url":"https://api.github.com/users/aforcier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aforcier/subscriptions","organizations_url":"https://api.github.com/users/aforcier/orgs","repos_url":"https://api.github.com/users/aforcier/repos","events_url":"https://api.github.com/users/aforcier/events{/privacy}","received_events_url":"https://api.github.com/users/aforcier/received_events","type":"User","site_admin":false},"labels":[{"id":433247404,"url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/labels/[Store]%20Media","name":"[Store] Media","color":"c5def5","default":false},{"id":433247319,"url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/labels/[Store]%20Post","name":"[Store] Post","color":"c5def5","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":7,"created_at":"2017-08-14T12:07:24Z","updated_at":"2017-09-06T16:52:18Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/pulls/533","html_url":"https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533","diff_url":"https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533.diff","patch_url":"https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533.patch"},"body":"Addresses #519.\r\n\r\nAdds an `UploadStore`, which shadows `PostStore`/`MediaStore` upload events and manages a new set of models, `PostUploadModel` and `MediaUploadModel.` The goal is to have a way to track uploading posts that are waiting for their media to upload inside FluxC, offloading a lot of logic from WPAndroid (which was previously receiving events from FluxC and managing multiple maps and lists). It also allows to add a bunch of tests.\r\n\r\n**Currently a work in progress.**\r\n\r\n### List of remaining tasks:\r\n\r\n* [x] Implement a `CANCEL_POST` action for marking 'registered' posts (posts that are queued to be uploaded but are waiting for their media to finish) as cancelled\r\n* [x] Add more convenience methods needed by WPAndroid:\r\n   * [x] `isPending/Failed/CancelledPost()`\r\n   * [x] `getUploadProgressForMedia()`\r\n   * [x] `getUploadErrorForPost()`\r\n   * [x] ~`setUploadErrorForPost()`~ (turns out to be unnecessary, handled by intercepting `UPDATE_MEDIA` and setting media/posts to failed with an error)\r\n   * [x] `getAllRegisteredPosts()`\r\n* [x] ~Move progress reporting/calculating logic to `UploadStore`~ (Not possible anymore since video optimization progress has been added WPAndroid-side)\r\n* [x] Move `UploadError` logic to `UploadStore`\r\n* [x] Refactor upload `onAction()`/`emitChange()` (see below)\r\n* [x] Clear cancelled/failed posts when new media are uploaded to them\r\n* [x] Potentially intercept `UPDATE_MEDIA` actions to reflect changes to media upload state (and add a `MediaError` when `FAILED`)\r\n* [x] Add DB migration when models are finalized\r\n* [x] Eliminate public methods that return `*UploadModel`s\r\n\r\n#### Lingering questions:\r\n* [ ] Possibly add `PostStore.getUploadErrorForPost()` as an alias for the same `UploadStore` method, to avoid requiring an `UploadStore` instance from the caller (which is not necessarily an upload manager)\r\n\r\n#### Independent items:\r\n* [ ] Update the documentation\r\n* [ ] Possibly inject the `UploadStore` into the `MediaStore`/`PostStore` instead of adding it to the constructor (I'd like to explore this separately from this PR)\r\n\r\n### Some issues\r\n\r\n#### `emitChange()` concurrency\r\nThe `UploadStore` receives actions through `onAction()` (using only the `PostAction` and `MediaAction`s relevant to uploads) just before the rest of the stores do. However, we're using `ThreadMode.ASYNC` for `onAction()` in stores, which does not wait for each `onAction()` method to return before calling the next, but rather will call multiple store `onAction()`s on multiple threads.\r\n\r\nThis means that, when `MediaStore` emits a `OnMediaUploaded` event (for example), we have no guarantee that `UploadStore` has already finished processing the `UPLOADED_MEDIA` event that generated that `OnMediaUploaded` event in the `MediaStore`. So, if we query the `UploadStore` from inside a `OnMediaUploaded` listener, the `UploadStore` might not be up to date yet. Given that I think the `UploadStore` makes sense as a separate store (rather than packing it all up inside `MediaStore`/`PostStore`), I see at least three possible solutions:\r\n\r\n1. **Use `ThreadMode.BACKGROUND`**\r\nThis guarantees sequential delivery of the same event (so `UploadStore.onAction()` would return before `MediaStore.onAction()` got called). However, it no longer guarantees that actions will be called on a background thread - if the calling thread is not the main thread, a new thread is not used ([`ThreadMode` ref](http://greenrobot.org/eventbus/documentation/delivery-threads-threadmode/)). This could lead to trouble with how we're using FluxC from WPAndroid, and we should not make such a sweeping change lightly.\r\n2. **Emit `OnUploadChanged` events from the `UploadStore`**\r\nThis would mean that anything depending on `UploadStore` state (pretty much, the `UploadService` in WPAndroid) would listen to `OnUploadChanged` instead of `OnPostUploaded`/`OnMediaUploaded`, and be guaranteed consistent `UploadStore` state. However, we sometimes need both `UploadStore` and `MediaStore`/`PostStore` to be in a consistent state, so emitting them independently is risky. It's a little unorthodox, but I think what we can do in this case is subscribe `UploadStore` to `OnPostUploaded`/`OnMediaUploaded` events, and 'repeat them' (but this requires some ugly logic to 'register' events as they come in so we can look for them when processing is done). We could also write some kind of middleware for the `Dispatcher`, that will hold `OnUploadChanged` events until a matching `OnMediaUploaded`/`OnPostUploaded` is emitted. This would work, but it's not easy to implement, and I'm not sure separating this logic from the stores is a good idea.\r\n3. **'MITM' `UPLOADED_MEDIA` and `PUSHED_POST` in `UploadStore`**\r\nWe would add `UploadAction.UPLOADED_MEDIA` and `UploadAction.PUSHED_POST`, and call those from the network clients instead of the `Post/MediaAction` ones. The `UploadStore` calls the `Post/MediaAction` versions when it's done with its own processing. We don't need to listen for `OnUploadChanged` anymore in this case, because the original `OnXUploaded` events aren't sent until the `UploadStore` is updated, so we can rely on them.\r\n\r\nI think 3 is the best option - it's the least error-prone, and the most straightforward to implement. I added this behavior in https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533/commits/2930653b321a10491cb9c7970c702379a2d3fd05."},"comment":{"url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/comments/327546251","html_url":"https://github.com/wordpress-mobile/WordPress-FluxC-Android/pull/533#issuecomment-327546251","issue_url":"https://api.github.com/repos/wordpress-mobile/WordPress-FluxC-Android/issues/533","id":327546251,"user":{"login":"mzorz","id":6597771,"avatar_url":"https://avatars0.githubusercontent.com/u/6597771?v=4","gravatar_id":"","url":"https://api.github.com/users/mzorz","html_url":"https://github.com/mzorz","followers_url":"https://api.github.com/users/mzorz/followers","following_url":"https://api.github.com/users/mzorz/following{/other_user}","gists_url":"https://api.github.com/users/mzorz/gists{/gist_id}","starred_url":"https://api.github.com/users/mzorz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mzorz/subscriptions","organizations_url":"https://api.github.com/users/mzorz/orgs","repos_url":"https://api.github.com/users/mzorz/repos","events_url":"https://api.github.com/users/mzorz/events{/privacy}","received_events_url":"https://api.github.com/users/mzorz/received_events","type":"User","site_admin":false},"created_at":"2017-09-06T16:52:18Z","updated_at":"2017-09-06T16:52:18Z","author_association":"CONTRIBUTOR","body":"> I think we can update the logic of that check and move on\r\n\r\nNice find @aforcier ! Totally, let's update that check 👍 "}}
{"actor":{"display_login":"mzorz"}}