Conversation
This is the most robust way of using this use case since clients do not need to manage the dispatcher themselves. This way the use case will always run in the background no matter which client will be using them. Also, this change reduces some of the duplication where every client needs to explicitly call the 'flowOn(...)' operator.
This duplicate check seems innocent, being just a duplicate, but it will later on cause problems with nullable 'downloadId' if not removed now.
This also removes the need to use 'coroutineScope.runBlockingTest' when not really necessary.
Also, this update includes the extra 'with download id' since in the next commit the corresponding 'without download id' will be added to complement this scenario as well.
This is required in order to allow running this use case from within the activity log screen when a web backup launch is triggered and the user (re)launching the screen or triggered a pull-to-refresh. When that happens the activity log screen will query for any backup that might be happening in order to show the progress row. Thus, this 'downloadId' needs to become optional for when that id is not known in advance. And based on that change the if logic to allow querying for status needs to be updated to allow this scenario.
These tests will fail as the implementation is not ready yet. The next commit will make the tests pass (TDD).
This commit makes the previous failing test pass (TDD). Also, this commit updates the 'retryAttempts' variable to '-1' since there is now an extra 'activityLogStore.fetchBackupDownloadState(...)' call happening with a nullable 'downloadId'.
This date will be used to show the actual backup from date on both the progress row and snackbars.
In addition to 'downloadId, this 'rewindId' is necessary in order to show the snackbar on the activity log/backup screen later on.
With this change the 'rewindId' and 'downloadId' are being passed from the 'BackupDownloadFragment' to the 'ActivityLogListFragment', which in turn will trigger 'onQueryBackupDownloadStatus(...)' on its view model layer.
This 'FluxC' tag updates FluxC to that 'develop' version where 'ActivityLogRestClient.fetchBackupDownloadState(...)' handles the empty backup download response and doesn't crash. This step is required in order to show the backup progress row on this screen.
This is necessary in order to later on differentiate between the 'Restore Progress' and 'Backup Download Progress' rows, while checking if the item is shown.
This is done in order to later on differentiate between the restore and backup progress items.
These tests will fail as the implementation is not ready yet. The next commit will make the tests pass (TDD).
This commit makes the previous failing test pass (TDD).
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Generated by 🚫 dangerJS |
|
You can test the changes on this Pull Request by downloading the APK here. |
…h-activity-log-backup-screen' of github.com:wordpress-mobile/WordPress-Android into issue/13329-connect-backup-download-with-activity-log-backup-screen � Conflicts: � WordPress/src/main/java/org/wordpress/android/ui/jetpack/backup/download/usecases/GetBackupDownloadStatusUseCase.kt
When the retry logic was added to this use case, the fetch 'activityLogStore.fetchBackupDownloadState(...)' call ended up presiding the get 'activityLogStore.getBackupDownloadStatusForSite(...)' call, which means that this starting block of code for the case where the 'downloadId' is null is no longer necessary.
There was a problem hiding this comment.
Great job @ParaskP7! I reviewed each commit and every commit message included the right information! If I noted any minor issue in one commit, I saw it fixed in the next few commits, so I don't really have any review comments :).
I just noticed that sometimes progress bar wasn't displayed even after snackbar to inform download has started was shown.
no.progress.bar.mov
Is it related to the issue you mentioned in the description?
Tested on: https://pressable-jetpack-complete.mystagingwebsite.com/
WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt
Outdated
Show resolved
Hide resolved
🙏
Thanks so much for the thorough testing @ashiagr ! 🌟 From the video you shared, I am seeing a couple of things happening:
Actually no, this is not related to the issue I mentioned. The issue I mentioned is causing a |
Yep I agree, progress bar might not have been shown as the download might have been completed very fast. |
Unfortunately, the way it is done atm, we should only see the snackbar if the code goes through the |
Alright, I just wanted to discuss this scenario before approving the PR. I think it is fine to be with this behaviour for now. I'm approving this PR now. Feel free to merge it when CI is done. Issue you mentioned in the description can be fixed in another PR. |
Once again, thank you so much for the thorough review! 🙏 I believe you are correct, I have the same feeling about it. I think it is okay to have such a behavior for the MVP. Also, it might be okay to proceed with merging even with the warning ( |
Parent #13329
This PR connects the backup download flow with the activity log/backup screens. This means that this PR makes sure that when a user presses the
Ok, notify me!button or presses/swipes back (triggering theonBackPressed()), while backup download is in progress from within the backup download screen, then a backup progress row appears on the activity log screen immediately.datais empty and anullmodel is returned back to thestorethen this can end up not overriding the previous progressing state, leaving the progress item loading on screen indefinitely (seeActivityLogStore.fetchBackupDownloadState(...)). @zwarm we should discuss that together to figure out what to do with that since just returning anullmodel it seems to not cover all cases to a 100%.To test:
Scenario.1 (Backup download flow)
My Sitetab,My Site->Toolbar Avatar->App Settings->Test feature configuration.BackupDownloadFeatureConfig, scroll down and click theRESTART THE APPbutton.Activity Logscreen,Download backupto launch theBackup Downloadscreen,Create downloadable filebutton.Ok, notify me!or navigate back manually (by clicking theXtoolbar button or pressing/swiping back).Activity Logscreen and a backup status will be triggered,Activity Logscreen open both, within the app and the web (so you can see the actual progress),Scenario.2 (Restore and backup download flow)
Restorefirst.PS: As always, I suggest doing a commit-by-commit review, from within the IDE, as this will make it much easier for you to review the overall solution. If you need to focus on the feature itself and not the refactor or tests filter the commits by
Featureto get a quick overall.Merge Instructions
Make sure PR Jetpack Section: Connect Restore with Activity Log/Backup Screen #13914 is merged to.developRemove the.Not Ready for MergelabelPR submission checklist:
RELEASE-NOTES.txtif necessary.