Skip to content
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

feat: Add first login timestamp of each user to oc_preferences and user:info output #49377

Merged
merged 11 commits into from
Jan 7, 2025

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Nov 19, 2024

See #22640
Close #48649

Summary

Add First login information in the user settings, as is done for last login

  • Add it to the UI
  • Decide how to handle migration

Checklist

@come-nc come-nc self-assigned this Nov 19, 2024
@come-nc come-nc added the 2. developing Work in progress label Nov 19, 2024
@come-nc come-nc added this to the Nextcloud 31 milestone Nov 19, 2024
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from eb486cc to fd366a6 Compare November 21, 2024 15:06
@come-nc
Copy link
Contributor Author

come-nc commented Nov 21, 2024

  • Decide how to handle migration

I now store -1 for users which have already logged in before upgrade to 31, and show that as "unknown" in user:info output.

@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from fd366a6 to f8c42b7 Compare December 3, 2024 09:20
@Pytal
Copy link
Member

Pytal commented Dec 5, 2024

✅ Add it to the UI
image

@@ -56,7 +72,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int
'groups' => $groups,
'quota' => $user->getQuota(),
'storage' => $this->getStorageInfo($user),
'last_seen' => date(\DateTimeInterface::ATOM, $user->getLastLogin()), // ISO-8601
'first_seen' => $firstSeen,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked user:info first_seen and looks good 👍

Can we also show this in user:list --info?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

But I’m remembering now that last time a PR tried to show never for last_seen @nickvergessen objected that this was a breaking change.
So not sure what is the best way ahead here. It is not appropriate to show fake timestamp for never/unknown, on the other hand scripts need to be aware of it.
Maybe we simply note this as a breaking change and still merge the change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Tested, thanks 🙏

@Pytal Pytal self-assigned this Dec 5, 2024
@come-nc
Copy link
Contributor Author

come-nc commented Dec 9, 2024

/compile /

@come-nc
Copy link
Contributor Author

come-nc commented Dec 9, 2024

✅ Add it to the UI image

Can you change the order to have first login before last login? Both in columns and settings. It’s disturbing to have last first.

@Pytal Pytal force-pushed the enh/add-first-login-timestamp branch from 159276f to f102154 Compare December 9, 2024 22:30
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from f00a83d to 99ba7d3 Compare December 10, 2024 09:19
@Pytal Pytal force-pushed the enh/add-first-login-timestamp branch from 6a68788 to 8f05db4 Compare December 10, 2024 18:03
@Pytal Pytal added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 10, 2024
@Pytal Pytal requested review from a team, icewind1991 and artonge and removed request for a team December 10, 2024 18:08
@Pytal Pytal enabled auto-merge December 10, 2024 18:32

/**
* updates the timestamp of the most recent login of this user
* Returns the timestamp of the user's first login, 0 if the user did never login, or -1 if the data is unknown (first login was on an older version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: int|null|false could make the three possible outcomes more explicit, and prevent someone from using the magic numbers for any kind of calculations.

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Dec 16, 2024
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from 8f05db4 to 7dc2bf9 Compare December 19, 2024 09:29
@come-nc
Copy link
Contributor Author

come-nc commented Dec 19, 2024

/compile /

@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Dec 19, 2024
@sorbaugh sorbaugh disabled auto-merge December 20, 2024 08:12
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from 4f20a9b to c5d3aa1 Compare January 7, 2025 09:37
@come-nc
Copy link
Contributor Author

come-nc commented Jan 7, 2025

/compile /

@come-nc come-nc mentioned this pull request Jan 7, 2025
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from e79ace7 to 60b2e22 Compare January 7, 2025 15:00
@come-nc come-nc force-pushed the enh/add-first-login-timestamp branch from 60b2e22 to de8c450 Compare January 7, 2025 15:30
@come-nc
Copy link
Contributor Author

come-nc commented Jan 7, 2025

/compile /

Signed-off-by: nextcloud-command <[email protected]>
@Pytal Pytal merged commit 51c2582 into master Jan 7, 2025
188 checks passed
@Pytal Pytal deleted the enh/add-first-login-timestamp branch January 7, 2025 17:17
*
* @since 31.0.0
*/
public function getFirstLogin(): int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the above/below change will break all 3rdparty user backends?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as getPasswordHash/setPasswordHash below did for 30.

But actually no, most apps do not implement IUser.
I see those:

  • lib/private/Remote/User.php:class User implements IUser {
  • lib/private/User/LazyUser.php:class LazyUser implements IUser {
  • lib/private/User/User.php:class User implements IUser {
  • apps/notifications/lib/FakeUser.php:class FakeUser implements IUser {
  • apps-extra/groupfolders/tests/stubs/oc_user_user.php:class User implements IUser {
  • apps-extra/guests/tests/stub.php: class User implements IUser {

So only notifications implements it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I mixed it up with the backends, I thought each backend implements their own IUser, but seems to not be the case.

@marcoambrosini
Copy link
Member

@Pytal one comment about the UI, could we remove the seconds count?

@Pytal
Copy link
Member

Pytal commented Jan 21, 2025

@Pytal one comment about the UI, could we remove the seconds count?

Done #50305 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: occ feature: users and groups pending documentation This pull request needs an associated documentation update 🍀 2025-Spring
Projects
Status: ☑️ Done
Development

Successfully merging this pull request may close these issues.

Show "First Login" and "Creation Date" it on the account Management Table as new columns
10 participants