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

Do not show 'all' limit selector option for VisitorLog & default to 25 if filter_limit=-1 used #12790

Merged
merged 7 commits into from May 4, 2018

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Apr 30, 2018

Changes:

  • added new disable_all_rows_filter_limit visualization property that can be used to disable the 'all' option from the limit selector.
  • use in VisitorLog to disable the 'all' option.
  • if -1 filter_limit found when trying to load VisitorLog, change it to another value.
  • change default used in this case from 20 => General.datatable_default_limit.

Fixes #12789

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Apr 30, 2018
@diosmosis diosmosis added this to the 3.5.0 milestone Apr 30, 2018
Copy link
Member

@mattab mattab 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, just a minor feedback

if (!is_numeric($this->requestConfig->filter_limit)
|| $this->requestConfig->filter_limit == -1 // 'all' is not supported for this visualization
) {
$this->requestConfig->filter_limit = 25;
Copy link
Member

Choose a reason for hiding this comment

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

here let's use General.datatable_default_limit

@sgiehl
Copy link
Member

sgiehl commented Apr 30, 2018

tiny feedback: Maybe you should avoid branches containing /, as this break the ui test viewer: https://builds-artifacts.matomo.org/matomo-org/matomo/12789/do-not-show-all-visitor-log/
Or we adjust the UI test viewer to handle such stuff correctly...

@diosmosis
Copy link
Member Author

Oh whoops, didn't realize that would happen to the UI test viewer. I'll create a new branch to view tests results from

@diosmosis diosmosis force-pushed the 12789/do-not-show-all-visitor-log branch from e13b0a1 to 235bc53 Compare April 30, 2018 16:49
@diosmosis
Copy link
Member Author

Created branch to view build here: https://travis-ci.org/matomo-org/matomo/builds/373122394

@diosmosis
Copy link
Member Author

Some UI test failures, should be fixed on merge since they will likely conflict w/ other PRs.

@diosmosis
Copy link
Member Author

@mattab can you approve if this is good to go now? will update the UI tests & merge at that point

@diosmosis diosmosis merged commit 453e3fb into 3.x-dev May 4, 2018
@diosmosis diosmosis deleted the 12789/do-not-show-all-visitor-log branch May 4, 2018 00:17
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
…5 if filter_limit=-1 used (matomo-org#12790)

* Add disable_all_rows_filter_limit viewdatatable config option & use in visitorlog visualization.

* If -1 filter_limit is used w/ visitor log visualization, change it to 25 (which is in the datatable_row_limits default config value).

* Use datatable_default_limit as limit to set when unsupported filter_limit used in VisitorLog visualization.

* Update screenshots.
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.

None yet

3 participants