@diosmosis opened this Pull Request on January 6th 2019 Member

Changes:

  • Allow annotations API to accept multiple periods (ie, date = 2018-02-01,2018-02-24 / period = day) so it will work when evolution graphs use those kinds of params.
  • Remove warning that is emitted when rows_to_display is left at false.
  • Allow individual cells in an HTML table based visualization to be styled.
  • Remove a TODO that isn't needed.
@diosmosis commented on January 28th 2019 Member

Actually shouldn't merge just yet, forgot to mention the latest change will require changes to premium plugins, since the 'rows' query param appears to be handled only in plugins not in core.

@tsteur commented on January 28th 2019 Member

Actually shouldn't merge just yet, forgot to mention the latest change will require changes to premium plugins, since the 'rows' query param appears to be handled only in plugins not in core.

Haven't looked at code or anything... will it be possible to keep BC? Or at least update plugins to be compatible with all Matomo versions?

@diosmosis commented on January 28th 2019 Member

I'm not to sure about BC. The change was to encode the values in the rows parameter so a row could have a comma in it (eg, Tue, Jan 28 or whatever). Otherwise the rows param would interpret it as 2 rows, 'Tue', 'Jan 28'. The problem is plugins use 'rows' to to set the rows_to_display ViewDataTable config, it's not done in core, so the corresponding urldecode has to be done in the plugins.

I guess we could modify the 'rows' query parameter in FrontController or something like it to provide BC (ie, urldecode it there before it gets to the evolution graph controller methods)... but I'm not sure how good a solution that is... We can certainly update premium plugins. Shouldn't be an issue for previous matomo versions since I wouldn't expect there to be urlencoded data in a row label (at least for plugins that support 'rows').

@mattab commented on February 13th 2019 Member

I'm not to sure about BC.

So is the best next step to release new versions of all premium plugins which use this feature (how many plugins are affected, both free/premium?) to be compatible with this latest Matomo/patch? @diosmosis @tsteur

@diosmosis commented on March 2nd 2019 Member

@tsteur finally got the tests to pass, I think this could use another review. One big change: turns out the code always copied the last log's properties, even if the last log was part of a different visit. Since we only looked back the visit standard length time, this wasn't an issue, but now since we can look further back, I had to modify the VisitRecognizer code more.

@tsteur commented on March 12th 2019 Member

Looks good to merge for me if tests pass 👍

@diosmosis commented on March 12th 2019 Member

@tsteur looks like the silent fail might be a feature: https://github.com/matomo-org/matomo/blob/3.x-dev/tests/PHPUnit/Fixtures/TwoSitesEcommerceOrderWithItems.php#L142-L147

I guess that means I should remove the new exception... not sure how else we could report the issue, maybe a tracker failure in the UI, but then those "receipt views" might result in a lot of tracker failures...

@tsteur commented on March 12th 2019 Member

@diosmosis maybe we could update the test to expect an error instead of self::checkResponse()? In general it be great to fail there so they have a chance to notice there was possibly something wrong. We could of course also just keep to silently fail as it does nothing in the end.

@diosmosis commented on March 12th 2019 Member

@tsteur I thought about that, but the specific case linked is refreshing a "receipt" page after the order is tracked, which seems like it could happen regularly. Ie, this might be a common occurrence and not just an error. Actually I guess it wouldn't be a big issue if we gave a 400... users would likely know that it's fine for them.

@tsteur commented on March 12th 2019 Member

Have no preference really, I reckon either is fine. I was thinking if there could be edge cases where it's maybe a problem to send the same request twice but can't really think of anything right now.

@diosmosis commented on March 12th 2019 Member

@tsteur will just update the tests, think it should be ok if in these cases there's an error response.

This Pull Request was closed on March 13th 2019
Powered by GitHub Issue Mirror