Start coroutines migration of KSApolloClientV2.getStoredCards()
#2223
Conversation
- Update PaymentMethodsViewModel + Tests - Update LatePledgeCheckoutViewModel + Tests
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2223 +/- ##
============================================
+ Coverage 68.11% 68.14% +0.02%
+ Complexity 2191 2188 -3
============================================
Files 354 354
Lines 23881 23904 +23
Branches 3505 3510 +5
============================================
+ Hits 16267 16289 +22
- Misses 5771 5773 +2
+ Partials 1843 1842 -1 ☔ View full report in Codecov by Sentry. |
| @@ -232,6 +237,9 @@ class KSApolloClientV2(val service: ApolloClient, val gson: Gson) : ApolloClient | |||
| disposables.clear() | |||
| } | |||
|
|
|||
| private fun <T : Any> emitAsObservable(block: suspend () -> T): Observable<T> = | |||
| Pair(userPrivacy, cards) | ||
| } | ||
| @OptIn(FlowPreview::class) | ||
| val user = currentUser.timeout(1.seconds).runCatching { first() }.getOrNull() |
There was a problem hiding this comment.
When there is no logged in user, environment.currentUserV2()?.loggedInUser() does not emit, which leaves a Collector suspended forever.
In tests, this is not as clear when running on backgroundScope since the scope cancels at the end of the test. In production, if we want to try to get the user, and fail if there isn't one, we need the timeout. Otherwise, if we want to collect forever intentionally, we should launch a separate coroutine earlier in the business logic.
This currently appears to not be a bug in production because we check for a logged in user before we call providePledgeData().
There was a problem hiding this comment.
alternative path to the timeout, can change from environment.currentUserV2()?.loggedInUser() over toenvironment.currentUser.observable() it will operate with a KSOptional<User?>
There was a problem hiding this comment.
Yes, that's perfect 👍 I tried to stay close to the original in case I missed something. Thanks!
| .build() | ||
| } ?: listOf() | ||
| } catch (apolloException: ApolloException) { | ||
| // parse network errors into specific type |
There was a problem hiding this comment.
If we wanted to provide some feedback to the user. For example a pretty used pattern now within the app is displaying a red snackbar on the UI, usually with a generic message "something went wrong if no message provided on the API". It was achieved before with the doOnError block.
We could add some kind of callback the consumer of _getStoredCards() to still be able to provide feedback to the user when a error happens.
| [email protected](it) | ||
| } | ||
| .onFailure { | ||
| [email protected](false) |
| var ret = listOf<StoredCard>() | ||
| try { | ||
| // `single()` vs `singleOrNull()` | ||
| val response = this.service.query(query).toFlow().single() |
There was a problem hiding this comment.
could be changed over val response = this.service.query(query).execute() it internally uses toFlow().single()
| this.refreshCards | ||
| .switchMap { getListOfStoredCards() } | ||
| .subscribe { this.cards.onNext(it) } | ||
| this.refreshCards.subscribe { |
There was a problem hiding this comment.
question here, I assume this piece is for demonstration purposes, because this piece over here be just exchanging within the switch map getListOfStoredCards() over apolloClient.getStoredCards(), let me know if I'm missing something!
Do not merge ❌
📲 What
Start coroutines migration of
KSApolloClientV2.getStoredCards()🤔 Why
Some background context on why the change is needed.
🛠 How
More in-depth discussion of the change or implementation.
👀 See
Trello, screenshots, external resources?
📋 QA
Instructions for anyone to be able to QA this work.
Story 📖
[Name of Trello Story](Trello link)