feat: do not display call feed back for short calls (WPB-15086) #3277

ohassine · 2025-02-04T13:07:41Z

WPB-15086 Fixing the cross-platform inconsistencies of the calling.call_quality_review event

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Description

Call feed back survey should not displayed if the call is too short, less than 1 minute.

Needs releases with:

GitHub link to other pull request

Testing

Test Coverage (Optional)

I have added automated test to this contribution

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)

Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References

https://sparkbox.com/foundry/semantic_commit_messages
https://github.com/wireapp/.github#usage
E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

…l-feed-back-for-short-calls

github-actions · 2025-02-04T13:38:47Z

Test Results

3 520 tests +1 3 412 ✅ +1 6m 11s ⏱️ -2s
603 suites ±0 108 💤 ±0
603 files ±0 0 ❌ ±0

Results for commit f415ef5. ± Comparison against base commit 7f5111d.

This pull request removes 4 and adds 5 tests. Note that renamed tests count towards both.

com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackInFuture_whenInvoked_thenFalseIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackInPast_whenInvoked_thenTrueIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackIsNegative_whenInvoked_thenFalseIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNoNextTimeForCallFeedbackSaved_whenInvoked_thenTrueIsReturned[jvm]

com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenCallDurationLessThanOneMinute_whenInvoked_thenCallDurationIsLessThanOneMinuteIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackInFuture_whenInvoked_thenNextTimeForCallFeedbackIsNotReachedIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackInPast_whenInvoked_thenShouldAskCallFeedbackIsReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNextTimeForCallFeedbackIsNegative_whenInvoked_thenNextTimeForCallFeedbackIsNotReachedReturned[jvm]
com.wire.kalium.logic.feature.user.ShouldAskCallFeedbackUseCaseTest ‑ givenNoNextTimeForCallFeedbackSaved_whenInvoked_thenShouldAskCallFeedbackIsReturned[jvm]

♻️ This comment has been updated with latest results.

github-actions · 2025-02-04T13:44:44Z

Bencher Report

Branch	do-not-display-call-feed-back-for-short-calls
Testbed	ubuntu-latest

⚠️ WARNING: No Threshold found!
Without a Threshold, no Alerts will ever be generated.
Latency (nanoseconds (ns))
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 flag.

Click to view all benchmark results

Benchmark	Latency	microseconds (µs)
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInFiles	📈 view plot ⚠️ NO THRESHOLD	681.93
com.wire.kalium.benchmarks.logic.CoreLogicBenchmark.createObjectInMemory	📈 view plot ⚠️ NO THRESHOLD	516,666.65
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.messageInsertionBenchmark	📈 view plot ⚠️ NO THRESHOLD	1,345,962.23
com.wire.kalium.benchmarks.persistence.MessagesNoPragmaTuneBenchmark.queryMessagesBenchmark	📈 view plot ⚠️ NO THRESHOLD	26,756.40

🐰 View full continuous benchmarking report in Bencher

datadog-wireapp · 2025-02-04T13:59:49Z

Datadog Report

Branch report: do-not-display-call-feed-back-for-short-calls
Commit report: 644ac93
Test service: kalium-jvm

✅ 0 Failed, 3412 Passed, 108 Skipped, 1m 1.46s Total Time

codecov-commenter · 2025-02-04T15:38:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.70%. Comparing base (7f5111d) to head (f415ef5).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3277   +/-   ##
========================================
  Coverage    50.69%   50.70%           
========================================
  Files         1611     1611           
  Lines        58102    58119   +17     
  Branches      5218     5222    +4     
========================================
+ Hits         29456    29467   +11     
- Misses       26639    26642    +3     
- Partials      2007     2010    +3

see 8 files with indirect coverage changes

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 7f5111d...f415ef5. Read the comment docs.

saleniuk · 2025-02-05T09:10:52Z

...ommonMain/kotlin/com/wire/kalium/logic/feature/call/usecase/ObserveAskCallFeedbackUseCase.kt

 }

-@Suppress("FunctionNaming")
-internal fun ObserveAskCallFeedbackUseCase(
+internal fun observeAskCallFeedbackUseCase(


question: does it need to be a function that creates anonymous object? Usually we just create

class ObserveAskCallFeedbackUseCaseImpl( endCallListener: EndCallResultListener ) : ObserveAskCallFeedbackUseCase {

To be fair, I'd even go the extra mile to make it look like a constructor when invoking. something like internal fun ObserveAskCallFeedbackUseCase. Just looks cleaner than having all Impl classes everywhere :D

saleniuk · 2025-02-05T09:11:28Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ShouldAskCallFeedbackUseCase.kt

 }

-@Suppress("FunctionNaming")
-internal fun ShouldAskCallFeedbackUseCase(
+internal fun shouldAskCallFeedbackUseCase(


same question here

I'd revert the upper to lowercase change.

Using "fake constructor functions" is a well established Kotlin pattern.

For example, when creating a CoroutineScope: https://github.com/Kotlin/kotlinx.coroutines/blob/fed40ad1f9942d1b16be872cc555e08f965cf881/kotlinx-coroutines-core/common/src/CoroutineScope.kt#L297

I guess I just missed the moment when we started doing it that way in use cases 😅
I agree, reverting it to uppercase would be better

vitorhugods · 2025-02-05T14:57:09Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/call/CallRepository.kt

@@ -475,7 +475,7 @@ internal class CallDataSource(
                val updatedCallMetadata = callMetadataProfile.data.toMutableMap().apply {
                    this[conversationId] = call.copy(
                        participants = participants,
-                        maxParticipants = max(call.maxParticipants, participants.size + 1),
+                        maxParticipants = max(call.maxParticipants, participants.size),


How is this related to call feedback? Could Could you please clarify?

so in the ticket call_participants event is "The maximum number of users in the call, not necessarily equal to the number of users in the conversation. Detected during the time the user is participating in the call."

I was mostly asking about the +1 removal. I'm not sure why it was there before, and why it is being removed now 😄

tbh I don't know why it was added, but certainly it's giving wrong value.

does call.maxParticipants return self ? maybe the +1 is for self ?

yes self is already included

vitorhugods · 2025-02-05T15:00:46Z

...ommonMain/kotlin/com/wire/kalium/logic/feature/call/usecase/ObserveAskCallFeedbackUseCase.kt

 }

-@Suppress("FunctionNaming")
-internal fun ObserveAskCallFeedbackUseCase(
+internal fun observeAskCallFeedbackUseCase(


To be fair, I'd even go the extra mile to make it look like a constructor when invoking. something like internal fun ObserveAskCallFeedbackUseCase. Just looks cleaner than having all Impl classes everywhere :D

vitorhugods · 2025-02-05T15:07:58Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ShouldAskCallFeedbackUseCase.kt

 }

-@Suppress("FunctionNaming")
-internal fun ShouldAskCallFeedbackUseCase(
+internal fun shouldAskCallFeedbackUseCase(


I'd revert the upper to lowercase change.

Using "fake constructor functions" is a well established Kotlin pattern.

For example, when creating a CoroutineScope: https://github.com/Kotlin/kotlinx.coroutines/blob/fed40ad1f9942d1b16be872cc555e08f965cf881/kotlinx-coroutines-core/common/src/CoroutineScope.kt#L297

logic/src/commonMain/kotlin/com/wire/kalium/logic/feature/user/ShouldAskCallFeedbackUseCase.kt

…alls

sonarqubecloud · 2025-02-14T16:24:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

feat: do not display call feed back for short calls

06b6018

echoes-hq bot added the echoes/initiative: product-metrics-in-countly Analytics and KPIs label Feb 4, 2025

ohassine requested review from a team, sbakhtiarov, alexandreferris, vitorhugods, saleniuk and emmaoke-w and removed request for a team February 4, 2025 13:07

github-actions bot assigned ohassine Feb 4, 2025

github-actions bot added 🚨 Potential breaking changes type: feature ✨ 👕 size: M labels Feb 4, 2025

ohassine added 2 commits February 4, 2025 14:26

feat: detekt

d3735cb

Merge remote-tracking branch 'origin/develop' into do-not-display-cal…

0e11108

…l-feed-back-for-short-calls

feat: unit test

2493acd

saleniuk reviewed Feb 5, 2025

View reviewed changes

vitorhugods reviewed Feb 5, 2025

View reviewed changes

chore: address comments

4572ce6

ohassine requested review from vitorhugods and saleniuk February 11, 2025 15:40

Merge branch 'develop' into do-not-display-call-feed-back-for-short-c…

cc2375c

…alls

ohassine requested a review from MohamadJaara February 12, 2025 12:44

yamilmedina approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'develop' into do-not-display-call-feed-back-for-short-c…

f415ef5

…alls

MohamadJaara approved these changes Feb 18, 2025

View reviewed changes

MohamadJaara added this pull request to the merge queue Feb 18, 2025

Merged via the queue into develop with commit 4abd575 Feb 18, 2025
22 checks passed

MohamadJaara deleted the do-not-display-call-feed-back-for-short-calls branch February 18, 2025 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: do not display call feed back for short calls (WPB-15086) #3277

feat: do not display call feed back for short calls (WPB-15086) #3277

ohassine commented Feb 4, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

⚠️ WARNING: No Threshold found!

datadog-wireapp bot commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 4, 2025 •

edited

Loading

saleniuk Feb 5, 2025

vitorhugods Feb 5, 2025

saleniuk Feb 5, 2025

vitorhugods Feb 5, 2025

saleniuk Feb 7, 2025

vitorhugods Feb 5, 2025

ohassine Feb 10, 2025

vitorhugods Feb 11, 2025

ohassine Feb 11, 2025

MohamadJaara Feb 11, 2025

ohassine Feb 12, 2025

vitorhugods Feb 5, 2025

vitorhugods Feb 5, 2025

sonarqubecloud bot commented Feb 14, 2025

feat: do not display call feed back for short calls (WPB-15086) #3277

feat: do not display call feed back for short calls (WPB-15086) #3277

Conversation

ohassine commented Feb 4, 2025 • edited by github-actions bot Loading

PR Submission Checklist for internal contributors

What's new in this PR?

Description

Testing

Test Coverage (Optional)

How to Test

Notes (Optional)

Attachments (Optional)

PR Post Submission Checklist for internal contributors (Optional)

PR Post Merge Checklist for internal contributors

References

github-actions bot commented Feb 4, 2025 • edited Loading

Test Results

github-actions bot commented Feb 4, 2025 • edited Loading

⚠️ WARNING: No Threshold found!

datadog-wireapp bot commented Feb 4, 2025 • edited Loading

Datadog Report

codecov-commenter commented Feb 4, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 14, 2025

Quality Gate passed

ohassine commented Feb 4, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

datadog-wireapp bot commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 4, 2025 •

edited

Loading