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

occ: Add --all support to user:lastseen #39669

Merged
merged 2 commits into from Mar 14, 2024

Conversation

nooblag
Copy link
Contributor

@nooblag nooblag commented Aug 2, 2023

Summary

Adds --all argument to user:lastseen for occ.

Checklist

@solracsf

This comment was marked as resolved.

@solracsf solracsf added this to the Nextcloud 28 milestone Aug 2, 2023
@nooblag nooblag force-pushed the lastseen-all branch 3 times, most recently from a7b8933 to e26ecb4 Compare August 3, 2023 05:31
@nooblag

This comment was marked as resolved.

@nooblag
Copy link
Contributor Author

nooblag commented Aug 3, 2023

Ooop, I spoke too soon. I see something has come up in the old code. I didn't change that. And I see that function in the rest of the user occ files are like that.

Should I add : void here, or leave it alone?

@joshtrichards joshtrichards changed the title occ: Add --all to user:lastseen. occ: Add --all support to user:lastseen Aug 27, 2023
@szaimen szaimen requested review from a team, ArtificialOwl, Fenn-CS and come-nc and removed request for a team August 27, 2023 07:13
core/Command/User/LastSeen.php Outdated Show resolved Hide resolved
core/Command/User/LastSeen.php Outdated Show resolved Hide resolved
core/Command/User/LastSeen.php Outdated Show resolved Hide resolved
} else {
$date = new \DateTime();
$date->setTimestamp($lastLogin);
$output->writeln($user->getUID() . "'s last login: " . $date->format('Y-m-d H:i'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the next feature request will be to be able to sort by last login, so maybe we should either sort, or do something like that so that it can be piped into sort:

Suggested change
$output->writeln($user->getUID() . "'s last login: " . $date->format('Y-m-d H:i'));
$output->writeln($date->format('Y-m-d H:i').' is the last login from '.$user->getUID());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, sure, if that change will align with future changes elsewhere. But for now, I didn't want to introduce breaking format change here. But happy for others to decide on how this should move forward.

@come-nc
Copy link
Contributor

come-nc commented Aug 28, 2023

You also need to run composer run cs:fix to fix codestyle

@nooblag
Copy link
Contributor Author

nooblag commented Aug 30, 2023

You also need to run composer run cs:fix to fix codestyle

Sorry, could you please provide more info? I ran the code through the suggested linter in the development docs. Is that not correct?

@come-nc
Copy link
Contributor

come-nc commented Aug 31, 2023

You also need to run composer run cs:fix to fix codestyle

Sorry, could you please provide more info? I ran the code through the suggested linter in the development docs. Is that not correct?

What linter are you talking about, can you share a link to this documentation?
https://docs.nextcloud.com/server/latest/developer_manual/getting_started/codingguidelines.html#php correctly references our coding standard configuration.

In the server repository there are helper scripts in the composer.json so that you can use composer run cs:check to check your code style and composer run cs:fix to fix it.

You branch is failing the code style check in the CI: https://github.com/nextcloud/server/actions/runs/5746991359/job/15580158407
(This is not the last run, since for the last commit it did not run yet but it should have the same result: https://github.com/nextcloud/server/actions/runs/6028377329/job/16386089395?pr=39669 )

@nooblag
Copy link
Contributor Author

nooblag commented Sep 6, 2023

Thanks for getting back. I'm sorry, it's so long ago now, I can't find what linter I ran in my history. It was most likely php-cs-fixer by itself however.

But I've now followed the instructions here and ran the composer line and fixed, and will push with those last commits as discussed above. Thanks for your help.

I hope that's better!

@come-nc
Copy link
Contributor

come-nc commented Sep 7, 2023

There were 2 errors:
214	
215	1) Tests\Core\Command\User\LastSeenTest::testValidUser with data set #0 (0, 'never logged in')
216	foreach() argument must be of type array|object, null given
217	
218	/drone/src/core/Command/User/LastSeen.php:79
219	/drone/src/tests/lib/TestCase.php:226
220	/drone/src/tests/Core/Command/User/LastSeenTest.php:88
221	
222	2) Tests\Core\Command\User\LastSeenTest::testValidUser with data set #1 (1693969625, 'last login')
223	foreach() argument must be of type array|object, null given
224	
225	/drone/src/core/Command/User/LastSeen.php:79
226	/drone/src/tests/lib/TestCase.php:226
227	/drone/src/tests/Core/Command/User/LastSeenTest.php:88

Could you try to see if you manage to run the test locally with:

./autotest.sh sqlite tests/Core/Command/User/LastSeenTest.php 

And then fix either the code or the test?

@nooblag
Copy link
Contributor Author

nooblag commented Sep 7, 2023

./autotest.sh sqlite tests/Core/Command/User/LastSeenTest.php

Running that returned this:

$ ./autotest.sh sqlite tests/Core/Command/User/LastSeenTest.php 
Using PHP executable /usr/bin/php
Using database oc_autotest
Setup environment for sqlite testing on local storage ...
Updated 0 paths from the index
Installing ....

In Install.php line 124:
                                                            
  [InvalidArgumentException]                                
  Database <sqlite> is not supported. mysql are supported.  
                                                            

Exception trace:
  at /home/nooblag/nextcloud-server/core/Command/Maintenance/Install.php:124
 OC\Core\Command\Maintenance\Install->validateInput() at /home/nooblag/nextcloud-server/core/Command/Maintenance/Install.php:99
 OC\Core\Command\Maintenance\Install->execute() at /home/nooblag/nextcloud-server/3rdparty/symfony/console/Command/Command.php:298
 Symfony\Component\Console\Command\Command->run() at /home/nooblag/nextcloud-server/3rdparty/symfony/console/Application.php:1040
 Symfony\Component\Console\Application->doRunCommand() at /home/nooblag/nextcloud-server/3rdparty/symfony/console/Application.php:301
 Symfony\Component\Console\Application->doRun() at /home/nooblag/nextcloud-server/3rdparty/symfony/console/Application.php:171
 Symfony\Component\Console\Application->run() at /home/nooblag/nextcloud-server/lib/private/Console/Application.php:213
 OC\Console\Application->run() at /home/nooblag/nextcloud-server/console.php:100
 require_once() at /home/nooblag/nextcloud-server/occ:11

@come-nc
Copy link
Contributor

come-nc commented Sep 7, 2023

@nooblag Are you missing php-sqlite3 maybe?

@nooblag
Copy link
Contributor Author

nooblag commented Sep 14, 2023

Hi @come-nc Yes, I was missing php-sqlite3 thank you, and yes can reproduce that error now as you have it. Unfortunately, I don't think I'm smart enough to fix the test, although I can see the array handling in lastseen.php needs improving?

I suspect the code needs to be changed in the test too, but I'm not really sure where to start.

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

core/Command/User/LastSeen.php Outdated Show resolved Hide resolved
Signed-off-by: Jordan Brown <code@jore.cc>
@nooblag
Copy link
Contributor Author

nooblag commented Mar 14, 2024

Now with fixes, thank you @nickvergessen @come-nc, it'd be great if this makes it in time for NC 29. 😉

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member

Only fixed the CS again:
grafik

There was a tab instead of a space behind : int

@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Mar 14, 2024
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
@nickvergessen nickvergessen merged commit d3da21b into nextcloud:master Mar 14, 2024
158 of 159 checks passed
Copy link

welcome bot commented Mar 14, 2024

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@nooblag
Copy link
Contributor Author

nooblag commented Mar 14, 2024

Great! :) I note the pending documentation tag. I'd be happy to contribute a further PR to add the merged changes to the documentation, if someone can point me in the right direction for that?

@nickvergessen
Copy link
Member

On https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/occ_command.html#user-commands-label if could be added (search for lastseen there). At the top of the page you find a "Edit on GitHub" that will bring you to the documentation repository with the file you need to edit

@nooblag
Copy link
Contributor Author

nooblag commented Apr 3, 2024

Nice work.

While we're in the space, just want to query if the times returned here will always be in UTC? And if so, whether we should be more explicit about that?

$ sudo -u www-data php /var/www/nextcloud/occ user:lastseen test
test's last login: 2024-04-03 11:29

Or, what do people think about ensuring the time is the same timezone as PHP set? For example, my locale set is Australia/Sydney, so times expected would be UTC+11 at the moment (and UTC+10 once daylight-saving is over).

i.e. My CLI php.in setting:

date.timezone = Australia/Sydney

So either:

$ sudo -u www-data php /var/www/nextcloud/occ user:lastseen test
test's last login: 2024-04-03 11:29 +00:00

Or:

$ sudo -u www-data php /var/www/nextcloud/occ user:lastseen test
test's last login: 2024-04-03 22:29 +11:00

I also notice, if we do work to improve here, that changes for consistency should also apply to occ user:info -- at the moment, my lines for that look like:

$ sudo -u www-data php /var/www/nextcloud/occ user:info test
  - user_id: test
  - display_name: test
  - email: test@test.local
  - cloud_id: test@test.local
  - enabled: true
  - groups:
    - admin
  - quota: none
  - storage:
    - free: 2198323200
    - used: 38251355
    - total: 2236574555
    - relative: 1.71
    - quota: -3
  - last_seen: 2024-04-03T11:29:06+00:00
  - user_directory: /var/www/nextcloud/data/test
  - backend: Database

Should I open a new issue to tackle the occ user:info part?

@nickvergessen
Copy link
Member

Yes, please create a new issue

@nooblag
Copy link
Contributor Author

nooblag commented Apr 3, 2024

Okay, thank you. Here it is for discussion: #44641.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: occ pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] Add ability to show all user's "last seen" date and time on CLI with occ
7 participants