Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Aug 18, 2025

Closes #13990

Disclaimer: This commit contains code from Forgejo, but is from a pull request that pre-dates the switch to the GPLv3 license.

Closes #13990

Disclaimer: This commit contains code from Forgejo,
but is from a pull request that pre-dates the switch
to the GPLv3 license.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 18, 2025
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Aug 18, 2025
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Thank you for submitting a PR for this. Usually, a new field on a struct would require a new migration, however since we are trying to limit the number of new columns added to common tables (such as user), the approach described in the ticket for this would be to use the user_settings table, and store/fetch the data from there (if key/row doesn't exist for a user, then we can treat it as not being set, otherwise we can use the value).

You can see an example of the user_settings table in use here:

gitea/models/user/user.go

Lines 155 to 160 in 57b8441

type Meta struct {
// Store the initial registration of the user, to aid in spam prevention
// Ensure that one IP isn't creating many accounts (following mediawiki approach)
InitialIP string
InitialUserAgent string
}

https://github.com/go-gitea/gitea/blob/main/models/user/user.go#L774-L788

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 18, 2025
@ghost ghost closed this Aug 18, 2025
@ghost
Copy link
Author

ghost commented Aug 18, 2025

Most of the code in this PR is from https://codeberg.org/forgejo/forgejo/pulls/1518, more precisely patches 1 and 20, for future reference.

@ghost ghost reopened this Aug 18, 2025
@ghost
Copy link
Author

ghost commented Aug 18, 2025

It appears that much of the similar user information is stored in user. Putting this into user_settings would feel out of place.

Consider rereviewing this.

@ghost
Copy link
Author

ghost commented Aug 18, 2025

I've also tested this code by upgrading from the latest stable Gitea to this patched version, and it appears no form of migration was needed. By default no users have pronouns, and each one can manually add their own just fine.

@ghost
Copy link
Author

ghost commented Aug 19, 2025

In theory this should be as seamless as the location feature, which has existed for many years, and I'm not aware of any complaints because it was added.

@lunny
Copy link
Member

lunny commented Aug 19, 2025

It appears that much of the similar user information is stored in user. Putting this into user_settings would feel out of place.

Consider rereviewing this.

But then the table will continue to grow larger and larger. All non-indexed columns can be stored in user_settings, which avoids the need for migrations.

Migrations are only required when introducing a new column. Otherwise, upgrades from older versions would break.

@ghost ghost closed this by deleting the head repository Aug 19, 2025
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] Optional pronoun field in profile

3 participants