Skip to content
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

Open
wants to merge 19 commits into
base: notification-preferences
Choose a base branch
from

Conversation

Dhanus3133
Copy link
Member

@Dhanus3133 Dhanus3133 commented Aug 24, 2024

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:

  • No expiry.
  • Invalidated once the user updates their password.

@nemesifier nemesifier changed the title [feat] Manage notification preferences to email notifications [feat] Add unsubscribe link to email notifications Aug 28, 2024
Copy link
Member

@nemesifier nemesifier left a 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

@Dhanus3133 Dhanus3133 changed the base branch from gsoc24-rebased to notification-preferences September 1, 2024 14:22
Copy link
Member

@pandafy pandafy left a 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/urls.py Outdated Show resolved Hide resolved
openwisp_notifications/utils.py Outdated Show resolved Hide resolved
openwisp_notifications/utils.py Outdated Show resolved Hide resolved
openwisp_notifications/base/views.py Outdated Show resolved Hide resolved
openwisp_notifications/base/views.py Outdated Show resolved Hide resolved
Copy link
Member

@pandafy pandafy left a 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.

tests/openwisp2/settings.py Show resolved Hide resolved
openwisp_notifications/views.py Outdated Show resolved Hide resolved
Copy link
Member

@nemesifier nemesifier left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants