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

Publish button in Post List should auto-upload the post #12571

Merged
merged 5 commits into from
Oct 1, 2019
Merged

Publish button in Post List should auto-upload the post #12571

merged 5 commits into from
Oct 1, 2019

Conversation

leandroalonso
Copy link
Contributor

Fixes #12382

To test

Case 1

  1. Being offline, create a new post
  2. Tap publish
  3. Tap Cancel button
  4. Tap Publish button
  5. Check that post will be published next time device is online
  6. Go online and make sure post is published

Case 2

  1. Being offline, create a new post
  2. Tap publish
  3. Tap Cancel button
  4. Go online
  5. Tap Publish
  6. Check that post is published right away

Update release notes:

  • 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 (I wasn't able to add a test to the PostListViewController, unfortunately)

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 27, 2019 •
edited
Loading

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

@shiki shiki self-assigned this Sep 27, 2019
shiki
shiki requested changes Sep 27, 2019
View reviewed changes
Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Thank you for working on this, @leandroalonso. Works as advertised. 🎉

I have a question on the success block. Please see my inline comment.

WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift Outdated Show resolved Hide resolved
@yaelirub yaelirub removed the request for review from jklausa September 27, 2019 21:46
@yaelirub
Copy link
Contributor

Hey @leandroalonso , thanks for working on this. The publish button in the post card seems to immediately publish scheduled posts. Is this the expected behavior?
If you tap publish in the editor for a scheduled post, it will add it to the scheduled list.
An auto-upload of a scheduled post will add it to the scheduled list as well.

I'm worried that this might be confusing to the users, thinking that the post will be scheduled to be published and not immediately published.

Thoughts?

@leandroalonso
Copy link
Contributor Author

@yaelirub I think we currently do not support scheduled posts, is that right?

In that case, this is a scenario for the future. What do you think?

@shiki
Copy link
Member

shiki commented Sep 30, 2019

To clarify #12571 (comment) more, we're focusing on getting the flow done as described in #12240. But that is only for drafts and published posts. These are currently ignored:

  • scheduled
  • private
  • pending

One of the reasons is to minimize the amount of manual testing and code reviews we have to do and focus on the flow instead of the post status. Second is adding support for the other post types is mainly going to be just:

  • adding messages for them
  • testing the flow (now, for all post types) -- which we're supposed to do after everything is done anyway.

These post types will be added in #12227. I was hoping that this strategy would save us some time. 😬

shiki
shiki reviewed Sep 30, 2019
View reviewed changes
WordPress/Classes/ViewRelated/Post/AbstractPostListViewController.swift
@@ -908,6 +908,7 @@ class AbstractPostListViewController: UIViewController,

apost.date_created_gmt = Date()
apost.status = .publish
apost.shouldAttemptAutoUpload = true
self.uploadPost(apost)
self.updateFilterWithPostStatus(.publish)
Copy link
Member

Choose a reason for hiding this comment

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

As discussed with @leandroalonso, we will remove these auto-switching of tabs in a separate PR (#12584).

shiki
shiki approved these changes Sep 30, 2019
View reviewed changes
Copy link
Member

@shiki shiki left a comment

Choose a reason for hiding this comment

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

Everything seems to work as expected. LGTM! :shipit:

@yaelirub Let me know if you still have some concerns.

@yaelirub
Copy link
Contributor

I still have some concerns. Let's chat. @leandroalonso , please hold off on merging 🙏

@shiki
Copy link
Member

shiki commented Oct 1, 2019

I talked to Yael about this and we concluded that her concerns were not caused by this PR. We created a separate issue: #12588.

:shipit:

@leandroalonso leandroalonso merged commit c83268b into issue/12240-master-branch Oct 1, 2019
@leandroalonso leandroalonso deleted the issue/12382_publish_button_should_auto_upload_post branch October 1, 2019 13:25
@shiki shiki mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shiki shiki shiki approved these changes

@yaelirub yaelirub Awaiting requested review from yaelirub

Assignees

@shiki shiki

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

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