Issue/13326 fix scan backup menu animation by malinajirka · Pull Request #13976 · wordpress-mobile/WordPress-Android

malinajirka · 2021-02-05T12:35:28Z

Parent issue #13326

This PR fixes two issues

List item added animation is played whenever you open MySite screen. This is solved by synchronously loading cached value (even when it's expired) of JetpackCapabilities. So unless the permissions changes or the user switches between sites the animation should not be played.

The app would crash if fetchJetpackCapabilities was requested before the previous request finished. This is fixed in f74b76e. New approach adds support for parallel request for different sites. If a duplicate request is initiated for the same site, the previous request is force finished with an error.

To test:

clear app data

Log in with an account with multiple jetpack sites (eg. the one from the doc)
Select "pressable-jetpack-complete" site and notice "Scan" and "Backup" items are shown (with animation)
Open Reader and go back to MySite
Notice "Scan" and "Backup" items are shown (withOUT animation)
Select pressable-jetpack-daily-scan site and notice "Scan" is shown (backup disappears with an animation)
Kill the app
Re-open the app and notice "Scan" is shown (no animation is played)

clear app data

Log in with an account with multiple jetpack sites (eg. the one from the doc)
Enable MySiteImprovements feature flag and repeat steps 2-7 from the above section

Select pressable-jetpack-daily-backup
Open Activity Log and notice filters are shown
Select pressable-jetpack-daily-scan
Open Activity Log and notice filters are NOT shown

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…3326-fix-scan-backup-menu-animation

peril-wordpress-mobile · 2021-02-05T12:35:35Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

malinajirka · 2021-02-05T12:36:35Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

-    suspend fun getJetpackPurchasedProducts(remoteSiteId: Long): JetpackPurchasedProducts {
-        val capabilities = withContext(bgDispatcher) {
-            getOrFetchJetpackCapabilities(remoteSiteId)
+    suspend fun getJetpackPurchasedProducts(remoteSiteId: Long) = flow {


This method now uses flow (it's used only for the new ImprovedMySiteFragment). It always returns cached capabilities and than initiates fetch of the up-to-date capabilities if needed.

malinajirka · 2021-02-05T12:38:23Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

-            fetchJetpackCapabilities(remoteSiteId)
-        }
-    }
+    fun getCachedJetpackPurchasedProducts(remoteSiteId: Long): JetpackPurchasedProducts =


getCachedJetpackPurchasedProducts and fetchJetpackPurchasedProducts are public since they are used directly from the legacy (currently used) My Site fragment. The legacy fragment doesn't use getJetpackPurchasedProducts (flow) -> it needs to synchronously load cached value since the state is refreshed from onResume().

malinajirka · 2021-02-05T12:41:28Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

+    }
+
+    private fun forceResumeDuplicateRequests(remoteSiteId: Long) {
+        continuation[remoteSiteId]?.let {


Instead of throwing an exception, we simply resume any duplicate requests with an exception.

malinajirka · 2021-02-05T12:42:27Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

+            it.resume(event)
+        }
+        if (continuation.isEmpty()) {
+            dispatcher.unregister(this@JetpackCapabilitiesUseCase)


We need to make sure to unregister the instance only when no requests are running (previously, multiple parallel requests for different sites were not supported at all)

malinajirka · 2021-02-05T12:43:15Z

WordPress/src/main/java/org/wordpress/android/ui/mysite/ScanAndBackupSource.kt

    private val jetpackCapabilitiesUseCase: JetpackCapabilitiesUseCase
 ) : MySiteSource<JetpackCapabilities> {
    override fun buildSource(siteId: Int) = flow {
-        emit(JetpackCapabilities(scanAvailable = false, backupAvailable = false))


We don't need to always set visibility to false before the request to fetch capabilities finished, since the use case now always returns the cached value first.

malinajirka · 2021-02-05T12:44:11Z

WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt

            else -> {
                viewModelScope.launch {
-                    val purchasedProducts = jetpackCapabilitiesUseCase.getJetpackPurchasedProducts(site.siteId)
+                    val purchasedProducts = jetpackCapabilitiesUseCase.getCachedJetpackPurchasedProducts(site.siteId)


ActivityLogViewModel will always use the cached value now.

malinajirka · 2021-02-05T12:46:40Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

+            mapToJetpackPurchasedProducts(getCachedJetpackCapabilities(remoteSiteId))

-    private suspend fun hasValidCache(remoteSiteId: Long): Boolean {
-        return withContext(bgDispatcher) {


I've removed all context switches from this class, since it doesn't perform any long-running operation so it can run even on the Ui thread. I previously considered loading data from sharedPreferences for a long-running operation - but it's causing more troubles than it's worth it.

I agree with the decision made here.

malinajirka · 2021-02-05T12:51:41Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

+    suspend fun fetchJetpackPurchasedProducts(remoteSiteId: Long): JetpackPurchasedProducts =
+            mapToJetpackPurchasedProducts(fetchJetpackCapabilities(remoteSiteId))
+
+    private fun mapToJetpackPurchasedProducts(capabilities: List<JetpackCapability>) =


This has not been changed - just extracted into a standalone method

malinajirka · 2021-02-05T12:51:58Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

+                    backup = capabilities.find { BACKUP_CAPABILITIES.contains(it) } != null
+            )
+
+    fun hasValidCache(remoteSiteId: Long): Boolean {


This method was not changed at all - I'm not sure why github marked it as changed.

Happens to me sometimes too .. I think it's AS .

malinajirka · 2021-02-05T12:52:37Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/JetpackCapabilitiesUseCase.kt

-                updateCache(remoteSiteId, capabilities)
-            }
-            return@withContext capabilities
+        forceResumeDuplicateRequests(remoteSiteId)


I've removed the "withContext" and replace throw exception with forceResumeDuplicateRequests(remoteSiteId), the rest is without changes.

peril-wordpress-mobile · 2021-02-05T12:54:21Z

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

zwarm

@malinajirka - Everything is working as expected. 👏 In addition to the tests you specified, I also attempted to recreate the crash and was unable to (switching site quickly - over & over again, back & forth through the tabs with site switching thrown in).

Thanks for the walkthrough, it was very helpful.

malinajirka added 13 commits February 5, 2021 09:57

Set default visibility of backup menu item to GONE

d29758b

Always load cached value first even when expired

05a03ec

Load cached value on main thread

f05eac8

Load cached value synchronously to suppress onItemAdded animation

ca87daa

Always use cached value on ActivityLog

9d037d2

Always use cached value first before fetching up-to-date capabilities

6e34da0

Remove withContext - there isn't any long running operation

a5ca23b

Emit state when site is empty

fac084e

Fix tests

73f75ab

Update JetpackCapabilitiesUseCaseTest

2b7187f

Add support for parallel requests for different sites

f74b76e

Merge branch 'feature/remove-empty-state-on-site-change' into issue/1…

4505372

…3326-fix-scan-backup-menu-animation

Merge branch 'develop' into issue/13326-fix-scan-backup-menu-animation

1fd57d3

malinajirka added /My site Jetpack Mobile labels Feb 5, 2021

malinajirka added this to the 16.7 milestone Feb 5, 2021

malinajirka requested a review from zwarm February 5, 2021 12:35

malinajirka self-assigned this Feb 5, 2021

malinajirka commented Feb 5, 2021

View reviewed changes

zwarm self-assigned this Feb 5, 2021

Remove unused import

fc5bbd1

zwarm approved these changes Feb 5, 2021

View reviewed changes

zwarm merged commit 297ac3d into feature/remove-empty-state-on-site-change Feb 5, 2021

zwarm deleted the issue/13326-fix-scan-backup-menu-animation branch February 5, 2021 14:08

malinajirka mentioned this pull request Feb 5, 2021

Merge changes from 13976 #13978

Merged

ashiagr mentioned this pull request Feb 9, 2021

Jetpack Section: Scan #13326

Closed

67 tasks

Conversation

malinajirka commented Feb 5, 2021

Uh oh!

peril-wordpress-mobile bot commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malinajirka Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malinajirka Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peril-wordpress-mobile bot commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwarm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

peril-wordpress-mobile bot commented Feb 5, 2021 •

edited

Loading

malinajirka Feb 5, 2021 •

edited

Loading

malinajirka Feb 5, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 5, 2021 •

edited

Loading