User creation API: allow custom "created" timestamps

[drsybren]

Allow back-dating user creation via the adminCreateUser API operation. CreateUserOption now has an optional field created_at, which can contain a datetime-formatted string. If this field is present, the user's created_unix database field will be updated to its value.

This is important for Blender's migration of users from Phabricator to Gitea. There are many users, and the creation timestamp of their account can give us some indication as to how long someone's been part of the community.

The back-dating is done in a separate query that just updates the user's created_unix field. This was the easiest and cleanest way I could find, as in the initial INSERT query the field always is set to "now".

[yardenshoham]

You mention back-dating a lot but this could also be used to set a future date

[drsybren]

You mention back-dating a lot but this could also be used to set a future date

That's true. Back-dating is the intended use, which IMO can be good to know for historical reasons. There is no code to prevent using future dates. Are you suggesting I add that code, or that I change the description?

[yardenshoham]

Change the description and add comments

[yardenshoham]

Please make another effort to include it in the initial SQL query

[yardenshoham]

Nice.

[drsybren]

The CI test fails with "gomail: could not send email 1: dial tcp 172.19.0.6:25: connect: connection refused", so that doesn't seem to be related to this particular PR. Can someone give it a kick to try again?

[lunny]

Please update the branch

[delvh]

@lunny That looks even more wrong to me?

[lunny]

@lunny That looks even more wrong to me?

Why did you think that?

[delvh]

Okay, yeah, the negation confused me as I had to invert the way I think about it to understand the logic.
What about using

       if u.UpdatedUnix < u.CreatedUnix {
               u.UpdatedUnix = u.CreatedUnix
       }

in the deleted block of your patch and

        if u.CreatedUnix != 0 {
                // Caller sets the timestamps themselves. They are responsible for ensuring
                // both `CreatedUnix` and `UpdatedUnix` are set appropriately.
                _, err = db.GetEngine(ctx).NoAutoTime().Insert(u)
        } else {
                // Caller expects auto-time for creation & update timestamps.
                err = db.Insert(ctx, u)
        }

for the insertion instead?
As far as I can see, that covers the edge cases even better, and allows to add support for setting the last updated setting in a migration later on.

[lunny]

I think your code is equal to mine and both are OK.

[drsybren]

       if u.UpdatedUnix < u.CreatedUnix {
               u.UpdatedUnix = u.CreatedUnix
       }

This doesn't seem right to me. Either the caller is providing the timestamps, and then they should be used as-is, or the caller is not, and then internal logic can take over. I wouldn't add logic that basically says "we'll use your timestamps, but not always".

This is fine for me:

       if u.UpdatedUnix == 0 {
               u.UpdatedUnix = u.CreatedUnix
       }

---

About the other block:

        if u.CreatedUnix != 0 {
                // Caller sets the timestamps themselves. They are responsible for ensuring
                // both `CreatedUnix` and `UpdatedUnix` are set appropriately.
                _, err = db.GetEngine(ctx).NoAutoTime().Insert(u)
        } else {
                // Caller expects auto-time for creation & update timestamps.
                err = db.Insert(ctx, u)
        }

Personally I don't like having a negation and an else clause. What's wrong with this?

        if u.CreatedUnix == 0 {
                // Caller expects auto-time for creation & update timestamps.
                err = db.Insert(ctx, u)
        } else {
                // Caller sets the timestamps themselves. They are responsible for ensuring
                // both `CreatedUnix` and `UpdatedUnix` are set appropriately.
                _, err = db.GetEngine(ctx).NoAutoTime().Insert(u)
        }

Also if we keep the if u.UpdatedUnix == 0 { u.UpdatedUnix = u.CreatedUnix } block, the comment " They are responsible for ensuring both CreatedUnix and UpdatedUnix are set appropriately " should be removed.

I do like @lunny's patch, as it restricts the handling of the timestamps to one block of code.

[delvh]

I wouldn't add logic that basically says we'll use your timestamps, but not always.

But we need to.
Otherwise, there can be an updated timestamp that is smaller than the created timestamp.
That's why we'll use your timestamps if they are valid.
I don't think it would be a good idea to allow updated timestamps that are before the user was created.
Furthermore, my approach covers the edge cases with UpdatedUnix = 0 and CreatedUnix = 0 well.

---

Regarding the "negation": Okay, I'm open to that.
I can live with both, for me at least it is easier to grasp that u.CreatedUnix != 0 means that the value is set and hence I don't see that as a negation…

[drsybren]

But we need to. Otherwise, there can be an updated timestamp that is smaller than the created timestamp.
My issue mostly is: this is a feature for migrating data from one system to another. As a rule of thumb, I'd try and keep that data the same whenever possible.

Of course there shouldn't be a situation where the last update is before the creation. However, that would have been the case in the original system as well. Unless there are areas in Gitea that are known to actually break when updated < created, I wouldn't bother changing the imported timestamps. The less mangling of data, the more predictable the system is.

---

I can live with both, for me at least it is easier to grasp that u.CreatedUnix != 0 means that the value is set and hence I don't see that as a negation…

To each their own. I still read it as "is not empty", because that's what's actually written.

[lunny]

But we need to. Otherwise, there can be an updated timestamp that is smaller than the created timestamp.
My issue mostly is: this is a feature for migrating data from one system to another. As a rule of thumb, I'd try and keep that data the same whenever possible.

Of course there shouldn't be a situation where the last update is before the creation. However, that would have been the case in the original system as well. Unless there are areas in Gitea that are known to actually break when updated < created, I wouldn't bother changing the imported timestamps. The less mangling of data, the more predictable the system is.

I can live with both, for me at least it is easier to grasp that u.CreatedUnix != 0 means that the value is set and hence I don't see that as a negation…

To each their own. I still read it as "is not empty", because that's what's actually written.

Please follow the suggestion you like.

[drsybren]

By removing the u.CreatedUnix = timeutil.TimeStampNow() code, the unit test fails as now the database layer takes the real clock instead of the mocked clock (see the timeutil.Set(fakeNow.AsTime()) call in the unit test).

Is there a way to set a custom "now" time in XORM?

[delvh]

.NoAutoTime() is the only way I know of, which is only somewhat related.

[drsybren]

Ok, so I'll rewrite the test to use the real clock then.

[lunny]

Please update branch @drsybren

[drsybren]

Done