Skip to content

Issue/13326 scan fixing in builder#13940

Merged
malinajirka merged 25 commits intodevelopfrom
issue/13326-scan-fixing-in-builder
Feb 4, 2021
Merged

Issue/13326 scan fixing in builder#13940
malinajirka merged 25 commits intodevelopfrom
issue/13326-scan-fixing-in-builder

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Feb 2, 2021

Parent #13326

This PR moves threat fixing state item building logic to the builder class. It fixes the issue when progress bar was not displayed when an ignored threat was fixed.

It also

  • updates header, description
  • removes labels from the progress bar
  • adds loading spinner on threat list items when threats are fixing
    to have a bit of similarity with the iOS app.
threats_fixing.mov

To test

Prerequisite:

  • A threat is added to the testing site. In case you run out of threats, re-install bad calendar plugin and re-run scan.
  • Scan feature flag is enabled
  1. Open Scan screen
  2. Start scan
  3. Click on "Fix threats" button
  4. Notice that
    • app displays "Fixing Threats" state and progress indicator is displayed on the threat items
    • on pressing back and returning to the screen while threats are fixing, "Fixing Threats" state is re-shown
    • as threats get fixed, they are removed from the list
    • once fixing threats list is empty, scan state is re-fetched and displayed

** Also test the flow when an ignored threat is fixed with steps from PR #13924.

Notes

  • Ignore styling

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.7 milestone Feb 2, 2021
@ashiagr ashiagr self-assigned this Feb 2, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 2, 2021

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

@ashiagr ashiagr mentioned this pull request Feb 2, 2021
3 tasks
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 2, 2021

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

@peril-wordpress-mobile
Copy link
Copy Markdown

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

…re-to-help

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/builders/ScanStateListItemsBuilder.kt
#	WordPress/src/test/java/org/wordpress/android/ui/jetpack/scan/builders/ScanStateListItemsBuilderTest.kt
@ashiagr ashiagr requested a review from malinajirka February 3, 2021 05:20
@malinajirka malinajirka self-assigned this Feb 3, 2021
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Looks great @ashiagr, thanks!

 There's now a separate threats fixing state in the scan state builder class and tests for threats fixing state content items are added to builder test class.
@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Feb 3, 2021

Thanks for the review @malinajirka, ready for another look.

@ashiagr ashiagr requested a review from malinajirka February 3, 2021 12:32
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements @ashiagr! I found one potential issue, let me know what you think ;).

@ashiagr ashiagr requested a review from malinajirka February 4, 2021 11:21
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@malinajirka malinajirka merged commit 3820777 into develop Feb 4, 2021
@malinajirka malinajirka deleted the issue/13326-scan-fixing-in-builder branch February 4, 2021 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants