@tsteur opened this Pull Request on September 5th 2019 Member

Follow up from https://github.com/matomo-org/matomo/pull/14848 refs https://github.com/matomo-org/matomo/issues/14535

When I tried to apply this change locally, I got this error

An error occurred when trying to change the field 'name' via

ALTER TABLE `log_action` CHANGE `name` `name` VARCHAR(65000)
 CHARACTER SET utf8
 COLLATE utf8_general_ci
 NULL
 DEFAULT ''

MySQL said: Column length too big for column 'name' (max = 21845); use BLOB or TEXT instead

It seems the column length is too big.

For log_conversion.url even only varchar(16000) works since there are so many more columns. I reckon it might be best to leave it as text. It be not good to have different lengths of url fields and not sure if by using varchar(16000) on log_conversion we might prevent in the future our chances of adding more columns...

fyi @mattab

@diosmosis commented on September 5th 2019 Member

Not sure how it's related to this PR, but the tests appear to be failing

@tsteur commented on September 5th 2019 Member

Not understanding it... thought maybe we use really long action names but that doesn't seem to be the case in https://travis-ci.org/matomo-org/matomo/jobs/580980359#L848

@mattab commented on September 10th 2019 Member

Feedback:

@tsteur commented on September 11th 2019 Member

Changed it to 4096 and fixed the tests.

The results in tests/PHPUnit/System/expected/ are correct. It is only the sorting that slightly changed because of the varchar when there is eg the same amount of visits.

The results in plugins/Contents/tests/System/expected/ are correct too. Took me quite some time to look into it and 100% understand things. The contentTarget for example changed because there are multiple impressions with the same content name but different targets see https://github.com/matomo-org/matomo/blob/3.12.0-b3/plugins/Contents/tests/Fixtures/TwoVisitsWithContents.php#L79-L84 where we use 'Click to download Piwik now', '' but also 'Click to download Piwik now', 'http://piwik.org/download'.

We actually do not show the content target in the UI (which would make it 3 level report). We're currently for some reason adding contentTarget to the report but the contentTarget value we use in the report is random if there are multiple content names/pieces with different targets. So no logic changed there and things are the same. Ideally at some point we may want to look at removing content target from the report and / or implement targets correctly see https://github.com/matomo-org/matomo/issues/6267 Not changing any logic there for now for this.

Ready for another review. Hoping the tests pass now.

@diosmosis commented on September 11th 2019 Member

Still one test failure, but I think it's expected?

@tsteur commented on September 12th 2019 Member

@diosmosis it seems the tests are failing now randomly and have different IDs on every request or different results in general :( the same tests are always failing in different ways. We'll need to see how this can be fixed... will likely take quite a while.

@tsteur commented on October 2nd 2019 Member

@katebutler can you also update the expected test results so the tests pass?

@tsteur commented on October 2nd 2019 Member

@katebutler is maybe this test failing still randomly? https://travis-ci.org/matomo-org/matomo/jobs/592761199#L820-L850
If that's the case we may need to have a secondary sort order column in the Sort datatable filter. This could fix this issue also for other reports in the future in https://github.com/matomo-org/matomo/blob/3.12.0-b4/core/DataTable/Filter/Truncate.php#L97 I reckon it could be good to enable $doSortBySecondaryColumn = true parameter

@tsteur commented on October 9th 2019 Member

🚀

This Pull Request was closed on October 9th 2019
Powered by GitHub Issue Mirror