Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

what's the intended behavior of apn_device.last_registered_at ? #5

Closed
michaelkirk opened this Issue Oct 28, 2010 · 3 comments

Comments

Projects
None yet
2 participants

from lib/app/models/device.rb

Line 20 is a bit confusing:

before_save :set_last_registered_at

Because the method set_last_registred_at only updates the time when the time wasn't previously set. Is this the intended behavior?

My suggestion, is to either make it before_create, or get rid of the nil check in set_last_registered_at. The first suggestion would mostly mimic the current behavior in a more coherent way, whereas the second suggestion would behave as I expected it (updating the last_registered_at value at each save).

I could submit a pull request if we can agree on a convention.

Member

rebeccanesson commented Nov 4, 2010

Hi Michael,

I've looked through the code and I can't come up with any justification for the nil check in setting last registered at. Given the way that feedback and process_devices work, the same results should occur either way. I'm a little wary of changing the behavior since the current choice was clearly intentional (as shown by the tests). However, I think your second option of removing the nil check makes the most sense.

My thinking as to why the check is there is that it makes the gem very careful to always respect feedback regardless of how often we actually query for feedback and process the devices. If we don't add the check a device could have a feedback date earlier than it's last registered date and would then not be removed. To my mind, however, this case still should not occur. A device should only have feedback if it has opted out of receiving push notifications from a given app. In that case it will not be registering with the server again.

If you agree that the second option makes the most sense and you'd like to make this change and update the tests and submit a pull request, I'll pull the change in. If you'd prefer the first option, let me know why you're leaning that way.

I'm actually leaning towards the first option, making the method before_create, as making it update every time we save could suprise some people. After all, "saving" is not synonymous with "registering", and I can imagine that someone might want to change a device without having re-registered it.

I'm not yet familiar with the feedback code; would this decision interfere with that?

Member

rebeccanesson commented Nov 9, 2010

After more though I agree with you. I'm not even sure it makes sense to automatically set it on create, but I don't think we should change that because users may be relying on it.

I'm planning a next release with a bunch of changes/fixes tomorrow and this will be in it.

Thanks for helping to think it through.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment