fcm-django icon indicating copy to clipboard operation
fcm-django copied to clipboard

Add updated_at field

Open nachov99 opened this issue 2 years ago • 21 comments

nachov99 avatar May 11 '23 22:05 nachov99

Yes, that would be a welcome addition. Pull requests welcome

xtrinch avatar May 12 '23 08:05 xtrinch

@xtrinch , May I work on this ? If yes, please assign this issue to me and provide some description about the issue.

mohitCodepy avatar May 24 '23 04:05 mohitCodepy

For sure @mohitCodepy. Essentially, it is a datetime field which updates automatically with a new datetime everytime an FCMDevice entry is updated

xtrinch avatar May 24 '23 06:05 xtrinch

Seems that date_created is enough for tracking expired tokens. Or @nachov99 is there specific case where that field could be helpful to have updated_at?

Maybe better instead of bloating models for all users that use that app, we could allow override models to users who needs that? It could be done with swapper. In the same way as it was done for django-cities

Akay7 avatar May 26 '23 07:05 Akay7

Tracking in https://github.com/xtrinch/fcm-django/pull/239, let's go with an override-able device model

xtrinch avatar Jun 06 '23 12:06 xtrinch

On the other hand, it takes one param to DatetimeField to save the model creation date or modification date automatically: https://docs.djangoproject.com/en/4.2/ref/models/fields/#django.db.models.DateField

If you name the fields created and updated they will even be the same as TimeStampedModel from django-model-utils!

merwok avatar Jun 06 '23 14:06 merwok

If you name the fields created and updated they will even be the same as TimeStampedModel from django-model-utils!

But is there's a reason for that? I though:

  1. there's no needs to everyone who would like to use that package to pay extra to store data that not required in their DB.
  2. and shouldn't be a limitation to store extra data if it's required according their business requirements

Akay7 avatar Jun 07 '23 06:06 Akay7

The official docs even recommend tracking the last activity of that token:

When stale tokens reach 270 days of inactivity, FCM will consider them expired tokens. Once a token expires, FCM marks it as invalid and rejects sends to it. However, FCM issues a new token for the app instance in the rare case that the device connects again and the app is opened.

Using the token prevents that inactivity and thus seems to be a valid use case to update the timestamp imo.

And another passage:

Retrieve registration tokens from FCM and store them on your server. An important role for the server is to keep track of each client's token and keep an updated list of active tokens. We strongly recommend implementing a token timestamp in your code and your servers, and updating this timestamp at regular intervals.

The last words in the last sentence explicitly recommend an update timestamp.

Eraldo avatar May 10 '24 06:05 Eraldo

If updated_at or any other field is required for your use-case it's possible to achieve via overridden Device model. You can check that at docs or example how that made at tests .

First version where it possible is 2.2.0rc0

Akay7 avatar May 29 '24 04:05 Akay7

Honestly @Akay7 , going from @Eraldo 's last comment - while this is now possible with the overrideable device model, I would still add this into the default one, it sounds like such a common use case

xtrinch avatar May 29 '24 08:05 xtrinch

it sounds like such a common use case

@xtrinch

To me and updating this timestamp at regular intervals sounds like typo. Regular updating timestamps on token doesn't make any sense to me. Maybe author wanna tell that we should keep actual token and update tokens instead of timestamps? Otherwise if we should regularly update timestamps then in what way and for what purpose? Ie if there will be some task that will update timestamp everyday then what is the purpose of that?

But I'm okay if there will be updated by default. It's just not clear to me why it would be useful.

Akay7 avatar May 29 '24 08:05 Akay7

@Akay7 If FCM issues new tokens for same app instance, and one would update the token on the same device model instance, then update date would be the one to look at for expiry, not create date?

xtrinch avatar May 29 '24 09:05 xtrinch

If FCM issues new tokens for same app instance, and one would update the token on the same device model instance, then update date would be the one to look at for expiry, not create date?

@xtrinch

  1. How can we match tokens on the backend that they came from the same device?
  2. What does the update date mean?

As of now we could remove tokens with which we unable to send message. But if we track tokens that have not been used for 30 days(could be different staleness window), so that they are not used later. It seems that it is necessary to track not in the device, but to invent something else where the timestamp of the last message sent for this specific token will be stored.

Akay7 avatar May 29 '24 09:05 Akay7

@Akay I think that kind of depends on the update strategy that the developer chooses.. Let's for example say we'd just update the device instance on a new token - always using the same device instance for all of user's tokens, then update date would be the one to use to track expiry. If one just creates new devices on new tokens and lets old ones be deactivated then create date is perfectly sufficient

As for removing tokens which we are unable to send messages to - that's what the deactivate feature already does and there's also a setting to auto remove these devices

xtrinch avatar May 29 '24 09:05 xtrinch

Let's for example say we'd just update the device instance on a new token - always using the same device instance for all of user's tokens, then update date would be the one to use to track expiry.

@xtrinch

I don't think that is possible to recognize the device on all platforms. Even if that is possible than bad actor will be able to rewrite identifiers of valid devices with invalid token. Since getting device identifier is possible only on client side code.

So in my opinion is best user case that use multiple Devices where each device will have specific FCM Token as it's independent devices in that case updated timestamp for Device(Token) itself is not useful at all.

Akay7 avatar May 29 '24 11:05 Akay7

@Akay7 I'm not sure what you mean by recognize. You'd basically just maintain one device instance per user - it shouldn't be that hard to securely upsert the device instance of the user?

xtrinch avatar May 29 '24 12:05 xtrinch

I'm not sure what you mean by recognize. You'd basically just maintain one device instance per user - it shouldn't be that hard to securely upsert the device instance of the user?

@xtrinch Oh okay. I missed point that there will be a single device per user. In that case everything will be slightly different. And possible it will be useful to keep something like recent FCM token that user had before recent update or something like that. And doesn't store information such as Device.name at all.

Akay7 avatar May 29 '24 13:05 Akay7

Yeah, well I'm guessing most use cases will involve one user having multiple devices but you never know. I'd still add the update date to the base model, it doesn't really hurt anything and it might just help someone.

xtrinch avatar May 29 '24 13:05 xtrinch

Just to make sure we're on the same page... 😉

  • According to what I read FCM tokens are device bound. Meaning different devices will automatically generate new tokens. I made some tests and "Device" does not refer to physical devices. So, for example: Pwa and native app on the same phone yield different devices and thus different tokens. :)
  • A token only expires if it is not used, so given the device that issued the token registers via their sdk repeatedly (regular intervals) the token will probably not expire (no guarantee). If there is no sign of usage, Firebase will flag the tokens as inactive and in the (according to them) rare case of reactivation, a new token might be issued. => Because of this, as a developer I would have auto expired all tokens that have an updated_at date (not created_at older than 30 days.

That's my understanding, however I might be wrong. 🤷‍♂️ I enjoy reading the discussion. 👍

And: Yippie! 🎉 Congrats to swappable devices being released and thanks for making me aware. 🙏

FYI: Django v5x GeneratedField can now also be used to rename fields like registration_id => token while staying backwards compatible by providing a generated (optionally virtual) proxy field. ;)

Eraldo avatar May 30 '24 18:05 Eraldo

Yes that also sounds like a valid use case with this time based auto-expiry instead of expiry-on-error @Eraldo .

xtrinch avatar May 31 '24 06:05 xtrinch

  • I would have auto expired all tokens that have an updated_at date (not created_at older than 30 days.

@Eraldo

There are nuances to this way of doing things, too.

  1. When will happening update of updated_at field? When application getting FCM token and send it to backend? What if there's a lot those updates and table at DB could be also big?
  2. It seems if update only timestamp updated_at on token and no any other updates of any other information then maybe we shouldn't store token and last used timestamp together?
  3. What if some tokens have 30 days and other 60 days of inactivity? Or such a change will have to occur sometime in the future.
  4. What could be the harm in trying to send a message with an expired token? Because if we predict expired tokens, and it's not actually expired tokens, then some users won't get notifications, even though they might otherwise.

I think disabling tokens that failed to send a message with is a more pythonic way(EAFP) to go. But since it is a package that can be used in different ways, I think it is possible to add some functionality that will allow to work with messages in some special ways. For example, check token expiration by time.

Akay7 avatar May 31 '24 08:05 Akay7