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

Only publish a post after all media has been uploaded #12452

Conversation

@leandroalonso
Copy link
Contributor

commented Sep 3, 2019

Fixes #12408

To test:

Bad connection

  1. Use a bad connection (you can use the throttle in Charles proxy or some tool like Network Conditioner)
  2. Create a post with some text and publish it
  3. Add some images to the post you just published
  4. Tap "Update"
  5. Close the app before the images were uploaded
  6. Open the app
  7. Go to the post and tap to edit it
  8. Restore your connection and tap "Update"
  9. Check that data-wp_upload_id is still present or if the path of the images were all replaced for its remote URL

Offline

  1. Open the app and publish a post
  2. Close the app and go offline
  3. Edit the post and add an image
  4. Go back online and tap "Update"
  5. Check that data-wp_upload_id is still present or if the path of the images were all replaced for its remote URL

Additional information

The issue was happening if the posts had all images in a failed state PLUS not being uploaded. If those conditions were satisfied the post was published right away and all core data identifiers were removed.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Tasks

  • 2 reviewers?
  • Tests?
@leandroalonso

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@shiki @diegoreymendez I've opened this new PR with the fix and without the collateral effect that @shiki previously mentioned.

Let me know what you think.

Kudos to @diegoreymendez for the solution. 🥇

@leandroalonso leandroalonso force-pushed the issue/12408-only-publish-post-after-all-media-has-been-uploaded-2 branch from cd60adf to 9d67d09 Sep 3, 2019
Copy link
Member

left a comment

This works nicely. I have one major concern, marked with ⚠️.

Also, would you reconsider adding tests for this? 😉

WordPress/Classes/Models/AbstractPost.swift Outdated Show resolved Hide resolved
WordPress/Classes/Models/AbstractPost.swift Outdated Show resolved Hide resolved
WordPress/Classes/Models/AbstractPost.swift Outdated Show resolved Hide resolved
@shiki

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

It'd also be nice to have someone else test this. @diegoreymendez or @jklausa?

@jklausa

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

On it.

@jklausa
jklausa approved these changes Sep 4, 2019
Copy link
Member

left a comment

One stylistic nitpick, but otherwise tested and good to go!

Copy link
Member

left a comment

@leandroalonso Thank you for making those changes and adding tests. 🎉 I'm requesting changes here because of a regression (marked 🛑) which causes posts to be marked as “Upload failed” right away.

WordPress/WordPressTest/PostBuilder.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostCoordinatorTests.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostCoordinatorTests.swift Outdated Show resolved Hide resolved
WordPress/Classes/Services/PostService.m Outdated Show resolved Hide resolved
WordPress/Classes/Services/PostCoordinator.swift Outdated Show resolved Hide resolved
private let context = TestContextManager().newDerivedContext()
private var context: NSManagedObjectContext!

override func setUp() {

This comment has been minimized.

Copy link
@shiki

shiki Sep 5, 2019

Member

Missing tearDown() for context.

@shiki shiki dismissed their stale review Sep 5, 2019

Found something that we need to confirm

@shiki
shiki approved these changes Sep 5, 2019
Copy link
Member

left a comment

:shipit:

@leandroalonso leandroalonso merged commit b862ae4 into develop Sep 5, 2019
6 checks passed
6 checks passed
Hound No violations found. Woof!
Peril All green. Well done.
Details
ci/circleci: Build UI Tests Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPad 6th generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone Xs) Your tests passed on CircleCI!
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details
@leandroalonso leandroalonso deleted the issue/12408-only-publish-post-after-all-media-has-been-uploaded-2 branch Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.