WIP: Remove duplicate escaping from branch dropdown

What does this MR do?

Remove one of the two places where branch names are escaped from the switch branch dropdown.

Why was this MR needed?

Branch names were escaped twice such that switching branch didn't work for certain branch names (e.g. feature/#1234).

• first place: escape()
• second place: $form.serialize()

see also https://jsfiddle.net/mep7v3rz/

What are the relevant issue numbers?

fixes #21248 (closed), #20759 (closed)

cc @smcgivern

@iamphill Can you please have a look at this?

Reassigned to @iamphill

Added 8 commits:

• bdaa9e9d...f2b36bfd - 7 commits from branch gitlab-org:master
• 04a996d3 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 8 commits:

• bdaa9e9d...f2b36bfd - 7 commits from branch gitlab-org:master
• 04a996d3 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 11 commits:

• 04a996d3...739620fe - 10 commits from branch gitlab-org:master
• acf5edf0 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 10 commits:

• acf5edf0...02591b04 - 9 commits from branch gitlab-org:master
• bd21565a - Remove duplicate escaping from branch dropdown (!5956 (closed))

@winniehell this fails a spinach spec, which looks legit! https://gitlab.com/winniehell/gitlab-ce/builds/3375111

Also, @iamphill is off this week, maybe try @alfredo1 or @jschatz1.

Also also, I think this missed 8.11.1 which is ready-pending-Omnibus.

Reassigned to @winniehell

this fails a spinach spec, which looks legit!

@smcgivern Yep, thank you for the ping. I just didn't have enough time to look into it.

I think this missed 8.11.1

Thank was expected—at least by me. It got delayed a few times by failing tests on master.

Somewhere another escaping of ' is done such that it ends up being %2527. That breaks the spinach test.

Added 1 commit:

• 4263617c - Remove duplicate escaping from branch dropdown (!5956 (closed))

@smcgivern The failing test was caused by this line. Because value now correctly contains a ' this concatenation didn't work.

OK It would be nice to build it with just selectors, but this works too.

@smcgivern Finally got escaping the ' to work...

I was curious and made a small performance test. It clearly shows we want to have the CSS selector: https://jsfiddle.net/zc0vd5sy/2/

This line, again, caused failing tests. In case value was undefined it was still added to the string, leading to "input[name='fieldName'][value='undefined']" which intentionally didn't match anything. I found that behavior a bit obscure.

Added 1 commit:

• 6ad7d006 - Remove duplicate escaping from branch dropdown (!5956 (closed))

@alfredo1 Can you review please?

Reassigned to @alfredo1

Resolved all discussions

Resolved all discussions

Added 1 commit:

• 1af12998 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 1 commit:

• 84580701 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Several tests failing, not sure yet why. Looks like value is undefined.

Reassigned to @winniehell

Marked this merge request as a Work In Progress

Added 1 commit:

• 7f8ed082 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 1 commit:

• 4b71c9f5 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Added 35 commits:

• 4b71c9f5...f52cf56e - 34 commits from branch gitlab-org:master
• b9febf80 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Reassigned to @alfredo1

Unmarked this merge request as a Work In Progress

Solved.

Added 470 commits:

• b9febf80...1c4e8663 - 469 commits from branch gitlab-org:master
• 71d19335 - Remove duplicate escaping from branch dropdown (!5956 (closed))

Deleted source branch ref-switch-escape

Target branch changed from master to multi-threading

Target branch changed from multi-threading to master

Added 1 commit:

• e417fc7f - Remove duplicate escaping from branch dropdown (!5956 (closed))

Restored source branch ref-switch-escape

Added 1 commit:

• cf42a3e6 - 1 commit from branch gitlab-org:master

mentioned in merge request !6237 (closed)

mentioned in issue #21880 (closed)

@winniehell will you be adding tests to this?

@ClemMakesApps That definitely makes sense! Not sure when I will get that done though...

Marked this merge request as a Work In Progress

Reassigned to @winniehell

@ClemMakesApps I find it very difficult to write tests for the dropdown because it encapsulates so much logic in it. If you know a way, feel free to replace this merge request by yours!

Yeah, I was getting stuck on it too @winniehell. Perhaps we can add a new test case that has a value containing spaces? (If that's not already in there, I'm not 100% sure)

mentioned in merge request !6313 (merged)

!6313 (merged) does in fact fix this :) Thanks for making me check :')

Status changed to closed