Lickability / PinpointKit

Send better feedback
MIT License
1.13k stars 79 forks source link

Mail Failure Alert Not Appearing #242

Closed mliberatore closed 6 years ago

mliberatore commented 6 years ago

The Mail Failure alert from #237 is not appearing when attempting to send with no mail accounts.

How to Reproduce

  1. Launch the example app in a simulator with no mail accounts set up.
  2. Tap "Send".
mliberatore commented 6 years ago

@grantjbutler, I had some time this morning, so I looked into possible solutions. How about the following for this:

The default delegate implementation changes to

func pinpointKit(_ pinpointKit: PinpointKit, didFailToSend feedback: Feedback, error: Error) {
    guard case MailSender.Error.mailCannotSend = error else { return }

    pinpointKit.presentFailureToComposeMailAlert()
}

The SenderDelegate method implementation in PinpointKit is implemented to also call presentFailureToComposeMailAlert() when lacking a delegate:

public func sender(_ sender: Sender, didFailToSend feedback: Feedback?, error: Error) {
    if case MailSender.Error.mailCanceled = error { return }

    guard let feedback = feedback else { return }
    if let delegate = delegate {
        delegate.pinpointKit(self, didFailToSend: feedback, error: error)
    } else if case MailSender.Error.mailCannotSend = error {
        presentFailureToComposeMailAlert()
    }
}

And the implementation for displaying the mail sending alert is left exposed as public, or perhaps open for maximum customization:

public func presentFailureToComposeMailAlert() {

    let alert = UIAlertController(
        title: "Can’t Send Email",
        message: "Make sure that you have at least one email account set up.",
        preferredStyle: .alert)

    let okAction = UIAlertAction(title: "OK", style: .default, handler: nil)
    alert.addAction(okAction)

    configuration.feedbackCollector.viewController.present(alert, animated: true, completion: nil)
}

And of course, we also make the change in MailSender to set feedback before error-ing out so we reach this code path.

grantjbutler commented 6 years ago

This seems like a good solution to me. The one thing that I might change is the retrieval of the view controller to present the alert on top of, but I don't know if that's something that can easily be injected to the presentFailureToComposeMailAlert method, so that that might be fine to leave as it.