Skip to content

Scan: Fetch scan state with polling#13740

Merged
ashiagr merged 11 commits intodevelopfrom
issue/13326-fetch-scan-state-with-polling
Jan 12, 2021
Merged

Scan: Fetch scan state with polling#13740
ashiagr merged 11 commits intodevelopfrom
issue/13326-fetch-scan-state-with-polling

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 11, 2021 •
edited
Loading

Parent: #13326

This PR

  • Adds use case to fetch scan state
  • Replaces fetching from ScanStatusService to newly added UseCase in the scan VM
  • Copies progress state, VH from backup download implementation to common section for reuse in scan
  • Updates scan state builder to show progress item for SCANNING scan state

Start Scan Child PR: #13741

To test

Prerequisite:

  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
  4. Return to My Site screen
  5. Start scan for the site from the browser (Login to WP.com account on the web browser, select the site -> Jetpack -> Scan, click "Scan now" button)
  6. Click scan menu item on My Site again while scanning is in progress
  7. Notice that scanning scan state is fetched and shown with a progress bar (progress updates shown in a label)
    (Tests included in FetchScanStateUseCaseTest and ScanViewModelTest )

Notes

  1. Ignore all styling
  2. (Initial) Loading/ Failure states not handled
  3. There's a small delay before UI is displayed on screen load (while first fetch is not complete). Showing last scan state for the site from the db (before first fetch) can be done either using a separate GetScanStateUseCase or from inside the FetchScanStateUseCase.

Merge Instructions

  1. Review FluxC PR ScanStore: Add method to store scan state model into DB from the client WordPress-FluxC-Android#1833, merge and create a tag in FluxC develop
  2. Replace FluxC hash with tag from develop
  3. Remove the "Not Ready for Merge label"
  4. Merge this PR

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 self-assigned this Jan 11, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 11, 2021 •
edited
Loading

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 11, 2021 •
edited
Loading

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

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 11, 2021 •
edited
Loading

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

@ashiagr ashiagr mentioned this pull request Jan 11, 2021
Merged
3 tasks
@malinajirka malinajirka assigned ashiagr and malinajirka and unassigned ashiagr Jan 11, 2021
@malinajirka malinajirka self-requested a review January 11, 2021 11:52
@ashiagr ashiagr added this to the 16.6 milestone Jan 11, 2021
ashiagr
ashiagr commented Jan 11, 2021
View reviewed changes
WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/usecases/FetchScanStateUseCase.kt
Comment on lines +41 to +44
fetchScanState(site = site, polling = true).collect {
emit(it)
delay(delayInMs)
}
Copy link
Copy Markdown
Contributor Author

@ashiagr ashiagr Jan 11, 2021

Choose a reason for hiding this comment

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

This is a tricky part with a recursive call to the fetchScanState function during polling. We might want to replace it with a while loop.

@ashiagr ashiagr mentioned this pull request Jan 11, 2021
Closed
67 tasks
malinajirka
malinajirka reviewed Jan 11, 2021
View reviewed changes
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 reviewed the changes and it looks great to me. However, I didn't time to finish testing the behavior. I'll wrap it up and merge the PR first thing in the morning.

WordPress/src/main/java/org/wordpress/android/ui/jetpack/scan/ScanViewModel.kt Outdated
private fun fetchScanState() {
launch {
fetchScanStateUseCase.fetchScanState(site = site)
.flowOn(bgDispatcher)
Copy link
Copy Markdown
Contributor

@malinajirka malinajirka Jan 11, 2021

Choose a reason for hiding this comment

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

🔍 Nitpick: We might want to consider setting the dispatcher in the usecase so no-one can invoke the usecase on the ui thread. Wdyt?

Copy link
Copy Markdown
Contributor Author

@ashiagr ashiagr Jan 12, 2021

Choose a reason for hiding this comment

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

Nice suggestion 👍.
In 79b6480

malinajirka
malinajirka approved these changes Jan 12, 2021
View reviewed changes
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.

I've tested the app and it all works as described, good job! ⭐

@ashiagr ashiagr mentioned this pull request Jan 12, 2021
Merged
@ashiagr ashiagr requested a review from malinajirka January 12, 2021 11:42
malinajirka
malinajirka approved these changes Jan 12, 2021
View reviewed changes
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! LGTM - feel free to merge it when you replace the fluxc hash.

@ashiagr ashiagr merged commit 97eef58 into develop Jan 12, 2021
@ashiagr ashiagr deleted the issue/13326-fetch-scan-state-with-polling branch January 12, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@malinajirka malinajirka malinajirka approved these changes

Assignees

@ashiagr ashiagr

@malinajirka malinajirka

Labels

[Type] Task

Projects

None yet

Milestone

16.6

Development

Successfully merging this pull request may close these issues.

2 participants

@ashiagr @malinajirka