-
-
Notifications
You must be signed in to change notification settings - Fork 351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create user last connection datetime #935
Changes from 65 commits
643daca
cd8667a
feafda8
62acdf3
f36f044
4d57c60
6a07bcb
73420a7
6610ab2
bf9bc76
22114c7
c418a0b
4428d8c
2c696c9
6d64b87
2f1ec75
5ffbf62
71d8d47
7d628db
eb6a6bd
519fdfa
ae48fb6
0ce7cda
fa5aa6a
6b061a6
403a480
d480eca
5a1d41a
51aa4e5
b701349
cc50893
0e78637
7ed70b9
b4344c1
f4f0c2b
b9600c5
42742b8
2c74062
792dc90
5d80db5
a7fbc2e
fe10c80
7c62a38
117717d
07bf8ef
0a3978c
2b49dee
ce9b1a8
224dbdf
f7bd265
76c2218
4ebdae6
8864643
8468fa5
822aefc
54e0d7e
bd27669
883b8e9
1081d63
ec55ec0
8fab3aa
4db9e7e
015dc5e
1fa3fb4
4ffb749
fc6de6b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import {User} from 'app/gen-server/entity/User'; | ||
import {makeId} from 'app/server/lib/idUtils'; | ||
import {chunk} from 'lodash'; | ||
import {MigrationInterface, QueryRunner} from "typeorm"; | ||
|
||
export class UserRefUnique1664528376930 implements MigrationInterface { | ||
|
@@ -9,12 +9,21 @@ export class UserRefUnique1664528376930 implements MigrationInterface { | |
|
||
// Update users that don't have unique ref set. | ||
const userList = await queryRunner.manager.createQueryBuilder() | ||
.select("users") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @CamilleLegeron. I have a question, can you try updating this migration, and the one above (
Here, I'm trying to force typeorm to not produce full SELECT, and I'm only specifying columns that I know exist. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooh wuaah yes it's works for the query function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems we can not use |
||
.from(User, "users") | ||
.where("ref is null") | ||
.getMany(); | ||
.select(["users.id", "users.ref"]) | ||
.from("users", "users") | ||
.where("users.ref is null") | ||
.getMany(); | ||
userList.forEach(u => u.ref = makeId()); | ||
await queryRunner.manager.save(userList, {chunk: 300}); | ||
|
||
const userChunks = chunk(userList, 300); | ||
for (const users of userChunks) { | ||
await queryRunner.connection.transaction(async manager => { | ||
const queries = users.map((user: any, _index: number, _array: any[]) => { | ||
return queryRunner.manager.update("users", user.id, user); | ||
}); | ||
await Promise.all(queries); | ||
}); | ||
} | ||
|
||
// Mark column as unique and non-nullable. | ||
const users = (await queryRunner.getTable('users'))!; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import {MigrationInterface, QueryRunner, TableColumn} from 'typeorm'; | ||
|
||
export class UserLastConnection1713186031023 implements MigrationInterface { | ||
|
||
public async up(queryRunner: QueryRunner): Promise<any> { | ||
await queryRunner.addColumn('users', new TableColumn({ | ||
name: 'last_connection_at', | ||
type: "date", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In
Can we reuse this pattern here? Or is there any other reason to use just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we don't want to know the time of the last connection, only know last date the user connect it's fine, and we will updated it maximum once a day There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since Sqlite doesn't have native |
||
isNullable: true | ||
})); | ||
} | ||
|
||
public async down(queryRunner: QueryRunner): Promise<any> { | ||
await queryRunner.dropColumn('users', 'last_connection_at'); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,8 @@ import {ActivationPrefs1682636695021 as ActivationPrefs} from 'app/gen-server/mi | |
import {AssistantLimit1685343047786 as AssistantLimit} from 'app/gen-server/migration/1685343047786-AssistantLimit'; | ||
import {Shares1701557445716 as Shares} from 'app/gen-server/migration/1701557445716-Shares'; | ||
import {Billing1711557445716 as BillingFeatures} from 'app/gen-server/migration/1711557445716-Billing'; | ||
import {UserLastConnection1713186031023 | ||
as UserLastConnection} from 'app/gen-server/migration/1713186031023-UserLastConnection'; | ||
|
||
const home: HomeDBManager = new HomeDBManager(); | ||
|
||
|
@@ -50,7 +52,8 @@ const migrations = [Initial, Login, PinDocs, UserPicture, DisplayEmail, DisplayE | |
CustomerIndex, ExtraIndexes, OrgHost, DocRemovedAt, Prefs, | ||
ExternalBilling, DocOptions, Secret, UserOptions, GracePeriodStart, | ||
DocumentUsage, Activations, UserConnectId, UserUUID, UserUniqueRefUUID, | ||
Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures]; | ||
Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, BillingFeatures, | ||
UserLastConnection]; | ||
|
||
// Assert that the "members" acl rule and group exist (or not). | ||
function assertMembersGroup(org: Organization, exists: boolean) { | ||
|
@@ -113,6 +116,33 @@ describe('migrations', function() { | |
// be doing something. | ||
}); | ||
|
||
it('can migrate UserUUID and UserUniqueRefUUID with user in table', async function() { | ||
this.timeout(60000); | ||
const runner = home.connection.createQueryRunner(); | ||
|
||
// Create 400 users to test the chunk (each chunk is 300 users) | ||
const nbUsersToCreate = 400; | ||
for (const migration of migrations) { | ||
if (migration === UserUUID) { | ||
for (let i = 0; i < nbUsersToCreate; i++) { | ||
await runner.query(`INSERT INTO users (id, name, is_first_time_user) VALUES (${i}, 'name${i}', true)`); | ||
} | ||
} | ||
|
||
await (new migration()).up(runner); | ||
} | ||
|
||
// Check that all refs are unique | ||
const userList = await runner.manager.createQueryBuilder() | ||
.select(["users.id", "users.ref"]) | ||
.from("users", "users") | ||
.getMany(); | ||
const setOfUserRefs = new Set(userList.map(u => u.ref)); | ||
assert.equal(nbUsersToCreate, userList.length); | ||
assert.equal(setOfUserRefs.size, userList.length); | ||
await addSeedData(home.connection); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding a test. One small suggestion: could you add a check here that goes through all the users and checks they each have a unique |
||
}); | ||
|
||
it('can correctly switch display_email column to non-null with data', async function() { | ||
this.timeout(60000); | ||
const sqlite = home.connection.driver.options.type === 'sqlite'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In line 403 we have this line:
Do you think we need it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we does not need it because we take the
moment()
starting on the dayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, one last question about the
date
column. I'm a little bit concerned here, what is actually stored and saved when we have 2 home servers in two different timezones. I did a bit of debugging and in Sqlite it stored:2024-06-20 22:00:00.000
(so UTC time ofday start
in my timezone +2), but in postgress only2024-06-21
. But in code both those values are mapped toDate
type, and I think represented in local time.So the concern (and question). Is it possible, that this code will update user multiple times a day, if requests will hit 2 different servers randomly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking, if it would be simpler if we just use
datetime
column in both Sqlite and Postgres. And treat it asunix timestamp
, the same way as we do for any othercreated_at
orupdated_at
columns. It will be easier to compare those if needed, and it is simpler to reason about, as having unix timestamps in database is a common approach.Math here is also very simple:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I hadn't seen that, thanks. In fact I think it's a good thing to use datetime, I will change it