Skip to content

Issue/13326 fetch fix threats status usecase#13797

Merged
malinajirka merged 5 commits intodevelopfrom
issue/13326-fetch-fix-threats-status-usecase
Jan 19, 2021
Merged

Issue/13326 fetch fix threats status usecase#13797
malinajirka merged 5 commits intodevelopfrom
issue/13326-fetch-fix-threats-status-usecase

Conversation

@ashiagr
Copy link
Copy Markdown
Contributor

@ashiagr ashiagr commented Jan 18, 2021

Parent #13798
This PR adds use case to fetch fix threats status.

To test:
That FetchFixThreatsStatusUseCaseTest tests run fine and cover action results.

Merge Instructions

  1. Make sure child PR Issue/13326 trigger fix threats #13795 is merged to develop
  2. Remove the "Not Ready for Merge label"
  3. 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 added this to the 16.6 milestone Jan 18, 2021
@ashiagr ashiagr self-assigned this Jan 18, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 18, 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 18, 2021

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

@ashiagr ashiagr requested a review from malinajirka January 18, 2021 12:59
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'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()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@ashiagr
Copy link
Copy Markdown
Contributor Author

ashiagr commented Jan 19, 2021

Thanks for the review @malinajirka! I've addressed your comments or added as action items.

Ready for another look 🙇‍♀️.

@ashiagr ashiagr requested a review from malinajirka January 19, 2021 07:46
Base automatically changed from issue/13326-handle-fix-threats-result to develop January 19, 2021 11:44
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.

LGTM, thanks for the changes!

@malinajirka malinajirka merged commit 3c05543 into develop Jan 19, 2021
@malinajirka malinajirka deleted the issue/13326-fetch-fix-threats-status-usecase branch January 19, 2021 11:51
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