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

Fix first visit in visitor profile #12375

Merged
merged 6 commits into from Mar 22, 2018
Merged

Fix first visit in visitor profile #12375

merged 6 commits into from Mar 22, 2018

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Dec 20, 2017

Visitor profile will now show the first visit the use made (within the profile limit)

fixes #12373

@sgiehl sgiehl added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Dec 20, 2017
@sgiehl sgiehl added this to the 3.2.2 milestone Dec 20, 2017
@sgiehl sgiehl force-pushed the firstvisit branch 5 times, most recently from b5850fb to 3abf27e Compare December 27, 2017 13:00
@sgiehl sgiehl force-pushed the firstvisit branch 2 times, most recently from 6b25d2a to 406e803 Compare December 29, 2017 12:39
@sgiehl sgiehl force-pushed the firstvisit branch 2 times, most recently from a9ac667 to 1b73dbb Compare January 12, 2018 11:22
$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);
Copy link
Member Author

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...

Copy link
Member

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 👍

Copy link
Member Author

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 ;-)

@sgiehl sgiehl requested a review from mattab January 26, 2018 15:29
@sgiehl
Copy link
Member Author

sgiehl commented Jan 26, 2018

@mattab I've now adjusted the PR to show the very first visit in the visitor profile. Feel free to review ;-)

@sgiehl sgiehl force-pushed the firstvisit branch 4 times, most recently from af1f913 to 1880954 Compare January 26, 2018 17:05
/**
* Returns the very first visit for the given visitorId
*
* @internal
Copy link
Member Author

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?

Copy link
Member

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 👍

@mattab mattab modified the milestones: 3.4.0, 3.3.1 Feb 1, 2018
@sgiehl sgiehl force-pushed the firstvisit branch 2 times, most recently from 04989e6 to b3cbaba Compare March 11, 2018 21:56
@diosmosis
Copy link
Member

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...

Piwik::checkUserHasViewAccess($idSite);

$model = new Model();
$data = $model->queryLogVisits($idSite, false, false, false, 0, 1, $visitorId, false, 'ASC');
Copy link
Member

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?

Copy link
Member Author

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

@sgiehl sgiehl force-pushed the firstvisit branch 2 times, most recently from 5021d99 to bb60376 Compare March 20, 2018 21:33
@mattab mattab merged commit 19ed478 into 3.x-dev Mar 22, 2018
@mattab mattab deleted the firstvisit branch March 22, 2018 00:42
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visitor Profile "First visit" does not always show the very first visit
4 participants