Issue/13326 fetch fix threats status usecase#13797
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
…-fetch-fix-threats-status-usecase
malinajirka
left a comment
There was a problem hiding this comment.
Thanks @ashiagr! I've left two comments, but it looks great overall.
| delayInMs: Long = FETCH_FIX_THREATS_STATUS_DELAY_MILLIS | ||
| ): Flow<FetchFixThreatsState> = flow { | ||
| while (true) { | ||
| if (!networkUtilsWrapper.isNetworkAvailable()) { |
There was a problem hiding this comment.
💡 We might want to consider moving the "internet connection" check above the "while" cycle or adding a retry behavior. I'm a bit worried the current implementation might show the error more often than necessary. Wdyt?
(even if you decide to make some changes, we can implement them later - up to you).
There was a problem hiding this comment.
I'm a bit worried the current implementation might show the error more often than necessary.
There's a return@flow after NetworkUnavailable is emitted. I think it will take care of it.
if (!networkUtilsWrapper.isNetworkAvailable()) {
emit(Failure.NetworkUnavailable)
return@flow
}
I can move it outside the while loop, if network is gone while fix status is in progress, RemoteRequestFailure will be returned instead of NetworkUnavailable. Is that fine?
adding a retry behavior
I'd really like to add a retry behavior, adding it to the backlog.
There was a problem hiding this comment.
I can move it outside the while loop, if network is gone while fix status is in progress, RemoteRequestFailure will be returned instead of NetworkUnavailable. Is that fine?
I think that's fine. However, if you plan to implement the retry behavior anyway, I guess we can just wait until it's implemented and do not make any changes now.
...test/java/org/wordpress/android/ui/jetpack/scan/usecases/FetchFixThreatsStatusUseCaseTest.kt
Outdated
Show resolved
Hide resolved
|
Thanks for the review @malinajirka! I've addressed your comments or added as action items. Ready for another look 🙇♀️. |
malinajirka
left a comment
There was a problem hiding this comment.
LGTM, thanks for the changes!
Parent #13798
This PR adds use case to fetch fix threats status.
To test:
That
FetchFixThreatsStatusUseCaseTesttests run fine and cover action results.Merge Instructions
PR submission checklist:
RELEASE-NOTES.txtif necessary.