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.

Sign up for GitHub

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

Jump to bottom

Automatically upload published and draft posts #12466

Merged
merged 14 commits into from
Sep 19, 2019
Merged

Automatically upload published and draft posts #12466

merged 14 commits into from
Sep 19, 2019

Conversation

shiki
Copy link
Member

@shiki shiki commented Sep 4, 2019 •
edited
Loading

Closes #12324.

This adds auto-uploading support for published and draft posts that were previously uploaded to the server. The main change is the removal of !post.hasRemote() in PostAutoUploadInteractor.

This also incorporates the related changes as described in the pending tasks in Android:

The rest of the changes are automated tests.

Previews and Testing Steps

Local draft

Before After
before local draft local draft
  1. Go offline.
  2. Create a post.
  3. Tap on X and tap Save Draft.
  4. Confirm the Post List and the notice says “Draft will be uploaded next time your device is online”
  5. Go online. Confirm the post is automatically uploaded.

Editing an existing draft

Before After
before existing draft existing draft
  1. Go offline.
  2. Edit a draft that has been previously uploaded to the server. Make some changes.
  3. Tap on X and tap Update Draft.
  4. Confirm the Post List and the notice says “Changes will be uploaded next time your device is online”
  5. Go online. Confirm the draft is automatically uploaded.

Publishing a local draft

Before After
before published draft published draft
  1. Go offline.
  2. Create a post.
  3. Tap on Publish.
  4. Confirm the Post List and notice says “Post will be published next time your device is online”
  5. Go online. Confirm the post is automatically published.

Editing an existing published post

Before After
before existing published post existing published post
  1. Go offline.
  2. Edit a published post that has been previously uploaded to the server. Make some changes.
  3. Tap on X and tap Update Post.
  4. Confirm the Post List and the notice says “Changes will be uploaded next time your device is online”
  5. Go online. Confirm the draft is automatically published.

Reviewing

Only 1 reviewer is needed but anyone can review.

Submitter Checklist

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
  • If it's feasible, I have added unit tests.

Tasks

shiki added 12 commits August 30, 2019 18:43
@shiki
Include posts with remote in auto-upload
873bd85
@shiki
Use a different message for auto-uploaded drafts
dd8d07a 
Instead of just “Upload failed”, we’ll show “Draft will be uploaded…” instead.
@shiki
Update PostCardStatusViewModelTests
21dd3ef
@shiki
Update PostCardCellTests for draft and published
ab13057
@shiki
Change local draft to just show “Local changes”
4ce88f3 
The message “Draft will not be uploaded” is probably not necessary for it since it’s not cancelaeable anyway.
@shiki
PostNoticeViewModel: Promote autoUploadInteractor
e564ff3
@shiki
PostNoticeViewModel: Move failure titles to enum
4b1485e
@shiki
Show “Draft will be uploaded…” notice
73a947c 
This will affect drafts with remote only.
@shiki
PostCardCell: Add test for local draft message
2bc133d
@shiki
Show “Changes will be uploaded” in Post List
9b47445 
This will be shown for posts with remote that have local changes and are confirmed to be automatically uploaded when the device is back online.

This also reverts 4ce88f3. Now, we will show “Draft will be uploaded…” for local drafts instead of ”Local changes” only. This was discussed with Chris and it seems much clearer for the user.
@shiki
Show “Draft will be uploaded” in notice
88b62b5 
And for posts with remote, always use “Changes will be uploaded…” as the notice message.
@shiki
Add additional tests in PostNoticeViewModelTests
2c6380a
@shiki shiki requested a review from osullivanchris September 4, 2019 17:03
@shiki shiki self-assigned this Sep 4, 2019
@shiki
Copy link
Member Author

shiki commented Sep 4, 2019

@osullivanchris I thought I'd send this to you first before I ask someone to review the code. Would you mind taking a quick look at the description and the screenshots above? Please let me know what you think. 🙂

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

osullivanchris
osullivanchris approved these changes Sep 5, 2019
View reviewed changes
Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

@shiki thanks for putting all these together, its great to see in one place and the before and after GIFs are really helpful. All looks great, +1 from me on this

@shiki shiki requested a review from leandroalonso September 5, 2019 16:50
@leandroalonso
Copy link
Contributor

@shiki When I tap "Update" (the right-upper button) on a published post, it shows me an alert containing an "Error occurred during saving".

If I close the app and open it again I can see that the content was saved and the post was scheduled to be auto-published. Do you think this is something to be fixed inside the scope of this task?

leandroalonso
leandroalonso requested changes Sep 6, 2019
View reviewed changes
Copy link
Contributor

@leandroalonso leandroalonso left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great!

I left some comments for us to discuss and this and this comments related to (possible) bugs in the feature.

Let me know if you think it is best to do them on another PR.

WordPress/Classes/ViewRelated/Post/PostCardStatusViewModel.swift Outdated Show resolved Hide resolved
WordPress/Classes/ViewRelated/Post/PostCardStatusViewModel.swift Outdated Show resolved Hide resolved
WordPress/WordPressTest/PostCardCellTests.swift Show resolved Hide resolved
WordPress/WordPressTest/Services/PostAutoUploadInteractorTests.swift Show resolved Hide resolved
WordPress/Classes/ViewRelated/Post/PostNoticeViewModel.swift Show resolved Hide resolved
@shiki shiki mentioned this pull request Sep 9, 2019
Closed
@shiki
Add PostCardStatusViewModel.generateFailedStatus()
adb140b 
This is just a helper method to make things a bit readable. It should only be used by `private var status: String?`
@shiki shiki requested a review from leandroalonso September 9, 2019 16:20
@shiki
Copy link
Member Author

shiki commented Sep 9, 2019

@leandroalonso Thank you for the review. I've responded to your comments. This is now ready for another look.

@shiki
Remove unreliable post.remoteStatus assertion
7736951 
This should have been a `post.isFailed` check but it’s better to just rely on the `guard` instead.
@shiki
Copy link
Member Author

shiki commented Sep 9, 2019

@leandroalonso

When I tap "Update" (the right-upper button) on a published post, it shows me an alert containing an "Error occurred during saving".

Good find here. That Update button requires the device to be online and I believe we decided not to touch it and keep it out of scope. However, we shouldn't mark the post as “confirmed for auto-upload” since we're not showing any message like “Changes will be published next time your device is online”. I'll take a look.

@shiki
Copy link
Member Author

shiki commented Sep 9, 2019

@leandroalonso Actually, mind if we merge this and I'll fix that in a separate PR? I have a feeling the fix will take longer and with more tests. I reckon it's going to be related to #12498 too. 😬

@yaelirub yaelirub mentioned this pull request Sep 19, 2019
Closed
1 task
@yaelirub
Copy link
Contributor

Since his issue is blocking #12362 and has been approved and confirmed for merge by @leandroalonso , I am merging this.
Hope thats ok, @shiki .

@yaelirub yaelirub merged commit fb953e4 into issue/12240-master-branch Sep 19, 2019
@yaelirub yaelirub deleted the issue/12324-upload-posts-with-remote branch September 19, 2019 19:15
@shiki shiki mentioned this pull request Oct 1, 2019
Closed
@shiki
Copy link
Member Author

shiki commented Oct 1, 2019

@leandroalonso Looping back on #12466 (comment). I created an issue for it: #12589.

@shiki shiki mentioned this pull request Oct 1, 2019
Closed
2 tasks
@leandroalonso leandroalonso mentioned this pull request Oct 1, 2019
Merged
2 tasks
@shiki shiki added this to the 13.6 milestone Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@osullivanchris osullivanchris osullivanchris approved these changes

@leandroalonso leandroalonso leandroalonso approved these changes

Assignees

@shiki shiki

Labels
Offline Support [Type] Task
Projects
None yet
Milestone
13.6
Development

Successfully merging this pull request may close these issues.
4 participants
@shiki @leandroalonso @yaelirub @osullivanchris