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

Jump to bottom

Row evolution on subtables can cause errors randomly #17374

Closed
sgiehl opened this issue Mar 22, 2021 · 3 comments · Fixed by #17383
Closed

Row evolution on subtables can cause errors randomly #17374

sgiehl opened this issue Mar 22, 2021 · 3 comments · Fixed by #17383
Assignees
@sgiehl
Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone
4.3.0

Comments

@sgiehl
Copy link
Member

sgiehl commented Mar 22, 2021

Expected Behavior

Opening a row evolution for a subtable should always work correctly.

Current Behavior

Sometimes when opening a row evolution for a subtable Matomo seems to choose the incorrect datatable to open the row evolution for and tries to open it with an incorrect action (and subtable id), which results in an error.

Steps to Reproduce (for Bugs)

  1. Go to demo.matomo.cloud
  2. Choose Campaign reports
  3. Open a subtable report for one row in the campaign name report
  4. Open a row evolution for one of the subtable records (-->should work)
  5. Open a row evolution for one of the base table records (--> should work)
  6. Open a row evolution for a subtable record again (--> might fail)

See this record:

row_evolution_bug.mp4

Locally this results in

Call to undefined method Piwik\DataTable\Map::getRowFromLabel() in /srv/matomo/plugins/MarketingCampaignsReporting/API.php line 82

The failing request tries to open a row evolution with a popover parameter like RowAction$3ARowEvolution$3AMarketingCampaignsReporting.getKeywordContentFromNameId$3A$257B$2522idSubtable$2522$253A$25222$2522$257D$3Aemail-jan2019$20$3E$20$40text, which is obviously incorrect, as the action of the parent table should be used. Correct would be RowAction$3ARowEvolution$3AMarketingCampaignsReporting.getName$3A$257B$2522flat$2522$253A0$257D$3Aemail-jan2019 for this case.

Context

This issue might be the real cause for matomo-org/plugin-MarketingCampaignsReporting#61 and maybe some other errors occurring in relation to row evolution
@sgiehl sgiehl added Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Mar 22, 2021
@sgiehl sgiehl added this to the 4.3.0 milestone Mar 22, 2021
@diosmosis
Copy link
Member

happened on the first try for me

@sgiehl sgiehl mentioned this issue Mar 22, 2021
Closed
@sgiehl
Copy link
Member Author

sgiehl commented Mar 22, 2021

it's seems to be kind of randomly. I guess the javascript uses the "wrong" instance to trigger the evolution, or at least the instance that is used is linked to the wrong datatable or something like that.

@diosmosis
Copy link
Member

@sgiehl found that this bug has something to do w/ this code working incorrectly: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/CoreHome/javascripts/dataTable_rowactions.js#L167-L171

When we click the row evolution icon, it will trigger that code on the row action instance for the subtable row. The code notices it's in a subtable, and triggers the action for the main row, but with parameters that correctly compute the label for the subtable row.

Up to this point, this.dataTable (for the row action) points to the subtable's instance. When the event is triggered again, it goes to https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/CoreHome/javascripts/dataTable_rowactions.js#L147. When everything is working correctly, that self.dataTable will be the parent table instance (where self.dataTable.params.method = 'MarketingCampaignReporting.getName'). When it fails, that instance will be the subtable instance (where self.dataTable.params.method = 'MarketingCampaignReporting.getKeywordFromNameId').

I'm not sure why those lines fail, I'm guessing the trigger call works on the wrong row instance. It's possible the rowactions are being created strangely or more than once, but I can't find evidence of that.

@sgiehl sgiehl self-assigned this Mar 23, 2021
@sgiehl sgiehl mentioned this issue Mar 23, 2021
Merged
10 tasks
@diosmosis diosmosis added Regression Indicates a feature used to work in a certain way but it no longer does even though it should. and removed Regression Indicates a feature used to work in a certain way but it no longer does even though it should. labels Mar 23, 2021
@ElLorans ElLorans mentioned this issue Mar 15, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgiehl sgiehl

Labels
Bug For errors / faults / flaws / inconsistencies etc. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging a pull request may close this issue.

Ensure correct rowaction instances are used matomo-org/matomo
2 participants
@diosmosis @sgiehl