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

Implements Uploader protocol in MediaCoordinator and PostCoordinator. #11839

Conversation

diegoreymendez
Copy link
Contributor

@diegoreymendez diegoreymendez commented Jun 1, 2019

Fixes #11785
Fixes #11784

Implements the Uploader protocol in both coordinator, and wires it so that it's triggered when the App is opened, when it comes to foreground, and when Reachability detects there's a connection.

Update release notes:

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

Known Issues / Scope limitations:

Reachability isn't working very well, but from my investigation this is not related to my PR. Reachability is simply failing to call the closures it should call.

I'll need to investigate this separately, as it goes beyond the scope of this specific PR.

Testing:

Test 1:

  1. Create a post with media while offline.
  2. Put the app in the background.
  3. Go back online.
  4. Bring the app back to foreground.
  5. Make sure the post is uploaded.

Test 2:

  1. Create a post with media while offline.
  2. Close the App.
  3. Go back online
  4. Launch the app again.
  5. Make sure the post is uploaded.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@yaelirub
Copy link
Contributor

yaelirub commented Jun 3, 2019

Hey @diegoreymendez, wonderful work here!
Test 1:
The media upload resumed when the app returned to foreground but the image overlay stayed.
media_background

Test 2:
The media upload resumes when the app was relaunched but the image was never attached to the post and only a placeholder is visible.
media_from_load
This seem to happen only in Aztec. Gutenberg works

@yaelirub
Copy link
Contributor

yaelirub commented Jun 3, 2019

Reachability isn't working very well, but from my investigation this is not related to my PR. Reachability is simply failing to call the closures it should call.

Diego, have you looked into using NWPathMonitor from Apple's Network framework?

@diegoreymendez
Copy link
Contributor Author

diegoreymendez commented Jun 3, 2019

Diego, have you looked into using NWPathMonitor from Apple's Network framework?

Only available in iOS 12 or newer, which makes it a no-go :(

One other option is probably this but I'd like to keep that as a separate task since I think fixing reachability is beyond the scope of this specific PR.

I've opened this issue for it: #11840

@diegoreymendez
Copy link
Contributor Author

@yaelirub - Awesome feedback... I've opened a new issue to tackle that problem separately here: #11846.

I've decided to split it because it's going to be easier for me to make progress on this incrementally, and because this issue is only present in the feature branch.

Rest assured we'll make sure everything is working before merging the feature branch.

Thanks for the review!

@diegoreymendez diegoreymendez merged commit ad2ed3b into feature/resuming-uploads Jun 3, 2019
@diegoreymendez diegoreymendez deleted the issue/11784-uploader-protocol-in-media-coordinator branch June 3, 2019 21:48

private func setupReachableBlock() {
reachability.reachableBlock = { _ in
self.resume()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a weak self?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Will submit a PR to fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants