Navigation Menu

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

Sorting Live.getLastVisitsDetails visits by a custom column #5950

Closed
mattab opened this issue Aug 7, 2014 · 5 comments
Closed

Sorting Live.getLastVisitsDetails visits by a custom column #5950

mattab opened this issue Aug 7, 2014 · 5 comments
Assignees
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone

Comments

@mattab
Copy link
Member

mattab commented Aug 7, 2014

Reproduce:

Fixing this issue will make the API more powerful and useful. Today we had to work around it costing 1-2 hours of our life...

@mattab mattab added this to the Piwik 2.5.0 milestone Aug 7, 2014
@mattab mattab added the Bug label Aug 7, 2014
@mattab mattab changed the title Sorting Live.getLastVisitsDetails by a column should work as expected Sorting Live.getLastVisitsDetails visits by a custom column Aug 7, 2014
tsteur added a commit that referenced this issue Aug 11, 2014
…an visitors filter immediately (before sort)
@tsteur
Copy link
Member

tsteur commented Aug 11, 2014

Using column visit_last_action_time should already work but would not make any sense of course since it is not visible in the output. And BTW: visit_last_action_time seems to be the default sort order anyway. So if no filter_column is set it could already work.

I am trying to apply the filter "clean visitors" before sorting but I do not know whether it has any side effects. I reckon one thing could be performance. So far the clean visitors filter was executed after GenericFilters and therefore only on the result set that is actually returned. Now we would execute it on all visitors, even on visitors that might get filtered out by GenericFilters. Also not sure whether still all other GenericFilters and all other use cases would work -> This would be a breaking API change.

Maybe we should rather run the sort filter twice and not execute the clean visitors filter immediately. So we would run GenericFilters which includes sort, clean the visitors and run the sort filter again.

tsteur added a commit that referenced this issue Aug 11, 2014
@tsteur
Copy link
Member

tsteur commented Aug 11, 2014

As the UI tests do not work I did some manual testing and also checked Piwik Mobile codebase. There is a filter_sort_column set in the VisitorLog visualization: https://github.com/piwik/piwik/blob/5950_sortGetLastVisitsDetails/plugins/Live/VisitorLog.php#L44

Both solutions that I tried change the oder of the VisitorLog slightly since it is now actually able to sort by idVisit which was not the case before. I am not sure if this would be a fix or by which column the VisitorLog should be sorted @mattab?

@tsteur tsteur self-assigned this Aug 11, 2014
@mattab
Copy link
Member Author

mattab commented Aug 11, 2014

@tsteur Visitor Log should be sorted by visit_last_action_time / lastActionTimestamp

(lots of users use the Visitor Log as main reporting tool, and refresh it frequently "today" to see the latest visitors and actions on their website)

@mattab
Copy link
Member Author

mattab commented Aug 11, 2014

Using column visit_last_action_time should already work but would not make any sense of course since it is not visible in the output

I didn't even think of that... at least now I get it why it didn't work :-)

I am trying to apply the filter "clean visitors" before sorting but I do not know whether it has any side effects. I reckon one thing could be performance.

performance may be an issue, because call to enrichVisitorArrayWithActions is heavy as it does JOINs on the raw log tables.

Maybe we should rather run the sort filter twice and not execute the clean visitors filter immediately. So we would run GenericFilters which includes sort, clean the visitors and run the sort filter again.

If this works then maybe it's a good solution?

@tsteur
Copy link
Member

tsteur commented Aug 12, 2014

Done. Gone with the second solution sorting a second time. I did some manual testing with the VisitorLog as the UI tests were not working (I think the build will work again since I fixed the travis.yml, will now in 30 min :) ). Will merge once the new integration tests pass and once it is in master I will check the UI tests just to keep it simple.

tsteur added a commit that referenced this issue Aug 12, 2014
…an visitors filter immediately (before sort)
tsteur added a commit that referenced this issue Aug 12, 2014
tsteur added a commit that referenced this issue Aug 12, 2014
tsteur added a commit that referenced this issue Aug 12, 2014
tsteur added a commit that referenced this issue Aug 12, 2014
refs #5950 add possibility to sort Live.getLastVisitsDetails
tsteur added a commit that referenced this issue Aug 12, 2014
@tsteur tsteur closed this as completed Aug 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Development

No branches or pull requests

2 participants