fix: self clients list is not updated when a new client event is received (WPB-5791) #2290

borichellow · 2023-12-06T12:05:50Z

What's new in this PR?

Issues

When I'm on a call in Proteus-verified conversation and I add a new client (login on new device). Conversation should degrade the verification status and end a call.

Causes (Optional)

a NewClientEvent handler doesn't save a new Client into the Client DB table, so the trigger for re-checking the conversation verification is not called -> conversation doesn't degrade -> call is not ended.

Solutions

Save new client into Client DB table on every NewClientEvent.

mchenani

Lgtm, and one question!

mchenani · 2023-12-06T12:09:56Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/client/ClientRepository.kt

@@ -253,6 +253,7 @@ class ClientDataSource(
        newClientEvent: Event.User.NewClient
    ): Either<CoreFailure, Unit> = wrapStorageRequest {
        newClientDAO.insertNewClient(clientMapper.toInsertClientParam(selfUserID, newClientEvent))
+        clientDAO.insertClient(clientMapper.toInsertClientParam(selfUserID, newClientEvent))


What's the difference between these two lines?

different DB-tables where we save clients into.
NewClient table containse only self-user clients that was added and user wasn't informed about it yet. So it's used for informing user about new devices by the dialog (kinda "there is a new device, was it you?").

And the Client table - is a table for all the clients

logic/src/commonTest/kotlin/com/wire/kalium/logic/data/client/ClientRepositoryTest.kt

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/client/ClientRepository.kt

github-actions · 2023-12-06T12:19:43Z

Test Results

2 310 tests - 375 2 244 ✔️ - 337 24s ⏱️ - 2m 30s
  395 suites -   85     66 💤 -   38
  395 files -   85       0 ❌ ±    0

Results for commit 5354485. ± Comparison against base commit 4e75095.

This pull request removes 2685 and adds 2310 tests. Note that renamed tests count towards both.

PocIntegrationTest ‑ givenApiWhenGettingACMEDirectoriesThenReturnAsExpectedBasedOnNetworkState
PocIntegrationTest ‑ givenEmailAndPasswordWhenLoggingInThenRegisterClientAndLogout
com.wire.kalium.HttpClientConnectionSpecsTest ‑ givenTheHttpClientIsCreated_ThenEnsureOnlySupportedSpecsArePresent[jvm]
com.wire.kalium.api.base.authenticated.notification.AccessUpdateTest ‑ givenPayloadWithAccessRoleAndDeprecatedAccessRoleField_whenDecoding_thenDeprecatedFieldIsPreferred[jvm]
com.wire.kalium.api.base.authenticated.notification.AccessUpdateTest ‑ givenPayloadWithDeprecatedAccessRoleField_whenDecoding_thenSuccess[jvm]
com.wire.kalium.api.base.authenticated.notification.AccessUpdateTest ‑ givenPayload_whenDecoding_thenSuccess[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ givenNoLocationInHeader_whenCallingSendAcmeRequestApi_theResponseShouldBeConfigureCorrectly[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ givenNoNonce_whenCallingSendAcmeRequestApi_theResponseShouldBeMissingNonce[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ whenCallingGetACMEDirectoriesApi_theResponseShouldBeConfigureCorrectly[jvm]
com.wire.kalium.api.common.ACMEApiTest ‑ whenCallingGetACMENonceApi_theResponseShouldBeConfigureCorrectly[jvm]
…

com.wire.kalium.logic.cache.SelfConversationIdProviderTest ‑ givenFailure_thenErrorIsPropagated[iosX64]
com.wire.kalium.logic.cache.SelfConversationIdProviderTest ‑ givenMLSClientHasBeenRegistered_thenMLSAndProteusSelfConversationAreReturned[iosX64]
com.wire.kalium.logic.cache.SelfConversationIdProviderTest ‑ givenMLSClientHasNotBeenRegistered_thenProteusSelfConversationIsReturned[iosX64]
com.wire.kalium.logic.client.E2EIClientProviderTest ‑ givenMLSClientWithE2EI_whenGettingE2EIClient_callsNewActivationEnrollment[iosX64]
com.wire.kalium.logic.client.E2EIClientProviderTest ‑ givenMLSClientWithoutE2EI_whenGettingE2EIClient_callsNewRotateEnrollment[iosX64]
com.wire.kalium.logic.client.E2EIClientProviderTest ‑ givenSelfUserNotFound_whenGettingE2EIClient_ReturnsError[iosX64]
com.wire.kalium.logic.configuration.ServerConfigMapperTest ‑ givenACommonApiVersion_whenMapping_thenValuesAreMappedCorrectly[iosX64]
com.wire.kalium.logic.configuration.ServerConfigMapperTest ‑ givenANetworkConfigEntity_whenMappingFromNetworkConfig_thenValuesAreMappedCorrectly[iosX64]
com.wire.kalium.logic.configuration.ServerConfigMapperTest ‑ givenAServerConfig_whenMappingToBackendConfig_thenValuesAreMappedCorrectly[iosX64]
com.wire.kalium.logic.configuration.ServerConfigMapperTest ‑ givenAServerConfig_whenMappingToNetworkConfig_thenValuesAreMappedCorrectly[iosX64]
…

This pull request removes 104 skipped tests and adds 66 skipped tests. Note that renamed tests count towards both.

PocIntegrationTest ‑ givenApiWhenGettingACMEDirectoriesThenReturnAsExpectedBasedOnNetworkState
PocIntegrationTest ‑ givenEmailAndPasswordWhenLoggingInThenRegisterClientAndLogout
com.wire.kalium.api.common.ACMEApiTest ‑ whenCallingSendChallengeRequestApi_theResponseShouldBeConfigureCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenActivationEmailWIthCode_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenRegisteringAccountWithEMail_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenAValidEmail_whenSendingActivationEmail_theRequestShouldBeConfiguredCorrectly[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenActivationCodeFail_thenErrorIsPropagated[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenRegistrationFail_whenRegisteringAccountWithEMMail_thenErrorIsPropagated[jvm]
com.wire.kalium.api.v0.user.register.RegisterApiV0Test ‑ givenSendActivationCodeFail_thenErrorIsPropagated[jvm]
com.wire.kalium.cryptography.CryptoUtilsTest ‑ givenDummyText_whenEncryptedAndDecryptedWithAES256_returnsOriginalText[js, browser]
…

com.wire.kalium.logic.data.reaction.ReactionRepositoryTest ‑ givenSelfUserReactionWasPersisted_whenObservingMessageReactions_thenShouldReturnReactionsPreviouslyStored[iosX64]
com.wire.kalium.logic.feature.backup.CreateBackupUseCaseTest ‑ givenSomeInvalidDBData_whenCreatingNonEncryptedBackup_thenTheRightErrorIsThrown[iosX64]
com.wire.kalium.logic.feature.backup.CreateBackupUseCaseTest ‑ givenSomeValidData_whenCreatingAnEncryptedBackup_thenTheFinalBackupFileIsCreatedCorrectly[iosX64]
com.wire.kalium.logic.feature.backup.CreateBackupUseCaseTest ‑ givenSomeValidData_whenCreatingNonEncryptedBackup_thenTheFinalBackupFileIsCreatedCorrectly[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectNonEncryptedBackupFileWithWrongAuthor_whenRestoring_thenTheCorrectErrorIsThrown[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectNonEncryptedBackupFileWithWrongMetadataFileName_whenRestoring_thenTheCorrectErrorIsThrown[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectNonEncryptedBackupFile_whenRestoring_thenTheBackupIsRestoredSuccessfully[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectlyEncryptedBackup_whenRestoringWithADBImportError_thenTheRightErrorIsThrown[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenACorrectlyEncryptedBackup_whenRestoringWithWrongPassword_thenTheRightErrorIsThrown[iosX64]
com.wire.kalium.logic.feature.backup.RestoreBackupUseCaseTest ‑ givenAValidEncryptedBackupFile_whenRestoring_thenTheBackupIsRestoredCorrectly[iosX64]
…

♻️ This comment has been updated with latest results.

datadog-wireapp · 2023-12-06T12:29:54Z

Datadog Report

All test runs 5199bfa 🔗

❌ 2 Total Test Services: 2 Failed, 0 with New Flaky, 0 Passed

Test Services

Service Name	Failed	Known Flaky	New Flaky	Passed	Skipped	Wall Time	Branch View
`kalium-ios`	2	0	0	373	36	12.05s	Link
`kalium-jvm`	2	0	0	635	41	5m 47.03s	Link

❌ Failed Tests (4)

givenNewClients_whenClearNewClients_thenNewClientEmptyListIsEmitted[iosX64] - com.wire.kalium.persistence.dao.newclient.NewClientDAOTest - Details

Expand for error

 co.touchlab.sqliter.interop.SQLiteExceptionErrorCode: Sqlite operation failure FOREIGN KEY constraint failed
 
 co.touchlab.sqliter.interop.SQLiteExceptionErrorCode: Sqlite operation failure FOREIGN KEY constraint failed
 	at kotlin.Throwable#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Throwable.kt:28)
 	at kotlin.Exception#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Exceptions.kt:23)
 	at co.touchlab.sqliter.interop.SQLiteException#<init>(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/SQLiteException.kt:3)
 	at co.touchlab.sqliter.interop.SQLiteExceptionErrorCode#<init>(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/SQLiteException.kt:5)
 	at co.touchlab.sqliter.interop.ActualSqliteStatement#resetStatement(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/ActualSqliteStatement.kt:132)
 	at co.touchlab.sqliter.native.NativeStatement#resetStatement(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/native/NativeStatement.kt:68)
 	at co.touchlab.sqliter.native.NativeStatement#executeUpdateDelete(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/native/NativeStatement.kt:52)
 ...

whenANewClientsIsAdded_thenNewClientListIsEmitted[iosX64] - com.wire.kalium.persistence.dao.newclient.NewClientDAOTest - Details

Expand for error

 co.touchlab.sqliter.interop.SQLiteExceptionErrorCode: Sqlite operation failure FOREIGN KEY constraint failed
 
 co.touchlab.sqliter.interop.SQLiteExceptionErrorCode: Sqlite operation failure FOREIGN KEY constraint failed
 	at kotlin.Throwable#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Throwable.kt:28)
 	at kotlin.Exception#<init>(/opt/buildAgent/work/f43969c6214a19e7/kotlin/kotlin-native/runtime/src/main/kotlin/kotlin/Exceptions.kt:23)
 	at co.touchlab.sqliter.interop.SQLiteException#<init>(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/SQLiteException.kt:3)
 	at co.touchlab.sqliter.interop.SQLiteExceptionErrorCode#<init>(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/SQLiteException.kt:5)
 	at co.touchlab.sqliter.interop.ActualSqliteStatement#resetStatement(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/interop/ActualSqliteStatement.kt:132)
 	at co.touchlab.sqliter.native.NativeStatement#resetStatement(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/native/NativeStatement.kt:68)
 	at co.touchlab.sqliter.native.NativeStatement#executeUpdateDelete(/Users/runner/work/SQLiter/SQLiter/sqliter-driver/src/nativeCommonMain/kotlin/co/touchlab/sqliter/native/NativeStatement.kt:52)
 ...

givenNewClients_whenClearNewClients_thenNewClientEmptyListIsEmitted[jvm] - com.wire.kalium.persistence.dao.newclient.NewClientDAOTest - Details

Expand for error

 org.sqlite.SQLiteException: [SQLITE_CONSTRAINT_FOREIGNKEY] A foreign key constraint failed (FOREIGN KEY constraint failed)
 
 org.sqlite.SQLiteException: [SQLITE_CONSTRAINT_FOREIGNKEY] A foreign key constraint failed (FOREIGN KEY constraint failed)
 	at app//org.sqlite.core.DB.newSQLException(DB.java:1179)
 	at app//org.sqlite.core.DB.newSQLException(DB.java:1190)
 	at app//org.sqlite.core.DB.execute(DB.java:985)
 	at app//org.sqlite.jdbc3.JDBC3PreparedStatement.lambda$execute$0(JDBC3PreparedStatement.java:57)
 	at app//org.sqlite.jdbc3.JDBC3Statement.withConnectionTimeout(JDBC3Statement.java:454)
 	at app//org.sqlite.jdbc3.JDBC3PreparedStatement.execute(JDBC3PreparedStatement.java:52)
 	at app//app.cash.sqldelight.driver.jdbc.JdbcPreparedStatement.execute(JdbcDriver.kt:279)
 ...

whenANewClientsIsAdded_thenNewClientListIsEmitted[jvm] - com.wire.kalium.persistence.dao.newclient.NewClientDAOTest - Details

Expand for error

 org.sqlite.SQLiteException: [SQLITE_CONSTRAINT_FOREIGNKEY] A foreign key constraint failed (FOREIGN KEY constraint failed)
 
 org.sqlite.SQLiteException: [SQLITE_CONSTRAINT_FOREIGNKEY] A foreign key constraint failed (FOREIGN KEY constraint failed)
 	at app//org.sqlite.core.DB.newSQLException(DB.java:1179)
 	at app//org.sqlite.core.DB.newSQLException(DB.java:1190)
 	at app//org.sqlite.core.DB.execute(DB.java:985)
 	at app//org.sqlite.jdbc3.JDBC3PreparedStatement.lambda$execute$0(JDBC3PreparedStatement.java:57)
 	at app//org.sqlite.jdbc3.JDBC3Statement.withConnectionTimeout(JDBC3Statement.java:454)
 	at app//org.sqlite.jdbc3.JDBC3PreparedStatement.execute(JDBC3PreparedStatement.java:52)
 	at app//app.cash.sqldelight.driver.jdbc.JdbcPreparedStatement.execute(JdbcDriver.kt:279)
 ...

codecov-commenter · 2023-12-06T12:30:36Z

Codecov Report

Merging #2290 (d376515) into develop (4e75095) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop    #2290      +/-   ##
=============================================
- Coverage      57.93%   57.93%   -0.01%     
  Complexity        21       21              
=============================================
  Files           1093     1093              
  Lines          41440    41446       +6     
  Branches        3826     3827       +1     
=============================================
+ Hits           24007    24010       +3     
- Misses         15820    15823       +3     
  Partials        1613     1613

Files	Coverage Δ
.../wire/kalium/logic/data/client/ClientRepository.kt	`48.51% <100.00%> (ø)`

... and 2 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 4e75095...d376515. Read the comment docs.

…:wireapp/kalium into fix/end_calls_when_self_added_new_device

MohamadJaara

Great work as always, just need to cover with some sweet tests

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/client/ClientRepository.kt

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq

persistence/src/commonMain/db_user/migrations/68.sqm

MohamadJaara

can you please add one more test
given new client is added, then the same client is added to the clients table

fix: End Calls when Self added new device

8904aa6

borichellow requested review from MohamadJaara and mchenani December 6, 2023 12:05

borichellow self-assigned this Dec 6, 2023

github-actions bot added 🚨 Potential breaking changes 👕 size: S type: bug / fix 🐞 labels Dec 6, 2023

mchenani added 2 commits December 6, 2023 13:08

Merge branch 'develop' into fix/end_calls_when_self_added_new_device

0810fb3

Merge branch 'develop' into fix/end_calls_when_self_added_new_device

427c456

mchenani approved these changes Dec 6, 2023

View reviewed changes

MohamadJaara reviewed Dec 6, 2023

View reviewed changes

logic/src/commonTest/kotlin/com/wire/kalium/logic/data/client/ClientRepositoryTest.kt Outdated Show resolved Hide resolved

MohamadJaara reviewed Dec 6, 2023

View reviewed changes

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/client/ClientRepository.kt Outdated Show resolved Hide resolved

MohamadJaara changed the title ~~fix: End Calls when Self added new device (WPB-5791)~~ fix: self clients list is not updated when a new client event is received (WPB-5791) Dec 6, 2023

mchenani and others added 4 commits December 6, 2023 14:04

Merge branch 'develop' into fix/end_calls_when_self_added_new_device

f54075c

Moved the changes into DB layer

09b3454

Merge branch 'develop' into fix/end_calls_when_self_added_new_device

14ca550

Merge branch 'fix/end_calls_when_self_added_new_device' of github.com…

d376515

…:wireapp/kalium into fix/end_calls_when_self_added_new_device

MohamadJaara requested changes Dec 6, 2023

View reviewed changes

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/client/ClientRepository.kt Outdated Show resolved Hide resolved

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq Outdated Show resolved Hide resolved

MohamadJaara requested changes Dec 6, 2023

View reviewed changes

persistence/src/commonMain/db_user/com/wire/kalium/persistence/Clients.sq Outdated Show resolved Hide resolved

MohamadJaara requested changes Dec 6, 2023

View reviewed changes

persistence/src/commonMain/db_user/migrations/68.sqm Outdated Show resolved Hide resolved

Moved transaction into DB layer

3507b48

github-actions bot removed the 🚨 Potential breaking changes label Dec 7, 2023

borichellow requested a review from MohamadJaara December 7, 2023 09:24

Fixed tests

b05a0d3

MohamadJaara requested changes Dec 7, 2023

View reviewed changes

Added test

5354485

MohamadJaara approved these changes Dec 7, 2023

View reviewed changes

borichellow added this pull request to the merge queue Dec 7, 2023

borichellow mentioned this pull request Dec 7, 2023

fix(RC): self clients list is not updated when a new client event is received (WPB-5791) #2290 #2293

Merged

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 7, 2023

MohamadJaara added this pull request to the merge queue Dec 7, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 7, 2023

borichellow added this pull request to the merge queue Dec 7, 2023

github-actions bot mentioned this pull request Dec 7, 2023

fix(RC): self clients list is not updated when a new client event is received (WPB-5791) #2290 #2294

Closed

Merged via the queue into develop with commit c13a83e Dec 7, 2023
17 checks passed

borichellow deleted the fix/end_calls_when_self_added_new_device branch December 7, 2023 12:53

echoes-hq bot added the echoes: features End-user visible changes intended to create customer value label Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: self clients list is not updated when a new client event is received (WPB-5791) #2290

fix: self clients list is not updated when a new client event is received (WPB-5791) #2290

borichellow commented Dec 6, 2023

mchenani left a comment

mchenani Dec 6, 2023 •

edited

Loading

borichellow Dec 6, 2023

github-actions bot commented Dec 6, 2023 •

edited

Loading

datadog-wireapp bot commented Dec 6, 2023 •

edited

Loading

codecov-commenter commented Dec 6, 2023 •

edited

Loading

MohamadJaara left a comment

MohamadJaara left a comment

fix: self clients list is not updated when a new client event is received (WPB-5791) #2290

fix: self clients list is not updated when a new client event is received (WPB-5791) #2290

Conversation

borichellow commented Dec 6, 2023

What's new in this PR?

Issues

Causes (Optional)

Solutions

mchenani left a comment

Choose a reason for hiding this comment

mchenani Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

borichellow Dec 6, 2023

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2023 • edited Loading

Test Results

datadog-wireapp bot commented Dec 6, 2023 • edited Loading

Datadog Report

❌ Failed Tests (4)

codecov-commenter commented Dec 6, 2023 • edited Loading

Codecov Report

MohamadJaara left a comment

Choose a reason for hiding this comment

MohamadJaara left a comment

Choose a reason for hiding this comment

mchenani Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited

Loading

datadog-wireapp bot commented Dec 6, 2023 •

edited

Loading

codecov-commenter commented Dec 6, 2023 •

edited

Loading