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

ENH Ensure users are sent emails when passwords are changed by default #10313

Conversation

GuySartorelli
Copy link
Member

Also a fix to remove the backslash that was being output in emails, and fixing some markdown linting issues in the changelog since I was already editing that file.

Note that each of the three commits has a different prefix for changelog purposes - no need to squash.

Parent issue

@michalkleiner
Copy link
Contributor

Looks good.

What was the rule around changing lang files through Transifex only? Does it also apply to default English files?

@GuySartorelli
Copy link
Member Author

I'm not sure, tbh... I think in this specific case it should be done here, since the translations were already pulled from transifex for this release as part of the beta. But I'm not sure what the general rule is for the default en files.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Just need to mention that there will be no behavioral change on sites with cwp/cwp-core installed

Also, would prefer that we only tried to fix one thing at a time. Even though it's more overhead, it's better to create a multiple PRs that each do a single thing.

docs/en/04_Changelogs/4.11.0.md Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

would prefer that we only tried to fix one thing at a time. Even though it's more overhead, it's better to create a multiple PRs that each do a single thing.

@emteknetnz Fair enough - so you want three separate PRs for this? Or do you just want the changelog linting separate (since the other two are related to each other, but that's kinda separate)?

@emteknetnz
Copy link
Member

The changelog modifications unrelated to the bug that's being fixed should be in its own PR

Seems OK to keep the translation fix here since that's related to change password email that's being fixed

@GuySartorelli
Copy link
Member Author

Linting PR: #10314

@GuySartorelli GuySartorelli force-pushed the pulls/4.11/email-when-password-changes branch from f41069c to cec2576 Compare May 11, 2022 23:42
@kinglozzer
Copy link
Member

kinglozzer commented May 12, 2022

While you’re in there, can you throw in a $this->extend('updatePasswordChangedEmail', $email); hook just before the email is sent? These emails are a pain in the backside to modify otherwise!

@GuySartorelli
Copy link
Member Author

What's the use case for that? You can change the body of the email easily by just overriding the template, and the subject is passed through _t() so you can override that string for your language of choice through normal i18n config.

@kinglozzer
Copy link
Member

We typically run our email templates through a pair of templates: a wrapper that renders the <head> and some conditional tags/compatibility stuff for old email clients, then a $Layout variable that renders the email content. Essentially it allows us to do this and saves duplicating a tonne of code across multiple email templates:

$html = $data->renderWith(['App\Email\ForgotPasswordEmail', 'App\Email\Email']);
$email->setBody($html);

@GuySartorelli
Copy link
Member Author

That's fair - but should probably be done in a separate PR, since you'd probably want to add an extension point for the forgot email password as well - and maybe others.

@michalkleiner
Copy link
Contributor

michalkleiner commented May 12, 2022

Separate PR would be better for the new extension point.

@emteknetnz emteknetnz merged commit 89582dd into silverstripe:4.11 May 12, 2022
@emteknetnz emteknetnz deleted the pulls/4.11/email-when-password-changes branch May 12, 2022 21:39
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.

4.11.0-beta1 PHP 7.4 - No confirmation email recieved after resetting password
4 participants