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

Add UI test for complex segment + fix couple bugs to get test to pass. #13909

Merged
merged 3 commits into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@diosmosis
Copy link
Member

diosmosis commented Dec 26, 2018

Fixes #11384

Bugs fixed:

  • When segment selector redirects to new segment, must make sure value is triple encoded to match 3 urldecodes done server side.
  • Quotes and ampersands must be replaced globally throughout whole segment string, not just for first occurrence.
@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Dec 26, 2018

When segment selector redirects to new segment, must make sure value is triple encoded to match 3 urldecodes done server side.

@diosmosis must have missed that. Can you point me to the change or where this is done? 3 urldecodes are quite a few :) were we also not breaking anything when we did that?

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Dec 26, 2018

@tsteur I wrote about it in this PR description: #13481

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Dec 26, 2018

i see guess that makes sense. not sure re possible side effects.

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Dec 26, 2018

These changes should only affect the segment editor (ie, the segment definition that gets saved & what we redirect to). I suppose it might not recognize definitions for old segments that were created improperly... not too sure. Will test a bit more.

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Dec 31, 2018

@diosmosis did you maybe have a chance to do more tests?

@diosmosis

This comment has been minimized.

Copy link
Member

diosmosis commented Jan 3, 2019

Did some more testing, seems to work, couldn't find any issues.

@diosmosis diosmosis modified the milestones: 3.8.0, 3.9.0 Jan 6, 2019

@diosmosis diosmosis merged commit f845d52 into 3.x-dev Jan 22, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@diosmosis diosmosis deleted the 11384-segment-strange branch Jan 22, 2019

@mattab mattab modified the milestones: 3.9.0, 3.8.1 Jan 22, 2019

flyapen added a commit to ICTO/matomo that referenced this pull request Jan 23, 2019

Add UI test for complex segment + fix couple bugs to get test to pass. (
matomo-org#13909)

* Add UI test for complex segment + fix couple bugs to get test to pass.

* fix test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment