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

Show the username as a fallback on feeds if full name is blank #10438

Merged
merged 4 commits into from
Feb 26, 2020

Conversation

jamesorlakin
Copy link
Contributor

@jamesorlakin jamesorlakin commented Feb 24, 2020

While the DEFAULT_SHOW_FULL_NAME works well, there's a bug on the feed/dashboard page that causes the username not be used as a fallback and nothing shown instead:
image

This PR copies the logic from DisplayName in models/user.go:741 to return the username as a fallback for the template:
image
(where review is the username, and Jimbo as a user with the full name set)

I'm happy to make a backport too. 😃

Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Not that we're the most efficient system but perhaps it would be better to restructure this so we only do the trimspace if the setting is set.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 24, 2020
@jamesorlakin
Copy link
Contributor Author

To be honest I'm not really sure what the trim is there for. I guess if the user puts in a full name of just spaces we should ignore it, but surely that validation should be done at setting time?

@codecov-io
Copy link

codecov-io commented Feb 24, 2020

Codecov Report

Merging #10438 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10438      +/-   ##
==========================================
- Coverage   43.69%   43.68%   -0.02%     
==========================================
  Files         586      586              
  Lines       81411    81411              
==========================================
- Hits        35574    35564      -10     
- Misses      41430    41437       +7     
- Partials     4407     4410       +3
Impacted Files Coverage Δ
routers/api/v1/repo/pull.go 34.63% <ø> (ø) ⬆️
routers/routes/routes.go 86.27% <100%> (ø) ⬆️
modules/indexer/stats/queue.go 62.5% <0%> (-18.75%) ⬇️
modules/indexer/stats/db.go 40.62% <0%> (-18.75%) ⬇️
modules/git/utils.go 65.67% <0%> (-4.48%) ⬇️
modules/queue/workerpool.go 56.93% <0%> (-1.07%) ⬇️
modules/git/repo.go 46.78% <0%> (-0.92%) ⬇️
models/notification.go 64.4% <0%> (+0.84%) ⬆️
modules/indexer/issues/indexer.go 58.9% <0%> (+1.36%) ⬆️

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 89639aa...c517044. Read the comment docs.

@jamesorlakin
Copy link
Contributor Author

@zeripath String trimming in both the user and action models are now done only if DEFAULT_SHOW_FULL_NAME is configured.

@GiteaBot GiteaBot 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 24, 2020
models/action.go Outdated
return a.GetActFullName()
trimmedFullName := strings.TrimSpace(a.GetActFullName())
if len(trimmedFullName) > 0 {
return a.GetActFullName()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return a.GetActFullName()
return trimmedFullName

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doh! 🤦‍♂

@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Feb 24, 2020
@lafriks lafriks added this to the 1.12.0 milestone Feb 24, 2020
@jamesorlakin jamesorlakin force-pushed the bugfix/feedUsernameFallback branch 2 times, most recently from 1ff004c to 3e6af9f Compare February 24, 2020 18:06
@GiteaBot GiteaBot 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 24, 2020
@zeripath zeripath merged commit 7ffc242 into go-gitea:master Feb 26, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

6 participants