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
Fix first visit in visitor profile #12375
Conversation
b5850fb
to
3abf27e
Compare
6b25d2a
to
406e803
Compare
a9ac667
to
1b73dbb
Compare
$visits = $this->loadLastVisitorDetailsFromDatabase($idSite, $period = false, $date = false, $newSegment, | ||
$offset = 0, $limit); | ||
$visits = $this->loadLastVisitorDetailsFromDatabase($idSite, $period = false, $date = false, $segment, | ||
$offset = 0, $limit, false, false, $visitorId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we added the visitorid to the segment instead of setting the parameter, as the used method directly supports it. The resulting query for fetching the visits changes only slightly:
with segment:
SELECT sub.*
FROM (
SELECT log_visit.*
FROM piwik_log_visit AS log_visit
WHERE (log_visit.idsite IN (?)) AND (log_visit.idvisitor = ?)
ORDER BY idsite DESC, visit_last_action_time DESC
LIMIT 0, 1000) AS sub
GROUP BY sub.idvisit
ORDER BY sub.visit_last_action_time DESC
LIMIT 100;
with param:
SELECT sub.*
FROM (
SELECT log_visit.*
FROM piwik_log_visit AS log_visit
WHERE log_visit.idsite IN (?) AND log_visit.idvisitor = ?
ORDER BY idsite DESC, visit_last_action_time DESC
LIMIT 0, 100) AS sub
GROUP BY sub.idvisit
ORDER BY sub.visit_last_action_time DESC
LIMIT 100;
Not sure if that has any impact on performance...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no difference in performance as MySQL will parse this the same way IMO 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a slightly difference as the inner query now only has a limit of 100
instead of 1000
, so maybe it might be even faster, as it has lower amount of rows to sort in outer query ;-)
@mattab I've now adjusted the PR to show the very first visit in the visitor profile. Feel free to review ;-) |
af1f913
to
1880954
Compare
/** | ||
* Returns the very first visit for the given visitorId | ||
* | ||
* @internal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
marked the new method as internal for now. Or should we make it public API? Any opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to leave it internal as fetching first visit can be done with the existing API methods 👍
04989e6
to
b3cbaba
Compare
LGTM, re making the API public, unless it's useful to display in a report on it's own, seems better not to introduce new API. Plugin devs can always call it themselves, though maybe there's a use case out there for finding a user's first visit via the API... |
plugins/Live/API.php
Outdated
Piwik::checkUserHasViewAccess($idSite); | ||
|
||
$model = new Model(); | ||
$data = $model->queryLogVisits($idSite, false, false, false, 0, 1, $visitorId, false, 'ASC'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we should it seems check that $visitorId
is set, and if not fail? as otherwise we may return possibly some other visits in queryLogVisits
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now returns an empty datatable if the visitorid is empty
5021d99
to
bb60376
Compare
* Use first visit of whole dataset in visitor profile summary * update UI files * fetch very first visit if needed * update test files * fix tests * check for present visitorId before fetching data
Visitor profile will now show the first visit the use made (within the profile limit)
fixes #12373