Skip to content

Show a link to password reset from user settings requiring a password #862

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2017
Merged

Show a link to password reset from user settings requiring a password #862

merged 1 commit into from
Mar 11, 2017

Conversation

strk
Copy link
Member

@strk strk commented Feb 7, 2017

... helpful, when you forgot your password thus cannot change it

@strk strk mentioned this pull request Feb 7, 2017
Merged
@strk strk changed the title Show a link to password reset from password change Show a link to password reset from user settings requiring a password Feb 7, 2017
@strk
Copy link
Member Author

strk commented Feb 7, 2017

btw, I'd also change the path, from current /forget_password to the more correct /forgot_password, what do you think @bkcsoft, @lunny, @tboerger ? Would that break any API ?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 7, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Feb 12, 2017

@strk Fine by me, it's not part of the API so I'm not that worried about it... just make sure that all references to it are changed :)

@strk strk force-pushed the forgot-password-on-change branch from 2ca58cd to 51c57f9 Compare February 20, 2017 18:27
@strk
Copy link
Member Author

strk commented Feb 20, 2017

@bkcsoft done and rebased. Can we get this into 1.1.0 ?

@lunny
Copy link
Member

lunny commented Feb 21, 2017

please reset all non-english translation changes since we use crowdin

@strk
Copy link
Member Author

strk commented Feb 21, 2017

I only changed the keys, not the translations. Is this something crowdin also does automatically ?

@lunny
Copy link
Member

lunny commented Feb 21, 2017

I don't know in fact. I'm not familiar with crowdin.

@strk
Copy link
Member Author

strk commented Feb 22, 2017 via email

@strk strk force-pushed the forgot-password-on-change branch from 51c57f9 to 155cace Compare February 22, 2017 11:18
@strk
Copy link
Member Author

strk commented Feb 22, 2017

Rebased to current master.

@strk
Copy link
Member Author

strk commented Feb 22, 2017

@lunny will I have your LGTM if I remove the forget -> forgot commit ? I don't want that to hold up the new link. Note I was asked by @bkcsoft to do that rename...

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 22, 2017
@lunny
Copy link
Member

lunny commented Feb 22, 2017

I think two problems:

  1. you should drop all the non-en translation so that we can sync with crowdin.
  2. It seems this is two PR. One is for typo, another is for put the link on the UI.

And I don't know why we should put the reset password link on the UI

@lunny lunny added this to the 1.2.0 milestone Feb 22, 2017
@strk strk force-pushed the forgot-password-on-change branch from 155cace to 1648c72 Compare February 22, 2017 16:36
@strk
Copy link
Member Author

strk commented Feb 22, 2017 via email

@strk strk force-pushed the forgot-password-on-change branch from 1648c72 to ea2af94 Compare February 23, 2017 19:30
@strk
Copy link
Member Author

strk commented Feb 23, 2017

Why does this go to 1.2 ? I rebased it, it's really a simple change, just an additional link where password is requested (and no more change to localization files)

@lunny
Copy link
Member

lunny commented Feb 24, 2017

@strk I think OpenID and this one could be in v1.2 since we have already delayed. I have moved many features to v1.2.

@tboerger tboerger added the type/enhancement An improvement of existing functionality label Feb 27, 2017
@strk
Show a link to password reset from password change and delete account
20b7ca8 
It's helpful when you forgot your password thus cannot change it
(can happen if you log in via OAuth2 or OpenID)

Also make sure that both the delete-account and password-change
links to forgot-password will have the primary email pre-filled
@strk strk force-pushed the forgot-password-on-change branch from ea2af94 to 20b7ca8 Compare March 7, 2017 16:39
@strk
Copy link
Member Author

strk commented Mar 10, 2017

Can this be merged now that 1.1.0 is out ? Please review :)

@strk
Copy link
Member Author

strk commented Mar 10, 2017

@lunny do I have your L-G-T-M then ? How about you @bkcsoft ? I have local change for the forget->forgot change waiting for this to be merged too

@lunny
Copy link
Member

lunny commented Mar 11, 2017

@strk don't know why we need put a link on this UI.

@strk
Copy link
Member Author

strk commented Mar 11, 2017

@lunny every single time you're asked for a password, it makes sense to me to provide a forgot password? link. Don't you agree ? GitHub does it, as far as I can tell.

@lunny
Copy link
Member

lunny commented Mar 11, 2017

trusted LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 11, 2017
@lunny lunny merged commit 8a98a25 into go-gitea:master Mar 11, 2017
@strk strk deleted the forgot-password-on-change branch March 11, 2017 09:44
@strk strk mentioned this pull request Mar 11, 2017
Merged
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@strk @bkcsoft @lunny @tboerger