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

Display username in password-reset mail #2960

Merged

Conversation

kimsible
Copy link
Contributor

@kimsible kimsible commented Jul 13, 2020

fixes #2954

@rigelk rigelk changed the title Fix: display username in password-reset mail Display username in password-reset mail Jul 14, 2020
@kimsible
Copy link
Contributor Author

kimsible commented Jul 14, 2020

@rigelk I think it is a fix for an important issue because when the user receives the notification email, when can it be known the notification concerns the user's account ? For us, as developer, it's pretty obvious yes but for the user it could be blocking.

Plus displaying $[to] instead of a username is very confusing and seen as a bug.

@rigelk
Copy link
Collaborator

rigelk commented Jul 14, 2020

Yes - I'm just renaming because prefixing with "fix :" is not describing the content of the PR.

@kimsible
Copy link
Contributor Author

kimsible commented Jul 14, 2020

Yes - I'm just renaming because prefixing with "fix :" is not describing the content of the PR.

Ok, it has nothing to do with the PR but is there a label to tag fixes ?

@rigelk
Copy link
Collaborator

rigelk commented Jul 14, 2020

For PRs, the determinant of its nature is the issue it solves, so using the fixes #.. syntax in the end of the description or as a "stamp" in a commit message helps determine that.

We use other labels to help us triage PRs (maintenance/priority), but not determine their fix/plain enhancement status.

@kimsible
Copy link
Contributor Author

For PRs, the determinant of its nature is the issue it solves, so using the fixes #.. syntax in the end of the description or as a "stamp" in a commit message helps determine that.

We use other labels to help us triage PRs (maintenance/priority), but not determine their fix/plain enhancement status.

Thanks !

Copy link
Collaborator

@rigelk rigelk left a comment

Choose a reason for hiding this comment

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

The PR sounds good to me.

If you want to go forward with this idea, I would suggest adding to pretty much all emails the username and displayName as local variable. This would allow much richer links in the footer on the long term, but at first I would certainly see it used as follows:

replace the bland Hi, with Hi {username},. It would solve the current problem and more to come.

@kimsible
Copy link
Contributor Author

replace the bland Hi, with Hi {username},. It would solve the current problem and more to come.

Hi {username} is already used in /server/lib/emails/common/greeting.pug

https://github.com/Chocobozzz/PeerTube/blob/develop/server/lib/emails/common/greetings.pug#L5

  if username
    p Hi #{username},
  else
    p Hi,

@rigelk
Copy link
Collaborator

rigelk commented Jul 15, 2020

Yes, I just meant to inject the variable in other email templates

@Chocobozzz Chocobozzz changed the base branch from develop to release/2.3.0 July 20, 2020 06:53
@Chocobozzz Chocobozzz changed the base branch from release/2.3.0 to develop July 20, 2020 06:54
@Chocobozzz
Copy link
Owner

Please use release/2.3.0 as base branch since it's a fix, so we can fix this issue in the next release

@kimsible kimsible force-pushed the fix/username-password-reset-mail branch from 53ccb87 to 6f48663 Compare July 20, 2020 12:59
@kimsible kimsible changed the base branch from develop to release/2.3.0 July 20, 2020 12:59
@Chocobozzz Chocobozzz merged commit 963023a into Chocobozzz:release/2.3.0 Jul 20, 2020
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.

Wrong username account displayed in reset-password mail
3 participants