dreipol / django-scarface

Send push notifications to mobile devices using Amazon SNS
MIT License
43 stars 21 forks source link

Implemented owner field on Device Model for unique push notifications #17

Closed denizs closed 2 years ago

denizs commented 7 years ago

Default is the current User Model. Use SCARFACE_DEVICE_OWNER_MODEL to define your custom model (such as your profile) formatted as app_label.ModelName. The related_name is devices, so in order to gain access to an owner's device(s), simply use your_owner_model.devices.all()

chschuermann commented 7 years ago

Thank you for submitting the pull request. We will look into it within the next couple of days.

denizs commented 7 years ago

Awesome thanks 👍 I'm already using it in my custom implementation and it works fine :-)

chschuermann commented 7 years ago

The problem with your solution is the missing migration. If we would add a migration with a foreign key to the AUTH_USER_MODEL, you would not be able to change it to your custom model in your project. What do you think about a many to many field pointing to the device model on your custom user model?

denizs commented 7 years ago

Sorry for replying so late! Uhm oddly, it seems to work on my Django 1.10. project! I've successfully changed it from User to my profile model, but a m2m field is fine for me as well!

maryokhin commented 7 years ago

Would a m2m field make sense though? A user can have many devices, but I don't think it makes sense for a device to belong to many users.

maryokhin commented 7 years ago

Since I can't use the library as-is I will test this branch out on our project before it's merged, with migrating and all. Otherwise our app won't understand how to send push notifications to users.

maryokhin commented 7 years ago

PR missing migration and adding SCARFACE_DEVICE_OWNER_MODEL to docs.

@denizs I added the missing migration https://github.com/villoid/django-scarface/commit/e4d1134f044d7480f5c4346ccfd671d072f2f8b7 in my fork, you can copy-paste into the PR if you want.

melbic commented 7 years ago

A user can have many devices, but I don't think it makes sense for a device to belong to many users.

This is correct. And therefor a foreign key on the device would be the correct solution. But actually in two of our products we haven't got any Users, but rather only devices, because we're not interested in the user itself. Those devices tend to have more fields like language or last_active those fields aren't always the same in all of our projects, so I think, it makes no sense to add those fields to the Device-model. Furthermore there are devices which can't receive pushs ( simulators etc.)

The library was designed with another aspect in mind. We usually have an own Device model which has an OneToOne-Field to a "SNS-Device".

Here an example

class Device(models.Model):
    udid = models.CharField(
            max_length=255,
            unique=True
    )
    language = models.CharField(
            max_length=2,
            null=True,
            blank=True,
    )
    last_active = models.DateTimeField(
            auto_now=True,
    )
    sns_device = models.OneToOneField(
            to=SNSDevice,
            null=True,
            blank=True,
            on_delete=models.CASCADE
    )

Does that make sense to you?

maryokhin commented 7 years ago

This is correct. And therefore a foreign key on the device would be the correct solution. But actually in two of our products we haven't got any Users, but rather only devices, because we're not interested in the user itself.

The user model link in nullable, so unless the application code specifically sets owner, there will be no owner for the device. Regarding no users at all in the application, I don't think it's possible (or very hard to do) to run a Django app without a user model, even if you really want to. So the migration will always work even if the user model is never used.

The library was designed with another aspect in mind. We usually have an own Device model which has an OneToOne-Field to a "SNS-Device".

Regarding this the Device model should either be swappable, or a much easier solution in my opinion is adding a bag field like:

class Device(models.Model):
   extra_data = models.TextField(
            null=True,
            blank=True,
    )

This would obviously not allow complex filtering, but then we can always go the swappable device model route.

Having an optional link to user model is a necessity IMO, I was surprised there wasn't one already. Most apps just want to send simple notifications to users, so it would be nice to be able to do this without extra hurdles. Everything on top of that could and is expected to require extra work, but this is basic functionality in my opinion.