Jetpack Mobile: Scan menu by ashiagr · Pull Request #13555 · wordpress-mobile/WordPress-Android

ashiagr · 2020-12-07T12:36:39Z

It displays scan menu on My Site for sites with scan capability for a user.

To test

Prerequisite: Make sure both Scan feature flag and MySiteImprovements flag are set to on in the App settings.
(Changes applied only to MySiteImprovements flow)

Test that scan menu is shown on My Site only when the user has scan feature unlocked.

My Site Scan Menu

Case	Site Type	Login Type	Sub Type	User Role	Show Scan	Scan State API Response (Reason)
1a	.com		simple		❌	unavailable (wpcom_site)
1b			atomic	admin	✅	available - idle or scanning
1c				non admin	❌	error (authorization_required)
2a	Jetpack	wp.com login	plan without scan feature		❌	unavailable (missing_scan_capability)
2b			plan with scan feature	admin	✅	available - idle or scanning
2c				non admin	❌	error (authorization_required)
2d		self-hosted login	plan without scan feature		❌	error (authorization_required)
2e			plan with scan feature		❌	error (authorization_required)

Questions

2d, 2e - Scan state rest API gives authorization_required error for self-hosted login for a Jetpack site. Should we always display scan menu via this flow? - Answered in #13555 (comment)

Not in scope
Scan menu icon fine-tuning
Click action

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.

Currently used to fetch or get site's scan state from the ScanStore.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt

peril-wordpress-mobile · 2020-12-07T12:36:45Z

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

peril-wordpress-mobile · 2020-12-07T12:45:41Z

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

malinajirka · 2020-12-08T11:14:25Z

Scan state rest API gives authorization_required error for self-hosted login for a Jetpack site. Should we always display scan menu via this flow?

Great question! I'd consider asking this question on slack and share the summary of our discussion here. I'd personally vote for making a consistent change across all the jetpack features within the app. We might want to always redirect the user to the .com login flow when they access a jetpack feature and they are using a self-hosted login (if we are sure the site has jetpack installed). If the site doesn't have jetpack installed it's a bit more tricky, but I think it's a different discussion.

ashiagr · 2020-12-08T13:41:12Z

Summary of our discussion from Slack:

2d, 2f - Since scan is accessible only with a paid plan and we don’t know what plan users are on if they're are not using wp.com login, we should keep scan menu hidden on My Site for self-hosted login.

Updated the table in the description.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt # build.gradle

malinajirka · 2020-12-15T10:29:40Z

Thanks @ashiagr for working on this PR. Sorry I didn't review it sooner. I marked the notification as read so it slipped through my filters.

I have a couple of points for discussion. I'm not sure which approach is better but I thought I'd share my thoughts.

💡 I'm wondering if we need a service for this task.

It's a simple one time request
Stateful singletons are imo always a bit tricky to work with
It seems the start() initiates the request, but when the request finishes the service doesn't do anything else, right? Until someone calls stop and start again?
Do we need start/stop methods? Have you considered passing the scope from the caller?
To summarize: I'm a bit worried it might be an overkill, but I might be missing something crucial. Wdyt?

Update: This comment is related to this discussion. Let's continue the discussion there and come back here when we decide on an approach.

malinajirka · 2020-12-15T12:55:25Z

Thanks @ashiagr! As we discussed on the call you can ignore the above comment. I didn't realize the RewindStatusService uses this approach. I agree we should keep them consistent and refactor all of them later.

I tested all the cases described above and I noticed a possible issue.

When I

select a site (plan with scan feature | admin)
Go to web in the browser and remove admin privileges to the user
Go back to the app and notice scan is still visible (expected)
Click on switch site
Select a different site
Click on switch site
Select site from step 1
Notice the scan is still visible even though the API response is "{"code":"authorization_required","message":"Can't query information about the current state.","data":{"status":401}}"

There is also a version conflict with develop.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt

…ady present in db for a site

ashiagr · 2020-12-16T06:16:32Z

I tested all the cases described above and I noticed a possible issue.

Great catch ! Fixed in 0603320. Also included unit test for it.
Since we don't update db on error, scan menu will be displayed briefly for scenarios like these (as it first displays last state in the db) before the request to get availability status fails. Is that ok?

malinajirka · 2020-12-16T07:04:06Z

Also included unit test for it.

❤️

Since we don't update db on error, scan menu will be displayed briefly for scenarios like these (as it first displays last state in the db) before the request to get availability status fails. Is that ok?

I think it'd be fine if we showed the "Scan" item once after you switch to the site. However, I noticed I can see the animation every time I select the site. It's not a huge issue, but feels weird. Could we perhaps delete the item from the database on Authorization Error?

# Conflicts: # build.gradle

ashiagr · 2020-12-17T07:04:32Z

Could we perhaps delete the item from the database on Authorization Error?

Yes I think we can do that. Added to backlog (#13326) as it will require fluxc changes:

Delete scan state from the database on Authorization Error

I'll link it here once it is done.

Can we merge this one as we've other PRs ready to merged built upon this one?

malinajirka

Sounds good to me, thank you! ;)

ashiagr added 6 commits December 7, 2020 11:20

Introduce service class

3ba2414

Currently used to fetch or get site's scan state from the ScanStore.

Display scan item based on availability on My Site

e38c0fe

Add tests

7a5dd9b

Merge branch 'issue/13326-scan-skeleton' into issue/13326-scan-menu

618c1a0

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt

Update fluxc hash

5fcc569

Merge branch 'develop' into issue/13326-scan-menu

8ea3955

ashiagr added Jetpack [Status] Not Ready for Merge labels Dec 7, 2020

ashiagr self-assigned this Dec 7, 2020

ashiagr mentioned this pull request Dec 7, 2020

ScanStore: Fix ScanState table fields wordpress-mobile/WordPress-FluxC-Android#1797

Merged

Fix test

0c836fd

ashiagr mentioned this pull request Dec 8, 2020

Jetpack Section: Scan #13326

Closed

67 tasks

Update fluxc commit hash

c62bf7a

ashiagr requested review from malinajirka and zwarm December 8, 2020 06:21

ashiagr marked this pull request as ready for review December 8, 2020 13:48

ashiagr added this to the 16.4 milestone Dec 8, 2020

ashiagr added 3 commits December 9, 2020 13:06

Merge branch 'develop' into issue/13326-scan-menu

21c10e6

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt # build.gradle

Update fluxc hash

6dac632

Update fluxc tag from develop

dfc2167

ashiagr added [Status] Not Ready for Review and removed [Status] Not Ready for Merge labels Dec 9, 2020

Fix multiple requests to get scan available

a9aca07

ashiagr removed the [Status] Not Ready for Review label Dec 9, 2020

ashiagr mentioned this pull request Dec 10, 2020

Jetpack Mobile: Scan Introduce UiState #13578

Merged

3 tasks

ashiagr modified the milestones: 16.4, 16.5 Dec 10, 2020

malinajirka self-assigned this Dec 15, 2020

malinajirka removed the request for review from zwarm December 15, 2020 08:49

ashiagr added 2 commits December 15, 2020 22:21

Merge branch 'develop' into issue/13326-scan-menu

845954d

# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteViewModel.kt # WordPress/src/test/java/org/wordpress/android/ui/mysite/MySiteViewModelTest.kt

Check for scanStateFetchError on scanAvailable update when state alre…

0603320

…ady present in db for a site

Merge branch 'develop' into issue/13326-scan-menu

0f48450

# Conflicts: # build.gradle

malinajirka approved these changes Dec 17, 2020

View reviewed changes

malinajirka merged commit 0d3c6bd into develop Dec 17, 2020

malinajirka deleted the issue/13326-scan-menu branch December 17, 2020 08:06

ashiagr changed the title ~~Jetpack Section: Scan menu~~ Jetpack Mobile: Scan menu Dec 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Mobile: Scan menu#13555

Jetpack Mobile: Scan menu#13555
malinajirka merged 15 commits intodevelopfrom
issue/13326-scan-menu

ashiagr commented Dec 7, 2020 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Dec 7, 2020 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Dec 7, 2020 •

edited

Loading

Uh oh!

malinajirka commented Dec 8, 2020

Uh oh!

ashiagr commented Dec 8, 2020

Uh oh!

malinajirka commented Dec 15, 2020 •

edited

Loading

Uh oh!

malinajirka commented Dec 15, 2020 •

edited

Loading

Uh oh!

ashiagr commented Dec 16, 2020

Uh oh!

malinajirka commented Dec 16, 2020 •

edited

Loading

Uh oh!

ashiagr commented Dec 17, 2020 •

edited

Loading

Uh oh!

malinajirka left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ashiagr commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Dec 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka commented Dec 8, 2020

Uh oh!

ashiagr commented Dec 8, 2020

Uh oh!

malinajirka commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka commented Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashiagr commented Dec 16, 2020

Uh oh!

malinajirka commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashiagr commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ashiagr commented Dec 7, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 7, 2020 •

edited

Loading

peril-wordpress-mobile bot commented Dec 7, 2020 •

edited

Loading

malinajirka commented Dec 15, 2020 •

edited

Loading

malinajirka commented Dec 15, 2020 •

edited

Loading

malinajirka commented Dec 16, 2020 •

edited

Loading

ashiagr commented Dec 17, 2020 •

edited

Loading