Skip to content

Issue/13326 ignore threat#13768

Merged
malinajirka merged 24 commits intodevelopfrom
issue/13326-ignore-threat
Jan 19, 2021
Merged

Issue/13326 ignore threat#13768
malinajirka merged 24 commits intodevelopfrom
issue/13326-ignore-threat

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 14, 2021

Parent #13326

This PR handles ignore threat action on the threat details screen.
Child PR for IgnoreThreatUseCase: #13767

It mimics behaviour on the web:

  1. Displays a confirmation dialog
  2. Disables buttons while action is taking place
  3. Displays success/ error messages when action completes
  4. Refreshes scan state when ignore threat action is successful
ignore.threat.web.mov
ignore.threat.mobile.mov

To test:
Prerequisite:

  • Make sure both Scan feature flag and MySiteImprovements flag are set to on in the App settings.
  • 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.
  • 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.
  1. Login to the app using above WP.com account and select the site on the My Site tab
  2. Click scan menu item on My Site
  3. Notice that scan state is fetched and shown with threats
  4. Click a threat
  5. Click ignore threat button on the threat details screen
  6. Notice that a confirmation dialog is shown (see video)
  7. Click ok in the confirmation dialog
  8. Notice that action buttons are disabled
  9. Wait for some time till ignore threat action completes
  10. Notice that success message "Threat ignored." is shown
  11. Notice that _update to scan state is triggered after ~2 secs

Repeat above steps and enable airplane mode after step 6. Notice that an error message is displayed and actions buttons are re-enabled.

(Tests included in ThreatDetailsViewModelTest )

Notes

  1. Ignore all styling
  2. Server credentials check is not handled
  3. Loading state while action is taking place - yet to be added

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.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 14, 2021

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

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

peril-wordpress-mobile bot commented Jan 14, 2021

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

@ashiagr ashiagr requested a review from malinajirka January 14, 2021 09:52
@ashiagr ashiagr mentioned this pull request Jan 14, 2021
67 tasks
Base automatically changed from issue/13326-ignore-threat-usecase to develop January 15, 2021 08:40
@ashiagr ashiagr force-pushed the issue/13326-ignore-threat branch from 9a4359e to 1542aa9 Compare January 18, 2021 06:04
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 @ashiagr!

I've left a few minor comments. One thing I noticed is that the confirmation dialog disappears after rotation - it's not a big deal, but we might want to fix it (feel free to add it into the backlog). Let me know what you think ;).

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 18, 2021

Thanks so much for the review @malinajirka! There were nice ideas that improved code readability. Thank you!

One thing I noticed is that the confirmation dialog disappears after rotation - it's not a big deal, but we might want to fix it (feel free to add it into the backlog).

Sounds good, added it to the backlog.

@ashiagr ashiagr requested a review from malinajirka January 18, 2021 11:38
@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

@ashiagr ashiagr mentioned this pull request Jan 19, 2021
3 tasks
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 changes! I've checked the code and smoke tested the app and it LGTM. Feel free to merge this when the CI finishes.

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