Skip to content

Fix missing translations when updating username. #3564

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 5 commits into from
Feb 25, 2018
Merged

Fix missing translations when updating username. #3564

merged 5 commits into from
Feb 25, 2018

Conversation

bugreport0
Copy link
Contributor

Gitea tries to display non-existing translation strings after failing to update a username on the 'Your Settings' page. This PR (hopefully) fixes three error cases. I'm no Go expert and I can't test this, but I think it should work.

@bugreport0 bugreport0 mentioned this pull request Feb 22, 2018
Closed
@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 23, 2018
@bugreport0
Copy link
Contributor Author

Unit tests fail for this PR because they are hardcoded to look for the faulty newName_ strings in the output. The test report shows the correct translation being served now. I don't know how to fix this. Can someone update TestRenameReservedUsername?

@lafriks
Copy link
Member

lafriks commented Feb 23, 2018

Tests should be updated also in this PR

@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Feb 23, 2018
@lafriks lafriks added this to the 1.5.0 milestone Feb 23, 2018
sapk
sapk suggested changes Feb 23, 2018
View reviewed changes
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

LGTM.
Just need to also update

gitea/integrations/user_test.go

Line 95 in befa744

i18n.Tr("en", "user.newName_reserved"),

@bugreport0
Copy link
Contributor Author

bugreport0 commented Feb 24, 2018 •
edited
Loading

@lafriks @sapk I updated my PR branch. Let's see if it still explodes.

Edit: Help. Apparently i18n.Tr in the unit test doesn't format string parameters like ctx.Tr does in the actual code. Can you give a hint? (I never programmed in Go before.)

sapk
sapk reviewed Feb 24, 2018
View reviewed changes
integrations/user_test.go Outdated
@@ -92,7 +92,7 @@ func TestRenameReservedUsername(t *testing.T) {
htmlDoc := NewHTMLParser(t, resp.Body)
assert.Contains(t,
htmlDoc.doc.Find(".ui.negative.message").Text(),
i18n.Tr("en", "user.newName_reserved"),
i18n.Tr("en", "form.name_reserved", reservedUsername),
Copy link
Member

@sapk sapk Feb 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should be i18n.Tr("en", "user.form.name_reserved", reservedUsername)
You just forgot the user.

Copy link
Contributor Author

@bugreport0 bugreport0 Feb 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I did your exact change, but that fails because i18n.Tr doesn't parse the %s format specifier in the translation string. See the CI report.

Copy link
Member

@sapk sapk Feb 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

No you write form.name_reserved not user.form.name_reserved

Copy link
Member

Choose a reason for hiding this comment

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

If it can find the string in translations it will default to the trad name so in your case name_reserved or previously newName_reserved and add all extra string that are not in the translated format after.

Copy link
Contributor Author

@bugreport0 bugreport0 Feb 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@sapk Thanks for the coaching, I've updated my branch again. Waiting for the CI now.

Edit: Bring out the champagne. It works!

@codecov-io
Copy link

Codecov Report

Merging #3564 into master will not change coverage.
The diff coverage is 33.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3564   +/-   ##
=======================================
  Coverage   35.79%   35.79%           
=======================================
  Files         285      285           
  Lines       40866    40866           
=======================================
  Hits        14630    14630           
- Misses      24066    24067    +1     
+ Partials     2170     2169    -1
Impacted Files Coverage Δ
routers/user/setting.go 24.75% <33.33%> (ø) ⬆️
models/repo_indexer.go 48.3% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc265b0...8c15ee9. Read the comment docs.

@bugreport0
Copy link
Contributor Author

Thanks to @sapk coaching everything now seems to be in order.

@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 Feb 25, 2018
sapk
sapk approved these changes Feb 25, 2018
View reviewed changes
Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

Thanks for the contrib.

@lafriks lafriks merged commit 521945a into go-gitea:master Feb 25, 2018
@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

@appleboy appleboy appleboy approved these changes

@lafriks lafriks lafriks approved these changes

@sapk sapk sapk 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. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
6 participants
@bugreport0 @lafriks @codecov-io @appleboy @sapk @tboerger