django-push-notifications
django-push-notifications copied to clipboard
Improve Webpush, registration id unique configurable and details
Hi!
I'm sorry for these 6 mixed commits, although I think it is a nice christmas present still from my mate @rubgombar1 and me :-)
We use django-push-notifications from 3 years ago and we are very grateful and interested in improve this increible Django app.
For this reason, 1 year ago we introduce Web Push feature, and now we have several changes for it. [b6935832fc1009e2cbba7e25128282bb754191d6][35222c0fd349e6a520da31ec196035f5bbdddb18]:
- If you wanted send a push for three devices and one was outdated, currently the bulk process did not send to the next devices.
- If a devices was outdated, it was not disabled.
Also, now registration_id field depends on model (push type) to define if it is unique or not. We have developed a new feature, and now it depends on the settings.[6da01a12883f66e67e22b85744cf542a38f48dea][68d244ce955dde9c59316d9630d1b26c25fe9bc9]
Also, we have changes cleaning code [1f526069a45d82ff873156dedf9c57868969b875] and fixing a silly error sending a message from admin [f4b8ebfa181cc7bc5c126396cbd44c9aee6f5a95]
And a new commit to fix tests [b38b5ff1e8e091ddcd0f72f56ab9df5f91279545]
These commits were in our production enviroment from:
- 2018-07-23: [1f526069a45d82ff873156dedf9c57868969b875][f4b8ebfa181cc7bc5c126396cbd44c9aee6f5a95][b6935832fc1009e2cbba7e25128282bb754191d6][6da01a12883f66e67e22b85744cf542a38f48dea][68d244ce955dde9c59316d9630d1b26c25fe9bc9]
- 2018-09-24: [35222c0fd349e6a520da31ec196035f5bbdddb18]
- it is a new commit: [b38b5ff1e8e091ddcd0f72f56ab9df5f91279545], but it is only for fix pep8 & pyflakes errors
So, these features were very tested, but we don't have had any time to do a PR until now. But, of course we understand that you want see/analyze these features calmly.
Issues fixed with this PR:
- #457
- #476
- registration_id unique: #442, #184, #41
I landed the first two commits, you can rebase on top of those. Rest needs review.
@jleclanche Done! I pulled master changes in my fork repository.
You can see the diff changes here: https://github.com/jazzband/django-push-notifications/pull/487/files
There are two features:
- Improve webpush, fixing several errors
- Improve registration_id. Depends on a settings variable.
@jleclanche Could you tell me something about it?
I'm not very available for the foreseeable future, sorry. The commit history has merge commits though so that's hard to review either way and can't be merged; make sure to rebase.
Hi @goinnn!
Thanks for the contribution, both the initial work with webpush and this PR. Due to some problems with the CI, your test doesn't pass. It would be nice if you could rebase this branch against the newest master branch, in order to fix the tests.
Also, it would have been awesome to split the webpush and the registration unique parts into two separate PRs. That would be way eaiser to review, and for other developers when they are coming to look at how the project is going.
Also, some tests would have been nice. I guess the webpush-stuff is kinda hard to test, but it doesn't hurt to try. Looking forward to getting this into master! :smile:
Superseded by #674