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

Post gets automatically published without a confirmation #10021

Closed
malinajirka opened this issue Jun 11, 2019 · 20 comments
Closed

Post gets automatically published without a confirmation #10021

malinajirka opened this issue Jun 11, 2019 · 20 comments
Assignees
Milestone

Comments

@malinajirka
Copy link
Contributor

malinajirka commented Jun 11, 2019

Update: The issue got hotfixed in 0abedf6. However, I believe we should keep this ticket open as there is an ongoing discussion about a proper fix.

Expected behavior

LocalDraftUploadService uploads only local drafts and never publishes them.

Actual behavior

LocalDraftUploadService publishes posts which status was set to "Publish" in the Post settings.

Steps to reproduce the behavior

  1. start a draft, enter title and text
  2. post settings -> change status from Draft to Publish
  3. Turn on airplane mode and exit the editor
  4. The post is saved as local draft
  5. Turn off airplane mode
  6. Notice the post gets published

  1. start a draft, enter title and text
  2. post settings -> change status from Draft to Publish
  3. exit the editor
  4. Notice the post gets published
Tested on [device], Android [version], WPAndroid [version]

Emulator API 27

@malinajirka
Copy link
Contributor Author

Solution to this ticket is dependant on(blocked by) #9555.

@yaelirub yaelirub added the Design Needed A design solution is needed. label Jul 2, 2019
@diegoreymendez
Copy link

Changing the status field in settings isn't a great experience, especially if it's going to be saved without confirmation. This is an issue that requires a UI/UX review.

@osullivanchris
Copy link

If I understand correctly this is the same as #9555 but just publishing from settings rather than the publish button. Could I suggest we bring the two in-line by doing the following when user changes post status to Publish

  1. Show the confirmation dialogue the same as when the user taps Publish
  2. Show the proposed toast message informing the user it will be published when they come online

That would bring the two experiences closer together

@malinajirka
Copy link
Contributor Author

@osullivanchris I'd like to confirm the behavior before we start implementing it.

change_post_status

I've added an extra step (confirmation dialog) into the current flow. However, the flow seems a bit inconsistent.

  1. The dialog says it'll be published immediately even though it won't. We simply change the status of the post but we don't upload it until the user leaves the editor.
  2. Related to the above - I'm not sure when to show the snackbar.

@osullivanchris
Copy link

@malinajirka thanks for pointing out this hole in the design! Thought it was gonna break it, but actually think we can just shift the confirmation dialogue a little.

IMG_1514

Nothing happens until the user leaves the editor. So when they are leaving the editor, then confirm that they really intend to publish the post. Brings it in line with the general solution I've proposed.

Then if online they get the usual toast message and moves to published. If offline they get the 'publishing when online' status, cancel etc per #9555

This means (a) we've gotten extra confirmation from the user on their intention and (b) we tell them that we're gonna do it for them when online. So the issue discussed shouldn't be a problem in future, as they'll know what to expect and can also cancel.

Hope you don't mind all the lo-fi sketches :D Just the quickest way to turn this around!

@shiki
Copy link
Member

shiki commented Jul 9, 2019

A user issue that's related to this: p4a5px-2pG-p2.

@malinajirka
Copy link
Contributor Author

malinajirka commented Jul 10, 2019

Thanks @osullivanchris. I like the idea and the flow makes sense. However, I believe the solution doesn't cover all the cases.

  1. The user can leave the app by pressing a home button before they leave the editor.
  2. The user can leave the app if another app comes to a foreground - eg. there is an incoming call.
  3. The app can crash or be forced closed before the user leaves the editor.

Having said all that, I think we could solve this by having a flag (eg. publishConfirmed) in the database as we discussed in some other ticket. The AutoUploader wouldn't publish any posts unless their publishing was explicitly confirmed by the user. If we had this, this flow could more or less work... We would still have posts with local 'published" status which haven't been confirmed. We could show a new label (eg. Waits for confirmation) and wait for a user interaction.
Wdyt? cc @shiki (btw this approach would also solve #10090)

OffTopic: Regarding the "online/offline" state in the sketch. I think it technically won't be online/offline state, but rather upload succeed/failed state. But let's leave this discussion for the ticket where we actually introduce "publishing when connection returns" message.

@osullivanchris
Copy link

@malinajirka I did think about that. I don't think we should ever set something to publish before reaching that confirmation dialogue. So the UX here basically says you haven't really updated the status of the post to published until you hit that confirmation dialogue.

Would it also be possible to limit that in the code, only commit that change to the status after the user hits the confirm on the dialogue? Then that would stop these scenarios from happening rather than having to come up with solutions for them.

Then if user changes from draft to publish in the UI, change is not committed on the backend. If they crash before they get to the dialogue, then open the app again, its back on draft.

@diegoreymendez
Copy link

@malinajirka - In iOS we have something like the flag you propose, but it's different. Instead of being a timestamp, it's a field that hold the desired status the user wanted for the post. This allows the user to signal not only that they wanted to publish a post, but even that they wanted to move a published post back to draft, or make other status changes.

@shiki
Copy link
Member

shiki commented Jul 10, 2019

@malinajirka I also like @diegoreymendez' suggestion. 😄 It looks like we can use the same field for the intent to send to trash?

@malinajirka
Copy link
Contributor Author

malinajirka commented Jul 11, 2019

Having said all that, I think we could solve this by having a flag (eg. publishConfirmed) in the database as we discussed in some other ticket.

We've improved this since I posted this comment. We were discussing using "publishConfirmedAt" timestamp instead. The main benefit of using a timestamps is that you don't need to change publishConfirmed back to false when you edit the post after it has been confirmed.

Would it also be possible to limit that in the code, only commit that change to the status after the user hits the confirm on the dialogue? Then that would stop these scenarios from happening rather than having to come up with solutions for them.

Thanks for sharing @osullivanchris ! Yes, it's possible. However, the fix is useful not just for this ticket but for other tickets as well.

Moreover, I'm a bit concerned it would bring another issues.
Eg.

  1. Imagine you confirm publishing -> you change the status to Publish -> you edit the post but you don't confirm publishing -> would we need to change the status back to draft?
  2. Imagine you are editing an already published post (the post is already in Published state) -> how do you indicate that the post has some changes which haven't been confirmed by the user yet and shouldn't be published?

In iOS we have something like the flag you propose, but it's different. Instead of being a timestamp, it's a field that hold the desired status the user wanted for the post.

Thanks for sharing @diegoreymendez. I think both solutions have their pros and cons. Tbh I feel like they are trying to solve a bit different issues.

If we use "publishConfirmedAt" timestamp
+ We can use it for any edits to the post what so ever (even to an already published post) - status, title, content, tags, featured image, ...
+ When the user confirms publishing the changes and they decide to edit the post, we can easily make sure the post with the new changes won't be published until the user confirms it again.
+ I think we can use it for moving a published post between draft/pending/private/published statuses (they all use the "pushPost" endpoint - I'm not 100% sure about it)
- We can't use it for post trashing as the UploadService needs to know when to call a different endpoint.

If we use "field that hold the desired status the user wanted for the post" (I'm not sure I understand it correctly, so please correct me if I'm wrong)
+ We can use it for moving a published post between draft/pending/private/published and even Trashed statuses
- We can't indicate that new changes to an already published post shouldn't be published
- We can't indicate that the user made some new changes to a post which was confirmed

I think it's worth considering combining these two solutions. Having access to the remotePostStatus makes sense to me and I believe it could be useful in various scenarios. However, we need to make sure it'll work as expected - afaik for example "Scheduled" status doesn't exist on the server, it's just a status in the Android (?iOS?) app. I'm also wondering if the action (push, trash) should belong to PostModel and not just UploadModel.

(It's also worth mentioning that the app currently relies on the "postStatus" field being up-to-date with what the user desires and the app changes the UI state accordingly. So it might require more changes than we can anticipate right now (🙈 changes in EditPostActivity).)

cc @shiki

@osullivanchris
Copy link

@malinajirka haven't had time to come back to this one yet, but now thinking it would be simpler to just remove publishing from settings, and I think this has traction. See the comment on wordpress-mobile/WordPress-iOS#12099 (comment)

@malinajirka
Copy link
Contributor Author

malinajirka commented Jul 15, 2019

@osullivanchris I think another scenario which might result in a post being published without user's confirmation is when the user sets a publish date to the future -> the post status is changed to "SCHEDULED". I'm not sure we can come up with a simple workaround for this case - we can't simply remove it from the options.
We are most probably going to introduce the "publishedConfirmedAt" field anyway. I believe it won't require any UI/UX changes. We'll simply make sure posts which haven't been confirmed yet won't be published.

@osullivanchris
Copy link

@malinajirka for scheduled, is your concern for an offline user or an online user?

For an online user, they choose a publishing time, and tap the word 'schedule'. I think its quite a deliberate action. I am not sure it requires an extra confirmation layer on top of what is there already.

I am primarily concerned with publishing from the 'Status' setting as it seems like a very light weight interaction considering the consequences. Scheduling does not feel so lightweight to me.

The only concerns I have looking at scheduling are:

  • If I don't tap 'Schedule' in the header after choosing a date/time I seem to end up in a limbo state where I haven't actually scheduled the post but the action in the header is 'schedule'. Perhaps if the user tries to leave the editor in that state we should force them to decide first (confirm/cancel scheduling)
  • If a user schedules offline (which is part of the offline actions proposal) there's probably a time period after which we can no longer consider their plan to schedule as 'confirmed' but I think it could inherit the same proposal to forget the action after 48 hours.

Let me know if we're not on the same page or if there's something I've missed here

@malinajirka
Copy link
Contributor Author

For an online user, they choose a publishing time, and tap the word 'schedule'. I think its quite a deliberate action.

The trick is that they don't need to click on "Schedule". As soon as they pick a date, the status of the post is changed to "Scheduled" (I'm 90% sure about this:D). It's not an issue now but will become an issue when we start auto-uploading. However, the "publishedConfirmedAt" solution fixes this, so it should be fine.

f I don't tap 'Schedule' in the header after choosing a date/time I seem to end up in a limbo state where I haven't actually scheduled the post but the action in the header is 'schedule'.

I think the users are used to this now, so I'd suggest not changing it. Basically we'll never publish/schedule a post unless the user explicitly clicks on the action.

@diegoreymendez
Copy link

diegoreymendez commented Jul 16, 2019

@malinajirka - This is a very interesting discussion because in iOS I currently have an issue assigned to try to also upload posts that weren't explicitly saved / published by the user.

Bear with me for a second for going back to the remoteStatus field. What it does is store the value failed when an upload attempt didn't succeed for that post. This means that there was an explicit action by the user to try and upload it, and it probably failed because of a crappy connection.

Is the publishConfirmedAt check we want to do in Android only for publishing posts, or is it also for drafts and other types of posts?

Does this flag you're adding prevent other types of unwanted status changes when uploading posts? (like draft to scheduled or vice-versa)

@diegoreymendez
Copy link

Also an interesting question: if the post status for a post was changed from published to draft, and the app crashed... when reopening it would the post be saved as a draft?

Bottom line is: I'm wondering if we should auto-upload at all when the user hasn't confirmed a save operation explicitly.

@malinajirka
Copy link
Contributor Author

malinajirka commented Jul 17, 2019

Bear with me for a second for going back to the remoteStatus field.

Tbh I'm really interested in understanding this approach. So please bear with me @diegoreymendez :D.

What it does is store the value failed when an upload attempt didn't succeed for that post. This means that there was an explicit action by the user to try and upload it, and it probably failed because of a crappy connection.

I'm sorry, but I'm probably still missing something here:(. I'd suggest discussing this synchronously so we can get on the same page:).

Some of the scenarios I'm not sure about:

  1. User wants to publish a draft -> remoteStatus is set to "Publish"
  2. The upload fails -> remoteStatus is set to "Failed"
  3. The app is restarted -> how does the app now the user wanted to Publish the post since the current value of remoteStatus is "Failed"?

  1. User wants to edit a published post in offline, but they don't click on "Publish/Update" -> I assume remoteStatus is still "Publish"
  2. The upload is not started -> the user didn't click on publish
  3. The app is restarted and the internet connection is available
  4. How does the app know, that it shouldn't auto-upload the changes to the published post, but it should auto-save them?

Is the publishConfirmedAt check we want to do in Android only for publishing posts, or is it also for drafts and other types of posts?
Does this flag you're adding prevent other types of unwanted status changes when uploading posts? (like draft to scheduled or vice-versa)

It's for any change to a post whatsoever. The name isn't final - the current proposal is changesConfirmedAt.

if the post status for a post was changed from published to draft, and the app crashed... when reopening it would the post be saved as a draft?

Any change needs to be confirmed. So if the user didn't click on save/submit/publish/schedule before the crash, the change wouldn't be visible to the readers of the site (it can be only autosaved or auto-uploaded if it's a remote draft).

@diegoreymendez
Copy link

@malinajirka - I'm interested in having a synch chat as well. It would be ideal for me if we could discuss it sometime soon, as it would unlock this open PR on my end: wordpress-mobile/WordPress-iOS#12178

@malinajirka
Copy link
Contributor Author

This was fixed via - #10174

@malinajirka malinajirka added this to the 13.4 milestone Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants