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.

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

Rename /forget_password url to /forgot_password #1219

Merged
merged 1 commit into from
Mar 14, 2017
Merged

Rename /forget_password url to /forgot_password #1219

merged 1 commit into from
Mar 14, 2017

Conversation

strk
Copy link
Member

@strk strk commented Mar 11, 2017

No description provided.

@lunny lunny added this to the 1.2.0 milestone Mar 11, 2017
@lunny lunny added the type/docs This PR mainly updates/creates documentation label Mar 11, 2017
lunny
lunny requested changes Mar 11, 2017
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

just remove the wrong line is ok

options/locale/locale_en-US.ini Outdated
@@ -166,7 +166,7 @@ disable_register_prompt = Sorry, registration has been disabled. Please contact
disable_register_mail = Sorry, Register Mail Confirmation has been disabled.
remember_me = Remember Me
forgot_password= Forgot Password
forget_password = Forgot password?
forgot_password = Forgot password?
Copy link
Member

Choose a reason for hiding this comment

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

duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed (I don't think this is kind/docs, btw)

@strk
Copy link
Member Author

strk commented Mar 11, 2017

Hold on a second, there are duplications in each and every translation :/
I'm not sure what happened. Maybe someone added a question mark after I initially made the PR, and the rebase didn't recognize it. Fixing it

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

lunny commented Mar 11, 2017

@strk every file has a duplicated line.

@strk
Copy link
Member Author

strk commented Mar 11, 2017

ok, figured. The forgot_password key is currently being used as the title for the Forgot Password page,
while the forget_password key is used for the link text.

So the new commit (replacing the others) changes forgot_password to forgot_password_title and then changes forget_password to forgot_password. Should be good now.

@bkcsoft
Copy link
Member

bkcsoft commented Mar 11, 2017

This needs a big disclaimer in the changelog 😛

@strk
Copy link
Member Author

strk commented Mar 11, 2017

@bkcsoft you seemed less concerned in #862 (comment) ? Or is this specifically for the translation keys ?

@bkcsoft
Copy link
Member

bkcsoft commented Mar 12, 2017

@strk Well, people might depend on the link, so we need a changelog entry, as for the API it doesn't care 🙂

@strk
Copy link
Member Author

strk commented Mar 12, 2017 via email

@lunny
Copy link
Member

lunny commented Mar 12, 2017

Why we need two similar strings?

@strk
Copy link
Member Author

strk commented Mar 12, 2017 via email

@lunny
Copy link
Member

lunny commented Mar 13, 2017

LGTM except @bkcsoft 's opinion

@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 Mar 13, 2017
@strk
Copy link
Member Author

strk commented Mar 13, 2017

@bkcsoft I'm ok with the changelog entry, should I add it to the current CHANGELOG, drafting the new line (and fixing conflicts as we go) ? Or we could use a [changelog:compatibility] tag in commit logs to extract later ? Or what else do you want me to do for this to be accepted ?

@bkcsoft
Copy link
Member

bkcsoft commented Mar 13, 2017

Put it in the Changelog under "Unreleased" (make a section at the top) and someone will take take of it 😉

@thibaultmeyer
Copy link
Contributor

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 13, 2017
bkcsoft
bkcsoft suggested changes Mar 13, 2017
View reviewed changes
cmd/web.go
@@ -254,8 +254,8 @@ func runWeb(ctx *cli.Context) error {
m.Any("/activate", user.Activate)
m.Any("/activate_email", user.ActivateEmail)
m.Get("/email2user", user.Email2User)
m.Get("/forget_password", user.ForgotPasswd)
m.Post("/forget_password", user.ForgotPasswdPost)
m.Get("/forgot_password", user.ForgotPasswd)
Copy link
Member

Choose a reason for hiding this comment

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

Just put a changelog somewhere (just so it can't be merged before that 😂 )

Copy link
Member Author

@strk strk Mar 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Are you @go-gitea/maintainers ok if we start dropping changelogs in files under a pending-changelogs/ directory, to be checked and cleaned up before final release ?

Please vote with thumbs up/down on this comment :)

Copy link
Member

Choose a reason for hiding this comment

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

@strk changelog/unreleased as per #1298 😉

@strk
Rename /forget_password url to /forgot_password
8bd5c90 
Also renames `forgot_password` translation key to
`forgot_password_title` and `forget_password` to
`forgot_password`

Includes entry in CHANGELOG about the breaking change
(and some markdown fixes in there)
@strk
Copy link
Member Author

strk commented Mar 13, 2017

@bkcsoft section added, and squash rebased against master

@bkcsoft bkcsoft merged commit 7d8f9d1 into go-gitea:master Mar 14, 2017
@strk strk deleted the forget-forgot branch March 15, 2017 09:09
@lunny lunny added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Aug 25, 2017
@jag3773 jag3773 mentioned this pull request Aug 30, 2017
Closed
2 tasks
@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

@bkcsoft bkcsoft bkcsoft approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/docs This PR mainly updates/creates documentation
Projects
None yet
Milestone
1.2.0
Development

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