Autosave or update draft

[oguzkocer]

This is a PR to discuss possible approaches to fix #10951. This issue is happening because /autosave endpoint has the following behavior:

• If the post is a draft, it directly updates the post. Due to a bug, while updating the post it also disables discussions on the post.
• If the post is not a draft, it creates an autosave (this is what we would have liked)

Unfortunately we are unable to address the issue in the API side, so we are left to fix it in the client in a somewhat hacky way. We'll do what the API does and update the actual draft instead of calling /autosave. This will fix the comments getting disabled issue and also make it obvious what's happening in the client side. (we didn't know that API was updating the actual draft until recently)

@malinajirka As discussed on Slack, I don't really know what the best way to go about this is. The approach I like the most is to encapsulate the solution as much as possible so it doesn't affect the rest of the app.

I have considered 3 types of use cases for this:

1. AutoSavePostOrUpdateDraftUseCase: If we were to go with this, we'd encapsulate everything in the use case and don't return a result back to PostUploadHandler until the whole thing is done.
2. AutoSavePostIfNotDraftUseCase: This is the use case I explored in this PR. The reason I tried this one over the first option is the complication around push post in PostUploadHandler. If we go with the first option we'll have to duplicate the push post logic or refactor that logic into a usecase and somehow chain them. Refactoring is probably a good idea, but I am not sure how complicated it'll be.
3. FetchPostStatusUseCase: This would be the simplest approach and we'll just return the remote post status to PostUploadHandler. Architecturally, this makes the most sense because it does one thing only. If we (only) do this, we can't encapsulate the complication. However, if we go with the first option, we can have this as a helper use case which would work very well.

There are other combinations of these approaches and we might even go with something completely different. I was hoping your experience with PostUploadHandler, however little that might be, could be helpful in deciding what makes the most sense time/value-wise.

Please keep in mind that this is a heavily WIP PR and the goal is to figure out the solution we want as fast as possible. The polish will come later and the TODOs in the PR points to things that I am already thinking about. That said, please feel free to comment on them, especially the question ones.

This is a discussion PR and will be closed once it serves it's purpose, so both testing steps and PR submission checklist is removed. I decided to keep this PR open since it took a lot of discussion to figure out the solution. I don't think opening a new PR would have served any real purpose after the solution was figured out.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[malinajirka]

Thanks @oguzkocer! I think this approach should work. I've left some questions to clarify some parts, but I like it overall.

[oguzkocer]

@malinajirka I've addressed almost everything we talked about yesterday and I think it's ready for a look. I still have documentation and unit tests to add, but to save time on those, I want to make sure we are all good with the approach and the behavior we want. One good news I have is, if we set the publish date of the post and then upload it as draft, the date is preserved and the Schedule button shows up in Calypso. In the app it says Publish but I think that's a bug. Everything seems to be working pretty nice, but I might have missed something.

I am not sure how we should test the errors, but I have added the test cases I was thinking of. Let me know what you think!

---

Test Notes

1. Do all the tests while online
2. Then do all the tests by enabling airplane mode after opening the post list, then make the change, go back to post list and then disable the airplane mode.
3. Finally, queue multiple uploads while in airplane mode.

The results should be the same. In the first set of tests, the upload will happen as soon as the editor is closed. In the second set, it'll happen as soon as connection is available. In the third, all changes will happen as soon as connection is available one after the other.

How to verify an autosave:

1. Open the post in web editor which should say:

There is an autosave of this post that is more recent than the version below. View the autosave
Clicking on the View the autosave link should show the change made.

2. Or, check stetho and make sure /autosave endpoint is called for that post

Test cases:

1. Go to a remote draft's post settings
2. Change the publish date
3. Go back to post list
4. Verify that the draft is updated notification shown
5. Go to Calypso and verify that the publish date is what you have set to in Step 2 and "Schedule" button is visible (instead of "Publish")
6. Go to draft in the app and verify "Schedule" button is visible (instead of "Publish")

---

1. Open a remote draft
2. Make any change
3. Go back to post list
4. Verify that the draft is updated notification shown
5. Verify that the draft is actually updated from Calypso
6. Verify that there are no local changes for the draft in the app

---

1. Open a published post
2. Make any change
3. Go back to post list
4. Verify that Local changes label is visible for that post
5. Verify that the post is auto-saved

---

1. Open a privately published post (It'll be in the published list but will have a Private label)
2. Make any change
3. Go back to post list
4. Verify that Local changes label is visible for that post
5. Verify that the post is auto-saved

---

1. Open a scheduled post
2. Make any change
3. Go back to post list
4. Verify that Local changes label is visible for that post
5. Verify that the post is auto-saved

[malinajirka]

I have reviewed the code again and tested all the scenarios. I have found some things I'm not sure about. Let me know what you think ;)

Dangling upload in progress notification

1. Turn on the airplane mode
2. Edit 3 different published posts and don't explicitly save the changes
3. Turn off the airplane mode
4. Notice the changes are auto-saved but the uploading notification doesn't disappear
   (I think it might be related to one of my in-code comments (event priority).)

One good news I have is, if we set the publish date of the post and then upload it as draft, the date is preserved and the Schedule button shows up in Calypso. In the app it says Publish but I think that's a bug.

Hmm, interesting. Tbh I'm not sure it's a bug. The editor actions rely on the state being valid. The fact that our architecture allows saving an invalid state (PostStatus: Published, Date: in the future) is unfortunate, but I think checking for this invalid state all over the place is even worse. The app shows "Publish" action since the PostStatus is "Draft" which is correct. If what I said is true, I'd consider reseting the date when we change the status to "Draft". Wdyt?

[malinajirka]

I'm sorry I missed this during the initial review 😞

The previous implementation was subscribed on the MAIN thread and had priority set to 9. We wanted to ensure the PostUploadHandler receives the event before UploadService.
The UploadService invokes stopServiceIfUploadsComplete and checks mPostUploadHandler.hasInProgressUploads(). If we don't use the priority, we'll introduce a race condition as mPostUploadHandler.hasInProgressUploads() will return true or false depending on which class received the event first, UploadService vs PostUploadHandler.

Wdyt?

[oguzkocer]

I have made this change in bb985b1, but I couldn't test it because I can't reproduce the issue even before the change. Also, as discussed on Slack, I think there is a better way to handle the priority issue, so I'll open an issue for that once this PR is merged.

[oguzkocer]

@malinajirka Thank you for the review. I've fixed the issues you mentioned and replied to your comments with the commit hashes. I've then added documentation in 1268a78. Finally, I worked on the unit tests in all the remaining commits (between 174b742 and 26db1f9) which are contained in one fileAutoSavePostIfNotDraftUseCaseTest, so you can review that separately. The only case I didn't intentionally cover in the unit tests is the IllegalArgumentException when a post is already being processed as I don't think it'll provide enough value.

Let me know what you think!

[malinajirka]

Thanks @oguzkocer! I've reviewed the changes and tested several scenarios. I haven't encountered any issues.

Feel free to merge this PR. I'm not merging it since the description still says This is a discussion PR and will be closed once it serves it's purpose ....

[oguzkocer]

Thanks @malinajirka! I intended to close this PR and open a fresh one when we figured out the approach. However, that happened fairly late in the PR, so I think opening a fresh one would just confuse us if we need to refer back to it.