Fix org contact email not clearable once set

modules/structs/org.go

@@ -66,23 +66,21 @@ type CreateOrgOption struct {
 	RepoAdminChangeTeamAccess bool `json:"repo_admin_change_team_access"`
 }
 
-// TODO: make EditOrgOption fields optional after https://gitea.com/go-chi/binding/pulls/5 got merged
-
 // EditOrgOption options for editing an organization
 type EditOrgOption struct {
 	// The full display name of the organization
-	FullName string `json:"full_name" binding:"MaxSize(100)"`
-	// The email address of the organization
-	Email string `json:"email" binding:"MaxSize(255)"`
+	FullName *string `json:"full_name" binding:"MaxSize(100)"`
+	// The email address of the organization; use empty string to clear
+	Email *string `json:"email" binding:"MaxSize(255)"`
 	// The description of the organization
-	Description string `json:"description" binding:"MaxSize(255)"`
+	Description *string `json:"description" binding:"MaxSize(255)"`
 	// The website URL of the organization
-	Website string `json:"website" binding:"ValidUrl;MaxSize(255)"`
+	Website *string `json:"website" binding:"ValidUrl;MaxSize(255)"`
 	// The location of the organization
-	Location string `json:"location" binding:"MaxSize(50)"`
+	Location *string `json:"location" binding:"MaxSize(50)"`
 	// possible values are `public`, `limited` or `private`
 	// enum: public,limited,private
-	Visibility string `json:"visibility" binding:"In(,public,limited,private)"`
+	Visibility *string `json:"visibility" binding:"In(,public,limited,private)"`
 	// Whether repository administrators can change team access
 	RepoAdminChangeTeamAccess *bool `json:"repo_admin_change_team_access"`
 }

routers/api/v1/org/org.go

@@ -5,6 +5,7 @@
 package org
 
 import (
+	"errors"
 	"net/http"
 
 	activities_model "code.gitea.io/gitea/models/activities"
@@ -14,6 +15,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/optional"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web"
 	"code.gitea.io/gitea/routers/api/v1/user"
 	"code.gitea.io/gitea/routers/api/v1/utils"
@@ -379,19 +381,21 @@ func Edit(ctx *context.APIContext) {
 
 	form := web.GetForm(ctx).(*api.EditOrgOption)
 
-	if form.Email != "" {
-		if err := user_service.ReplacePrimaryEmailAddress(ctx, ctx.Org.Organization.AsUser(), form.Email); err != nil {
-			ctx.APIErrorInternal(err)
+	if err := org.UpdateOrgEmailAddress(ctx, ctx.Org.Organization, form.Email); err != nil {
+		if errors.Is(err, util.ErrInvalidArgument) {
+			ctx.APIError(http.StatusUnprocessableEntity, err)
 			return
 		}
+		ctx.APIErrorInternal(err)
+		return
 	}
 
 	opts := &user_service.UpdateOptions{
-		FullName:                  optional.Some(form.FullName),
-		Description:               optional.Some(form.Description),
-		Website:                   optional.Some(form.Website),
-		Location:                  optional.Some(form.Location),
-		Visibility:                optional.FromMapLookup(api.VisibilityModes, form.Visibility),
+		FullName:                  optional.FromPtr(form.FullName),
+		Description:               optional.FromPtr(form.Description),
+		Website:                   optional.FromPtr(form.Website),
+		Location:                  optional.FromPtr(form.Location),
+		Visibility:                optional.FromMapLookup(api.VisibilityModes, optional.FromPtr(form.Visibility).Value()),
 		RepoAdminChangeTeamAccess: optional.FromPtr(form.RepoAdminChangeTeamAccess),
 	}
 	if err := user_service.UpdateUser(ctx, ctx.Org.Organization.AsUser(), opts); err != nil {

routers/web/org/setting.go

@@ -5,6 +5,7 @@
 package org
 
 import (
+	"errors"
 	"net/http"
 	"net/url"
 
@@ -69,24 +70,25 @@ func SettingsPost(ctx *context.Context) {
 	}
 
 	org := ctx.Org.Organization
-
-	if form.Email != "" {
-		if err := user_service.ReplacePrimaryEmailAddress(ctx, org.AsUser(), form.Email); err != nil {
+	if err := org_service.UpdateOrgEmailAddress(ctx, org, form.Email); err != nil {
+		if errors.Is(err, util.ErrInvalidArgument) {
 			ctx.Data["Err_Email"] = true
 			ctx.RenderWithErrDeprecated(ctx.Tr("form.email_invalid"), tplSettingsOptions, &form)
 			return
 		}
+		ctx.ServerError("UpdateOrgEmailAddress", err)
+		return
 	}
 
 	opts := &user_service.UpdateOptions{
-		FullName:                  optional.Some(form.FullName),
-		Description:               optional.Some(form.Description),
-		Website:                   optional.Some(form.Website),
-		Location:                  optional.Some(form.Location),
-		RepoAdminChangeTeamAccess: optional.Some(form.RepoAdminChangeTeamAccess),
+		FullName:                  optional.FromPtr(form.FullName),
+		Description:               optional.FromPtr(form.Description),
+		Website:                   optional.FromPtr(form.Website),
+		Location:                  optional.FromPtr(form.Location),
+		RepoAdminChangeTeamAccess: optional.FromPtr(form.RepoAdminChangeTeamAccess),
 	}
 	if ctx.Doer.IsAdmin {
-		opts.MaxRepoCreation = optional.Some(form.MaxRepoCreation)
+		opts.MaxRepoCreation = optional.FromPtr(form.MaxRepoCreation)
 	}
 
 	if err := user_service.UpdateUser(ctx, org.AsUser(), opts); err != nil {

services/forms/org.go

@@ -36,13 +36,13 @@ func (f *CreateOrgForm) Validate(req *http.Request, errs binding.Errors) binding
 
 // UpdateOrgSettingForm form for updating organization settings
 type UpdateOrgSettingForm struct {
-	FullName                  string `binding:"MaxSize(100)"`
-	Email                     string `binding:"MaxSize(255)"`
-	Description               string `binding:"MaxSize(255)"`
-	Website                   string `binding:"ValidUrl;MaxSize(255)"`
-	Location                  string `binding:"MaxSize(50)"`
-	MaxRepoCreation           int
-	RepoAdminChangeTeamAccess bool
+	FullName                  *string `binding:"MaxSize(100)"`
+	Email                     *string `binding:"MaxSize(255)"`
+	Description               *string `binding:"MaxSize(255)"`
+	Website                   *string `binding:"ValidUrl;MaxSize(255)"`
+	Location                  *string `binding:"MaxSize(50)"`
+	MaxRepoCreation           *int
+	RepoAdminChangeTeamAccess *bool
 }
 
 // Validate validates the fields

services/org/org.go

@@ -168,3 +168,20 @@ func ChangeOrganizationVisibility(ctx context.Context, org *org_model.Organizati
 		return nil
 	})
 }
+
+// UpdateOrgEmailAddress validates and updates the organization's contact email.
+// A nil email means no change.
+func UpdateOrgEmailAddress(ctx context.Context, org *org_model.Organization, email *string) error {
+	if email == nil {
+		return nil
+	}
+
+	if *email != "" {
+		if err := user_model.ValidateEmail(*email); err != nil {
+			return err
+		}
+	}
+
+	org.Email = *email
+	return user_model.UpdateUserCols(ctx, org.AsUser(), "email")
+}

services/org/org_test.go

@@ -10,28 +10,54 @@ import (
 	repo_model "code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/models/unittest"
 	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestMain(m *testing.M) {
 	unittest.MainTest(m)
 }
 
-func TestDeleteOrganization(t *testing.T) {
-	assert.NoError(t, unittest.PrepareTestDatabase())
-	org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6})
-	assert.NoError(t, DeleteOrganization(t.Context(), org, false))
-	unittest.AssertNotExistsBean(t, &organization.Organization{ID: 6})
-	unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: 6})
-	unittest.AssertNotExistsBean(t, &organization.Team{OrgID: 6})
-
-	org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
-	err := DeleteOrganization(t.Context(), org, false)
-	assert.Error(t, err)
-	assert.True(t, repo_model.IsErrUserOwnRepos(err))
-
-	user := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 5})
-	assert.Error(t, DeleteOrganization(t.Context(), user, false))
-	unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
+func TestOrg(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	t.Run("UpdateOrgEmailAddress", func(t *testing.T) {
+		org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+		originalEmail := org.Email
+
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, nil))
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: originalEmail})
+
+		newEmail := "contact@org3.example.com"
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, &newEmail))
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: newEmail})
+
+		invalidEmail := "invalid email"
+		err := UpdateOrgEmailAddress(t.Context(), org, &invalidEmail)
+		require.ErrorIs(t, err, util.ErrInvalidArgument)
+		unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: newEmail})
+
+		require.NoError(t, UpdateOrgEmailAddress(t.Context(), org, new("")))
+		org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3, Email: ""})
+		assert.Empty(t, org.Email)
+	})
+
+	t.Run("DeleteOrganization", func(t *testing.T) {
+		org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 6})
+		assert.NoError(t, DeleteOrganization(t.Context(), org, false))
+		unittest.AssertNotExistsBean(t, &organization.Organization{ID: 6})
+		unittest.AssertNotExistsBean(t, &organization.OrgUser{OrgID: 6})
+		unittest.AssertNotExistsBean(t, &organization.Team{OrgID: 6})
+
+		org = unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+		err := DeleteOrganization(t.Context(), org, false)
+		assert.Error(t, err)
+		assert.True(t, repo_model.IsErrUserOwnRepos(err))
+
+		user := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 5})
+		assert.Error(t, DeleteOrganization(t.Context(), user, false))
+		unittest.CheckConsistencyFor(t, &user_model.User{}, &organization.Team{})
+	})
 }

tests/integration/api_org_test.go

@@ -138,33 +138,53 @@ func TestAPIOrgGeneral(t *testing.T) {
 
 	t.Run("OrgEdit", func(t *testing.T) {
 		org := api.EditOrgOption{
-			FullName:    "Org3 organization new full name",
-			Description: "A new description",
-			Website:     "https://try.gitea.io/new",
-			Location:    "Beijing",
-			Visibility:  "private",
+			FullName:    new("new full name"),
+			Description: new("new description"),
+			Website:     new("https://org3-new-website.example.com"),
+			Location:    new("new location"),
+			Visibility:  new("private"),
 		}
 		req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token)
 		resp := MakeRequest(t, req, http.StatusOK)
-
-		var apiOrg api.Organization
-		DecodeJSON(t, resp, &apiOrg)
+		apiOrg := DecodeJSON(t, resp, &api.Organization{})
 
 		assert.Equal(t, "org3", apiOrg.Name)
-		assert.Equal(t, org.FullName, apiOrg.FullName)
-		assert.Equal(t, org.Description, apiOrg.Description)
-		assert.Equal(t, org.Website, apiOrg.Website)
-		assert.Equal(t, org.Location, apiOrg.Location)
-		assert.Equal(t, org.Visibility, apiOrg.Visibility)
+		assert.Equal(t, *org.FullName, apiOrg.FullName)
+		assert.Equal(t, *org.Description, apiOrg.Description)
+		assert.Equal(t, *org.Website, apiOrg.Website)
+		assert.Equal(t, *org.Location, apiOrg.Location)
+		assert.Equal(t, *org.Visibility, apiOrg.Visibility)
+	})
+
+	t.Run("OrgEdit", func(t *testing.T) {
+		org3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "org3"})
+		assert.NotEqual(t, api.VisibleTypeLimited, org3.Visibility)
+
+		// try to update some fields
+		req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &api.EditOrgOption{
+			Email:      new("org3-new-email@example.com"),
+			Visibility: new("limited"),
+		}).AddTokenAuth(user1Token)
+		resp := MakeRequest(t, req, http.StatusOK)
+		apiOrg := DecodeJSON(t, resp, &api.Organization{})
+		assert.Equal(t, "org3-new-email@example.com", apiOrg.Email)
+		assert.Equal(t, "limited", apiOrg.Visibility)
+		org3 = unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "org3"})
+		assert.Equal(t, api.VisibleTypeLimited, org3.Visibility)
+
+		// empty email can clear the email, nil fields won't change the settings
+		req = NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &api.EditOrgOption{
+			Email: new(""),
+		}).AddTokenAuth(user1Token)
+		resp = MakeRequest(t, req, http.StatusOK)
+		apiOrg = DecodeJSON(t, resp, &api.Organization{})
+		assert.Empty(t, apiOrg.Email)
+		assert.Equal(t, "limited", apiOrg.Visibility)
 	})
 
-	t.Run("OrgEditBadVisibility", func(t *testing.T) {
+	t.Run("OrgEditInvalidVisibility", func(t *testing.T) {
 		org := api.EditOrgOption{
-			FullName:    "Org3 organization new full name",
-			Description: "A new description",
-			Website:     "https://try.gitea.io/new",
-			Location:    "Beijing",
-			Visibility:  "badvisibility",
+			Visibility: new("invalid-visibility"),
 		}
 		req := NewRequestWithJSON(t, "PATCH", "/api/v1/orgs/org3", &org).AddTokenAuth(user1Token)
 		MakeRequest(t, req, http.StatusUnprocessableEntity)

tests/integration/integration_test.go

@@ -410,12 +410,13 @@ func logUnexpectedResponse(t testing.TB, recorder *httptest.ResponseRecorder) {
 	}
 }
 
-func DecodeJSON(t testing.TB, resp *httptest.ResponseRecorder, v any) {
+func DecodeJSON[T any](t testing.TB, resp *httptest.ResponseRecorder, v T) (ret T) {
 	t.Helper()
 
 	// FIXME: JSON-KEY-CASE: for testing purpose only, because many structs don't provide `json` tags, they just use capitalized field names
 	decoder := json.NewDecoderCaseInsensitive(resp.Body)
 	require.NoError(t, decoder.Decode(v))
+	return v
 }
 
 func VerifyJSONSchema(t testing.TB, resp *httptest.ResponseRecorder, schemaFile string) {