Save user's first login time and output it in `occ user:info` by akhil1508 · Pull Request #37328 · nextcloud/server

akhil1508 · 2023-03-21T19:10:08Z

Resolves: User created date #22640 & Account creation date versus Last login bpcurse/nextcloud-userexport#52

Signed-off-by: Akhil <[email protected]>

akhil1508 · 2023-03-21T19:11:55Z

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 <[email protected]>

lib/private/User/Session.php

core/Command/User/Info.php

lib/private/User/Session.php

Signed-off-by: Akhil <[email protected]>

come-nc

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.

come-nc · 2023-03-28T09:41:54Z

lib/private/User/User.php

+	/** @var int|null */
+	private $firstLogin;


Suggested change

/** @var int|null */

private $firstLogin;

private ?int $firstLogin;

Please strong type new code

come-nc · 2023-03-28T09:43:21Z

lib/public/IUser.php

+	 * @return int
+	 * @since 27.0.0
+	 */
+	public function getFirstLogin();


Suggested change

public function getFirstLogin();

public function getFirstLogin(): int;

come-nc · 2023-03-28T09:43:33Z

lib/public/IUser.php

+	 * updates the timestamp of the first login of this user
+	 * @since 27.0.0
+	 */
+	public function updateFirstLoginTimestamp(int $timestamp);


Suggested change

public function updateFirstLoginTimestamp(int $timestamp);

public function updateFirstLoginTimestamp(int $timestamp): void;

come-nc · 2023-03-28T09:44:36Z

core/Command/User/Info.php

 			'user_directory' => $user->getHome(),
-			'backend' => $user->getBackendClassName()
+			'backend' => $user->getBackendClassName(),
+			'created' => date(\DateTimeInterface::ATOM, $user->getFirstLogin())


Creation and first login are not the same thing.
Also, this should handle properly the case where firstlogin is 0.

come-nc · 2023-03-28T09:51:18Z

lib/private/User/User.php

+	public function updateFirstLoginTimestamp(int $timestamp) {
+		$this->firstLogin = $timestamp;
+		$this->config->setUserValue($this->uid, 'login', 'firstLogin', (string)$this->firstLogin);
+	}


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.

come-nc · 2023-03-28T09:53:37Z

lib/private/User/Session.php

+			$userObj = $this->getUser();
+			$userObj->updateFirstLoginTimestamp($userObj->getLastLogin());
 			// trigger any other initialization
 			\OC::$server->getEventDispatcher()->dispatch(IUser::class . '::firstLogin', new GenericEvent($this->getUser()));


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 · 2023-03-29T10:22:02Z

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!

icewind1991 · 2023-03-29T16:43:57Z

lib/private/User/User.php

+	 * @return int
+	 */
+
+	public function getFirstLogin() {


might be better to return false for cases when the user first logged in, but I have no strong feelings either way.

icewind1991 · 2023-03-29T16:44:41Z

lib/public/IUser.php

+	 * updates the timestamp of the first login of this user
+	 * @since 27.0.0
+	 */
+	public function updateFirstLoginTimestamp(int $timestamp);


Since we only call this from core, does this need to be in the public interface?

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.

skjnldsv · 2024-08-16T08:27:04Z

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 · 2024-08-16T08:34:30Z

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!

akhil1508 added 2 commits March 22, 2023 00:33

Add first login methods to IUser and User

badd79d

Signed-off-by: Akhil <[email protected]>

Add created to occ user:info

64b870f

Signed-off-by: Akhil <[email protected]>

Fix comment

5d64fe0

Signed-off-by: Akhil <[email protected]>

szaimen added this to the Nextcloud 27 milestone Mar 21, 2023

szaimen added enhancement 3. to review Waiting for reviews labels Mar 21, 2023

szaimen requested review from a team, blizzz, come-nc and icewind1991 March 21, 2023 20:51

github-advanced-security bot found potential problems Mar 21, 2023

View reviewed changes

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

akhil1508 added 2 commits March 22, 2023 17:25

Fix wrong type

bf411a4

Signed-off-by: Akhil <[email protected]>

Fix wrong ATOM type

450cba7

Signed-off-by: Akhil <[email protected]>

akhil1508 changed the title ~~User created time~~ Save user's first login time and output it in occ user:info Mar 24, 2023

Fix CS

881e272

Signed-off-by: Akhil <[email protected]>

come-nc reviewed Mar 28, 2023

View reviewed changes

icewind1991 reviewed Mar 29, 2023

View reviewed changes

This was referenced May 3, 2023

27.0.0 beta 1 #38038

Merged

27.0.0 beta 2 #38137

Closed

27.0.0 beta 2 #38139

Merged

blizzz mentioned this pull request May 17, 2023

27.0.0 RC1 #38341

Merged

blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

blizzz mentioned this pull request Nov 6, 2023

28.0.0 beta 2 #41314

Merged

This was referenced Nov 10, 2023

28.0.0 beta 3 #41379

Merged

28.0.0 beta 4 #41477

Merged

blizzz mentioned this pull request Nov 20, 2023

28.0.0 RC1 #41621

Merged

5 tasks

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

This was referenced Mar 20, 2024

29.0.0 beta 5 #44361

Merged

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request Mar 28, 2024

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

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

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv closed this Aug 14, 2024

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

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

	public function getFirstLogin();
	public function getFirstLogin(): int;

	public function updateFirstLoginTimestamp(int $timestamp);
	public function updateFirstLoginTimestamp(int $timestamp): void;

Uh oh!

Conversation

akhil1508 commented Mar 21, 2023

Uh oh!

akhil1508 commented Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhil1508 commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skjnldsv commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akhil1508 commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

akhil1508 commented Mar 21, 2023 •

edited

Loading

akhil1508 commented Mar 29, 2023 •

edited

Loading

skjnldsv commented Aug 16, 2024 •

edited

Loading

akhil1508 commented Aug 16, 2024 •

edited

Loading