Conversation
This is required in order to allow running this use case from within the activity log screen when that screen is launched with a rewind request from the activity log detail screen, which doesn't have any 'type' selection functionality. See 'ActivityLogViewModel.onRewindConfirmed(...)' which only holds a reference to the 'rewindId' and doesn't know anything about the 'types'.
This is required in order to allow running this use case from within the activity log screen when a web restore 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 restore that might be happening in order to show the progress row. Thus, this 'restoreId' 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.
This is necessary in order to show progress before the actual rewind running happens. Without that in place, if the user would click the 'Ok, notify me!' button too quick from within the restore screen, then the progress row will not appear immediately on the activity log screen.
Again, this is required in order to allow running this use case from within the activity log screen when a web restore 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 restore that might be happening in order to show the progress row. However, unless the 'ActivityLogStore.fetchActivitiesRewind(...)' is launched at least once 'ActivityLogStore.getRewindStatusForSite(...)' will not get a valid 'rewind' object back. Thus, this fetching of the activities rewind is necessary for that scenario to work properly.
This is done in order to remove the corresponding duplication with the fetch activities rewind triggering and error handling logic.
This filter doesn't allow any nullable 'ActivityTypeModel' object and as
such the 'mapNotNull { ... }' function is not necessary since it can be
replaced with plain 'map { ... }'.
This is done in order for the test to be read with the minimum of effort in terms of understanding where the arrange, act and assert related code is.
|
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. |
My previous commit missed those two cases in this commit because I didn't understand that KtLint will actually fail when something like that happens and not report the next issues on the same file. Thus, I had to run it every time, fix the next issue, rerun, until KtLint succeeds. PS: Detekt will fix all these unnecessary problems with KtLint.
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.
There was a problem hiding this comment.
Really great job @ParaskP7 🥇 ! The logic is incomparably easier to follow now and the concerns are nicely separated ❤️ . I have left a few comments, but nothing vital. I tested the behavior with feature flags on/off and it worked pretty well - see possible issues below.
Note: This PR is quite huge and I found it difficult to review. You did a great job documenting your every step, but review requires focus and being focused while reviewing 70+ commits quite tricky (even if each commit would be extremely simply, which some of them aren't). Tbh I gave up the review-by-commit and I reviewed just the changes - which turned out to be ok, since most of the changes are in tests which don't require such a focused review.
Please always consider splitting PRs into smaller chunks - each of the PRs can be merged into a working branch and you can request that only the final PR needs to be tested. Thanks!
Possible issues
⚠️ When I turn off all feature flags -> open Activity Log -> click on Restore -> it takes +- 5s before the item at the top with progress appears. Before this PR, the item would appear right away.- When I enable "restore" feature flag -> open Activity Log -> Click on "Restore" -> Click on "Restore site" -> Click on "Confirm restore site" -> click on "Ok notify me" -> the progress item doesn't appear, unless I pull down to refresh. (I think this behavior was the same even before this PR)
WordPress/src/main/java/org/wordpress/android/ui/jetpack/restore/RestoreStates.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/jetpack/restore/RestoreStates.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/jetpack/restore/usecases/GetRestoreStatusUseCase.kt
Outdated
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/jetpack/restore/usecases/GetRestoreStatusUseCase.kt
Outdated
Show resolved
Hide resolved
...s/src/main/java/org/wordpress/android/ui/jetpack/restore/usecases/GetRestoreStatusUseCase.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/jetpack/restore/usecases/PostRestoreUseCase.kt
Show resolved
Hide resolved
Thanks for the review @malinajirka ! 🌟
Yes, I could probably merge some of the commits together, like the import commits. Even with test refactoring, maybe there I could have been more relaxed as well, will consider it for next time. Many thanks! 🙏
I really considered that, it is just that the overall solution was keep getting a bit more involved as I was progressing with it. Now that you know what was involved, how would you split the PR further, can you make a suggestion? 🙏
Yes, nice catch, I should have mentioned that in the description as well. This is expectable because of the way the use case works and the 5' seconds delay. I didn't want to complicate things further thus I decided to not deal with it now. Do you this it is fine if you launch with that as an MVP and improve later. I'll then add a task on the parent ticket. 🌟
Yes, this is going to be part of my next PR. The current |
|
@ParaskP7 - So much easier to understand, very nicely done! 🎆 I've have a few takeaways that I will be refactoring in GetBackupDownloadStatus based off of the comments here:
One comment not related to the code in this PR, but related to the use case. |
I usually prefer to have PRs with a single focus and this PR was making a lot of improvements not directly related to the task. I really love these improvements. I was wondering if we could have for example created a separate PR for test refactoring + the new tests added before you actually started working on the changes. Perhaps it wasn't feasible, it's hard to tell as a reviewer. Thank you so much for considering it!
👍
❤️ |
ashiagr
left a comment
There was a problem hiding this comment.
@ParaskP7 , I reviewed each commit and appreciate the level of effort put into building the solution. I really liked the refactoring effort like
- re-ordering functions close to the places they're used
- creating private group
- extracting smaller functions out of bigger ones, increasing readability
Great job!
I agree with @malinajirka that this PR could've been split into smaller PRs. But I also understand that sometimes it is difficult to do that while being so much involved in building a solution. It happens to everyone of us. Nice thing is commits are split into separate ones specific to their purpose. We might consider cherry picking some of the similar commits (like for tests refactoring) and creating a separate PR out of it. It might be too much of an effort, and as always we have to find a balance.
I just left one question. Approving it from side.
WordPress/src/test/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModelTest.kt
Show resolved
Hide resolved
|
👋 @zwarm !
Thank YOU! ❤️
I am going to create 2 PRs today, one for the restore integration and one for the backup integration. It is probably better if I do the change on the
I remember the 3x error retry login on the |
|
👋 @malinajirka !
I so much agree, and apologies for making the move and created one PR instead of two, I could definitely do what you suggested (refactor + tests) and then the actual solution. Please expect that next time. I probably got caught in the fever of delivering since I have been delaying this solution more than I expected (or wanted). 🙏 |
|
👋 @ashiagr !
❤️
Yes, I totally agree with you all, I should have split this PR at least into two (refactor + tests). Thanks so much such suggestions. Please expect me to do that next time. 🙏 |
Please don't apologize :). It's very difficult to find the balance before you start working on a task. As you progress it becomes more and more difficult to split the work up. Thank you for all the effort you put into documenting your every step and for the great detailed PR description! It helped all of us ;) |
|
👋 @malinajirka !
The PR I was referring to is now ready, you can check it really quick if you are curious: #13914 |
Parent #13328
This PR replaces the
RewindStatusServicewith thePostRestoreUseCaseandGetRestoreStatusUseCase. This step is necessary in order to reuse those use cases, remove the complicated status services and in general reduce the complexity within theActivityLogViewModel. Also, this step is a prerequisite and as much necessary for another feature that is about to be added to this screen, which is showing a progress row when aBackup Downloadhas been triggered.developis will affect the next release as this refactor is not guarded on a feature flag. Make sure to do a thorough code review. @zwarm @malinajirka and @ashiagr I have added all of you as reviewers to this PR as this is a complicated one and I would love to get as much comments as possible. At least, testing the PR and finding out bugs would be a major help.ℹ️ Don't be scared by the amount of commits (77), the file changes are not that many (9). The commits are that many because of the many refactors I had to do in order to improve the state of the
ActivityLogViewModeland its corresponding tests to be able to remove the status service and replace that with the use cases. If you do this code review commit-by-commit it will help you enormously.Future TODOs:
RewindStatusServicehas not been completely removed due to the fact that theActivityLogDetailViewModelalso uses it just to show/hide theRESTOREbutton. An item for that will be added on the parent ticket.To test:
Scenario.1 (App flow)
My Sitetab,Activity Logscreen,RESTOREicon and then on the dialog that appears, click on theRESTORE SITEbutton,Activity Log Detailsscreen, click theRESTOREbutton and then on the dialog that appears, click on theRESTORE SITEbutton,Activity Logscreen, on top of the list, and a restore will be triggered,RESTOREicon, for every rewindable item, is not shown while restore progress is ongoing.Activity Logscreen open both, within the app and the web (so you can see the actual progress),activityLogStore.fetchActivities(...)API call). In case the new restore item does not appear, wait a bit (maybe 10'' seconds) and trigger a manual pull-to-refresh. Verify that the new restore item appears after some time.RESTOREicon, for every rewindable item, is now shown again.Scenario.2 (Web flow)
My Sitetab,Activity Logscreen, or trigger a pull-to-refresh if you are already within that screen.RESTOREicon, for every rewindable item, is not shown while restore progress is ongoing.Activity Logscreen open both, within the app and the web (so you can see the actual progress),activityLogStore.fetchActivities(...)API call). In case the new restore item does not appear, wait a bit (maybe 10'' seconds) and trigger a manual pull-to-refresh. Verify that the new restore item appears after some time.RESTOREicon, for every rewindable item, is now shown again.Scenario.3 (Go wild flow)
Activity Log Detailpage, and then go back. Go to the top of the list and make sure the progress item is still shown.Scenario.4 (Restore flow)
GetRestoreStatusUseCaseit is recommended that theRestore Flowis tested as well.My Sitetab,My Site->Toolbar Avatar->App Settings->Test feature configuration.RestoreFeatureConfig, scroll down and click theRESTART THE APPbutton.Activity Logscreen,Restore to this pointto launch the `Restore screen,Restore sitebutton. On the confirmation screen, click on theConfirm restore sitebutton.Doneand in general verify everything works as expected.PS: I REALLY suggest doing a commit-by-commit review as this will make it much easier for you to review the overall solution.
PR submission checklist:
RELEASE-NOTES.txtif necessary.