Skip to content

Issue/13326 trigger fix threats#13795

Merged
ashiagr merged 16 commits intodevelopfrom
issue/13326-handle-fix-threats-result
Jan 19, 2021
Merged

Issue/13326 trigger fix threats#13795
ashiagr merged 16 commits intodevelopfrom
issue/13326-handle-fix-threats-result

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 18, 2021 •
edited
Loading

Parent #13326

This PR triggers fix threats action on scan state screen and handles result from FixThreatsUseCase.
Child PR for FixThreatsUseCase: #13796

As fix threats API is asynchronous, polling for fix status is done in a separate PR: #13798
[PR: #13798 also includes threat fixing flow (web vs mobile) video]

To test

Prerequisite:

  1. Make sure both Scan feature flag and MySiteImprovements flag are set to on in the App settings.
  2. You have access to WP.com account with a site having scan capability and threats (e.g. https://pressable-jetpack-daily-scan.mystagingwebsite.com). Server credentials should be set on the site.
  3. I've added a threat to the site. In case you run out of threats, re-install bad calendar plugin from here and re-run scan.
  • 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 x threats" button on scan state screen with threats
  • Notice that
    • A confirmation dialog is displayed
    • Action buttons are disabled when fix threats action is triggered (when ok clicked on confirmation dialog)
    • (Action) Started message is displayed when action succeeds
    • Error message is displayed when action fails
    • Action buttons are enabled when action fails

(Tests included in ScanViewModelTest)

Notes:

  1. Ignore all styling
  2. Server credentials check is not handled
  3. Message texts are kept close to web version but might not match exactly. Ensure that they'er relevant and we can update them during copy review.

Merge Instructions

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.

@ashiagr
Update ThreatItemState to include fixable field
1a068e8 
Including fixable field in the ThreatItemState will later on facilitate in
- getting ids for fixable threats from the Content UiState in the ScanViewModel (only fixable threat ids should be sent to the fix threats use case).
- distinguishing between fixable and non-fixable threat list items
@ashiagr
Show confirmation dialog on fix all threats click
b66c3c8
@ashiagr
Show fixable threats count on confirmation dialog and fix all button
12bf974
@ashiagr
Tests: Add test for confirmation dialog display on fix all click
28b80be
@ashiagr
Refactor: Move buildContentUiState down in the VM
600c417
@ashiagr
Disable action buttons when fix threats is triggered
d55780e
@ashiagr
Handle fixAllThreats action result
028406c
@ashiagr
Tests: Add tests for fixThreats snack bar messages
adb2156
@ashiagr
Tests: Add tests for disable action buttons on fixThreats action
a30d6b5
@ashiagr ashiagr added this to the 16.6 milestone Jan 18, 2021
@ashiagr ashiagr self-assigned this Jan 18, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 18, 2021 •
edited
Loading

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

@ashiagr ashiagr mentioned this pull request Jan 18, 2021
Merged
3 tasks
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 18, 2021 •
edited
Loading

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

@ashiagr ashiagr changed the title Issue/13326 handle fix threats result Issue/13326 trigger fix threats Jan 18, 2021
@ashiagr ashiagr mentioned this pull request Jan 18, 2021
Closed
67 tasks
@ashiagr ashiagr marked this pull request as ready for review January 18, 2021 08:14
@ashiagr ashiagr mentioned this pull request Jan 18, 2021
Merged
3 tasks
@ashiagr ashiagr requested a review from ParaskP7 January 18, 2021 12:50
Base automatically changed from issue/13326-fix-threats-usecase to issue/13326-ignore-threat January 19, 2021 09:01
ParaskP7
ParaskP7 approved these changes Jan 19, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @ashiagr !

I have reviewed and tested this PR as per the instruction, great job! 🌟

I have left a couple of minor (🔍) comments. I am going to approve this PR anyway, since none is blocking. Also, 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: The one thingy that I noticed while testing this PR is that I had to trigger Scan now twice for it to work. It seemed that the first time it gets triggered it returns immediately as green and only a second trigger would actually triggered a scan and show threats. Not sure if this was a timing matter, I just thought I should notice that here in case you experienced the same at some point.

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 19, 2021

Thanks for the review @ParaskP7! I've included your suggestions.

Ready for another look 🙇‍♀️.

@ParaskP7
Copy link
Copy Markdown
Contributor

ParaskP7 commented Jan 19, 2021

Ready for another look 🙇‍♀️.

🟢 from me @ashiagr ! Thank you so much. 🙏

Feel free to merge when you are ready.

Base automatically changed from issue/13326-ignore-threat to develop January 19, 2021 11:01
@ashiagr
Merge branch 'develop' into issue/13326-handle-fix-threats-result
2ecc149 
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanViewModel.kt
@ashiagr ashiagr merged commit 019d8eb into develop Jan 19, 2021
@ashiagr ashiagr deleted the issue/13326-handle-fix-threats-result branch January 19, 2021 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ParaskP7 ParaskP7 ParaskP7 approved these changes

Assignees

@ashiagr ashiagr

Labels

None yet

Projects

None yet

Milestone

16.6

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @ParaskP7