Skip to content

Issue/13326 fetch fix threats status#13798

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

Issue/13326 fetch fix threats status#13798
ashiagr merged 9 commits intodevelopfrom
issue/13326-handle-fetch-fix-threats-status-result

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 18, 2021 •
edited
Loading

Parent #13326

This PR fetches fix threats status after fix threats action is started and handles result from FetchFixThreatsStatusUseCase.
Child PR for FetchFixThreatsStatusUseCase: #13797

Threat Fixing Flow

fix.threats.web.success.mov
fix.threats.mobile.success.mp4

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
  • Click ok on the confirmation dialog
  • Notice that
    • Indeterminate progress bar is shown and action buttons are disabled while threat is fixing
    • Action buttons are re-enabled when threat fixing completes
    • Success/ error message is shown when threat fixing completes

(Tests covered in ScanViewModelTest)

Notes:

  1. Ignore all styling.
  2. Server credentials check is not handled.
  3. While the scan design i5 (pcdRpT-8X) displays an incremental progress bar, an indeterminate progress bar is shown as we do not get threat fixing progress updates from the API.
  4. Fix threats status progress bar is shown only while user doesn't leave scan state screen. Once user leaves and re-enters scan state screen from the My Site screen, latest scan state is fetched and shown. It will display scanning state incremental progress bar if this latest state is found to be scanning. Note that web version doesn't display "fix threats status progress bar". Suggestions are welcome to streamline this flow.

Merge Instructions

  1. Make sure child PR Issue/13326 fetch fix threats status usecase #13797 is merged to develop
  2. Remove the "Not Ready for Merge label"
  3. Merge this PR

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 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
Closed
67 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.

This was referenced Jan 18, 2021
Merged
Merged
@ashiagr ashiagr requested a review from malinajirka January 18, 2021 13:40
@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Jan 18, 2021

Thanks @ashiagr, the code looks great.

One thing I'd personally consider is not showing the snackbar. It feels unnecessary, since we show the progress bar. One possible enhancement would be updating texts on the screen when the action is in progress. It feels a bit weird that we show "The scan found 1 potential threat ...." with a progress bar, wdyt?

Base automatically changed from issue/13326-fetch-fix-threats-status-usecase to develop January 19, 2021 11:51
@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 19, 2021 •
edited
Loading

One thing I'd personally consider is not showing the snackbar.

I'll add a label while threat is fixing and remove snackbar for fix started, complete. We'll still need snackbars for the failure states, right?

@malinajirka
Copy link
Copy Markdown
Contributor

malinajirka commented Jan 19, 2021

I'll add a label while threat is fixing and remove snackbar for fix started, complete. We'll still need snackbars for the failure states, right?

Looking at this again, perhaps we could remove just the "We are hard at work ..." snackbar. Showing snackbars for complete and failed states feels ok. Or we can ask Chris what he thinks about it.

Feel free to merge this PR and make changes in an upcoming PR.

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 19, 2021

Feel free to merge this PR and make changes in an upcoming PR.

I'll address it in a separate PR. Need approval for merging this one.

@ashiagr ashiagr merged commit 641f364 into develop Jan 19, 2021
@ashiagr ashiagr deleted the issue/13326-handle-fetch-fix-threats-status-result branch January 19, 2021 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka 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 @malinajirka