jazzband / django-push-notifications

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

registration_id uniqueness? #442

Open theromis opened 6 years ago

theromis commented 6 years ago

constantly getting APNS errors

APNS device with Registration ID already exists.

with update turned on 'UPDATE_ON_DUPLICATE_REG_ID': True,

I think it because of unique=True is it possible to get rid of uniqueness or fix this issue other way?

registration_id = models.CharField(
                verbose_name=_("Registration ID"), max_length=200, unique=True
        )
goinnn commented 6 years ago

I want the opposite thing. I want unique=True in the other registration_id fields, models: GCMDevice, WNSDevice and WebPushDevice. Now it is the worst case, one of a way and rest of another way.

I think it can be configurable by settings. @jleclanche, @jamaalscarlett if we define it, I can do a pull request.

matthewh commented 6 years ago

The unique=True should be removed from the registration id. It causes more problems than it solves. We need to provide documentation for subclassing the models for those who desire unique=True across all models. Using a setting for this is not advisable. We miss out on database integrity if it is exposed as a setting or we would generate a migration each time the setting was changed.

On Jan 31, 2018, at 3:44 AM, Pablo Martín notifications@github.com wrote:

I want the opposite thing. I want unique=True in the other registration_id fields, models: GCMDevice, WNSDevice and WebPushDevice. Now it is the worst case, one of a way and rest of another way.

I think it can be configurable by settings. @jleclanche, @jamaalscarlett if we define it, I can do a pull request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

ko1es commented 6 years ago

If you remove unique flag from registration_id field, extra validations inside serializers supposed to be made...otherwise your Device model will be full of non usable data.

jleclanche commented 6 years ago

@matthewh go ahead and drop uniqueness if you think it's more troublesome than worth.

matthewh commented 6 years ago

Let me get a PR together. My forked version has been humming along with it turned off for about 2 years :)

On Feb 3, 2018, at 4:03 PM, Jerome Leclanche notifications@github.com wrote:

@matthewh go ahead and drop uniqueness if you think it's more troublesome than worth.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

pzanitti commented 6 years ago

The unique=True should be removed from the registration id. It causes more problems than it solves.

Care to elaborate?

matthewh commented 6 years ago

@pzanitti Enforcing unique=True makes assumptions about multiple users sharing the same device. Out of the box the DPNS DRF API grants the push token to the first user who registers it. The DPNS DRF API will raise an exception, APNS device with Registration ID already exists, if another user tries to register their account (with push service) on the same device. That's downright frustrating to the user who has no way to fix the issue. It's also frustrating to the developer who probably thinks this project should support multiple users on the same device without further customization.

I've had to debug this scenario frequently enough with customers that I disabled unique=True on APNS and never looked back. Plus, DPNS does not enforce it on GCM/FCM so having it on one and not the other is confusing to developers.

jleclanche commented 6 years ago

For context, a lot of the design decisions in DPN were made at a time where:

In other words, I won't need much convincing if real-world usage of DPN proves to need some structural changes.

tobyspark commented 3 years ago

I was surprised to see duplicate APNS devices when my iOS app re-uploads it’s device token. I get the reasoning above, but following that then shouldn’t the constraint be unique_together = ['registration_id', 'user’] rather than none?

(or Django 2.2+ constraint equivalent)

And – thanks all for DPN!