Skip to content

Fix org contact email not clearable once set#36975

Merged
wxiaoguang merged 12 commits into
go-gitea:mainfrom
bircni:fix/unset-org-email
Mar 25, 2026
Merged

Fix org contact email not clearable once set#36975
wxiaoguang merged 12 commits into
go-gitea:mainfrom
bircni:fix/unset-org-email

Conversation

@bircni
Copy link
Copy Markdown
Member

@bircni bircni commented Mar 24, 2026

When the email field was submitted as empty in org settings (web and API), the previous guard if form.Email != "" silently skipped the update, making it impossible to remove a contact email after it was set.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 24, 2026
@silverwind silverwind requested a review from Copilot March 24, 2026 16:42

This comment was marked as off-topic.

@lunny lunny added the type/bug label Mar 24, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 24, 2026

Actually .... you only need a few lines like

Email *string `json:"email" binding:"MaxSize(255)"`  (or Optional)

if form.Email != nil {
}

and you don't need that complex tests .....

AI really tends to make things unnecessary complex.

@wxiaoguang
Copy link
Copy Markdown
Contributor

Dismissed the nonsense AI reviews #36975 (review)

AI vs AI, more slop

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Mar 24, 2026

I am Not ready 😭 it's still WIP

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 24, 2026

I have told @silverwind many times: don't ask AI to review if the PR is not ready or good enough, for example:

It only wastes time.

@silverwind
Copy link
Copy Markdown
Member

I use those reviews to get a general feeling about a PR. Copilot reviews are not to be taken too seriously, often it latches onto useless stuff.

@wxiaoguang
Copy link
Copy Markdown
Contributor

I use those reviews to get a general feeling about a PR. Copilot reviews are not to be taken too seriously, often it latches onto useless stuff.

But almost all authors have been misled by your AI reviews, the latest one, 10 minutes ago:

Please you tell authors that don't trust the AI reviews, don't follow the AI slop

@wxiaoguang
Copy link
Copy Markdown
Contributor

I use those reviews to get a general feeling about a PR. Copilot reviews are not to be taken too seriously, often it latches onto useless stuff.

You can also just use your local AI tools to review, don't pollute the GitHub review history.

@bircni bircni force-pushed the fix/unset-org-email branch from 7d36e2a to b8e050e Compare March 24, 2026 17:56
@bircni bircni marked this pull request as ready for review March 24, 2026 17:56
@bircni
Copy link
Copy Markdown
Member Author

bircni commented Mar 24, 2026

@wxiaoguang what do you think - is that ok?

Comment thread routers/api/v1/org/org.go Outdated
Comment thread services/user/email.go Outdated
@bircni bircni marked this pull request as draft March 24, 2026 18:12
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 24, 2026

Can we have a common function like user_service.UpdateOrgEmailAddress(ctx, org, *form.Email)?

(Forget whether there is "org_service" package, if there is, it's the better package for the function)

@bircni
Copy link
Copy Markdown
Member Author

bircni commented Mar 24, 2026

Can we have a common function like user_service.UpdateOrgEmailAddress(ctx, org, *form.Email)?

(Forget whether there is "org_service" package, if there is, it's the better package for the function)

done yes it exists

@bircni bircni marked this pull request as ready for review March 24, 2026 20:18
@wxiaoguang
Copy link
Copy Markdown
Contributor

Made more changes and fixed TODO: make EditOrgOption fields optional after https://gitea.com/go-chi/binding/pulls/5 got merged

@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 Mar 25, 2026
@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 Mar 25, 2026
@lunny lunny added this to the 1.26.0 milestone Mar 25, 2026
@bircni
Copy link
Copy Markdown
Member Author

bircni commented Mar 25, 2026

@lunny can we Backport?

@wxiaoguang
Copy link
Copy Markdown
Contributor

It should focus on releasing 1.26

If you really need to backport:

  • If there is no conflict, then backport by cherrypick
  • If there are conflicts, backport doesn't need to be the same as the complete fix, you can only backport the minimal change. e.g.: only change a few lines.

image

@wxiaoguang wxiaoguang merged commit e24c3f7 into go-gitea:main Mar 25, 2026
26 checks passed
@bircni bircni deleted the fix/unset-org-email branch March 25, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants