Refactor decidePolicyForNavigationAction

[Ceri-anne]

Refactored decidePolicyForNavigationAction function to remove force unwraps and hopefully make intent clearer

[benhalpern]

This mostly seems good, but it looks like the "login" use case is not handled properly.

A user should be able to go and come back from the log in screens. If you try logging in with these refactoring, it should be clear what I mean. Other than that, it looks good!

[Ceri-anne]

Oops! Hopefully fixed now and tests added

[nebloc]

Does this PR also fix issue with mailto link in contacts? (#97)

[benhalpern]

@nebloc

It does not, I believe. But it's related. I'll give that a look ASAP.

@Ceri-anne I'll merge soon. Travis is giving false failures. Will figure it out tomorrow. If you want to give #97 a look and include the fix in this PR before it gets merged, be my guest. Otherwise we'll get to it after.

[Ceri-anne]

I'll add the fix for #97 before it gets merged @benhalpern. There's a couple of linting errors to fix too so I'll do those

[nebloc]

Would it be cleaner to move navigationPolicy to use a switch statement? I.e.

 switch url {
 case url.scheme == "mailto": 
    openURL(url)
    return .cancel
 case url.absoluteString == "about:blank": 
    return .allow
 case isAuthLink(url):
    return .allow
 case url.host != "dev.to" && navigationType.rawValue == 0:
    loadInBrowserView(url: url)
    return .cancel
 default:
    return .allow
 }

[Ceri-anne]

Yep! Good thinking 😊 Although we’re not switching on the url directly...

[benhalpern]

Travis is failing but I think it's misconfigured. I'll be merging this later today.

[nebloc]

Yep! Good thinking blush Although we’re not switching on the url directly...

@Ceri-anne You are totally right! 😁 What about this?

if url.scheme == "mailto" {
            openURL(url)
            return .cancel
}

if url.absoluteString == "about:blank" {
            return .allow
} 

if isAuthLink(url) {
            return .allow
} 

if url.host != "dev.to" && navigationType.rawValue == 0 {
            loadInBrowserView(url: url)
            return .cancel
}

return .allow

Just think that it would make it a bit more readable for new comers..

[Ceri-anne]

I agree that it’s more readable but it also implies that those if statements can be reordered and it would still work but that’s not true