@sgiehl opened this Pull Request on February 20th 2017 Member

I've removed all encodeURI/decodeURI stuff from the segments. Angular already encodes it in the URL automatically, so it was encoded twice in some cases.
I've currently tested the changes successfully on:

  • Internet Explorer 11
  • Chrome 56
  • Firefox 47
  • Vivaldi 1.7

Hope I haven't missed any test case...

Maybe anyone could test it on some other browsers @mattab @tsteur

side note: as the double encoded segments worked in many cases before, it might eventually be possible that this changes breaks some bookmarks having double encoded segments in it. But I haven't found any so far

fixes #11321
maybe refs #10973

@mattab commented on February 20th 2017 Member

Seems to work well :+1: well done. I would suggest to release this in a beta ASAP and ask wider feedback

@sgiehl commented on February 20th 2017 Member

Ok. Sounds good. Let's wait for the tests to pass and then we can merge and release a beta

@sgiehl commented on February 20th 2017 Member

Build fixed. Merging now :)

@tsteur commented on February 20th 2017 Member

What I would recommend to test is the segment selector in exported widgets (if not done yet). Also to eg load it with a segment in the URL and hash, with a segment in the URL but different segment in hash, only a segment in hash. Each of them then afterwards also change it a few times etc. Also each case would be ideally tested with lots of multiple different conditions and with &"'#... in the values to be sure to not introduce a regression again. I remember there were some problems that time that needed some extra encoding - I think.

@sgiehl commented on February 20th 2017 Member

Tested the segment selector in widgetized dashboad and also in various combinations in url, hash, or directly selecting or adding one. But I'm not sure if I missed one.
Maybe it would be good to create a follow up issue to create UI tests for every test case that comes up to our mind. So we can prove everything works and will work in the future.

@tsteur commented on February 20th 2017 Member

👍 on all the tests. It is not trivial...

One thing I noticed that doesn't work is when editing a segment like this and then "saving". It shows "custom segment" afterwards and I'm not sure if it's actually applied. But since @mattab changed the "save" logic to Piwik 2 logic a while ago it's likely not be a regression.

screenshot at feb 21 10-46-54

This Pull Request was closed on February 20th 2017
Powered by GitHub Issue Mirror