@diosmosis opened this Pull Request on November 5th 2019 Member

FIxes #15080

Changes:

  • When trying to see whether we should decode a segment, favor encoded segment since it appears to be more common. (This fixes issue in #15080)
  • Replace existing param in URL if it exists but empty (eg, segment=). (this fixes issue that occurs when on All Visits and editing/adding a segment, the URL ends up containing &segment=&segment=...).
  • If editing segment during comparison, remove comparison in case edited segment is one being compared.
@tsteur commented on November 5th 2019 Member

@diosmosis there are quite a few system tests failing: https://travis-ci.org/matomo-org/matomo/jobs/607490406#L849

Not sure this is expected?

@diosmosis commented on November 6th 2019 Member

Tried looking for the cause of the regressions, but couldn't figure out the other encoding issue so I ended up solving this a different way.

@tsteur commented on November 6th 2019 Member

@diosmosis looks good. There's a failing system tests still though https://travis-ci.org/matomo-org/matomo/jobs/607961524#L810 probably from the fixture change in the PR. Not sure that fixture change is still needed or expected? Do you reckon it's possible to add a test for the case it fixes?

@diosmosis commented on November 6th 2019 Member

Shouldn't be a fixture change here, that's probably something that happens locally on os x. Maybe. There's a change to the test file in this PR I'll just remove.

@tsteur commented on November 6th 2019 Member

@diosmosis if a test can be added for this that be great, otherwise feel free to merge.

This Pull Request was closed on November 8th 2019
Powered by GitHub Issue Mirror