Show currently logged in users

[yusufraji]

This shows users who are currently logged in.

Changes:

• adds a new is_logged_in field to the User struct and table on the backend
• sets the field to false by default
• sets the field to true after logging in
• set the field to false after logging out

I still need help working on the frontend for the full implementation.

Resolves #3251

[yusufraji]

Perhaps @Lionqueen94 or @stacktraceghost could have a look.

[ring-ring-ring]

This is cool! Thnx @yusufraji

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.19%. Comparing base (42350ea) to head (5dd6995).
⚠️ Report is 1 commits behind head on main.

[stacktraceghost]

Thank you for your PR! I've reviewed it and have some comments.

[praseodym]

@yusufraji Looks good! The only thing remaining to resolve #3251 would be to add the blue indicator dot for users that are logged in. Your initial PR body says you need help with the frontend implementation, are there any specific points you're struggling with?

[yusufraji]

@yusufraji Looks good! The only thing remaining to resolve #3251 would be to add the blue indicator dot for users that are logged in. Your initial PR body says you need help with the frontend implementation, are there any specific points you're struggling with?

Not necessarily, just a skill issue, since I'm new to react/Typescript and all that's frontend. I looked into it just a little bit before raising the PR. I'll have a look again.

[praseodym]

@yusufraji let us know if you want to spend more time on this, otherwise I'd be happy to fix these last points and merge this PR.

[yusufraji]

@yusufraji let us know if you want to spend more time on this, otherwise I'd be happy to fix these last points and merge this PR.

No worries, I've committed changes to address some of the review comments. But for the last comment, please go ahead and make the changes since I am not sure of how to resolve that comment.

We should keep the WHERE username = ? COLLATE NOCASE so that this query keeps using the username COLLATE NOCASE index

I look forward to learning how I could have implemented this from your changes.

[praseodym]

@yusufraji this has a merge conflict after merging #3432

[praseodym]

I notice that the tests fail because of the session unique constraint. #3458 also adds this constraint and fixes failing tests.

[pflanze]

I notice that the tests fail because of the session unique constraint. #3458 also adds this constraint and fixes failing tests.

Yes; this PR primarily added the index for performance reasons. Given that your PR is merged, this PR can be rebased on top of the new main again and then doesn't need to add the constraint anymore.

@yusufraji, we should add running the tests to our run-left-hooks script; I didn't realize it didn't do that already. (Explanation to the others: we turned the left hooks into a shell script so we could commit without the hooks running all the time; we run the script when we clean up the history.)

[praseodym]

(Explanation to the others: we turned the left hooks into a shell script so we could commit without the hooks running all the time; we run the script when we clean up the history.)

You can always commit using --no-verify to skip the pre-commit hooks.

[pflanze]

(Explanation to the others: we turned the left hooks into a shell script so we could commit without the hooks running all the time; we run the script when we clean up the history.)

You can always commit using --no-verify to skip the pre-commit hooks.

OK, I wasn't aware of that. I would probably end up just running with that option all the time, though, since my workflow commits very often and then I clean up things, and so will end up still preferring an explicit script.

BTW I'm considering parallelizing the actions (in my/our script) to make use of my many cores when I actually do run the actions; I didn't see anything about lefthook that would do that, have I missed it?

[praseodym]

BTW I'm considering parallelizing the actions (in my/our script) to make use of my many cores when I actually do run the actions; I didn't see anything about lefthook that would do that, have I missed it?

Lefthook runs all its commands in parallel in our setup:

abacus/lefthook.yml

Line 4 in fbf31e2

parallel: true

Also see https://lefthook.dev/configuration/parallel/.

[pflanze]

This PR is primarily @yusufraji 's work, so take these comments as ideas, I'm not invested in them:

• The commit message of the first commit still mentions adding the unique index, maybe worth removing; really minor, though.

• Given that is_logged_in is a derived value, might it be cleaner to leave struct User as is and instead add a wrapper that holds the User instance and the is_logged_in value? In garbage-collected language there's an incentive against wrapping like that since it costs a memory indirection, but that's not the case with Rust, where doing cleaner abstractions like this has no runtime cost. There's probably no point stopping this PR from being merged, but I could evaluate the feasibility in a separate PR, unless you already have reasons not to want that.