Display all product categories if present in the visitor log #14083
Conversation
diosmosis
added
not-in-changelog
| @@ -370,7 +370,11 @@ protected function renderDataTable($array, $prefixLine = "") | |||
| foreach ($row as $name => $value) { | |||
| // handle the recursive dataTable case by XML outputting the recursive table | |||
| if (is_array($value)) { | |||
| $value = "\n" . $this->renderDataTable($value, $prefixLine . "\t\t"); | |||
| if (is_array(reset($value))) { | |||
There was a problem hiding this comment.
can you add a (system) test for this maybe?
There was a problem hiding this comment.
Technically since it's used in the <categories> array, it's already tested in this PR. If possible I can add a unit test as well.
| $suffix = $i === 0 ? '' : $i; | ||
| $column = $i === 0 ? 'idaction_category' : ('idaction_category' . ($i + 1)); | ||
| $categorySelects[] = 'log_action_category' . $suffix . '.name as itemCategory' . $suffix; | ||
| $categoryJoins[] = 'LEFT JOIN ' . Common::prefixTable('log_action') . " AS log_action_category$suffix |
There was a problem hiding this comment.
I reckon performance won't be a big problem because of adding so many more joins but may be still good to do (maybe we have many categories in the demo on staging?).
There was a problem hiding this comment.
You mean performance test on staging?
There was a problem hiding this comment.
Yeah I wonder if we should deploy the change on the server and check if it still loads as fast when eg loading 500 or 1000 ecommerce log entries compared to before? not sure we have many entries with categories though
There was a problem hiding this comment.
Could always add some more categories to the test data and load it on staging if not. Would take a couple days though.
There was a problem hiding this comment.
Nah not needed to add more categories. Not worth it I think
|
UI and System test files need an update. |
|
I added an extra category to a system test and regenerated OmniFixture (not sure there was an existing fixture w/ multiple categories), hopefully it will show up in the ecommerce log UI test result. |
|
Looks good to merge once tests pass. There are a few failing system tests and to fix the UI tests might need to be merge 3.x-dev into it. Couldn't see multiple categories in a UI test but I think it's not needed. |
Fixes #14072