App loading checks. Realm migration.

[Kharchevskyi]

Add step approach for app loading: close #185

• Setup Core of the project
• Setup database and perform migration if needed
• Setup services required for user notification (Google SDK)
• Setup a session. Create or renew Imap session
• Check if user has encrypted storage
• Presenting flow for user

Remove encrypted storage access checks: close #183
Realm Migration: close #190
Minor fix for renewing access token

[tomholub]

Please see comments.

While there are a lot of nitpicks (typos and English), there are also some more important questions and requests.

[tomholub]

It looks good. I'll test it and have a second look tomorrow

[tomholub]

But the tests are failing for some reason @Kharchevskyi

[tomholub]

The startupflow is over-engineered.

You have:

• LaunchFlowController
• FlowControllerType
• FlowController
• LaunchContext
• LaunchStepType
• LaunchFlow
• LaunchFlowStepFactory
• CoreLaunchStep
• AuthLaunchStep
• DataBaseLaunchStep
• SessionLaunchStep
• MainLaunchStep

All of this complexity to find out if a few things are as expected. With promises, this could be one class with 6 methods, 5 of them private.

Pseudo-code:

struct LaunchContext {
    let window: UIWindow
    let aplication: UIApplication
    let launchOptions: [UIApplication.LaunchOptionsKey: Any]?
}

class StartupChecks {
  
  public fun runChecks(context: LaunchContext): Promise<void>  {
    return Promise.all([
      this.checkCore(context),
      this.checkAuthentication(context),
      this.checkDatabase(context),
      this.checkSession(context),
      this.checkMain(context)
    ]);
  }

  private fun checkCore(context: LaunchContext): Promise<void> {
    return Promise<void> { _ in
        DispatchQueue.promises = .global()
        core.startInBackgroundIfNotAlreadyRunning()
    }
  }

  // private fun checkAuthentication() ...

  // ...
}

It's faster to write, less effort to maintain. The more classes and abstractions you hide a problem behind, the harder it is to solve the problem.

Keep it simple. Simple code is easy to read. Simple code is easy to understand. Simple code is easy to review for security.

I'll refactor it.

[tomholub]

todo:

• test migration
• remove local storage token after migration done
• test AppStartup error rendering
• fix failing tests
• run UITests
• encryptedConfiguration cannot be optional

[tomholub]

Cannot run UI tests, stuck on #191

[tomholub]

Looks ok, but please also review it @Kharchevskyi

[Kharchevskyi]

This solution is not testable. And not scalable.
This complexity was added for the purpose.

Every of this launch steps ( CoreLaunchStep, AuthLaunchStep,DataBaseLaunchStep,SessionLaunchStep,MainLaunchStep) can be tested separately or mocked for tests. All of them has it's own responsibility.

LaunchFlow and LaunchFlowStepFactory create steps, base on a context. In case we receive push notification or open app from deeplink it will create Launchsteps which are responsible for handling events.

I don't agree about over-engineering

This was scalable, testable and reusable. Now it's only one god object class.

[tomholub]

To me, simplicity is more important. I need our code to be easily readable and reviewable, and I need it to be of minimal complexity.

Every of this launch steps ( CoreLaunchStep, AuthLaunchStep,DataBaseLaunchStep,SessionLaunchStep,MainLaunchStep) can be tested separately or mocked for tests. All of them has it's own responsibility.

Factory/step approach:

• 381 lines over 12 classes, 11 files
• difficult to follow business logic
• each piece testable individually
• difficult to do security review

Single class approach:

• 60 lines, 1 class
• easy to follow business logic, chronological
• hard to test individually
• trivial security review

For our usecase, the tradeoffs are different from a regular app. We need simplicity, readability above everything else.

LaunchFlow and LaunchFlowStepFactory create steps, base on a context. In case we receive push notification or open app from deeplink it will create Launchsteps which are responsible for handling events.

To handle deeplinks, edit chooseView. Add an argument containing the possible deeplink + 1 if.

---

The added complexity comes with a tradeoff, and the tradeoff is not worth it for us.

[tomholub]

As for testing, consider this:

• CoreLaunchStep - we already test core extensively
• AuthLaunchStep - just fills a few constants, nothing to test
• DataBaseLaunchStep - if this fails, no UITests will work, already tested. Plus we aren't going to test migrations, anyway.
• SessionLaunchStep - if this doesn't work, it will show in other tests
• MainLaunchStep - This can be easily tested with UITests

I prefer tests that are not tightly coupled to implementation details. Any tests you write that would be testing the factory/step design directly would necessarily be tightly tied to implementation. That is not the kind of tests I want to focus on.