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

remove $CleartextPassword from ChangePasswordEmail.ss #5221

Merged
merged 1 commit into from
Mar 23, 2016

Conversation

christopherdarling
Copy link
Contributor

4.x version of #5194

@dhensby
Copy link
Contributor

dhensby commented Mar 22, 2016

Line 5 really needs changing as it doesn't show the credentials any more, just email

This should probably just say something like "The password for account with email address $Email has been changed. If you didn't change your email please change your password (link to password reset form) urgently"

@christopherdarling
Copy link
Contributor Author

updated CHANGEPASSWORDTEXT2 with your suggestion and removed the output of Email on line 9 as it's now in the text of CHANGEPASSWORDTEXT2

@dhensby
Copy link
Contributor

dhensby commented Mar 22, 2016

(link to password reset form) needs to be an actual link ;)

Maybe easier with translations to say "using the link below" then include a link to Security/changepassword

@christopherdarling
Copy link
Contributor Author

Whoops! PR updated with link on new line. One thought is from @jonom's comment on #5194 should we be displaying different text ('for account with email...') if Member.unique_identifier_field is not Email?

@tractorcow
Copy link
Contributor

I think it's probably fine; If you're using a custom field, you can also just as easily add a custom template too.

<p>
<%t ChangePasswordEmail_ss.EMAIL 'Email' %>: $Email<br />
<%t ChangePasswordEmail_ss.PASSWORD 'Password' %>: $CleartextPassword
<%t ChangePasswordEmail_ss.CHANGEPASSWORDTEXT2 'The password for account with email address {email} has been changed. If you didn't change your password please change your password using the link below' email=$Email %><br />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't re-define translation strings re-using old keys; You'll need to create a new translation string instead.

All translations are shared across all versions, so this would cause this string to appear on all 3.x releases, unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, my bad.

@tractorcow
Copy link
Contributor

One more problem to fix please. :)

@christopherdarling
Copy link
Contributor Author

Good point. All done :-)

@tractorcow tractorcow merged commit c6c71fa into silverstripe:master Mar 23, 2016
@christopherdarling christopherdarling deleted the patch-1 branch April 13, 2016 13:53
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.

None yet

3 participants