LocalDraftUploadStarter: Upload from anywhere

[shiki]

Closes #9852.

This changes LocalDraftUploadStarter to queue uploads from anywhere. To accomplish this, I converted the class to a @Singleton so it can live in the background and watch for internet connection changes and app foreground events.

I also added a few helpers:

• 77e5a86 UploadServiceFacade to make LocalDraftUploadStarter testable.
• d3c1200 LiveData.skip(times:) to prevent triggering uploads twice on app start.

Decisions

Some events like App start trigger uploads of all local drafts of all sites. I could not find an easy way to capture the currently selected site from the LocalDraftUploadStarter class. But I believe this is what the user would expect anyway.

For events triggered in the Post List, I opted to upload only the local drafts of the current site. I initially thought this would be good for UX perceived-performance when we include all Post statuses like local changes in LocalDraftUploadStarter soon. I was worried that the user would think nothing is happening while we are querying too many things. Now, I'm not so sure. Perhaps we can set it so the current site is prioritized in the queue instead. 🤔

I also pondered making LocalDraftUploadStarter a Service but it looks like Services run on their own process which kind of makes it quite heavy and complicated as opposed to a singleton.

Events

The following shows the trigger changes and which events trigger what operation.

Event	Before	After	Uploads Which Site?
App started	✖️	✔️	all
App placed in foreground	✖️	✔️	all
Internet connection is restored while Post List is open	✔️	✔️	all (used to be the current only)
Internet connection is restored while in other pages	✖️	✔️	all
The Post List is opened	✔️	✔️	current
Swipe to refresh on Post List	✔️	✔️	current

Testing

1. Go offline
2. Create a local draft
3. Trigger one of the events above. A snackbar should be shown that the local draft(s) were uploaded.

Please also test local drafts from sites that are not currently selected. Depending on the event, they should be automatically uploaded too.

Release Notes

• If there are user-facing changes, I have added an item to RELEASE-NOTES.txt.

[peril-wordpress-mobile]

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

[shiki]

@malinajirka @maxme This is now ready for review. The app changes are short but the new tests made this go over 500 lines. 😄 Please let me know if there's anything I can do to make this easier to internalize.

[malinajirka]

Thank you @shiki ;). Great job!

This changes LocalDraftUploadStarter to queue uploads from anywhere. To accomplish this, I converted the class to a @singleton so it can live in the background and watch for internet connection changes and app foreground events.

I also pondered making LocalDraftUploadStarter a Service but it looks like Services run on their own process which kind of makes it quite heavy and complicated as opposed to a singleton.

I believe Services don't run in a separate process by default A service runs in the main thread of its hosting process; the service does not create its own thread and does not run in a separate process unless you specify otherwise.. Having said that I kind of like using Singleton for this use case.

SideNote: I wasn't 100% sure as I was still a bit worried we might have some troubles with the device falling asleep or our application being killed after the WorkManager triggers (as background applications which don't have a service running have a low priority and are prone to being killed by the system). During a research whether WorkManager automatically holds a WakeLock a found this developers should use android.app.job.JobScheduler to schedule a job, and this does not require that the app hold a wake lock while doing so (the system will take care of holding a wake lock for the job). This made me realize we are still talking about using WorkManager for the periodic jobs, but we should be probably be using JobScheduler (cc @maxme). Btw it's my bad I think I was the one who started talking about WorkManager 😞 .

I could not find an easy way to capture the currently selected site from the LocalDraftUploadStarter class.

This is an issue I keep encountering on. The currentlySelectedSite is stored in WPMainActivity. The id of the selected site is also stored in AppPrefs.getSelectedSite(), but it's quite inconvenient to load the siteModel from the database + it might not work in some edge cases.
We might want start a discussion about the issue and consider introducing something like SelectedSiteRepo with selectedSite LiveData in FluxC. Wdyt?

But I believe this is what the user would expect anyway.

💯

For events triggered in the Post List, I opted to upload only the local drafts of the current site.

I think it makes sense. I'd probably vote for keeping it as it is.

[shiki]

@malinajirka, thank you for the wonderful review. I have applied some of your suggestions. For the rest, I have responded to them. Please let me know what you think.

We might want start a discussion about the issue and consider introducing something like SelectedSiteRepo with selectedSite LiveData in FluxC

I somehow think the selectedSite should be in WPAndroid instead of FluxC. But maybe not if it's useable in Woo? I feel like this might be abused though. For example, in cases of launched Activities, they should still accept SITE_ID and perform a query instead of getting it from SelectedSiteRepo. I think there was a way for Activities to be restored or launched with a different Site. We might end up some weird bugs because of it. I'm gonna stop the discussion here. We can open an issue if you'd like. 😄

This made me realize we are still talking about using WorkManager for the periodic jobs, but we should be probably be using JobScheduler

I believe we can still use WorkManager but I want to wait until we see what @maxme comes up with to comment more. 🙂

[malinajirka]

Thank you so much for making all the changes @shiki ! I've added two non-blocking comments and approved the PR.

Added error handling which I forgot. I'm not sure how to add a test for this though.

Thanks, I'm not sure how to test it either. I wish CrashLoggingUtils was a mockable class. You could call cancel() on starter.queueUploadFromSite(site) 😞 and verify CrashLoggingUtils.log(.) has been called.

[malinajirka]

How would you feel about catching a more specific exception type. I believe we should follow the Fail Fast principle and avoid hiding unexpected exceptions. Wdyt?

[shiki]

I initially thought of failing fast for this class. But realized that it's a class that will be executed on multiple scenarios like app start, app foreground, etc. If the crash is consistent, users would not be able to use the app anymore.

But if you feel that crashes are unlikely, we can remove the exception handling. I think they're unlikely but I opted to be on the safe side.

[shiki]

As for the specific exception type handling, I only know of the documented @throws in context.startService. What exceptions should we expect from FluxC queries? I only saw RuntimeException from WellSql which is not as specific as I wanted it to be. 😬

[malinajirka]

I initially thought of failing fast for this class. But realized that it's a class that will be executed on multiple scenarios like app start, app foreground, etc. If the crash is consistent, users would not be able to use the app anymore.

Good point 👍

As for the specific exception type handling, I only know of the documented @throws in context.startService. What exceptions should we expect from FluxC queries? In only saw RuntimeException from WellSql which is not as specific as I wanted it to be.

I'm a bit confused - this class doesn't perform any FluxC queries, right?

I think we can expect the documented @throws in context.startService and CancellationException. But I agree with your comment that we don't want to crash the app since this class will be started automatically on app-start, so we might actually want to keep the general "Exception" here.

[malinajirka]

I'd suggest adding a comment with explanation this test suite should be separated from the LocalDraftUploadStarterTest as it contains non-deterministic tests.

[shiki]

Done in 3e675a7.