occ: Improve `user:lastseen` timestamp #44666

nooblag · 2024-04-04T23:50:36Z

Resolves: occ: Improve user:lastseen timestamp #44641

Summary

TODO

Decide whether to display times always in UTC or pull Timezone from php.ini setting
Fix bug in user:info where last login is zero (don't show Unix epoch time)

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

nooblag · 2024-04-07T03:01:38Z

Hello all @nickvergessen @skjnldsv @ArtificialOwl @Fenn-CS @come-nc -- I understand you've got a lot going on, but just a friendly question to see if the timetable for this might make it in before NC29 final at the end of this month, by any chance? If so, I'll get working on the documentation changes too, etc?

Fenn-CS · 2024-04-07T22:02:13Z

but just a friendly question to see if the timetable for this might make it in before NC29 final at the end of this month, by any chance? If so, I'll get working on the documentation changes too, etc?

The next RC release for Nextcloud 29 is on May 30th, not sure if that answers your question but I believe as soon as this is ready and is reviews it would definitely get into the next release if back-ported to NC29 as master is now NC30.

nooblag · 2024-04-07T22:55:22Z

Hi @Fenn-CS Thanks for all that, it's helpful information. This is ready for review now.

nickvergessen · 2024-04-08T07:41:29Z

core/Command/User/Info.php

+		if ($user->getLastLogin() == 0) {
+			$lastseen = "never";
+		} else {
+			$lastseen = date('Y-m-d H:i:s T', $user->getLastLogin());


Both, the format change as well as the timezone change are breaking behaviour of the command that will result in breaking all tools/scripts that utilize those commands.

My recommendation would be to add an optional --timezone argument and use the given timezone when provided and leave any calls without it untouched.

Thank you for the input. I understand it's a breaking change. The point of this PR is to unify the timestamps though, and since 29.0.0 by way of semantic versioning denotes breaking changes in the API, breaking changes are acceptable, no?

At the moment we have different formatting at user:info and user:lastseen anyway, so unifying them (along with the bug fixing too) will be breaking changes in either case.

While we try to follow semantic versioning, breaking should be avoided as much as possible, that includes changing defaults and things like this here.
A newly added optional parameter can be used to fix your case without breaking existing usages.

Thank you for the note.

I am saying there will be a breaking change regardless though. Putting the formatting aside, fixing the bug when last seen time = 0 by saying "never" instead of returning Unix epoch will be a breaking change. And so if we're fixing an issue that will result in a breaking change anyway, why not unify the timestamp formatting while we're at it? Unless there's a reason that unifying the formatting change here is specifically undesirable?

Is the existing DateTimeInterface::ATOM format consistent and the established default in occ in other areas?

For background, and FWIW, the changes merged in #39669 changed the timestamp formatting in user:lastseen from d.m.Y H:i to Y-m-d H:i without any controversy, and that's technically a breaking change for scripts.

I'm not trying to be difficult, just trying to understand the hesitancy.

just trying to understand the rationale.

I missed it in the other PR.

Well, you'll also note there then that @come-nc wanted to do some other cool stuff like a complete overhaul of the timestamp line to show the date first to allow better sorting (which would also be good)... But anyways.

I'd obviously like to see some changes here to have consistency, which is the whole point of this discussion, but if that stalemates, I'll retract. I'd rather not introduce a --timezone argument that only applies to user:info and not user:lastseen -- my whole point here is looking for some consistency across occ, which at the moment, we don't have (along with the bug fix).

But the timezone used is consistent accross occ, no? Only the date format is not always the same.

Correct. 👍

core/Command/User/Info.php

nooblag · 2024-04-27T03:53:48Z

Hi all, so just wondering how to move forward with this, given it's been a few weeks since our discussion where we've landed on the decision around keeping everything in UTC. Just wondering about the other parts:

Is the bug fix for user:info lastseen=0 okay?
Seems there's strong desire to keep the time and date format for user:info as ATOM. How about for user:lastseen though? Change that to ATOM too? Or leave it and just improve the "prettiness" (i.e. adding seconds and timezone)?

github-actions · 2024-04-28T01:46:06Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

nooblag · 2024-05-17T09:52:38Z

Just a friendly follow up on #44666 (comment)

come-nc · 2024-05-21T10:04:41Z

Hi all, so just wondering how to move forward with this, given it's been a few weeks since our discussion where we've landed on the decision around keeping everything in UTC. Just wondering about the other parts:

Is the bug fix for user:info lastseen=0 okay?

Well, it’s complicated. I agree it would be better to show "never" rather than epoch, but changing that will break scripts relying on the output of the command.

Seems there's strong desire to keep the time and date format for user:info as ATOM. How about for user:lastseen though? Change that to ATOM too? Or leave it and just improve the "prettiness" (i.e. adding seconds and timezone)?

I have no strong opinion about adding seconds and timezone to user:lastseen.
Do you have before/after screenshots?

nooblag · 2024-05-21T11:13:48Z

Thanks for getting back.

Well, it’s complicated. I agree it would be better to show "never" rather than epoch, but changing that will break scripts relying on the output of the command.

I appreciate and understand that. If we don't want to break things, then we can just leave it as it is. Or, as I've said above, I'm open to suggestions on how to make changes in the least disruptive/most useful way.

PS what are some examples of all these scripts that are being referred to? And how is the importance of such 3rd party products considered against/above making changes to Nextcloud core?

I have no strong opinion about adding seconds and timezone to user:lastseen. Do you have before/after screenshots?

I would prefer to introduce seconds to user:lastseen along with the timezone to make it explicit to the user that the returned values are hardcoded UTC, and not local settings (I've been confused by this in the past, without the timezone information, thinking these times were local times). Sure, here are some screenshots.

Before PR:

Current PR:

nooblag · 2024-07-01T13:42:51Z

Hi all, just another friendly ping on #44666 (comment).

nooblag · 2024-07-23T09:07:09Z

Hi all, just another friendly ping on #44666 (comment), please?

come-nc · 2024-08-01T15:57:49Z

Thanks for getting back.

Well, it’s complicated. I agree it would be better to show "never" rather than epoch, but changing that will break scripts relying on the output of the command.

I appreciate and understand that. If we don't want to break things, then we can just leave it as it is. Or, as I've said above, I'm open to suggestions on how to make changes in the least disruptive/most useful way.

PS what are some examples of all these scripts that are being referred to? And how is the importance of such 3rd party products considered against/above making changes to Nextcloud core?

I do not have a good answer for that. I do not have an example of a script, it’s just that there may be some out there and we should not break compatibility without a good reason.
Whether showing never instead of epoch is a good enough reason is hard to arbitrate. I’d say it should not be changed in the json output of the command.
Regarding the default output, yeah it’s nicer to see never but it’s not that bad to see epoch, no one has Nextcloud servers running since 1970 so it’s quite easy to understand that this means never.

I have no strong opinion about adding seconds and timezone to user:lastseen. Do you have before/after screenshots?

I would prefer to introduce seconds to user:lastseen along with the timezone to make it explicit to the user that the returned values are hardcoded UTC, and not local settings (I've been confused by this in the past, without the timezone information, thinking these times were local times). Sure, here are some screenshots.

Before PR:

Current PR:

That part looks nicer indeed and I’d be fine with merging that.

nooblag · 2024-08-02T03:29:14Z

Thanks for responding.

Great, if we agree on the --lastseen changes, let's please merge them for NC 30?

And as I say, I won't keep pushing on the other, apart to still note in a friendly way, that it remains a bit confusing/dubious the reluctance over the tiniest breaking change for 3rd party scripts that are both uncited and still being considered above the level of core development...

Again, I'm no longer pushing, we can compromise here, and I'll remove it from the PR, but I do want to say that I think returning epoch date is not only incorrect data (even if no NC servers have been running since 1970), it remains my view that it's a bug, and it should say 'never', which is the correct data. But I've said all this already many times ;)

Anyway, will add a commit to reflect these discussions, and just accept the --lastseen change.

Thanks!

come-nc · 2024-08-05T15:41:05Z

CI failures unrelated, good for merging

AndyScherzinger

🐘

Signed-off-by: Jore <contact@jore.cc>

nickvergessen reviewed Apr 8, 2024

View reviewed changes

come-nc added this to the Nextcloud 30 milestone Apr 8, 2024

come-nc added enhancement 2. developing Work in progress feature: occ labels Apr 8, 2024

come-nc reviewed Apr 8, 2024

View reviewed changes

core/Command/User/Info.php Outdated Show resolved Hide resolved

github-actions bot added the feedback-requested label Apr 28, 2024

nooblag mentioned this pull request Jul 23, 2024

Wrong time when I use command user:lastseen or files:transfer-ownership #30387

Closed

joshtrichards added the feature: users and groups label Jul 23, 2024

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

Altahrim mentioned this pull request Aug 5, 2024

30.0.0 beta 4 #47028

Merged

come-nc approved these changes Aug 5, 2024

View reviewed changes

come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Aug 5, 2024

AndyScherzinger approved these changes Aug 6, 2024

View reviewed changes

occ: Improve timestamps for user:lastseen.

3e69668

Signed-off-by: Jore <contact@jore.cc>

nooblag added 5 commits August 7, 2024 09:33

occ: Improve timestamps for user:info.

bd81967

Signed-off-by: Jore <contact@jore.cc>

occ: Add check for never logged in for user:info.

e16e16c

Signed-off-by: Jore <contact@jore.cc>

occ: Add locale setting timezone to timestamps.

a0566f8

Signed-off-by: Jore <contact@jore.cc>

occ: Remove timezone changes.

96638fd

Signed-off-by: Jore <contact@jore.cc>

occ: Revert all changes to Info.php

5ec3a7f

Signed-off-by: Jore <contact@jore.cc>

skjnldsv force-pushed the occ-user branch from 0298955 to 5ec3a7f Compare August 7, 2024 07:33

skjnldsv merged commit 80fc2a9 into nextcloud:master Aug 7, 2024
3 of 4 checks passed

Altahrim mentioned this pull request Aug 7, 2024

30.0.0 beta 5 #47100

Merged

nooblag changed the title ~~occ: Improve and make consistent user:lastseen and user:info timestamps~~ occ: Improve user:lastseen timestamp Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

occ: Improve `user:lastseen` timestamp #44666

occ: Improve `user:lastseen` timestamp #44666

nooblag commented Apr 4, 2024 •

edited

Loading

nooblag commented Apr 7, 2024 •

edited

Loading

Fenn-CS commented Apr 7, 2024

nooblag commented Apr 7, 2024

nickvergessen Apr 8, 2024

nooblag Apr 8, 2024 •

edited

Loading

nickvergessen Apr 8, 2024

nooblag Apr 8, 2024 •

edited

Loading

nickvergessen Apr 8, 2024

nooblag Apr 8, 2024 •

edited

Loading

come-nc Apr 9, 2024

nooblag Apr 9, 2024

nooblag commented Apr 27, 2024

github-actions bot commented Apr 28, 2024

nooblag commented May 17, 2024

come-nc commented May 21, 2024

nooblag commented May 21, 2024 •

edited

Loading

nooblag commented Jul 1, 2024

nooblag commented Jul 23, 2024

come-nc commented Aug 1, 2024

nooblag commented Aug 2, 2024

come-nc commented Aug 5, 2024

AndyScherzinger left a comment

occ: Improve user:lastseen timestamp #44666

occ: Improve user:lastseen timestamp #44666

Conversation

nooblag commented Apr 4, 2024 • edited Loading

Summary

TODO

Checklist

nooblag commented Apr 7, 2024 • edited Loading

Fenn-CS commented Apr 7, 2024

nooblag commented Apr 7, 2024

nickvergessen Apr 8, 2024

Choose a reason for hiding this comment

nooblag Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

nickvergessen Apr 8, 2024

Choose a reason for hiding this comment

nooblag Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

nickvergessen Apr 8, 2024

Choose a reason for hiding this comment

nooblag Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

come-nc Apr 9, 2024

Choose a reason for hiding this comment

nooblag Apr 9, 2024

Choose a reason for hiding this comment

nooblag commented Apr 27, 2024

github-actions bot commented Apr 28, 2024

nooblag commented May 17, 2024

come-nc commented May 21, 2024

nooblag commented May 21, 2024 • edited Loading

nooblag commented Jul 1, 2024

nooblag commented Jul 23, 2024

come-nc commented Aug 1, 2024

nooblag commented Aug 2, 2024

come-nc commented Aug 5, 2024

AndyScherzinger left a comment

Choose a reason for hiding this comment

occ: Improve `user:lastseen` timestamp #44666

occ: Improve `user:lastseen` timestamp #44666

nooblag commented Apr 4, 2024 •

edited

Loading

nooblag commented Apr 7, 2024 •

edited

Loading

nooblag Apr 8, 2024 •

edited

Loading

nooblag Apr 8, 2024 •

edited

Loading

nooblag Apr 8, 2024 •

edited

Loading

nooblag commented May 21, 2024 •

edited

Loading