Enhancement: Align gravatar and federated avatar to enable in Admin/Configuration

models/avatars/avatar.go

@@ -216,8 +216,8 @@ func generateEmailAvatarLink(ctx context.Context, email string, size int, final
 		return urlStr
 	}
 
-	disableGravatar := setting.Config().Picture.DisableGravatar.Value(ctx)
-	if !disableGravatar {
+	enableGravatar := setting.Config().Picture.EnableGravatar.Value(ctx)
+	if enableGravatar {
 		// copy GravatarSourceURL, because we will modify its Path.
 		avatarURLCopy := *avatarSetting.gravatarSourceURL
 		avatarURLCopy.Path = path.Join(avatarURLCopy.Path, HashEmail(email))

models/avatars/avatar_test.go

@@ -19,12 +19,14 @@ const gravatarSource = "https://secure.gravatar.com/avatar/"
 func disableGravatar(t *testing.T) {
 	err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableFederatedAvatar.DynKey(): "false"})
 	assert.NoError(t, err)
-	err = system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.DisableGravatar.DynKey(): "true"})
+	// EnableGravatar.SelectFrom == picture.disable_gravatar for backwards compatibility; .Value will flip correctly but the true value here is counterintuitive
+	err = system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableGravatar.SelectFromKey(): "true"})
 	assert.NoError(t, err)
 }
 
 func enableGravatar(t *testing.T) {
-	err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.DisableGravatar.DynKey(): "false"})
+	// EnableGravatar.SelectFrom == picture.disable_gravatar for backwards compatibility; .Value will flip correctly but the false value here is counterintuitive
+	err := system_model.SetSettings(t.Context(), map[string]string{setting.Config().Picture.EnableGravatar.SelectFromKey(): "false"})
 	assert.NoError(t, err)
 	setting.GravatarSource = gravatarSource
 }

models/system/setting.go

@@ -150,3 +150,34 @@ func (d *dbConfigCachedGetter) InvalidateCache() {
 func NewDatabaseDynKeyGetter() config.DynKeyGetter {
 	return &dbConfigCachedGetter{}
 }
+
+type dbConfigSetter struct {
+	mu sync.RWMutex
+}
+
+var _ config.DynKeySetter = (*dbConfigSetter)(nil)
+
+func (d *dbConfigSetter) SetValue(ctx context.Context, dynKey, value string) error {
+	d.mu.RLock()
+	defer d.mu.RUnlock()
+	_ = GetRevision(ctx) // prepare the "revision" key ahead
+	return db.WithTx(ctx, func(ctx context.Context) error {
+		e := db.GetEngine(ctx)
+		res, err := e.Exec("UPDATE system_setting SET version=version+1, setting_value=? WHERE setting_key=?", value, dynKey)
+		if err != nil {
+			return err
+		}
+		rows, _ := res.RowsAffected()
+		if rows == 0 { // if no existing row, insert a new row
+			if _, err = e.Insert(&Setting{SettingKey: dynKey, SettingValue: value}); err != nil {
+				return err
+			}
+		}
+
+		return nil
+	})
+}
+
+func NewDatabaseDynKeySetter() config.DynKeySetter {
+	return &dbConfigSetter{}
+}

models/user/avatar.go

@@ -69,12 +69,12 @@ func (u *User) AvatarLinkWithSize(ctx context.Context, size int) string {
 	useLocalAvatar := false
 	autoGenerateAvatar := false
 
-	disableGravatar := setting.Config().Picture.DisableGravatar.Value(ctx)
+	enableGravatar := setting.Config().Picture.EnableGravatar.Value(ctx)
 
 	switch {
-	case u.UseCustomAvatar:
+	case u.UseCustomAvatar, enableGravatar:
 		useLocalAvatar = true
-	case disableGravatar, setting.OfflineMode:
+	case setting.OfflineMode:
 		useLocalAvatar = true
 		autoGenerateAvatar = true
 	}

modules/setting/config.go

@@ -11,7 +11,7 @@ import (
 )
 
 type PictureStruct struct {
-	DisableGravatar       *config.Value[bool]
+	EnableGravatar        *config.Value[bool]
 	EnableFederatedAvatar *config.Value[bool]
 }
 
@@ -66,7 +66,7 @@ func initDefaultConfig() {
 	config.SetCfgSecKeyGetter(&cfgSecKeyGetter{})
 	defaultConfig = &ConfigStruct{
 		Picture: &PictureStruct{
-			DisableGravatar:       config.ValueJSON[bool]("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}),
+			EnableGravatar:        config.ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "DISABLE_GRAVATAR"}).Invert(),
 			EnableFederatedAvatar: config.ValueJSON[bool]("picture.enable_federated_avatar").WithFileConfig(config.CfgSecKey{Sec: "picture", Key: "ENABLE_FEDERATED_AVATAR"}),
 		},
 		Repository: &RepositoryStruct{

modules/setting/config/setter.go

@@ -0,0 +1,29 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package config
+
+import (
+	"context"
+	"sync"
+)
+
+var setterMu sync.RWMutex
+
+type DynKeySetter interface {
+	SetValue(ctx context.Context, dynKey, value string) error
+}
+
+var dynKeySetterInternal DynKeySetter
+
+func SetDynSetter(p DynKeySetter) {
+	setterMu.Lock()
+	dynKeySetterInternal = p
+	setterMu.Unlock()
+}
+
+func GetDynSetter() DynKeySetter {
+	getterMu.RLock()
+	defer getterMu.RUnlock()
+	return dynKeySetterInternal
+}

modules/setting/config/value.go

@@ -19,11 +19,12 @@ type CfgSecKey struct {
 type Value[T any] struct {
 	mu sync.RWMutex
 
-	cfgSecKey CfgSecKey
-	dynKey    string
+	cfgSecKey             CfgSecKey
+	dynKey, selectFromKey string
 
-	def, value T
-	revision   int
+	def, value  T
+	revision    int
+	flipBoolean bool
 }
 
 func (value *Value[T]) parse(key, valStr string) (v T) {
@@ -33,9 +34,41 @@ func (value *Value[T]) parse(key, valStr string) (v T) {
 			log.Error("Unable to unmarshal json config for key %q, err: %v", key, err)
 		}
 	}
+
+	return value.invert(v)
+}
+
+func (value *Value[T]) invertBoolStr(val string) (inverted string) {
+	if val == "true" {
+		return "false"
+	}
+
+	return "true"
+}
+
+func (value *Value[T]) invert(val T) (v T) {
+	v = val
+	if value.flipBoolean {
+		// if value is of type bool
+		if _, ok := any(val).(bool); ok {
+			// invert the boolean value upon retrieval
+			v = any(!any(val).(bool)).(T)
+		} else {
+			log.Warn("Ignoring attempt to invert key '%q' for non boolean type", value.selectFromKey)
+		}
+	}
+
 	return v
 }
 
+func (value *Value[T]) getKey() string {
+	if value.selectFromKey != "" {
+		return value.selectFromKey
+	}
+
+	return value.dynKey
+}
+
 func (value *Value[T]) Value(ctx context.Context) (v T) {
 	dg := GetDynGetter()
 	if dg == nil {
@@ -57,7 +90,7 @@ func (value *Value[T]) Value(ctx context.Context) (v T) {
 
 	// try to parse the config and cache it
 	var valStr *string
-	if dynVal, has := dg.GetValue(ctx, value.dynKey); has {
+	if dynVal, has := dg.GetValue(ctx, value.getKey()); has {
 		valStr = &dynVal
 	} else if cfgVal, has := GetCfgSecKeyGetter().GetValue(value.cfgSecKey.Sec, value.cfgSecKey.Key); has {
 		valStr = &cfgVal
@@ -79,6 +112,10 @@ func (value *Value[T]) DynKey() string {
 	return value.dynKey
 }
 
+func (value *Value[T]) SelectFromKey() string {
+	return value.selectFromKey
+}
+
 func (value *Value[T]) WithDefault(def T) *Value[T] {
 	value.def = def
 	return value
@@ -93,6 +130,32 @@ func (value *Value[T]) WithFileConfig(cfgSecKey CfgSecKey) *Value[T] {
 	return value
 }
 
+func (value *Value[bool]) Invert() *Value[bool] {
+	value.flipBoolean = true
+	return value
+}
+
+func (value *Value[any]) SelectFrom(sectionName string) *Value[any] {
+	value.selectFromKey = sectionName
+	return value
+}
+
+func (value *Value[any]) SetValue(val string) error {
+	ctx := context.Background()
+	ds := GetDynSetter()
+	if ds == nil {
+		// this is an edge case: the database is not initialized but the system setting is going to be used
+		// it should panic to avoid inconsistent config values (from config / system setting) and fix the code
+		panic("no config dyn value getter")
+	}
+
+	if value.flipBoolean {
+		return ds.SetValue(ctx, value.getKey(), value.invertBoolStr(val))
+	}
+
+	return ds.SetValue(ctx, value.getKey(), val)
+}
+
 func ValueJSON[T any](dynKey string) *Value[T] {
 	return &Value[T]{dynKey: dynKey}
 }

modules/setting/config/value_test.go

@@ -0,0 +1,152 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package config
+
+import (
+	"testing"
+)
+
+func TestValue_parse(t *testing.T) {
+	tests := []struct {
+		name string // description of this test case
+		// Named input parameters for target function.
+		key    string
+		valStr string
+		want   bool
+	}{
+		{
+			name:   "Parse Invert Retrieval",
+			key:    "picture.disable_gravatar",
+			valStr: "false",
+			want:   true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			value := ValueJSON[bool]("picture.disable_gravatar").Invert()
+			got := value.parse(tt.key, tt.valStr)
+
+			if got != tt.want {
+				t.Errorf("parse() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValue_getKey(t *testing.T) {
+	tests := []struct {
+		name       string // description of this test case
+		valueClass *Value[bool]
+		want       string
+	}{
+		{
+			name:       "Custom dynKey name",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar"),
+			want:       "picture.disable_gravatar",
+		},
+		{
+			name:       "Normal dynKey name",
+			valueClass: ValueJSON[bool]("picture.disable_gravatar"),
+			want:       "picture.disable_gravatar",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.valueClass.getKey()
+
+			if got != tt.want {
+				t.Errorf("getKey() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValue_invert(t *testing.T) {
+	tests := []struct {
+		name string // description of this test case
+		// Named input parameters for target function.
+		valueClass *Value[bool]
+		want       bool
+	}{
+		{
+			name:       "Invert typed true",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(true).Invert(),
+			want:       false,
+		},
+		{
+			name:       "Invert typed false",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(false).Invert(),
+			want:       true,
+		},
+		{
+			name:       "Invert typed Does not invert",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").WithDefault(false),
+			want:       false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.valueClass.invert(tt.valueClass.def)
+
+			if got != tt.want {
+				t.Errorf("invert() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValue_invertBoolStr(t *testing.T) {
+	tests := []struct {
+		name string // description of this test case
+		// Named input parameters for target function.
+		valueClass *Value[bool]
+		val        string
+		want       string
+	}{
+		{
+			name:       "Invert boolean string true",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar"),
+			val:        "true",
+			want:       "false",
+		},
+		{
+			name:       "Invert boolean string false",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar"),
+			val:        "false",
+			want:       "true",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.valueClass.invertBoolStr(tt.val)
+			if got != tt.want {
+				t.Errorf("invertBoolStr() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestValue_SelectFromKey(t *testing.T) {
+	tests := []struct {
+		name string // description of this test case
+		// Named input parameters for target function.
+		valueClass *Value[bool]
+		want       string
+	}{
+		{
+			name:       "SelectFrom set and get",
+			valueClass: ValueJSON[bool]("picture.enable_gravatar").SelectFrom("picture.disable_gravatar"),
+			want:       "picture.disable_gravatar",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.valueClass.SelectFromKey()
+
+			if got != tt.want {
+				t.Errorf("SelectFromKey() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}