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

Save user's first login time and output it in occ user:info #37328

Closed
wants to merge 6 commits into from

Conversation

akhil1508
Copy link
Contributor

Signed-off-by: Akhil <akhil@e.email>
Signed-off-by: Akhil <akhil@e.email>
@akhil1508
Copy link
Contributor Author

akhil1508 commented Mar 21, 2023

It would be very simple to just use setUserValue maybe even in the "firstLogin" hook but seems like the correct way is to add to the IUser interface and proceed from there, exposing it as a method of ever $user. If a maintainer could comment on whether this is the right approach or whether it is better to just add with setUserValue to the hook directly, it would be great.

Signed-off-by: Akhil <akhil@e.email>
@szaimen szaimen added this to the Nextcloud 27 milestone Mar 21, 2023
@szaimen szaimen added enhancement 3. to review Waiting for reviews labels Mar 21, 2023
@szaimen szaimen requested review from come-nc, blizzz, icewind1991 and a team March 21, 2023 20:51
lib/private/User/Session.php Fixed Show fixed Hide fixed
core/Command/User/Info.php Fixed Show fixed Hide fixed
lib/private/User/Session.php Fixed Show fixed Hide fixed
Signed-off-by: Akhil <akhil@e.email>
Signed-off-by: Akhil <akhil@e.email>
@akhil1508 akhil1508 changed the title User created time Save user's first login time and output it in occ user:info Mar 24, 2023
Signed-off-by: Akhil <akhil@e.email>
Copy link
Contributor

@come-nc come-nc left a 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.

Comment on lines +92 to +93
/** @var int|null */
private $firstLogin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** @var int|null */
private $firstLogin;
private ?int $firstLogin;

Please strong type new code

* @return int
* @since 27.0.0
*/
public function getFirstLogin();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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())
Copy link
Contributor

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.

Comment on lines +283 to +286
public function updateFirstLoginTimestamp(int $timestamp) {
$this->firstLogin = $timestamp;
$this->config->setUserValue($this->uid, 'login', 'firstLogin', (string)$this->firstLogin);
}
Copy link
Contributor

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.

Comment on lines +563 to 566
$userObj = $this->getUser();
$userObj->updateFirstLoginTimestamp($userObj->getLastLogin());
// trigger any other initialization
\OC::$server->getEventDispatcher()->dispatch(IUser::class . '::firstLogin', new GenericEvent($this->getUser()));
Copy link
Contributor

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.

@akhil1508
Copy link
Contributor Author

akhil1508 commented Mar 29, 2023

whether we want creation date or first login date (or both?), this is not the same thing.

@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() {
Copy link
Member

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);
Copy link
Member

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?

Copy link
Contributor

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.

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@blizzz blizzz mentioned this pull request Nov 6, 2023
This was referenced Nov 10, 2023
@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024
This was referenced Jul 30, 2024
This was referenced Aug 5, 2024
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv closed this Aug 14, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Aug 16, 2024

Hey @akhil1508 !
Thanks for the pull request. As we would like to push #22640 forward, and this PR had some unresolved comments, we'll close and move this to a different ticket.

I suggest you keep an eye on #22640 to be notified of its resolution 🚀
Thank you for your help and opening this ticket!

Have a nice day and weekend! 🌞
Cheers, John

@akhil1508
Copy link
Contributor Author

akhil1508 commented Aug 16, 2024

Thanks for the pull request. As we would like to push #22640 forward, and this PR had some unresolved comments, we'll close and move this to a different ticket.

@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 :)

Have a nice day and weekend! 🌞

You too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement stale Ticket or PR with no recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User created date
6 participants