jazzband / django-push-notifications

Send push notifications to mobile devices through GCM or APNS in Django.
MIT License
2.28k stars 618 forks source link

APNS code should check result for "Success" and propagate errors #398

Open chrisballinger opened 7 years ago

chrisballinger commented 7 years ago

In the below code the function _apns_send returns a dict of the token(s) and return value. It appears from my debugging session that the following are possible return values:

Right now the library only checks for Unregistered tokens but doesn't provide feedback for other types of APNS errors. I propose exposing these errors further up the stack so they can be handled in some way in application code.

def apns_send_message(registration_id, alert, application_id=None, certfile=None, **kwargs):
    """
    Sends an APNS notification to a single registration_id.
    This will send the notification as form data.
    If sending multiple notifications, it is more efficient to use
    apns_send_bulk_message()

    Note that if set alert should always be a string. If it is not set,
    it won"t be included in the notification. You will need to pass None
    to this for silent notifications.
    """

    try:
        result = _apns_send(
            registration_id, alert, application_id=application_id,
            certfile=certfile, **kwargs
        )
    except apns2_errors.APNsException as apns2_exception:
        if isinstance(apns2_exception, apns2_errors.Unregistered):
            device = models.APNSDevice.objects.get(registration_id=registration_id)
            device.active = False
            device.save()
        raise APNSServerError(status=reason_for_exception_class(apns2_exception.__class__))
    return result

def apns_send_bulk_message(
    registration_ids, alert, application_id=None, certfile=None, **kwargs
):
    """
    Sends an APNS notification to one or more registration_ids.
    The registration_ids argument needs to be a list.

    Note that if set alert should always be a string. If it is not set,
    it won"t be included in the notification. You will need to pass None
    to this for silent notifications.
    """

    results = _apns_send(
        registration_ids, alert, batch=True, application_id=application_id,
        certfile=certfile, **kwargs
    )
    inactive_tokens = [token for token, result in results.items() if result == "Unregistered"]
    models.APNSDevice.objects.filter(registration_id__in=inactive_tokens).update(active=False)
    return results
kit-cat commented 7 years ago

In the below code the function _apns_send returns a dict of the token(s) and return value. It appears from my debugging session that the following are possible return values:

Success MissingTopic Unregistered and more?

Return values of _apns_send can be "Success" or any of these errors: https://github.com/jleclanche/django-push-notifications/blob/master/push_notifications/apns_errors.py

What kind of error exposing would be most useful for you?

chrisballinger commented 7 years ago

I'm not sure exactly what would fit best without breaking the existing API, but I was having trouble understanding why my requests weren't working and the only way to figure it out was to pause in the debugger and inspect the return values.

Although apns_send_bulk_message returns results, the code I copied above actually contains some modifications to capture and return the result in apns_send_message. I'll submit a PR for that and add docs about the return values.