django-newsletter
django-newsletter copied to clipboard
Add uuid activation code and logic to make it backwards compatible
Add the new activation_uuid as a UUIDField but leave the previous code field alone since an installation may have outstanding subscriptions with that old code, and those are not compatible with a UUID.
Make the old field allow blanks, and default to blank, but the new field defaults to a valid uuid.uuid4(). Now all new records will get a new uuid4() value, but not an old value.
Modify logic to check the UUID using the get_activation_code() method, and the form validates with a new 'valid_activation(data)' method that checks against the old code and the new code, converting the data to a UUID if needed.
Tests pass, at least under Python 3.7
This same test fails for me locally (on python 3.7), and it even fails on 'master' branch. I'm not sure what is going on there, or why it passed before, but it is related to a date field that is created and then later tested. A Y-M-D is setup, and in my case, this FAILS because of a time zone issue. The original date is for "today", but the new date is for "tomorrow", but UTC (or, something like that).
I suspect this will pass at some time of the day, and, probably at whatever time zone you are in!!
Coverage decreased (-0.6%) to 88.023% when pulling 877d0fee96a74443f4fb5cf4a94566e96fb89ecb on sharpertool:feature/update_to_uuid into 30c3ec3f3bea93b22c6640aed40ee2c8040a4f46 on dokterbob:master.
See, this passed once my timezone was close enough to UTC. I should add some additional tests though to improve coverage and to check that the previous formats work.
@kutenai This is super interesting! I'm sorry I have been very busy, I haven't had time to reply or look into this before.
I will likely be quite busy for some time to come. Perhaps someone else could give this a preliminary review? @claudep @pcraston @dsanders11
@kutenai This PR looks quite clean to me and also your description in the initial issue explain what you have changed well.
But I'm kind of missing the why. What is the driver/motivation behind this change?
@kutenai Great stuff! @frennkie Here's why: #273
Let's see if we can get this merged. :)
Code looks good to me, great work! There's some minor issues though, that I'd love to have fixed before fitting this in. It would be great if we could ship this in a 1.0 release, some time soon!
I am really, really sorry I didn't get to review this before. I've been badly burned out in the last year, sadly. But, recuperating. :)
- Code coverage As this is an essential feature, and we really need to guarantee backwards compatibility, it'd be great to have that. It will make maintenance towards the future a lot smoother.
Specifically:
- https://coveralls.io/builds/22231314/source?filename=newsletter%2Fmodels.py#L313
- https://coveralls.io/builds/22231314/source?filename=newsletter%2Fviews.py#L510
Note the TODO comments on missing coverage. If you don't do it, it's likely nobody else will either!
-
Deprecation policy; I would like to propose we deprecate the old activation mechanism with 1.1, clearly mark the legacy code as such and add prepare a note towards this in the 1.0 changelog (you may be so bold to add this).
-
Remove the bump in the version creating a merge conflict and resolve the one in models.py.
-
Regenerate the migration.