Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Ask User for call quality feedback [WPB-9452] #2966

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

borichellow
Copy link
Contributor

@borichellow borichellow commented Aug 22, 2024

TaskWPB-9452 [Android] : Call Quality Feedback - First Pass

What's new in this PR?

Issues

Every at least 3 days, when user ended a call we want to ask Call Quality feedback.

Causes (Optional)

There is a task saying so.

Solutions

Implement it, by adding ShouldAskCallFeedbackUseCase which will be called on every call ending and if user should asked for feedback kalium will call updated EndCallResultListener
And new ObserveAskCallFeedbackUseCase which observes flow EndCallResultListener for corresponding result when user should or should not be asked for feedback.

Not related changes:

while updating tests I noticed that we have 2 CallRepositoryArrangement, one in arrangement.repository one in arrangement package. So i merge them into one.

Copy link
Contributor

github-actions bot commented Aug 22, 2024

Test Results

3 172 tests  +6   3 067 ✔️ +6   3m 14s ⏱️ -9s
   545 suites +2      105 💤 ±0 
   545 files   +2          0 ±0 

Results for commit d4c9b50. ± Comparison against base commit dccef05.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Aug 22, 2024

🐰Bencher

ReportMon, August 26, 2024 at 10:20:49 UTC
Projectkalium
Branchfeat/ask_user_for_call_quality_feedback
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

  • Latency (latency)

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-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencyLatency Results
nanoseconds (ns)
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles➖ (view plot)688,508.08
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory➖ (view plot)340,722,453.56
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark➖ (view plot)923,739,351.26
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark➖ (view plot)21,428,007.07

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 34.04255% with 31 lines in your changes missing coverage. Please review.

Project coverage is 52.54%. Comparing base (dccef05) to head (d4c9b50).

Files Patch % Lines
...n/com/wire/kalium/logic/feature/call/CallsScope.kt 0.00% 11 Missing ⚠️
...ogic/feature/call/usecase/EndCallResultListener.kt 0.00% 7 Missing ⚠️
...ture/call/usecase/ObserveAskCallFeedbackUseCase.kt 0.00% 5 Missing ⚠️
...kalium/logic/configuration/UserConfigRepository.kt 0.00% 3 Missing ⚠️
...serveEndCallDueToConversationDegradationUseCase.kt 0.00% 3 Missing ⚠️
...ire/kalium/persistence/dao/unread/UserConfigDAO.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2966      +/-   ##
===========================================
- Coverage    52.56%   52.54%   -0.02%     
===========================================
  Files         1291     1294       +3     
  Lines        49579    49620      +41     
  Branches      4606     4609       +3     
===========================================
+ Hits         26059    26075      +16     
- Misses       21651    21676      +25     
  Partials      1869     1869              
Files Coverage Δ
...call/usecase/EndCallOnConversationChangeUseCase.kt 97.22% <ø> (ø)
...alium/logic/feature/call/usecase/EndCallUseCase.kt 92.00% <100.00%> (+1.09%) ⬆️
...logic/feature/user/ShouldAskCallFeedbackUseCase.kt 100.00% <100.00%> (ø)
.../feature/user/UpdateNextTimeCallFeedbackUseCase.kt 100.00% <100.00%> (ø)
...ire/kalium/persistence/dao/unread/UserConfigDAO.kt 17.44% <0.00%> (-0.42%) ⬇️
...kalium/logic/configuration/UserConfigRepository.kt 36.76% <0.00%> (-0.55%) ⬇️
...serveEndCallDueToConversationDegradationUseCase.kt 0.00% <0.00%> (ø)
...ture/call/usecase/ObserveAskCallFeedbackUseCase.kt 0.00% <0.00%> (ø)
...ogic/feature/call/usecase/EndCallResultListener.kt 0.00% <0.00%> (ø)
...n/com/wire/kalium/logic/feature/call/CallsScope.kt 0.00% <0.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dccef05...d4c9b50. Read the comment docs.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Aug 22, 2024

Datadog Report

Branch report: feat/ask_user_for_call_quality_feedback
Commit report: 0bdd894
Test service: kalium-jvm

✅ 0 Failed, 3067 Passed, 105 Skipped, 10.36s Total Time

Copy link
Contributor

@alexandreferris alexandreferris left a comment

Choose a reason for hiding this comment

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

Great work!

Just have a tiny question just to make sure I understood whats expected, but approved nonetheless 👌🏻

override suspend fun invoke(): Boolean =
userConfigRepository.getNextTimeForCallFeedback().map {
it > 0L && DateTimeUtil.currentInstant() > Instant.fromEpochMilliseconds(it)
}.getOrElse(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

the .getOrElse(true) will mean that the first call of a fresh logged in user will display the feedback right?

Comment on lines +47 to +49
sealed class EndCallResult {
data object VerificationDegraded : EndCallResult()
data class AskForFeedback(val shouldAsk: Boolean) : EndCallResult()
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Just a small naming convention nitpick. great work 🚀

@@ -46,6 +47,8 @@ interface EndCallUseCase {
internal class EndCallUseCaseImpl(
private val callManager: Lazy<CallManager>,
private val callRepository: CallRepository,
private val endCallListener: EndCallResultListener,
private val shouldAskCallFeedbackUseCase: ShouldAskCallFeedbackUseCase,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val shouldAskCallFeedbackUseCase: ShouldAskCallFeedbackUseCase,
private val shouldAskCallFeedback: ShouldAskCallFeedbackUseCase,

Copy link

sonarcloud bot commented Aug 26, 2024

@borichellow borichellow added this pull request to the merge queue Aug 26, 2024
Merged via the queue into develop with commit 770f49f Aug 26, 2024
22 checks passed
@borichellow borichellow deleted the feat/ask_user_for_call_quality_feedback branch August 26, 2024 10:54
MohamadJaara pushed a commit that referenced this pull request Oct 18, 2024
* feat: Ask User for Call quality feedback

* Code-style fix

* Review fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants