[AUT-3670] fix: removing hb special symbols around placeholder

[KirylHatalski]

Ticket: AUT-3670

What's Changed

• Fixed the saving of item interaction when it have curly braces around placeholder

Demo

Env

http://test-kiril.playground.kitchen.it.taocloud.org:40351/tao/Main/login
admin/admin

[viktar-dzmitryieu-tao]

Thought: if '{' is a { sign - can we use a standard HTML entity for this: '{'
and possibly use the regular expression in order to match all occurrences:
.replace(/\{/g, '{').
Could you please test if it will work in your case?

[jsconan]

thought: I'm not sure { is better than {. It is less readable. The numeric version is acceptable when replacing chars with entities using their Unicode value without bothering about a dictionary. But I'd rather suggest, whenever possible, to always use the named entities for the sake of readability.

[KirylHatalski]

Good catch regarding single occurrence, but HB parse brokes only first open brace in the begin of placeholder, other { can be ignored.
Agreed with @jsconan { is more readable

[jsconan]

While the change applies (the { is replaced with the HTML entity), it does not fix the issue.

Moreover, it is not robust enough for surviving to:

• multiple elements in the same template
• serial duplicates

[jsconan]

thought: I'm not sure { is better than {. It is less readable. The numeric version is acceptable when replacing chars with entities using their Unicode value without bothering about a dictionary. But I'd rather suggest, whenever possible, to always use the named entities for the sake of readability.

[jsconan]

suggestion: The temporary replacement with the final value for transforming the remaining keywords is not robust as this final value may exist in other places. I'd rather suggest an approach where possible collision is avoided. Moreover, the tpl variable is shared for the whole loop and it may introduce issues when multiple elements are used. A better solution should be implemented.

Regarding the placeholder replacer, one could do this:

Suggested change

	tpl = tpl
	.replace(elt.placeholder(), serial)
	.replace('{', '{')
	.replace(serial, '{{{' + serial + '}}}');
	// Use a robust placeholder for replacing the element while converting entities
	const tmp = `<!##${Math.floor(Math.random() * Math.pow(2, 32)).toString(16)}##!>`;
	tpl = tpl
	.replace(elt.placeholder(), tmp)
	// Replace each occurrence of braces with computed HTML entities
	.replace(/([{}])/gm, (match, group) => `&#${group.charCodeAt(0)};`)
	.replace(tmp, '{{{' + serial + '}}}');

However, it will still create issues when having multiple elements in the same template, See:

And with the original proposed solution:

thought: I'm not sure we can replace all braces this way. An approach would be to rewrite the loop, splitting it in several occurrences: a first loop to remove the elements with robust placeholders, another loop to neutralize the braces, and finally a last loop to replace back elements with their serial... I'll let you figure out the implementation of this.

[KirylHatalski]

Thanks for suggestions. I changed a bit approach of replacing.
As I see, serials is unique for each item and this replacement is happen for every item in container, so I not really sure about collision possibility.
Also important detail -- only open brace before placeholder breaks the flow

[github-actions]

Version

Target Version	2.3.2
Last version	2.3.1

There are 0 BREAKING CHANGE, 0 feature, 2 fixes

[github-actions]

Coverage Report

Totals
Statements:	67.42% ( 9158 / 13583 )
Methods:	69.48% ( 1054 / 1517 )
Lines:	71.93% ( 5437 / 7559 )
Branches:	59.17% ( 2667 / 4507 )

[jsconan]

Indeed the last changes are way better, with a smarter approach. Good job!

Review Checklist

• New code is covered by tests (if applicable)
• Tests are running successfully (old and new ones) on my local machine (if applicable)
• New code respects code style rules
• New code respects best practices
• New code is not subject to concurrency issues (if applicable)
• Feature is working correctly on my local machine (if applicable)
• Acceptance criteria are respected
• Pull request title and description are meaningful
• Pull request's target is not master
• Commits are following conventional commits
• Commits messages are meaningful
• Commits are atomic

[viktar-dzmitryieu-tao]

• New code is covered by tests (if applicable)
• Tests are running successfully (old and new ones) on my local machine (if applicable)
• New code is respecting code style rules
• New code is respecting best practices
• New code is not subject to concurrency issues (if applicable)
• Feature is working correctly on my local machine (if applicable)
• Acceptance criteria are respected
• Pull request title and description are meaningful
• Pull request's target is not master