New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Critical error handling and presentation #367

Merged

hzalaz merged 3 commits into v2 from feature_critical_errors

Jan 12, 2017

Member

cocojoe commented Jan 10, 2017

Added Error support to CDNLoader callback
Added ConnectionError Presenter/View
Added Option to disable full screen critical errors
Added ConnectionError Route
Updated CDNLoader callback tests
Added Tests for new Presenter/View


          Added Error support to CDNLoader callback

30af43b

Added ConnectionError Presenter/View
Added Option to disable full screen critical errors
Added ConnectionError Route
Updated CDNLoader callback tests
Added Tests for new Presenter/View

cocojoe requested a review from hzalaz

January 10, 2017 17:12

hzalaz suggested changes

View reviewed changes

Lock/CDNLoaderInteractor.swift

-                  func load(_ callback: @escaping (Connections?) -> ()) {
+                  func load(_ callback: @escaping (Connections?, RemoteConnectionLoaderError?) -> ()) {
+                      let sessionConfiguration = URLSessionConfiguration.default
+                      sessionConfiguration.timeoutIntervalForRequest = 30 // Default 60

Member

hzalaz Jan 10, 2017

why the change?

Member Author

cocojoe Jan 10, 2017

oh yeah should have mentioned, I added it for testing but 60 is a LONG time and we did mention timeout tweaking. Happy to remove if you feel it is too short?

Member

hzalaz Jan 10, 2017

Let's use the default, probably we can inject the session to the interactor in some way

Lock/CDNLoaderInteractor.swift

                           guard error == nil else {
                               self.logger.error("Failed to load with error \(error!)")
-                              callback(nil)
+                              if let error = error, error._code == NSURLErrorTimedOut {

Member

hzalaz Jan 10, 2017

Might rewrite this whole error handling like

guard error?._code != NSURLErrorTimedOut else { return callback(.connectionTimeout) }
guard error == nil else { return callback(.requestIssue) }

Lock/CDNLoaderInteractor.swift

                           }
                           guard 200...299 ~= response.statusCode else {
                               self.logger.error("HTTP response was not successful. HTTP \(response.statusCode) <\(payload ?? "No Body")>")
-                              return callback(nil)
+                              if response.statusCode == 403 {

Member

hzalaz Jan 10, 2017

Why not have a guard before this one? (or an if) like

guard response.statusCode != 403 else { return callback(nil, RemoteConnectionLoaderError.invalidClient) }

Lock/CDNLoaderInteractor.swift

                           }
                           guard var jsonp = payload else {
                               self.logger.error("HTTP response had no jsonp \(payload ?? "No Body")")
-                              return callback(nil)
+                              return callback(nil, RemoteConnectionLoaderError.invalidClientInfo)

Member

hzalaz Jan 10, 2017

We can omit the class name here

Lock/CDNLoaderInteractor.swift

                               info.oauth2.forEach { strategy in
                                   strategy.connections.forEach { connections.social(name: $0.name, style: AuthStyle.style(forStrategy: strategy.name, connectionName: $0.name)) }
                               }
-                              callback(connections)
+                              if connections.isEmpty {

Member

hzalaz Jan 10, 2017

why not use guard?

Lock/RemoteConnectionLoaderError.swift


		import Foundation

		enum RemoteConnectionLoaderError: Error, LocalizableError {

Member

hzalaz Jan 10, 2017

We have a unrecoverable error enum with some of this errors already

Lock/RemoteConnectionLoaderError.swift

+                  case invalidClient
+                  case invalidClientInfo
+                  case noConnections
+                  case requestIssue

Member

hzalaz Jan 10, 2017

I see this one and the next have no message so why have a different case?

Member Author

cocojoe Jan 10, 2017

I wanted to distinguish the error from the ones we care about, from the users perspective they will show connectionTimeout message. However, they are not technically the same thing.

Member

hzalaz Jan 10, 2017

what I mean we can merge them and said just request failed since at this point we discarded network or client issues so it only is that the server is unavailable

Lock/RemoteConnectionLoaderError.swift

+              enum RemoteConnectionLoaderError: Error, LocalizableError {
+                  case connectionTimeout
+                  case invalidClient

Member

hzalaz Jan 10, 2017

This one and the next are exactly the same thing from user perspective

Member Author

cocojoe Jan 10, 2017

It depends on the circumstance, invalidClientInfo could be a temp issue as your credentials are valid but for whatever reason the response isn't right.
invalidClient is more a developer one really, saying you're credentials are invalid based upon the 403. Although again technically could be a temp server issue.

Member

hzalaz Jan 10, 2017

But for the developer is the same thing (check your clientid and try again) and the end user is even simpler. If we need more debug info or fine grained errors for it the developer can use the log or even debug it

Lock/ConnectionErrorPresenter.swift


		import Foundation

		class ConnectionErrorPresenter: Presentable, Loggable {

Member

hzalaz Jan 10, 2017

Let's make this presenter (and views) generic like UnrecoverableErrorPresenter or similar so we can reuse

Lock/RemoteConnectionLoaderError.swift

+                  var localizableMessage: String {
+                      switch self {
+                      case .invalidClient:
+                          return "No client information found, please check your Auth0 client credentials.".i18n(key: "com.auth0.lock.critical.credentials", comment: "Invalid client credentials")

Member

hzalaz Jan 10, 2017

Let's use the prefix com.auth0.lock.error and maybe unrecoverable too

Member Author

cocojoe commented Jan 11, 2017 •

edited

Loading

I've unified the errors into UnrecoverableError. However there is an annoying issue due to equitability. We have

Route: Equatable

https://github.com/auth0/Lock.iOS-OSX/blob/feature_critical_errors/Lock/Routes.swift#L74

This is a requirement due to one line of code in Lock:

https://github.com/auth0/Lock.iOS-OSX/blob/feature_critical_errors/Lock/Router.swift#L170

Now because UniversalError will now contain an enum case with a value.

case invalidOptions(cause: String)

It will also need to be made equatable to satisfy as the errors are of type UniversalError

case (.connectionError(let lhsError), .connectionError(let rhsError)):
        return lhsError == rhsError

Rather than having having two custom equatable enums and two equatable unit tests (both need to be maintained for any additions). Could we remove all of this and simply satisfy the original requirement in enum Route?

var isRoot: Bool {
    switch self {
    case .root:
        return true
    default:
        return false
    }
}

UPDATE: Actually there are a few genuine requirements in LockTests for Route Equatable. I've made UniversalError: Equatable for now.


          Unified RemoteConnectionError into UnrecoverableError

5b9fefe

Soft errors now default, option removed.
Added more tests, updated existing tests
General code style improvements

cocojoe force-pushed the feature_critical_errors branch from 952c1b0 to 5b9fefe Compare

January 11, 2017 14:48

Member Author

cocojoe commented Jan 11, 2017

hzalaz suggested changes

View reviewed changes

Lock/UnrecoverableError.swift

+                  var localizableMessage: String {
+                      switch self {
+                      case .invalidClientOrDomain:
+                          return "No client information found, please check your Auth0 client credentials.".i18n(key: "com.auth0.lock.error.invalidclient", comment: "Invalid client")

Member

hzalaz Jan 11, 2017

Let's add to the prefix com.auth0.lock.error an extra part like unrecoverable for all keys of unrecoverable error

Member

hzalaz Jan 11, 2017

Also move this error to the requestIssue case

Lock/UnrecoverableError.swift

+                      case .invalidClientOrDomain:
+                          return "No client information found, please check your Auth0 client credentials.".i18n(key: "com.auth0.lock.error.invalidclient", comment: "Invalid client")
+                      case .clientWithNoConnections:
+                          return "No connections available for this client, please check your Auth0 client setup.".i18n(key: "com.auth0.lock.error.noconnections", comment: "No client connections")

Member

hzalaz Jan 11, 2017

No authentication methods found for this client. Please check your client setup.

Lock/UnrecoverableError.swift

+                      case .clientWithNoConnections:
+                          return "No connections available for this client, please check your Auth0 client setup.".i18n(key: "com.auth0.lock.error.noconnections", comment: "No client connections")
+                      case .connectionTimeout, .requestIssue:
+                          return "There was a problem with your request, please try again later.".i18n(key: "com.auth0.lock.error.requestfailed", comment: "Generic request failure")

Member

hzalaz Jan 11, 2017

Use https://github.com/auth0/lock/blob/master/src/i18n/en.js#L97

Lock/UnrecoverableError.swift

+                  var userVisible: Bool {
+                      switch self {
+                      case .missingDatabaseConnection, .invalidOptions:

Member

hzalaz Jan 11, 2017

Also invalidClientOrDomain

Member

hzalaz Jan 11, 2017

Let's make all visible, those who were previously non visible we should show the message https://github.com/auth0/lock/blob/master/src/i18n/en.js#L97

Lock/UnrecoverableError.swift

+                          return "No connections available for this client, please check your Auth0 client setup.".i18n(key: "com.auth0.lock.error.noconnections", comment: "No client connections")
+                      case .connectionTimeout, .requestIssue:
+                          return "There was a problem with your request, please try again later.".i18n(key: "com.auth0.lock.error.requestfailed", comment: "Generic request failure")
+                      case .missingDatabaseConnection:

Member

hzalaz Jan 11, 2017

Move this error to the requestIssue case


          Update UnrecoverableError messages

Update UnrecoverableError Tests

cocojoe force-pushed the feature_critical_errors branch from d7bbb07 to 1792803 Compare

January 12, 2017 09:52

hzalaz merged commit b86c88e into v2

hzalaz deleted the feature_critical_errors branch

January 12, 2017 14:59

hzalaz added the CH: Added label

hzalaz added this to the v2-Next milestone

hzalaz modified the milestones: v2-Next, 2.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels