-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Save user's first login time and output it in occ user:info
#37328
Conversation
akhil1508
commented
Mar 21, 2023
- Resolves: User created date #22640 & Account creation date versus Last login bpcurse/nextcloud-userexport#52
Signed-off-by: Akhil <akhil@e.email>
Signed-off-by: Akhil <akhil@e.email>
It would be very simple to just use |
Signed-off-by: Akhil <akhil@e.email>
Signed-off-by: Akhil <akhil@e.email>
Signed-off-by: Akhil <akhil@e.email>
occ user:info
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 am not against the idea but a choice needs to be made on whether we want creation date or first login date (or both?), this is not the same thing.
I have no strong opinion about whether to advertize it in IUser, it feels pretty natural how it is done in the PR as it mimics lastLogin API.
/** @var int|null */ | ||
private $firstLogin; |
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.
/** @var int|null */ | |
private $firstLogin; | |
private ?int $firstLogin; |
Please strong type new code
* @return int | ||
* @since 27.0.0 | ||
*/ | ||
public function getFirstLogin(); |
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.
public function getFirstLogin(); | |
public function getFirstLogin(): int; |
* updates the timestamp of the first login of this user | ||
* @since 27.0.0 | ||
*/ | ||
public function updateFirstLoginTimestamp(int $timestamp); |
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.
public function updateFirstLoginTimestamp(int $timestamp); | |
public function updateFirstLoginTimestamp(int $timestamp): void; |
@@ -79,7 +79,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
'storage' => $this->getStorageInfo($user), | |||
'last_seen' => date(\DateTimeInterface::ATOM, $user->getLastLogin()), // ISO-8601 | |||
'user_directory' => $user->getHome(), | |||
'backend' => $user->getBackendClassName() | |||
'backend' => $user->getBackendClassName(), | |||
'created' => date(\DateTimeInterface::ATOM, $user->getFirstLogin()) |
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.
Creation and first login are not the same thing.
Also, this should handle properly the case where firstlogin is 0.
public function updateFirstLoginTimestamp(int $timestamp) { | ||
$this->firstLogin = $timestamp; | ||
$this->config->setUserValue($this->uid, 'login', 'firstLogin', (string)$this->firstLogin); | ||
} |
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.
Should there be some kind of check about whether a first login timestamp was already set for this user?
But not sure what could be done if it’s a case, maybe at least log an error.
$userObj = $this->getUser(); | ||
$userObj->updateFirstLoginTimestamp($userObj->getLastLogin()); | ||
// trigger any other initialization | ||
\OC::$server->getEventDispatcher()->dispatch(IUser::class . '::firstLogin', new GenericEvent($this->getUser())); |
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.
It would feel prettier if the call to updateFirstLoginTimestamp is instead in a Listener for the event firstLogin.
Sadly it seems there is no modern event for this yet, only the deprecated GenericEvent.
There is a UserCreatedEvent
on the other hand.
@come-nc I think both are useful to keep(both for info and investigating as a bunch of actions happen either after creation or after first login). I resolve rest of your comments asap and push my fixes. Thanks a lot! |
* @return int | ||
*/ | ||
|
||
public function getFirstLogin() { |
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.
might be better to return false
for cases when the user first logged in, but I have no strong feelings either way.
* updates the timestamp of the first login of this user | ||
* @since 27.0.0 | ||
*/ | ||
public function updateFirstLoginTimestamp(int $timestamp); |
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.
Since we only call this from core, does this need to be in the public interface?
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.
Well even in core most of the time the objects will be typed as IUser and not User, most likely this is the case here as well? Especially if it gets moved to a listener, most likely the event will give an IUser instance.
Hey @akhil1508 ! I suggest you keep an eye on #22640 to be notified of its resolution 🚀 Have a nice day and weekend! 🌞 |
@skjnldsv I've unfortunately been unable to continue my work on this PR in a timely manner. No worries at all, hopefully another PR gets merged or I can work on it soon, I would like first and foremost for the feature to make it to a release :)
You too! |