Skip to content

Issue/13326 hide scan on wpcom sites#13962

Merged
malinajirka merged 8 commits intodevelopfrom
issue/13326-hide-scan-on-business-sites
Feb 5, 2021
Merged

Issue/13326 hide scan on wpcom sites#13962
malinajirka merged 8 commits intodevelopfrom
issue/13326-hide-scan-on-business-sites

Conversation

@malinajirka
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka commented Feb 4, 2021

Parent issue #13326

This PR hides Scan menu item on .com sites (even atomic). The scan feature is supported on sites with Business plan, however, it's auto-managed so the Scan feature shouldn't be visible to the users on such sites.

To test:

  1. Make sure Scan feature flag is enabled
  2. Select a site with Business plan
  3. Make sure Scan item is not visible
  1. Make sure Scan feature flag and MySiteImprovements feature flag are enabled
  2. Select a site with Business plan
  3. Make sure Scan item is not visible

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.

@malinajirka malinajirka added this to the 16.7 milestone Feb 4, 2021
@malinajirka malinajirka requested review from a team and ParaskP7 and removed request for a team February 4, 2021 12:03
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 4, 2021 •
edited
Loading

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

@malinajirka malinajirka changed the title Issue/13326 hide scan on business sites Issue/13326 hide scan on wpcom sites Feb 4, 2021
@malinajirka malinajirka self-assigned this Feb 4, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 4, 2021 •
edited
Loading

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

@malinajirka malinajirka mentioned this pull request Feb 4, 2021
Closed
67 tasks
@ParaskP7 ParaskP7 self-assigned this Feb 5, 2021
ParaskP7
ParaskP7 approved these changes Feb 5, 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.

👋 @malinajirka !

I have reviewed and tested this PR as per the instruction, everything works as expected, good job! 🌟

I have left only minor (🔍) comments and some suggestions (💡) for you to consider. 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.

@malinajirka
Copy link
Copy Markdown
Contributor Author

malinajirka commented Feb 5, 2021

Thanks for the review @ParaskP7! I've left some comments with my thoughts - I'd really like to hear your thoughts before we merge this PR. Thanks!

@malinajirka
Copy link
Copy Markdown
Contributor Author

malinajirka commented Feb 5, 2021

Thanks @ParaskP7!

I've extracted the shared logic into SiteUtils - I added TODOs there, so we inline the logic into the ScanAndBackupSource when the legacy MySiteFragment is removed. I also decided to use thenCallRealMethod() in tests in order to keep the tests in the ScanAndBackupSourceTest.

@malinajirka malinajirka requested a review from ParaskP7 February 5, 2021 14:47
@ParaskP7
Copy link
Copy Markdown
Contributor

ParaskP7 commented Feb 5, 2021

Thank YOU @malinajirka ! 🌟

I've extracted the shared logic into SiteUtils - I added TODOs there, so we inline the logic into the ScanAndBackupSource when the legacy MySiteFragment is removed. I also decided to use thenCallRealMethod() in tests in order to keep the tests in the ScanAndBackupSourceTest.

I have reviewed the changes and they LGTM! 🌟

Please feel free to merge this PR when CI is complete. 🙏

@malinajirka malinajirka merged commit 0376472 into develop Feb 5, 2021
@malinajirka malinajirka deleted the issue/13326-hide-scan-on-business-sites branch February 5, 2021 15:04
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

@malinajirka malinajirka

@ParaskP7 ParaskP7

Labels

None yet

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

2 participants

@malinajirka @ParaskP7