Skip to content
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

Jump to bottom

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

Merged
merged 3 commits into from Jan 22, 2019
Merged

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

merged 3 commits into from Jan 22, 2019

Conversation

diosmosis
Copy link
Member

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.

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Dec 26, 2018
@diosmosis diosmosis added this to the 3.8.0 milestone Dec 26, 2018
@tsteur
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
Copy link
Member Author

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

@tsteur
Copy link
Member

tsteur commented Dec 26, 2018

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

@diosmosis
Copy link
Member Author

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
Copy link
Member

mattab commented Dec 31, 2018

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

@diosmosis
Copy link
Member Author

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
@diosmosis diosmosis deleted the 11384-segment-strange branch January 22, 2019 04:24
@mattab mattab modified the milestones: 3.9.0, 3.8.1 Jan 22, 2019
@c960657 c960657 mentioned this pull request Jan 24, 2019
Closed
@c960657
Copy link
Contributor

c960657 commented Jan 24, 2019

This introduced a new test failure. Fixed in #14021.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
3.8.1
Development

Successfully merging this pull request may close these issues.

Add a UI test for a complex segment AND/OR/ and strange encoding
4 participants
@diosmosis @tsteur @mattab @c960657