Create user last connection datetime #935

CamilleLegeron · 2024-04-15T13:21:54Z

resolve #924

Each time the a Grist page is reload the last_connection_at of the user is updated

CamilleLegeron · 2024-04-15T13:51:03Z

test/gen-server/migrations.ts

@@ -49,7 +51,7 @@ 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];
+                    Forks, ForkIndexes, ActivationPrefs, AssistantLimit, Shares, UserLastConnection];


Tests fails with error no such column: users.last_connection_at triggered when run migration UserUUID
because it uses the entity User that has no last_connection_at for the moment (the migration come later) : see here
Locally when I put the migration UserLastConnection before UserUUID it works but I think it's not a good fixe.
What about duplicate the entity inside the migration as it has to be at this point ? (Not convince about this approach)

We'll need to update UserUUID and any other affected migrations to not depend on the User entity. I don't know of any workarounds, unfortunately.

How do you imagine updating UserUUID to not depend ont the User entity has it exists to modify this entity ?

Replacing the methods called on the User entity with equivalent raw SQL is one idea. (typeorm supports executing raw SQL via the query method.)

The chunk option could be replaced with a for loop that does the UPDATE calls in chunks.

fflorent · 2024-04-15T13:59:07Z

After a quick glance, it looks fine to me.

Though I wonder what happens if a user continues to be logged in as their session won't expire? In such a case, even if that's not really probable that their session won't never expire in years (that being said, maybe this can happen?), the data may not reflect correctly the last connection.

Maybe we could take advantage of the Authorizer and specifically of the addUserRequest method to update the field?

grist-core/app/server/lib/Authorizer.ts

Line 141 in 4567fad

export async function addRequestUser(

We may only store the date without the time (so a date type instead of a datetime) in lastConnectionAt, and update only when the current date is not the same as in stored in the database?

And if an admin wants to know the activity of a user to the nearest second, they may take a look at the logs for that.

CamilleLegeron · 2024-04-15T15:55:41Z

We may only store the date without the time (so a date type instead of a datetime)

I'm agree and I can change that

Though I wonder what happens if a user continues to be logged in as their session won't expire?

In the code I made the last connection is updated when the page is loaded or reloaded don't no matter user login. But in your proposal we updated it on each action, it's more precise

fflorent

I pushed a comment, but I would need to take time to make this more helpful regarding the timezone.

app/gen-server/lib/HomeDBManager.ts

app/server/lib/Client.ts

…igrations

georgegevoian · 2024-05-20T16:38:18Z

app/server/lib/Authorizer.ts

@@ -358,6 +359,10 @@ export async function addRequestUser(
          mreq.user = user;
          mreq.userId = user.id;
          mreq.userIsAuthorized = true;
+          const today = moment().startOf('day');
+          if (today !== moment(user.lastConnectionAt).startOf('day')) {
+            await dbManager.updateUser(mreq.userId, {lastConnectionAt: today.toDate()});


Could we do this from getUserByLoginWithRetry instead? It also (optionally) updates the user.

app/gen-server/migration/1663851423064-UserUUID.ts

… queries

paulfitz

The functionality seems basically ok, thanks @CamilleLegeron . I am grateful that the user record isn't updated excessively. I have a collection of very nitpicky, linty comments.

app/gen-server/sqlUtils.ts

app/gen-server/migration/1663851423064-UserUUID.ts

app/gen-server/migration/1664528376930-UserRefUnique.ts

app/gen-server/migration/1713186031023-UserLastConnection.ts

paulfitz · 2024-05-28T20:23:41Z

app/gen-server/sqlUtils.ts

+      const queries = users.map((user: any, _index: number, _array: any[]) => {
+        return manager.query(
+          `UPDATE users
+          SET ref = ${addParamToQuery(dbType, 1)}


Overall with the addParamToQuery function included this feels quite messy compared to the original queryRunner.manager.save setup? I probably missed it earlier in development, but what was the main reason for the change?

There's some context here.

Thanks, I understand now. Would it be possible to narrow the scope of the selects, like for example in

grist-core/app/gen-server/migration/1568238234987-TeamMembers.ts

Lines 10 to 33 in 5956c20

public async up(queryRunner: QueryRunner): Promise<any> {

// Get all orgs and add a team member ACL (with group) to each.

const orgs = await queryRunner.manager.createQueryBuilder()

.select("orgs.id")

.from(Organization, "orgs")

.getMany();

for (const org of orgs) {

const groupInsert = await queryRunner.manager.createQueryBuilder()

.insert()

.into(Group)

.values([{name: roles.MEMBER}])

.execute();

const groupId = groupInsert.identifiers[0].id;

await queryRunner.manager.createQueryBuilder()

.insert()

.into(AclRuleOrg)

.values([{

permissions: Permissions.VIEW,

organization: {id: org.id},

group: groupId

}])

.execute();

}

}

? Hmm but then there is the actual updates to do...

Bundling does seem important since there are a lot of users. But I'm afraid if this code had a subtle bug we wouldn't catch it, and previous times we have had subtle bugs in migrations it has been a mess. I think I need to ask for this method's functionality to be given a set of unit tests. The addParamToQuery function that does one thing for Postgres and then ignores an argument for SQLite, relying on the user to call things in the right order, also feels dangerous - does it really need to be exported, can it be at least kept private?

In my last commit I added a test to be sure it enters inside the for loop of the addRefToUserList function and I remove the addParamToQuery function that is not safe

Currently translated at 13.7% (184 of 1334 strings) Translation: Grist/client Translate-URL: https://hosted.weblate.org/projects/grist/client/sk/

…ristlabs#1024) The example key shown on the admin panel to users who are not known to be administrators is generated using a method that is only available in secure environments. This adds a fallback for insecure environments. The key is less solid but again, it is just an example, and for an insecure environment. Tested manually running locally and using a hostname set in /etc/hosts.

Summary: - Makes EE decide which ActivationPage to use - Makes ProductUpgrades use core implementation if not activated - Changes banners to proxy to core implementation if EE not activated - [Fix] Enables new site creation in EE as in Core: - Core enables people to freely create new team sites. - Enterprise currently redirects to the pricing page. - This enables enterprise to also create team sites, instead of redirecting. Test Plan: Manually test in EE, unit tests in Jenkins Reviewers: paulfitz, jordigh Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D4264

Summary: - lookupOne/lookupRecords explain `sort_by` param better, and link to more detailed article. - Incorporate a typo fix from Help Center - Fix the omission of TASTEME never having been documented. Test Plan: Corresponding update to Help Center can be reviewed at gristlabs/grist-help#351 Reviewers: jarek Reviewed By: jarek Subscribers: jarek Differential Revision: https://phab.getgrist.com/D4269

Summary: The GRIST_DEFAULT_PRODUCT wasn't used for grist-ee, now it is respected. Test Plan: I've build grist-ee docker image from github and run it using our instruction (both for recreating the issue and confirming it is fixed) ``` docker run -p 8484:8484 \ -v $PWD:/persist \ -e GRIST_SESSION_SECRET=invent-a-secret-here \ -e GRIST_SINGLE_ORG=cool-beans -it gristlabs/grist-ee ``` For grist-core I recreated/confirmed it is fixed it just by `GRIST_SINGLE_ORG=team npm start` in the core folder. I also created some team sites using stubbed UI and confirmed that they were using the GRIST_DEFAULT_PRODUCT product. Reviewers: paulfitz Reviewed By: paulfitz Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D4271

… product if it was set be default Summary: After release on 2024-06-12 (1.1.15) the GRIST_DEFAULT_PRODUCT env variable wasn't respected by the method that started the server in single org mode. In all deployments (apart from saas), the default product used for new sites is set to `Free`, but the code that starts the server enforced `teamFree` product. This change adds a fix routine that fixes this issue by rewriting team sites from `teamFree` product to `Free` product only if: - The default product is set to `Free` - The deployment type is something other then 'saas'. Additionally there is a test that will fail after 2024.10.01, as this fix should be removed before this date. Test Plan: Added test Reviewers: paulfitz Reviewed By: paulfitz Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D4272

Summary: For non-owners, the timing section of Document Settings is now disabled. For non-editors, the "Reload" section is disabled. Test Plan: Added a test case for timing being disabled. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D4275

Summary: fixSiteProducts was always called with a dry option. This option was just added for debuging test failure, it should have been removed. Test Plan: Manual. - on grist core, prepare site with `teamFree` product - then to recreate run the previous version as `GRIST_SINGLE_ORG=cool-beans GRIST_DEFAULT_PRODUCT=Free npm start` - then to confirm it is fixed, run the same command as above Site should be changed from `teamFree` to `Free`. Reviewers: paulfitz Reviewed By: paulfitz Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D4276

Co-authored-by: Paul's Grist Bot <paul+bot@getgrist.com>

…nt in its own module (gristlabs#1049) The HomeDBManager remains the exposed class to the other parts of the code: any module under gen-server/lib/homedb like UsersManager is intended to be used solely by HomeDBManager, and in order to use their methods, an indirection has to be created to pass through HomeDBManager.

This is a new entrypoint, mostly intended for Docker, so we have one simple process controlling the main Grist process. The purpose of this is to be able to make Grist easily restartable with a new environment.

This adds an endpoint for the admin user to be able to signal to a controlling process to restart the server. This is intended for `docker-runner.mjs`.

…or comparasion

berhalak · 2024-06-21T09:33:52Z

app/gen-server/lib/homedb/UsersManager.ts

@@ -432,6 +433,11 @@ export class UsersManager {
        user.options = {...(user.options ?? {}), authSubject: userOptions.authSubject};
        needUpdate = true;
      }
+      const today = moment().startOf('day');
+      if (!user.lastConnectionAt || !today.isSame(moment(user.lastConnectionAt).startOf('day'))) {
+        user.lastConnectionAt = today.toDate();


In line 403 we have this line:

nowish.setMilliseconds(0);

Do you think we need it here?

No, we does not need it because we take the moment() starting on the day

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 of day start in my timezone +2), but in postgress only 2024-06-21. But in code both those values are mapped to Date 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?

I'm thinking, if it would be simpler if we just use datetime column in both Sqlite and Postgres. And treat it as unix timestamp, the same way as we do for any other created_at or updated_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:

const timeStamp = Math.floor(Date.now() / 1000); // unix timestamp seconds from epoc const startOfDay = timestamp - (timestamp % 86400 /*24h*/); // start of a day in seconds since epoc

Oh yes, I hadn't seen that, thanks. In fact I think it's a good thing to use datetime, I will change it

app/gen-server/migration/1663851423064-UserUUID.ts

app/gen-server/lib/homedb/UsersManager.ts

berhalak

Thanks @CamilleLegeron.

georgegevoian

Thank you @CamilleLegeron!

Each time the a Grist page is reload the `last_connection_at` of the user is updated resolve [#924](#924)

Create user last connection datetime

643daca

CamilleLegeron requested review from fflorent and removed request for fflorent April 15, 2024 13:22

CamilleLegeron commented Apr 15, 2024

View reviewed changes

CamilleLegeron requested a review from fflorent April 15, 2024 13:52

CamilleLegeron self-assigned this Apr 15, 2024

CamilleLegeron added the anct label Apr 15, 2024

use date and not datetime for lastConnectionAt

cd8667a

CamilleLegeron force-pushed the last-connection branch from a27256a to cd8667a Compare April 16, 2024 09:35

CamilleLegeron requested a review from georgegevoian April 16, 2024 09:36

fflorent reviewed Apr 18, 2024

View reviewed changes

app/gen-server/lib/HomeDBManager.ts Outdated Show resolved Hide resolved

fflorent self-requested a review April 18, 2024 12:57

georgegevoian reviewed Apr 23, 2024

View reviewed changes

app/server/lib/Client.ts Outdated Show resolved Hide resolved

CamilleLegeron added 5 commits April 30, 2024 17:56

Merge branch 'main' into last-connection

feafda8

use moment to get today date

62acdf3

Merge branch 'main' into last-connection

f36f044

update user last connection on each authorization request

4d57c60

fix (migration-test): use SQL queries to avoid using User entity in m…

6a07bcb

…igrations

CamilleLegeron requested a review from georgegevoian May 20, 2024 15:48

georgegevoian reviewed May 20, 2024

View reviewed changes

app/gen-server/migration/1663851423064-UserUUID.ts Outdated Show resolved Hide resolved

CamilleLegeron added 4 commits May 22, 2024 11:03

update lastConnectionAt directly in getUserByLogin

73420a7

feat [last-connection]: use chunk in migration and parameterizing the…

6610ab2

… queries

Merge branch 'main' into last-connection

bf9bc76

remove Trailing spaces

22114c7

CamilleLegeron requested a review from georgegevoian May 28, 2024 14:58

paulfitz reviewed May 28, 2024

View reviewed changes

reorganize imports

c418a0b

h0r0m and others added 19 commits June 20, 2024 10:52

Translated using Weblate (Slovak)

2b49dee

Currently translated at 13.7% (184 of 1334 strings) Translation: Grist/client Translate-URL: https://hosted.weblate.org/projects/grist/client/sk/

v1.1.15

ce9b1a8

Update README to rebrand grist-electron (gristlabs#1039)

8864643

automated update to translation keys (gristlabs#1053)

bd27669

Co-authored-by: Paul's Grist Bot <paul+bot@getgrist.com>

supervisor: new file

1081d63

This is a new entrypoint, mostly intended for Docker, so we have one simple process controlling the main Grist process. The purpose of this is to be able to make Grist easily restartable with a new environment.

FlexServer: add new admin restart endpoint

ec55ec0

This adds an endpoint for the admin user to be able to signal to a controlling process to restart the server. This is intended for `docker-runner.mjs`.

Dockerfile: use docker-runner.mjs as new entrypoint

8fab3aa

Merge branch 'main' into last-connection

4db9e7e

fix: use only needed columns on db query

015dc5e

fix: check if lastConnectionAt exist and use moment isSame function f…

1fa3fb4

…or comparasion

fix: use update instead of save function to avoid using user entity

4ffb749

CamilleLegeron requested a review from berhalak June 20, 2024 16:09

berhalak reviewed Jun 21, 2024

View reviewed changes

app/gen-server/migration/1663851423064-UserUUID.ts Outdated Show resolved Hide resolved

fix: use datetime and timestamp for lastConnectionAt

fc6de6b

CamilleLegeron commented Jun 28, 2024

View reviewed changes

app/gen-server/lib/homedb/UsersManager.ts Show resolved Hide resolved

berhalak approved these changes Jul 1, 2024

View reviewed changes

georgegevoian approved these changes Jul 1, 2024

View reviewed changes

berhalak merged commit 61421e8 into gristlabs:main Jul 1, 2024
13 checks passed

SleepyLeslie pushed a commit that referenced this pull request Jul 8, 2024

Create user last connection datetime (#935)

28a22ca

Each time the a Grist page is reload the `last_connection_at` of the user is updated resolve [#924](#924)

fflorent deleted the last-connection branch July 24, 2024 15:04

fflorent removed their request for review October 17, 2024 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create user last connection datetime #935

Create user last connection datetime #935

CamilleLegeron commented Apr 15, 2024

CamilleLegeron Apr 15, 2024 •

edited

Loading

georgegevoian May 6, 2024

CamilleLegeron May 17, 2024

georgegevoian May 20, 2024

fflorent commented Apr 15, 2024

CamilleLegeron commented Apr 15, 2024 •

edited

Loading

fflorent left a comment

georgegevoian May 20, 2024

paulfitz left a comment

paulfitz May 28, 2024

georgegevoian May 28, 2024

paulfitz May 30, 2024

CamilleLegeron Jun 3, 2024

berhalak Jun 21, 2024

CamilleLegeron Jun 21, 2024

berhalak Jun 21, 2024 •

edited

Loading

berhalak Jun 21, 2024

CamilleLegeron Jun 25, 2024

berhalak left a comment

georgegevoian left a comment

	public async up(queryRunner: QueryRunner): Promise<any> {
	// Get all orgs and add a team member ACL (with group) to each.
	const orgs = await queryRunner.manager.createQueryBuilder()
	.select("orgs.id")
	.from(Organization, "orgs")
	.getMany();
	for (const org of orgs) {
	const groupInsert = await queryRunner.manager.createQueryBuilder()
	.insert()
	.into(Group)
	.values([{name: roles.MEMBER}])
	.execute();
	const groupId = groupInsert.identifiers[0].id;
	await queryRunner.manager.createQueryBuilder()
	.insert()
	.into(AclRuleOrg)
	.values([{
	permissions: Permissions.VIEW,
	organization: {id: org.id},
	group: groupId
	}])
	.execute();
	}
	}

Create user last connection datetime #935

Create user last connection datetime #935

Conversation

CamilleLegeron commented Apr 15, 2024

CamilleLegeron Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent commented Apr 15, 2024

CamilleLegeron commented Apr 15, 2024 • edited Loading

fflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfitz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berhalak Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berhalak left a comment

Choose a reason for hiding this comment

georgegevoian left a comment

Choose a reason for hiding this comment

CamilleLegeron Apr 15, 2024 •

edited

Loading

CamilleLegeron commented Apr 15, 2024 •

edited

Loading

berhalak Jun 21, 2024 •

edited

Loading