-
-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] Add unsubscribe link to email notifications #307
base: notification-preferences
Are you sure you want to change the base?
[feat] Add unsubscribe link to email notifications #307
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- please add tests
- please remove time based expiration
- if the user unsubscribes, we need to turn off all email notifications
openwisp_notifications/templates/openwisp_notifications/unsubscribe.html
Outdated
Show resolved
Hide resolved
openwisp_notifications/templates/openwisp_notifications/unsubscribe.html
Outdated
Show resolved
Hide resolved
openwisp_notifications/templates/openwisp_notifications/unsubscribe.html
Outdated
Show resolved
Hide resolved
… global setting is disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature need a lot of improvements in the current state. We shall work on the shortcomings and ensure the logic is robust as this will be a vital feature of the notifications module.
openwisp_notifications/templates/openwisp_notifications/unsubscribe.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick review of the code and found the following areas for improvement. I will do another round of functional testing and share my findings.
openwisp_notifications/static/openwisp-notifications/js/unsubscribe.js
Outdated
Show resolved
Hide resolved
openwisp_notifications/templates/openwisp_notifications/unsubscribe.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some conflicts here.
Fixes: #256
FYR, Please note that this feature depends on PR #290 to manage global email notification preferences, which is not implemented in this PR. Currently, the user's first notification will be treated as their global email notification preference.
Regarding the email token generated: