refactor: implement new NSE mechanism and pull pending events - WPB-10219

[jullianm]

WPB-10219 [iOS] Pull pending events when receiving an APNS push

Key points

This PR introduces notification service extension related components to receive push notifications, pull the pending events from the server and generate new content based on these events, specifically:

1. Receives a push notification in NotificationService
2. Pull pending events from the server in NotificationSession
3. Generate new notification content based on these events (will be tackled in WPB-11175)
4. Show the notification content to the user

This PR also introduces a lightweight dependency injection mechanism which automatically resolves some already initialized components required by the NotificationService.

Testing

• NotificationSession callback is properly triggered when observing the pending events.
• Injector properly resolves a registered service

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]

Test Results

    7 files      8 suites   5m 49s ⏱️
4 734 tests 4 706 ✅ 28 💤 0 ❌
4 744 runs  4 716 ✅ 28 💤 0 ❌

Results for commit 29d5784.

♻️ This comment has been updated with latest results.

[datadog-wireapp]

Datadog Report

Branch report: refactor/nse-pull-pending-events
Commit report: 263bb85
Test service: wire-ios-mono

✅ 0 Failed, 4496 Passed, 28 Skipped, 2m 14.98s Total Time

[jullianm]

This will probably evolve to a more stable solution (e.g Needle) at some point in the refactoring process.

[netbe]

Added some comments before approving.

[netbe]

question: in what case do we need multiple arguments?

[jullianm]

When we want to resolve a given dependency with a set of arguments that we only have access to at some point in the app lifecycle. We don't access the concrete type directly, we'll just provide the arguments it needs using the (public) protocol it conforms to.

[netbe]

we don't assert anything here, or am I missing something?

[jullianm]

no because if the instance cannot be resolved, it's a fatal error. The intent here was to not allow any chances of trying to resolve a dependency that has not been already registered (that's why we don't use a throwing function), I think this mistake should be caught early on at development stage and fixed right away.

[netbe]

why do we need to cast here?

[jullianm]

because the underlying type of entry.factory is Any so we need to force cast to deal with a same expected generic type (Factory). Also, at this point the force cast is without any risks because we know we're dealing with a matching entry in the services dict.

[netbe]

ok I feel like a tuple call will help me better understand it, I wonder if we can avoid casting using generic

[netbe]

So just for my understanding, when and where would we initialize the Assembly.

One thing to have in mind there can be multiple process of NSE or iOS can reuse the same process to process different notifications payload

[jullianm]

what I had in mind is that the assembly is the entry point of WireDomain so when the app starts, we use that entry point to setup all of our dependencies, we also take that occasion to register (using the lightweight DI mechanism Injector) some of the dependencies the notification service needs so we don't have to initialize them again every time we receive a new notification.

For instance, the NotificationSession is initialized everytime we receive a new notification, given some of these dependencies were already registered before, we can just resolve them and only pass variable userID which comes from the current notification payload :

let updateEventsRepository = UpdateEventsRepository(
            userID: userID,
            selfClientID: selfClientID,
            // these were already initialized, resolving them
            updateEventsAPI: Injector.resolve(),
            pushChannel: Injector.resolve(),
            updateEventDecryptor: Injector.resolve(),
            updateEventsLocalStore: Injector.resolve()
        )

also mentioning @johnxnguyen because it is related

[netbe]

sounds good

[johnxnguyen]

@netbe, you, and I are all facing the question of how to set up the dependency graph, and we all have various solutions. I propose we meet to find a common solution.

[johnxnguyen]

I feel like introducing a dependency injection system is out of the scope of this task, or is there a specific reason to include it here?

[jullianm]

It was out of the scope but I also felt like we kind of needed to introduce such mechanism to easily resolve the dependencies required by the NotificationSession object and avoid initializing the same objects over and over again.

For instance, to initialize an UpdateEventsLocalStore I would need a context, where I would find this context ? This is the kind of question (and work) the notification service (I think) doesn't need to do since we have the entry point that setup of all of WireDomain dependencies and basically does that work already.

So I figured: let's introduce a lightweight DI mechanism and use our entry point to register the dependencies we need so we don't have to do that work over and over again in the NotificationService.

also mentioning @netbe because it is related to the same topic

[netbe]

@jullianm Ok, I think it would be better to create a dedicated PR for the Injector and to present to the rest of the team before going further with this DI mechnism.

[johnxnguyen]

Since the notification could be for one of user accounts, shouldn't this check happen inside the notification session for that account?

[jullianm]

yes, you're right thank you. fixed in 9378165

[johnxnguyen]

@netbe, you, and I are all facing the question of how to set up the dependency graph, and we all have various solutions. I propose we meet to find a common solution.

[johnxnguyen]

Nitpick: be consistent with previous guard

[johnxnguyen]

Do we need to call super? Also, should we cancel the ongoing task?

[johnxnguyen]

I would move this after we know the user is authenticated.

[johnxnguyen]

Perhaps we can add errorDescription to all the errors and just log them inline.

[johnxnguyen]

We must be careful here to handle interleaved pushes for different accounts. For example, suppose the same service is invoked:

• received push for user A, start a task
• received push for user B, ... what do we do now?

Probably, for each user, we only want to have a single ongoing task at a time, and if a new push comes in, we cancel the existing task and start a new one. But since we may receive pushes for different users, we would need to have a cache of notification sessions.

Which leads me to think that perhaps the session should keep track of its task management.

[johnxnguyen]

I'm wondering if we need to retain the content handler. Do you think it could be passed to the session and the session takes care of invoking it?

[johnxnguyen]

If there's no last event id then it means we need to perform a slow sync, so I would throw an error here. In this case, we actually have no use for the eventID argument.

[johnxnguyen]

What's your reasoning for having a separate publisher to get the events? In my opinion, the benefit of receiving the events directly when calling pullPendingEvents is a linear flow of control (call a method, get a result). Here we are mandating thress separate steps in different places (setup observation, call method, teardown observation).