Issue/13326 fix threats status progress labels#13872
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
ParaskP7
left a comment
There was a problem hiding this comment.
👋 @ashiagr !
I have reviewed and tested this PR as per the instruction, great job! 🌟
I have left one minor (🔍) comment and question (❓). I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.
PS: What is the history behind the ScanStateListItemsBuilder class and its test? I am seeing all of them being commented out. I really think that if test are written for this class it would help enormously, even with the code review process. For example, having this buildFixThreatsProgressInfoLabel(...) function being tested with all the various results it can produce it could have been a great help. Wdyt?
WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanViewModel.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the review Petros, I've addressed your comment and answered the question.
These tests were introduced when screen items were not added as list items, I have to tweak them for the updated structure. Adding it to the backlog. Ready for another look 🙇♀️. |
|
👋 @ashiagr ! Thanks for applying my suggestions, it all looks good to me! 🙏 |
Parent #13326
This PR
jetpack_list_progress_itemlayout and updates correspondingProgressState.To test:
Prerequisite:
Login to the app using above WP.com account and select the site on the My Site tab
Click scan menu item on My Site
Click fix threats button on the scan screen + ok in the confirmation dialog
Notice that
PR submission checklist:
RELEASE-NOTES.txtif necessary.