Handle errors in GCM responses

[tasn]

Delete devices from the database if the appropriate errors are returned. If "error" is different than "Unavailable", delete the device.

This is according to: http://developer.android.com/google/gcm/http.html

[tasn]

This pull request depends on #15 (and includes those commit in it).

[tasn]

I updated this according to the latest changes in api.py. This remains mostly untested as I didn't really have the time unfortunately, though it should be solid. Please take a look.

[bogdal]

@tasn In my opinion we shouldn't delete anything from the db. All devices have unique device id and they can be related with other project models (e.g. for history, stats, etc). Instead of deleting we should mark them as inactive (is_active flag).

[tasn]

I didn't want to extend GCMMessage, and opted for a separation so there'll be a clear separation between the low level GCM interaction (GCMMessage), and the higher level database management. Those are completely different things and should not be mixed.

As for we don't need it/we should mark as non-active/we should return the error list:

Unregistered Device
  An existing registration ID may cease to be valid in a number of scenarios, including:

    If the application manually unregisters by issuing a com.google.android.c2dm.intent.UNREGISTER intent.
    If the application is automatically unregistered, which can happen (but is not guaranteed) if the user uninstalls the application.
    If the registration ID expires. Google might decide to refresh registration IDs.
    If the application is updated but the new version does not have a broadcast receiver configured to receive com.google.android.c2dm.intent.RECEIVE intents.

For all these cases, you should remove this registration ID from the 3rd-party server and stop using it to send messages.
Happens when error code is NotRegistered.

Those are all "dead" cases. Dead as in not-coming-back dead, and that's why I removed it from the db then. The thing is, and that's maybe where we differ, is that I think about my (can't see how to make it better) real life scenario. Because there's no consistent working way of getting a unique identifier on android (some botched the device id, and you have many other issues), in my implementation I use a UUID I generate and keep on the device. When the app is removed (and thus data is removed) this UUID dies to never return. Sure we could make it inactive and then I'll have a cleaner task to handle it, it's probably nicer because it eliminates the possibility of users attaching info on the Device, info we'll delete without letting them know, but I felt like is_active should be used by other things, like temporary request to stop sending notification (initiated by user), where we want to keep the details but just stop notifications for a bit.

Thoughts?

[bogdal]

As far as GCMMessage class is concerned, I agree with you, but I suggest to use a more meaningful name e.g. GCMMessage(api.GCMMessage) or something.

Inactive flag means that we cannot use this device regardless of the reason. Check this out. We mark the device as inactive during unregistration. In your scenario we should delete it in this place. That's what we want to achieve depends on the individual project so in my opinion this should be configurable as much as posible. One of the solutions is create and use of signal e.g. post_send which takes req_ids param. This could be the easiest way to extend the functionality in your project.

[tasn]

I agree with the first part (better naming).

As for the second, I know I have other solutions I can do in my project, and I guess I will. What you are saying makes sense. Deletion is too aggressive. I wonder though if we should maybe change is_active to "activity" or something that is a choice that indicates the activity status which can be active, inactive, invalid; the last one being the reason you put when Google rejects the regid. Just putting it out there. Anyhow, as we've established, it doesn't really matter to me. On 12 Aug 2014 20:33, "Adam Bogdał" notifications@github.com wrote:

As far as GCMMessage class is concerned, I agree with you, but I suggest to use a more meaningful name e.g. GCMMessage(api.GCMMessage) or something.

Inactive flag means that we cannot use this device regardless of the reason. Check this out https://github.com/bogdal/django-gcm/blob/develop/gcm/forms.py#L25. We mark the device as inactive during unregistration. In your scenario we should delete it in this place. That's what we want to achieve depends on the individual project so in my opinion this should be configurable as much as posible. One of the solutions is create and use of signal e.g. post_send which takes req_ids param. This could be the easiest way to extend the functionality in your project.

— Reply to this email directly or view it on GitHub https://github.com/bogdal/django-gcm/pull/17#issuecomment-51956996.

[bogdal]

Maybe we should add status field with the following choices:

• registered
• unregistered
• invalid

Devices with flag is_active = False would be treated as deleted.

[tasn]

If we add the status field, is_active becomes implied, and thus our data is redundant. I'd drop the is_active and just add methods for compat.

Thoughts? On 14 Aug 2014 07:48, "Adam Bogdał" notifications@github.com wrote:

Maybe we should add status field with the following choices:

• registered
• unregistered
• invalid

Devices with flag is_active = False would be treated as deleted.

— Reply to this email directly or view it on GitHub https://github.com/bogdal/django-gcm/pull/17#issuecomment-52149338.

[tasn]

Hey, any thoughts on the matter? I tried thinking about it a bit more, and because of the way the split to chunks and the return values are done, it's quite annoying to implement the is_active=False outside of the class (as you need to know internal information, like chunk size). It should really be in.

Also, thinking about the is_active and status fields a bit more, I don't think the status field is needed. If I'd like to add status as a user of this class, I'll just add a new field called cancelled_by_user or something that indicates I cancelled it. So it's definitely not needed internally.

I wonder though if we should have a function (that can be overridden) on the model that's called mark_invalid() that essentially does it in this case. This is most pluggable and let's the most amount of control.

Any thoughts regarding this suggestion?

[bogdal]

Hi @tasn. Sorry for the delay in response. I think it could be a good idea with additional method, however I would change the name to e.g. mark_inactive, because unregister action will call it as well.

[tasn]

OK, I'll change it to "mark_inactive()", though one thing then, should I have a named parameter "error" that indicates the error that has happened (if there has been one) or None (default) for marked by user/code request?

[tasn]

Updated the PR according to our discussion. We should probably create an "Error" class rather than use strings, but this is for a future change.