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

Set type of email notification #947

Open
wants to merge 2 commits into
base: 8.next
Choose a base branch
from
Open

Conversation

ravage84
Copy link
Contributor

Without this no email is sent.

This has become necessary after the following change from #685

https://github.com/CakeDC/users/pull/685/files#diff-087e5d4ddeb9b25aa37f089c2ce923ee418537310623b29d393404528f1797a3R73-R80

@ravage84
Copy link
Contributor Author

By the way, I haven't checked whether there are other places to adjust.

@@ -239,6 +239,7 @@ public function passwordEmail()
'expiration' => Configure::read('Users.Token.expiration'),
'checkActive' => false,
'sendEmail' => true,
'type' => 'passord',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:embarrassing: Fixed

@steinkel
Copy link
Member

Please could you describe the use case when email is not being sent? I'd like to cover it with a unit test.

Thanks,

@ravage84
Copy link
Contributor Author

Please could you describe the use case when email is not being sent? I'd like to cover it with a unit test.

Thanks,

I think we tried to send the reset password link through the CLI, thus the adjusted Shell class, but no mail was sent due to the recent change mentioned.

@steinkel
Copy link
Member

Thank you, we'll try to reproduce the issue in a unit test and fix it, thanks!

@steinkel
Copy link
Member

well, you fix it already ;) I mean merge it

@steinkel steinkel self-assigned this Jul 5, 2021
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.

2 participants