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

Allow to provide secondary sort column for reports #12128

Merged
merged 4 commits into from Jun 13, 2018
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Sep 27, 2017

Aims to make it possible to sort reports by a custom secondary column as described in #12127

This is only a prove of concept atm.
If we decide to use it that way I'd go on with adding some tests for it...

* ```
* @return null|callable
*/
public function getSecondSortColumnCallback()
Copy link
Member

Choose a reason for hiding this comment

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

would rename to getSecondarySortColumnCallback

@mattab
Copy link
Member

mattab commented Oct 5, 2017

this looks good to me 👍 We could later also make this available as a global API parameter filter.

@mattab mattab added this to the 3.3.0 milestone Oct 5, 2017
@mattab mattab added c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. labels Oct 5, 2017
@mattab mattab added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Oct 19, 2017
@mattab
Copy link
Member

mattab commented Oct 19, 2017

Feedback

  • need some unit tests, and then good to merge probably?

@sgiehl sgiehl changed the title [POC] Allow to provide secondary sort column for reports Allow to provide secondary sort column for reports Oct 20, 2017
@sgiehl
Copy link
Member Author

sgiehl commented Oct 20, 2017

Added some unit tests

@sgiehl sgiehl removed their assignment Oct 20, 2017
@sgiehl sgiehl added Needs Review PRs that need a code review and removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. labels Oct 20, 2017
@sgiehl sgiehl force-pushed the secondarysort branch 2 times, most recently from b889769 to 22a654c Compare March 11, 2018 21:55
@mattab mattab modified the milestones: 3.5.0, 3.4.0 Mar 19, 2018
@sgiehl sgiehl force-pushed the secondarysort branch 2 times, most recently from ca199fd to 3db458c Compare March 30, 2018 21:39
@mattab mattab modified the milestones: 3.6.0, 3.5.0 Apr 9, 2018
@@ -90,7 +93,11 @@ public function filter($table)
$config->primaryColumnToSort = $sorter->getPrimaryColumnToSort($table, $this->columnToSort);
$config->primarySortOrder = $sorter->getPrimarySortOrder($this->order);
$config->primarySortFlags = $sorter->getBestSortFlags($table, $config->primaryColumnToSort);
$config->secondaryColumnToSort = $sorter->getSecondaryColumnToSort($row, $config->primaryColumnToSort);
if (!empty($this->secondaryColumnSortCallback)) {
$config->secondaryColumnToSort = call_user_func($this->secondaryColumnSortCallback, $config->primaryColumnToSort);
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to provide the table as well to the callback function? I'm wondering if it could be useful in some case to make the sort column change based on the contents of the report...

Copy link
Member Author

Choose a reason for hiding this comment

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

added $table as additional param. might be useful in some cases...

@diosmosis diosmosis merged commit 8b5f334 into 3.x-dev Jun 13, 2018
@diosmosis diosmosis deleted the secondarysort branch June 13, 2018 21:05
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Allow to provide secondy sort column for reports

* rename method

* Adds two simple tests to prove secondary column sorting

* invoke sort callback with datatable as second parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants