Skip to content

Issue/13326 fix threats status progress labels#13872

Merged
ParaskP7 merged 11 commits intodevelopfrom
issue/13326-fix-threats-status-progress-labels
Jan 25, 2021
Merged

Issue/13326 fix threats status progress labels#13872
ParaskP7 merged 11 commits intodevelopfrom
issue/13326-fix-threats-status-progress-labels

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 25, 2021

Parent #13326

This PR

  • Adds different progress labels (for showing "Fixing threat" and threat info) to the jetpack_list_progress_item layout and updates corresponding ProgressState.
  • Hides action buttons when threats are fixing.
Before After
before after

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.
  • A threat is added 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. Click fix threats button on the scan screen + ok in the confirmation dialog

  4. Notice that

    • Fix threats state and info labels are shown in the progress layout
    • Action buttons are not shown

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 Jan 25, 2021
@ashiagr ashiagr requested a review from ParaskP7 January 25, 2021 06:57
@ashiagr ashiagr self-assigned this Jan 25, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 25, 2021

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

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 25, 2021

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

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 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?

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 25, 2021

Thanks for the review Petros, I've addressed your comment and answered the question.

What is the history behind the ScanStateListItemsBuilder class and its test?

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 🙇‍♀️.

@ParaskP7
Copy link
Copy Markdown
Contributor

👋 @ashiagr !

Thanks for applying my suggestions, it all looks good to me!

🙏

@ParaskP7 ParaskP7 merged commit 73ca2c5 into develop Jan 25, 2021
@ParaskP7 ParaskP7 deleted the issue/13326-fix-threats-status-progress-labels branch January 25, 2021 11:02
@ParaskP7 ParaskP7 self-assigned this Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants