Skip to content

Conversation

@silverwind
Copy link
Member

@silverwindsilverwind commented Dec 5, 2025

  1. Add e-mail tooltip over non-existing usernames, only for logged-in users.
  2. Use a shared template to render commit author.

@GiteaBotGiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 5, 2025
@github-actionsgithub-actionsbot added the modifies/templates This PR modifies the template files label Dec 5, 2025
@GiteaBotGiteaBot 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 Dec 5, 2025
@lafriks
Copy link
Member

I don't think showing email in the UI is a good idea, maybe only for authorized users

@silverwind
Copy link
MemberAuthor

It was already shown previously as a tooltip on the avatar in the commit view, but was not consistent.

@silverwind
Copy link
MemberAuthor

And yes, I agree it should be hidden for anonymous users, but imho thats a separate topic and we need to validate all places where we pass these addresses to the frontend.

@silverwind
Copy link
MemberAuthor

silverwind commented Dec 5, 2025

I think I could actually try to fix this at least in these 3 places, make a subtemplate for "avatar and username" and do the logged in check there, wip until then.

@silverwindsilverwind marked this pull request as draft December 5, 2025 11:25
@silverwindsilverwind changed the title Improve and unify non-existing user e-mail tooltipImprove and unify non-existing git commit user e-mail tooltipDec 5, 2025
@lunny
Copy link
Member

lunny commented Dec 5, 2025

Since the repository is public, it can be accessed freely. Anyone can clone it and obtain the email information locally anyway.

@silverwind
Copy link
MemberAuthor

I guess it has a small benefit against scrape bots.

@lafriks
Copy link
Member

Yes, GitHub also does not show emails in git for the very same reason most probably

@silverwind
Copy link
MemberAuthor

This proves harder than I thought because these templates are such a mess, there is .Author, .Commit.Author, .User and I don't know the difference or why there are three variables for the same thing.

@silverwind
Copy link
MemberAuthor

silverwind commented Dec 11, 2025

Shared template added but there is a unresolved issue on latest commit page and commit view:

Render failed, failed to render template: repo/home, error: template error: builtin(static):repo/commit_author:25:26 : executing "repo/commit_author" at <.Author.FullName>: can't evaluate field FullName in type interface{} 

if .Author.FullName crashes and I don't know how to avoid it. Maybe the template code needs a IsEmptyInterface or something?

@silverwind
Copy link
MemberAuthor

Fixed above by not passing these invalid .Author on the two affected pages. I think this is ready now.

The E-Mail adress is also no longer rendered into HTML for signed-out users.

@silverwindsilverwind marked this pull request as ready for review December 11, 2025 01:50
@silverwindsilverwind changed the title Improve and unify non-existing git commit user e-mail tooltipImprove and unify commit author renderingDec 11, 2025
@@ -0,0 +1,56 @@
{{/* TemplateAttributes:
* User: Theuserassociatedwiththecommit, ifany
* Commit: Thecommit
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you should pass so many different structs.

Commit is not really used. What you need is a "git user" or "gitea user".

Copy link
MemberAuthor

@silverwindsilverwindDec 11, 2025

Choose a reason for hiding this comment

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

Commit is used for .Commit.Author vs .Author. Maybe it can be unified, not sure. The three pages all pass different context variables.

Comment on lines +13 to +17
{{ifand.User.FullNameDefaultShowFullName}}
{{$username = .User.FullName}}
{{elseif .User.Name}}
{{$username = .User.Name}}
{{end}}
Copy link
Contributor

Choose a reason for hiding this comment

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

imageimage

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I wonder if *User will also be compatible with .Author.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if *User will also be compatible with .Author.

No. More complicated.

@silverwindsilverwind marked this pull request as draft December 11, 2025 11:45
@github-actionsgithub-actionsbot added the modifies/go Pull requests that update Go code label Dec 13, 2025
{{else}}
{{.Author.Name}}
{{end}}
</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

There are full of "if-else" blocks.

I don't think such "shared template" can be right. It is very fragile and only causes maintainability problems.

image

Copy link
MemberAuthor

@silverwindsilverwindDec 13, 2025

Choose a reason for hiding this comment

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

I don't have a full picture what the 3 variants of data being passed into here are. Deeper analysis will be needed to determine if any of this can be reduced. All your remaining comments are about this same topic. If you have a better picture of the data passed in, feel free to change it yourself.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I don't have a full picture at the moment either. The details I know were written in #24087 (comment) (commit is not really related).

I think I don't have time or motivation to do it at the moment.

If you'd like to use the "shared templates", I think you need to make it clear and maintainable, but not leave the problem to the future maintainers ....

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 1This PR needs approval from one additional maintainer to be merged.modifies/goPull requests that update Go codemodifies/templatesThis PR modifies the template files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@silverwind@lafriks@lunny@wxiaoguang@delvh@GiteaBot