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

Jump to bottom

Add the time a user was last seen to UsersManager::getU… #12107

Closed
wants to merge 3 commits into from
Closed

Add the time a user was last seen to UsersManager::getU… #12107

wants to merge 3 commits into from

Conversation

Morerice
Copy link
Contributor

last_seen variable is added to API UsersManager/API::getUsers if it exists.

@Morerice Morerice changed the title refs #10550 - Add the time a user was last seen to UsersManager::getU… Add the time a user was last seen to UsersManager::getU… Sep 23, 2017
sgiehl
sgiehl reviewed Sep 24, 2017
View reviewed changes
plugins/UsersManager/API.php Outdated
$lastSeen = $this->addLastSeen($user);

if ($lastSeen) {
$user['last_seen'] = $lastSeen;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to always set the last_seen, so the returned data has a consistent format. We could set it to 0 or false if it would be empty

@tsteur
Copy link
Member

tsteur commented Sep 24, 2017

I'm a bit worried about performance and wondering if we could rather have a new API for that? We have a couple of usages even within Piwik where we call UsersManager.getUsers. We would ideally test it with eg 30K users (as some Piwik installations do have that many users). The option seems to be autoloaded so it will likely not trigger DB query for each user but still good to check before we regress there

@Morerice
Copy link
Contributor Author

I've updated it according to @sgiehl and @mattab 's requests.

@mattab mattab added the Needs Review PRs that need a code review label Sep 25, 2017
@mattab mattab added this to the 3.2.0 milestone Sep 25, 2017
@diosmosis
Copy link
Member

This PR may need to be re-worked now. There's a method in UsersManager\API to add last_seen to multiple users: enrichUsersWithLastSeen. Used in a new API method.

@tsteur
Copy link
Member

tsteur commented Dec 19, 2018

@Morerice if you could look at the last comments and maybe tweak the PR that would be great 👍

@mattab mattab removed this from the 3.8.0 milestone Dec 31, 2018
@mattab mattab added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Dec 31, 2018
@mattab mattab closed this Jun 27, 2019
@mattab
Copy link
Member

mattab commented Jun 27, 2019

Thank you for this proposed pull request.

Because it was last updated more than one month ago, it is our policy to close pull requests opened for a long time without updates. If you would like to continue work on the pull request, please simply ping us to have it re-opened (after you have pushed a new commit).

We hope you understand this and we look forward to seeing an update from you on this pull request or another one!

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl left review comments

Assignees
No one assigned
Labels
Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Morerice @tsteur @diosmosis @mattab @sgiehl