refactor: remove not needed usage of observe self user [WPB-15414] #3283
Conversation
echoes-hq
bot
added
the
echoes: technical-roadmap/technical-debt
github-actions
bot
added
🚨 Potential breaking changes
type: refactoring 🛠
👕 size: M
labels
|
|
Bencher Report| Branch | refactor/remove-not-needed-usage-of-observe-self-user |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | microseconds (µs) |
|---|---|---|
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles | 📈 view plot | 682.75 |
| com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory | 📈 view plot | 325,214.30 |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark | 📈 view plot | 1,335,762.37 |
| com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark | 📈 view plot | 22,089.93 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3283 +/- ##
===========================================
- Coverage 50.87% 50.85% -0.03%
===========================================
Files 1606 1607 +1
Lines 57189 58051 +862
Branches 5105 5199 +94
===========================================
+ Hits 29097 29519 +422
- Misses 26078 26517 +439
- Partials 2014 2015 +1 see 35 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 3405 Passed, 108 Skipped, 1m 1.19s Total Time |
| override suspend operator fun invoke(): SelfUser? = withContext(dispatcher.io) { | ||
| userRepository.getSelfUser() | ||
| } | ||
| override suspend operator fun invoke(): SelfUser? = userRepository.getSelfUser().getOrNull() |
There was a problem hiding this comment.
Where do we switch to IO dispatcher for an IO call? I do not see it in the repository.
There was a problem hiding this comment.
For network calls, Ktor will do it under the hood.
For DB, we should be doing it.
So, to keep it all tight, I guess we could do it on a repository or UseCase level. Nice catch :)
There was a problem hiding this comment.
for db we always have withContext on the DB dispatcher
|
|
||
| val avatarAsset = when (val publicAsset = session.users.getPublicAsset(selfUser.previewPicture!!)) { | ||
| val avatarAsset = when (val publicAsset = session.users.getPublicAsset(selfUser?.previewPicture!!)) { |
There was a problem hiding this comment.
Why do we not handle null value in MainActivity (previewPicture!! and selfUser!!) ?
There was a problem hiding this comment.
This android module within kalium is just a Sandbox for testing code.
It's an archeological relic from the times where we were bootstraping the new code in a sample app before actually porting everything to the wire/android project.
We could delete the whole module, from my personal point of view. But I guess it could be useful at some point to just test some changes in kalium without having to wait for a PR on Android ?
| override suspend operator fun invoke(): SelfUser? = withContext(dispatcher.io) { | ||
| userRepository.getSelfUser() | ||
| } | ||
| override suspend operator fun invoke(): SelfUser? = userRepository.getSelfUser().getOrNull() |
There was a problem hiding this comment.
For network calls, Ktor will do it under the hood.
For DB, we should be doing it.
So, to keep it all tight, I guess we could do it on a repository or UseCase level. Nice catch :)
MohamadJaara
deleted the
refactor/remove-not-needed-usage-of-observe-self-user
branch
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764The PR Description
What's new in this PR?
Issues
ObserveSelfUserUseCase is often used like this:
val userId = observeSelf().first()
This may cause two issues:
Solutions
Replace with GetSelfUserUseCase and handle possible null return value.
Dependencies (Optional)
If there are some other pull requests related to this one (e.g. new releases of frameworks), specify them here.
Needs releases with:
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.