No, this is not right. The next serval `err` have override the first one.

IIRC, the `NewSession` should not be used now (a recent refactoring), there is a `TxContext` instead.

Here we need a `insert on duplicate ignore` or ignore some unique-duplicated-errors. Because the `update` before may return 0 if the record already exists but it isn't changed. And that's why I usually do `revision=revision+1` to make sure the update always returns 1 if there is already an existing row (then we can do a simple insert, no need to worry about an existing record.) ps: If no easy way to do `insert on duplicate ignore` or we do not want to use an extra `revision` column and we want a general method to do safe upsert, here we can do a `Get(SELECT)`, since the txn is locked, if this `SELECT` returns 0 rows, then it guarantee that there is no duplicated rows and it's safe to do a simple `INSERT`. eg: ```sql create table t ...; insert into t (c1, v2) values (1,2); update t set c2=2 where c1=1; // updated 0 rows update t set c2=c2+1 where c1=1; // updated 1 rows ```

maybe `rows > 0` is better.

maybe `GetSettings` is a better name.

How about move this into `models/user/setting.go` so that we don't need move it again in future PRs.

Here mismatches the real `UserSetting`. Otherwise L-G-T-M

Should we just return a map? It will be easier to be used for callers.

The `order by` seems no affect.

Maybe `SettingKey` and `SettingValue` are better, since `Key` and `Value` all seem to be possible SQL keywords

Only one `sess.Delete`, maybe we do not need transaction here.

The name can be `GetUserAllSettings`. `GetAllUserSettings` sounds like it gets the settings of all users. ps: Should we just return a map? It will be easier to be used for callers.

Maybe it's caller's responsibility to make the keys identical. Changing the character cases seem strange. The caller will be confused that the value of an object was changed silently.

What about extracting the used values to variables so that they can be changed easily and to ensure that everything has the same value?

Shouldn't it be ```suggestion settings := make([]*UserSetting, 0, len(keys)) ``` as we expect an entry for each provided key?

`GetAllUserSettings`?

I think there is a `BeforeDelete` missing here.

Copy-paste error? ```suggestion // ErrUserSettingNotExists represents a "setting does not exist for user" error. ```

```suggestion ```

```suggestion UserID int64 `xorm:"index unique(key_userid)"` // to load all of someone's settings Key string `xorm:"varchar(255) index unique(key_userid)"` // ensure key is always lowercase ```

If the intention is that there is only one value per key+userID pair we should add a uniqueness constraint: ```suggestion type UserSetting struct { ID int64 `xorm:"pk autoincr"` UserID int64 `xorm:"index unique(key_userid)"` // to load all of someone's settings Key string `xorm:"varchar(255) index unique(key_userid)"` // ensure key is always lowercase Value string `xorm:"text"` } ``` Then you can use an UPSERT pattern to update the key below.

Why not just overwrite on update?

We need a `insertOrUpdateUserSetting` method to do the thing.

`Exist()` method has better performance than `Get()`

And we should also have `BeforeUpdate`

2021