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 "Pivot is not correctly remembered" #9951
Conversation
{ | ||
// Both are the same HtmlTable, just the Pivot one has some extra logic in case Pivot is used. | ||
// We don't want to use the same HtmlTable twice in the UI. Therefore we always need to remove one. | ||
if (Common::getRequestVar('pivotBy', '')) { |
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.
This actually caused the problem, as for an initially loaded report no request vars are set.
@@ -143,7 +143,8 @@ public function configureViewDataTable(ViewDataTable $view) | |||
$secondaryDimension = $this->getSecondaryDimensionFromRequest(); | |||
$view->config->subtable_controller_action = API::getInstance()->getActionToLoadSubtables($apiMethod, $secondaryDimension); | |||
|
|||
if (Common::getRequestVar('pivotBy', false) === false) { | |||
$pivotBy = Common::getRequestVar('pivotBy', false); | |||
if (empty($pivotBy)) { |
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.
When unsetting the pivot pivotBy
is set to the string 0
. This wasn't catched here, so the displayed columns were incorrect in this case.
|
||
public function beforeLoadDataTable() | ||
{ | ||
$this->requestConfig->request_parameters_to_modify['pivotBy'] = null; // always unset pivotBy |
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.
this fixes another issue that was occurring when switching to all columns view after pivot was activated.
Have you seen the 2 failing UI tests? http://builds-artifacts.piwik.org/piwik/piwik/pivot_9667/18398/PivotByDimension_pivoted_columns_report and http://builds-artifacts.piwik.org/piwik/piwik/pivot_9667/18398/PivotByDimension_pivoted ? I tried to find a CSS selector that matches |
@tsteur Yes, the small changes in the UI tests are because the css class of the datatable changed. But it is somehow expected. |
Good find @sgiehl ! 👍 |
Moving all logic from the separate
PivotBy
viz to theHtmlTable
viz solves the problem.fixes #9667
might also be a solution for #9716