refactor: user legalhold enable event - WPB-10195 #2019

jullianm · 2024-10-11T12:25:02Z

WPB-10195 [iOS] Process UserLegalholdEnableEvent

Key points

This PR is part of the quick sync refactoring plan and is related to processing the multiple events we receive from the backend or the push channel.

Specifically, this PR is about implementing UserLegalholdEnableEvent event.

This PR also introduces a new dedicated ClientAPI to fetch data related to clients from backend and a ClientRepository to perform client related storage logic.

⚠️ There was no legacy code for that event, in other words, this event was not processed before. Thus, the implementation is based on what's already been done on Android side with some adaptation to the iOS legacy codebase.

For reference, on Android side, this event is handled in LegalHoldHandlerImpl specifically in this method:
override suspend fun handleEnable(legalHoldEnabled: Event.User.LegalHoldEnabled): Either<CoreFailure, Unit>.

Testing

UTs cover the following use cases, ensuring that

self clients are properly fetched from API
clients for given users are properly fetched from API
a client is properly deleted from storage
legalhold enable event is properly processed

Checklist

Title contains a reference JIRA issue number like [WPB-XXX].
Description is filled and free of optional paragraphs.
Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

Make sure you use the API for UI elements that support large fonts.
All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
New UI elements have Accessibility strings for VoiceOver.

github-actions · 2024-10-11T12:32:47Z

Test Results

166 tests 166 ✅ 1s ⏱️
19 suites 0 💤
2 files 0 ❌

Results for commit 5d62cda.

♻️ This comment has been updated with latest results.

jullianm · 2024-10-11T12:36:04Z

Some necessary context here: based on Android implementation, when receiving the LegalholdEnable event, some logic is performed not only for self user but also for other users. Quite similar logic is performed for LegalholdDisable event, again for self user and other users.

However on iOS side, regarding the legacy LegalholdDisable event, we only perform logic for the self user.

While this PR is based on Android implementation and adapted to the existing iOS legacy code, I figured it'd make sense that, when receiving the LegalholdEnable event, we only perform logic for self user as well.

Feel free to share your thoughts on this

netbe · 2024-10-14T18:32:29Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIBuilder.swift

+    ///
+    /// - Parameter httpClient: A http client.
+
+    public init(httpClient: any HTTPClient) {


I thought as we go further we would use the APIService here, no?

netbe · 2024-10-14T18:37:46Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIV0.swift

+    func getClients(for userIDs: Set<UserID>) async throws -> [UserClients] {
+        let body = try JSONEncoder.defaultEncoder.encode(UserClientsRequestV0(qualifiedIDs: Array(userIDs)))
+        let request = HTTPRequest(
+            path: "/users/list-clients/v2", /// v2 suffix required for api version v0 and v1, suffix removed from next versions


Suggested change

path: "/users/list-clients/v2", /// v2 suffix required for api version v0 and v1, suffix removed from next versions

path: "/users/list-clients/v2", // v2 suffix required for api version v0 and v1, suffix removed from next versions

I may be wrong but I would keep /// for comments documenting functions or params and // for code comments

netbe · 2024-10-14T18:40:41Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIV0.swift

+    }
+
+    func toAPIModel() -> [UserClients] {
+        let userClients = payload.reduce(into: [UserClients]()) { partialResult, dict in


Suggested change

let userClients = payload.reduce(into: [UserClients]()) { partialResult, dict in

let userClients = payload.reduce(into: [UserClients]()) { partialResult, dictionary in

prefer long form

netbe · 2024-10-14T18:48:30Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+            }
+        }
+
+        try context.save()


Suggested change

try context.save()

if !localUserClient.1 {

try context.save()

}

we only need to save when the client is new

netbe · 2024-10-14T18:50:08Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+            with: id,
+            in: context
+        ) else {
+            return WireLogger.userClient.error(


error or warn?

netbe · 2024-10-14T18:51:50Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+            in: context
+        ) else {
+            return WireLogger.userClient.error(
+                "Failed to find existing client with id: \(id)"


can we obfuscate the id or is it missing from WireFondation?

netbe · 2024-10-14T18:54:23Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+            selfUser.selfClient()?.updateSecurityLevelAfterDiscovering(Set([localClient]))
+        }
+
+        try context.save()


save is outside perform here

johnxnguyen · 2024-10-16T08:54:51Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPI.swift

+
+// sourcery: AutoMockable
+/// An API access object for endpoints concerning clients.
+public protocol ClientAPI {


To be consistent with the other APIs:

Suggested change

public protocol ClientAPI {

public protocol UserClientsAPI {

johnxnguyen · 2024-10-16T08:55:02Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPI.swift

+//
+
+// sourcery: AutoMockable
+/// An API access object for endpoints concerning clients.


Suggested change

/// An API access object for endpoints concerning clients.

/// An API access object for endpoints concerning user clients.

johnxnguyen · 2024-10-16T08:55:41Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIBuilder.swift

+/// A builder of `ClientAPI`.
+
+public struct ClientAPIBuilder {


Suggested change

/// A builder of `ClientAPI`.

public struct ClientAPIBuilder {

/// A builder of `UserClientsAPI`.

public struct UserClientsAPIBuilder {

johnxnguyen · 2024-10-16T08:56:36Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIError.swift

+/// Errors originating from `ClientAPI`.
+
+public enum ClientAPIError: Error {}


Is it necessary now?

johnxnguyen · 2024-10-16T08:58:14Z

WireAPI/Sources/WireAPI/APIs/ClientAPI/ClientAPIV0.swift

+    }
+}
+
+struct ListUserClientV0: Decodable, ToAPIModelConvertible {


Is this necessary? Can't we decode directly as [UserClientV0].self?

johnxnguyen · 2024-10-16T09:06:33Z

...urces/WireDomain/Event Processing/UserEventProcessor/UserLegalholdEnableEventProcessor.swift

+    private func processSelfUserClients() async throws {
+        let remoteSelfClients = try await clientRepository.fetchSelfClients()
+        let localSelfClients = await context.perform {
+            let selfUser = userRepository.fetchSelfUser()
+            return selfUser.clients
+        }
+
+        for remoteSelfClient in remoteSelfClients {
+            let localUserClient = try await clientRepository.fetchOrCreateClient(
+                with: remoteSelfClient.id
+            )
+
+            try await clientRepository.updateClient(
+                with: remoteSelfClient.id,
+                from: remoteSelfClient,
+                isNewClient: localUserClient.isNew
+            )
+        }
+
+        let deletedSelfClientsIDs = localSelfClients.compactMap(\.remoteIdentifier).filter { !remoteSelfClients.map(\.id).contains($0)
+        }
+
+        for deletedSelfClientID in deletedSelfClientsIDs {
+            await clientRepository.deleteClient(with: deletedSelfClientID)
+        }
+    }


I think this should all be inside the client repository.

johnxnguyen · 2024-10-16T09:07:32Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+    public func fetchSelfClients() async throws -> [WireAPI.UserClient] {
+        try await clientAPI.getSelfClients()
+    }


Prefer the convention pullSelfClients, which doesn't return it but stores them locally. I don't think the repository should leak api models.

johnxnguyen · 2024-10-16T09:07:46Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepository.swift

+    public func fetchClients(for userIDs: Set<UserID>) async throws -> [WireAPI.UserClients] {
+        try await clientAPI.getClients(for: userIDs)
+    }


Same as previous comment.

johnxnguyen · 2024-10-16T09:08:23Z

WireDomain/Sources/WireDomain/Repositories/Client/ClientRepositoryError.swift

+
+/// Errors originating from `ClientRepository`.
+
+enum ClientRepositoryError: Error {}


Is it necessary now?

johnxnguyen · 2024-10-16T09:09:43Z

WireDomain/Tests/WireDomainTests/Helpers/UUID+Mock.swift

+import Foundation
+import WireAPI
+
+extension UUID {


Perhaps we should move this (and the ones in WireAPI) to a test utilities target.

jullianm added 4 commits October 11, 2024 14:01

create dedicated ClientAPI and UTs - WPB-10195

2762e5d

create ClientRepository and UT - WPB-10195

8ca670c

use ClientRepository where necessary, fix UTs - WPB-10195

8b263d4

process legalhold enable event, add UTs - WPB-10195

6bead10

echoes-hq bot added echoes: maintenance Maintenance activity - Refactoring , Preventive , Improvements to code , Performance improvements echoes/initiative: ios-sync-refactoring labels Oct 11, 2024

github-actions bot assigned jullianm Oct 11, 2024

jullianm requested review from johnxnguyen and netbe October 11, 2024 12:38

lint and format - WPB-10195 (#2019)

5d62cda

netbe reviewed Oct 14, 2024

View reviewed changes

johnxnguyen reviewed Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: user legalhold enable event - WPB-10195 #2019

refactor: user legalhold enable event - WPB-10195 #2019

jullianm commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

jullianm commented Oct 11, 2024 •

edited

Loading

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

netbe Oct 14, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

johnxnguyen Oct 16, 2024

	path: "/users/list-clients/v2", /// v2 suffix required for api version v0 and v1, suffix removed from next versions
	path: "/users/list-clients/v2", // v2 suffix required for api version v0 and v1, suffix removed from next versions

	let userClients = payload.reduce(into: [UserClients]()) { partialResult, dict in
	let userClients = payload.reduce(into: [UserClients]()) { partialResult, dictionary in

	/// An API access object for endpoints concerning clients.
	/// An API access object for endpoints concerning user clients.

		/// A builder of `ClientAPI`.

		public struct ClientAPIBuilder {

		/// Errors originating from `ClientAPI`.

		public enum ClientAPIError: Error {}


		/// Errors originating from `ClientRepository`.

		enum ClientRepositoryError: Error {}

refactor: user legalhold enable event - WPB-10195 #2019

Are you sure you want to change the base?

refactor: user legalhold enable event - WPB-10195 #2019

Conversation

jullianm commented Oct 11, 2024 • edited Loading

Key points

Testing

Checklist

UI accessibility checklist

github-actions bot commented Oct 11, 2024 • edited Loading

Test Results

jullianm commented Oct 11, 2024 • edited Loading

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

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

jullianm commented Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 11, 2024 •

edited

Loading

jullianm commented Oct 11, 2024 •

edited

Loading