fix #7332 - saving vis with % in name causes error

[ppisljar]

Fixes #7332

• slugify_id.js utility was updated to replace % sign with -percentage-
• tests for slugify_id utility were updated

I was looking for UI test of saving visualization but couldn't locate it .. can you please help me locate it ?

[Bargs]

LGTM

You probably already found them, but I think these are the tests you were looking for https://github.com/elastic/kibana/blob/e7189e5349f91887e7eb1a0861e2613755075365/test/functional/apps/visualize/index.js

[ppisljar]

would this be considerent sufficient test ?

• i changed one of the tests where it saves and loads visualization to actually use a name with % inside ...
• as opposed to adding a seperate test just for this (as saving and loading is already tested with each chart type i think this is more efficent and tells you enought about test-case if it fails

[Bargs]

In theory I'm a fan of anything that speeds up the functional tests, but without an explicit test case I don't think future readers will understand what the % is for, and might remove it. Maybe you could add one test case for all special characters in slugify_id (like literally test creating a viz with the title /?&=%)?

[ppisljar]

i added the test but need to fix #7733 first for them to pass

[ppisljar]

strings with special charracters are not found unless we add a space after it (searching for "test %*" wont work but searching for "test % *" will )

i added a simple check if the last character in the query string is a special one additional space is appended before the asterisk *

let me know if this looks ok to you ?

[Bargs]

strings with special charracters are not found unless we add a space after it (searching for "test %*" wont work but searching for "test % *" will )

I don't think this is quite the right solution. As far as I can tell, foo=* wouldn't match the title foo= because queries using * don't get analyzed, whereas the visualization title field is analyzed. What this means is, the title foo= would get stored under just foo in the inverted index, because the term is split into tokens on word dividers like = (based on the field's analyzer). A query with foo=* will try to find an exact match for foo=, which it won't find.

The reason why adding a space sort of works is that it's splitting the query string into two tokens: foo and *. That's my understanding at least, analyzed fields are complicated. So the downside to adding a space after special chars is that the list of special chars we need to handle is based on the field analyzer defined in the elasticsearch mapping, and we probably don't want to couple these things together.

Long story short, I think the correct solution here is to add the analyze_wildcard: true param to the query body. This will split a query like foo=* into the tokens we want and successfully match the title foo=. This solution would also match a title like foo=bar with a query string like foo=b, which neither the current code or the whitespace based solution would handle correctly.

[Bargs]

I don't think there's really any reason to wait for the toast message, load the viz and wait for it if we're not running assertions on it. I think the only reason the test below does so is to set things up for the test that follows it.

[Bargs]

This screenshot uses an identical name to the one saved in the test below. Also, I don't think there's a need to save a screenshot at all here, right @LeeDr?

[Bargs]

Looks like the name check in the new test is failing.

[LeeDr]

I agree that a screenshot is not necessary.

[ppisljar]

test needs to wait for the toast message to dissappear, else it ruins the next test

[Bargs]

LGTM. Make sure to get one more reviewer, maybe @LeeDr could take a look since this more tests than anything?

[LeeDr]

LGTM