feat: User verification status Proteus (WPB-1775) #2111

borichellow · 2023-10-04T11:35:06Z

What's new in this PR?

updated OtherUser and Client models (added isProteusVerified field)
added UserDetails viewTable in DB (it's the same as User + is_proteus_verified field which is calculated in DB level, by checking if user has any client and if all of them are verified)

CLAassistant · 2023-10-04T11:35:11Z

All committers have signed the CLA.

github-actions · 2023-10-04T11:59:17Z

Unit Test Results

  442 files ±0   442 suites ±0 3m 10s ⏱️ +48s
2 400 tests ±0 2 296 ✔️ ±0 104 💤 ±0 0 ❌ ±0

Results for commit 594be39. ± Comparison against base commit c923de5.

♻️ This comment has been updated with latest results.

datadog-wireapp · 2023-10-04T12:07:23Z

Datadog Report

Branch report: feat/user_verification_status_proteus
Commit report: d8c537f

✅ kalium-jvm: 0 Failed, 0 New Flaky, 1 Passed, 0 Skipped, 2.77s Wall Time

gongracr

LGTM 🍨 just a suggestion to add more logs 📝

gongracr · 2023-10-04T13:27:37Z

...monMain/kotlin/com/wire/kalium/logic/feature/client/UpdateClientVerificationStatusUseCase.kt

+            .fold({ error -> Result.Failure(error) },
+                { Result.Success }


Do we want to log this info to Datadog? I guess it might not harm as it might be useful identifying and tracking possible issues in the future?

mchenani

Having the isProteus verified in the client table is ok, but having the same value as aggregated one for the user in my opinion is hard to maintain. I suggest only having the value for the client and whenever we want the aggregated value for the user we need to calculate it on the fly.

mchenani · 2023-10-10T14:36:14Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/publicuser/PublicUserMapper.kt

+    fun fromDetailsEntityToUserSummary(userDetailsEntity: UserDetailsEntity): UserSummary
+    fun fromUserDetailsEntityToOtherUser(userDetailsEntity: UserDetailsEntity): OtherUser


suggestion: in the first function we have fromDetails, but in the second one is fromUserDetails, I suggest make them the same, either mention fromUserDetails... or fromDetails...

mchenani · 2023-10-10T14:41:01Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserMapper.kt

+        userTypeEntity: UserTypeEntity
+    ): UserDetailsEntity
+
+    fun fromFailedUserToDetailEntity(userId: NetworkQualifiedId): UserDetailsEntity


suggestion: since it's details not detail I suggest to keep the name as it is in the function naming.

mchenani · 2023-10-10T14:42:01Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserMapper.kt

@@ -209,6 +308,38 @@ internal class UserMapperImpl(
            defederated = false
        )

+    /**
+     * Null and default/hardcoded values will be replaced later when fetching known users.


suggestion: add todo to it would be better in my opinion.

removed this mapper, as it's not needed

mchenani · 2023-10-10T14:42:12Z

logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserMapper.kt

@@ -209,6 +308,38 @@ internal class UserMapperImpl(
            defederated = false
        )

+    /**
+     * Null and default/hardcoded values will be replaced later when fetching known users.


suggestion: add todo to it would be better in my opinion.

mchenani · 2023-10-10T14:45:12Z

...onTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt

@@ -18,6 +18,7 @@
 package com.wire.kalium.logic.util.arrangement.repository

 import com.wire.kalium.logic.CoreFailure
+import com.wire.kalium.logic.StorageFailure


question: do we need this import anymore?

nope, removed, strange that build script didn't find it

mchenani · 2023-10-10T14:45:13Z

...onTest/kotlin/com/wire/kalium/logic/util/arrangement/repository/UserRepositoryArrangement.kt

@@ -18,6 +18,7 @@
 package com.wire.kalium.logic.util.arrangement.repository

 import com.wire.kalium.logic.CoreFailure
+import com.wire.kalium.logic.StorageFailure


question: do we need this import anymore?

mchenani · 2023-10-10T14:57:47Z

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

+CREATE VIEW IF NOT EXISTS UserDetails AS
+SELECT
+User.qualified_id,
+User.name,
+User.handle,
+User.email,
+User.phone,
+User.accent_id,
+User.team,
+User.connection_status,
+User.preview_asset_id,
+User.complete_asset_id,
+User.user_availability_status,
+User.user_type,
+User.bot_service,
+User.deleted,
+User.incomplete_metadata,
+User.expires_at,
+User.defederated,
+CASE IFNULL ((SELECT COUNT (*) FROM Client WHERE Client.user_id = User.qualified_id), 0)
+    -- no clients means not verified
+    WHEN 0 THEN 0
+    ELSE (CASE IFNULL ((SELECT COUNT (*) FROM Client WHERE Client.user_id = User.qualified_id AND is_proteus_verified = 0), 0)
+            WHEN 0 THEN 1
+            -- at least one client is not verified -> user is not verified
+            ELSE 0
+        END)
+END AS is_proteus_verified
+FROM User;


suggestion: my suggestion here is to use group-by instead of nested queries.
See this for example:

SELECT User.qualified_id AS user_id, CASE WHEN SUM(CASE WHEN Client.is_proteus_verified = 1 THEN 1 ELSE 0 END) = COUNT(*) THEN 1 ELSE 0 END AS isProteusVerified FROM User LEFT JOIN Client ON User.qualified_id = Client.user_id GROUP BY User.qualified_id;

nice, i make it even better :)

datadog-wireapp · 2023-10-11T12:16:19Z

Datadog Report

All test runs 818a196 🔗

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

Test Services

Service Name	Failed	Known Flaky	New Flaky	Passed	Skipped	Wall Time	Branch View
`kalium-ios`	0	0	0	1982	68	8m 22.14s	Link
`kalium-jvm`	0	0	0	2296	104	11m 50.01s	Link

mchenani

👏🏄‍♂️ Well done buddy

feat: User verification status Proteus

8310797

borichellow requested review from gongracr, MohamadJaara, vitorhugods, mchenani and Garzas October 4, 2023 11:35

github-actions bot assigned borichellow Oct 4, 2023

Merge branch 'develop' into feat/user_verification_status_proteus

541ef67

github-actions bot added 🚨 Potential breaking changes type: feature ✨ 👕 size: L labels Oct 4, 2023

borichellow mentioned this pull request Oct 4, 2023

feat: User verification status Proteus (WPB-1775) wireapp/wire-android#2299

Merged

borichellow added 2 commits October 4, 2023 14:49

Code style fix

4c3b44f

Code style fix: again

ca85792

Fix tests

eb0cfe9

gongracr approved these changes Oct 4, 2023

View reviewed changes

borichellow added 2 commits October 5, 2023 13:38

feat: User verification status Proteus: review

8c40f34

Merge branch 'develop' into feat/user_verification_status_proteus

3ba7b9c

mchenani requested changes Oct 5, 2023

View reviewed changes

borichellow added 2 commits October 6, 2023 14:47

feat: User verification status Proteus: new DB view table

f87d782

Merge branch 'develop' into feat/user_verification_status_proteus

3988499

github-actions bot added 👕 size: XL and removed 👕 size: L labels Oct 6, 2023

borichellow added 4 commits October 6, 2023 16:11

Code style fix

6beed54

Code style fix

207c67b

Merge branch 'develop' into feat/user_verification_status_proteus

1c5edd3

fixed tests and mapping

79c5ffc

github-actions bot added 👕 size: XXL and removed 👕 size: XL labels Oct 10, 2023

borichellow added 2 commits October 10, 2023 17:05

Fix unot tests

5762a22

Merge branch 'develop' into feat/user_verification_status_proteus

b0896dd

borichellow requested a review from mchenani October 10, 2023 14:07

mchenani reviewed Oct 10, 2023

View reviewed changes

Removed duplicated mapper and fixed tests

a7da852

mchenani reviewed Oct 10, 2023

View reviewed changes

borichellow added 2 commits October 11, 2023 12:07

feat: User verification status Proteus: review fixes

1ed3fa8

fixed build

67fc035

borichellow requested a review from mchenani October 11, 2023 10:36

Merge branch 'develop' into feat/user_verification_status_proteus

ea0dfc3

Trying to fix build

a41a7f7

mchenani approved these changes Oct 11, 2023

View reviewed changes

borichellow added 2 commits October 12, 2023 14:39

Fixed DB migration

7e642d2

Fixed DB migration: again

594be39

borichellow added this pull request to the merge queue Oct 12, 2023

Merged via the queue into develop with commit 675404c Oct 12, 2023
16 of 17 checks passed

borichellow deleted the feat/user_verification_status_proteus branch October 12, 2023 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: User verification status Proteus (WPB-1775) #2111

feat: User verification status Proteus (WPB-1775) #2111

borichellow commented Oct 4, 2023 •

edited

CLAassistant commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

datadog-wireapp bot commented Oct 4, 2023 •

edited

gongracr left a comment

gongracr Oct 4, 2023

mchenani left a comment

mchenani Oct 10, 2023

mchenani Oct 10, 2023

mchenani Oct 10, 2023

borichellow Oct 11, 2023

mchenani Oct 10, 2023

mchenani Oct 10, 2023

borichellow Oct 11, 2023

mchenani Oct 10, 2023

mchenani Oct 10, 2023 •

edited

borichellow Oct 11, 2023

datadog-wireapp bot commented Oct 11, 2023 •

edited

mchenani left a comment

		fun fromDetailsEntityToUserSummary(userDetailsEntity: UserDetailsEntity): UserSummary
		fun fromUserDetailsEntityToOtherUser(userDetailsEntity: UserDetailsEntity): OtherUser

feat: User verification status Proteus (WPB-1775) #2111

feat: User verification status Proteus (WPB-1775) #2111

Conversation

borichellow commented Oct 4, 2023 • edited

What's new in this PR?

CLAassistant commented Oct 4, 2023 • edited

github-actions bot commented Oct 4, 2023 • edited

Unit Test Results

datadog-wireapp bot commented Oct 4, 2023 • edited

Datadog Report

gongracr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mchenani left a 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

mchenani Oct 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datadog-wireapp bot commented Oct 11, 2023 • edited

Datadog Report

mchenani left a comment

Choose a reason for hiding this comment

borichellow commented Oct 4, 2023 •

edited

CLAassistant commented Oct 4, 2023 •

edited

github-actions bot commented Oct 4, 2023 •

edited

datadog-wireapp bot commented Oct 4, 2023 •

edited

mchenani Oct 10, 2023 •

edited

datadog-wireapp bot commented Oct 11, 2023 •

edited