search

chenejac / VIVOTestMigrationJIRA

0 stars 0 forks source link

VIVO-1842: i18n - Freemarker templates that insert values into Javascript code do not always handle single quotes properly #1733

Closed chenejac closed 3 years ago

chenejac commented 4 years ago

Brian Lowe (Migrated from VIVO-1842) said:

As discussed on the [21 Apr 2020 developers' call|[https://wiki.lyrasis.org/display/VIVO/2020-04-21+-+VIVO+Development+IG]], templates that insert strings into Javascript tend to cause errors when those strings contain single quotes/apostrophes, as is likely to occur in French-language VIVOs.

Any such templates should be examined.  The following Freemarker built-ins may be useful: 

[https://freemarker.apache.org/docs/ref_builtins_string.html#ref_builtin_js_string]

[https://freemarker.apache.org/docs/ref_builtins_string.html#ref_builtin_json_string]

chenejac commented 4 years ago

Andrew Woods said:

Example of the current workaround: $ cd VIVO $ diff -b ./webapp/src/main/webapp/templates/freemarker/edit/forms/manageGrantsForIndividual.ftl ../VIVO-languages/fr_CA/webapp/src/main/webapp/templates/freemarker/edit/forms/manageGrantsForIndividual_fr_CA.ftl

...produces:

<     itemSuccessfullyExcluded: '${i18n().grant_successfully_excluded}',
<     errorExcludingItem: '${i18n().error_excluding_grant}'
---
>     itemSuccessfullyExcluded: "${i18n().grant_successfully_excluded}",
>     errorExcludingItem: "${i18n().error_excluding_grant}"
chenejac commented 4 years ago

Nicolas B Dickn said:

[~accountid:557058:3f53c733-6ac2-4e92-9f12-e34e297283a3] Discussing this with [~accountid:60785ded115da6006f540529] , I pointed out that processing this ticket in a comprehensive manner would mean to modify a huge amount of files. Since the problem was only noticed when fr_CA properties would cause JS unbalanced quotation, we decided to replace single quotes to double quotes only in the occurrences identified in the fr_CA Freemarker files.

If it is deemed necessary to uniformize the entire code, then we could reopen the ticket.

chenejac commented 4 years ago

Nicolas B Dickn said:

-[https://github.com/vivo-project/VIVO/pull/178]- -[https://github.com/vivo-project/Vitro/pull/172]- -[https://github.com/vivo-project/Vitro-languages/pull/25]- -[-https://github.com/vivo-project/VIVO-languages/pull/60-]-

 

Reverted to Draft. While checking some unrelated thing, I stumbled upon a case of variable declaration with single quotes that was not resolved in the fr_CA custom FreeMarker. Therefore, a more thorough modification of the core files is needed.

chenejac commented 4 years ago

Nicolas B Dickn said:

More infos : the fr_CA files are fixing (most of) the cases where the string passed to the variable comes from a .properties file. However, the strings can also come from the database, and might include single quotes (see screenshot included). Since it's impossible to foresee every possible case, a comprehensive conversion of single quotes to double quotes is indeed needed.

 

!image-2020-06-30-16-03-47-598.png!

chenejac commented 4 years ago

Brian Lowe said:

If we're going to do a comprehensive change of multiple files, I would advocate doing it the right way the first time.  Just switching to double quotes will just make the problem reappear at some point when someone (intentionally or not) ends up with double quotes in their data.  I believe the ?js_string builtin is the correct way of escaping Javascript literal values but this merits confirmation.

chenejac commented 4 years ago

Andrew Woods said:

It would seem that the most robust approach will be to both:

Some experimentation and verification should first be performed to validate this (or another) approach.

chenejac commented 4 years ago

Dominik Feldschnieders said:

I can confirm that, by using the  function js_string ([https://freemarker.apache.org/docs/ref_builtins_string.html#ref_builtin_js_string]), the problem would be solved and there is no need to replace single quotes with double quotes. Or am i missing something?

With using the json_string function ([https://freemarker.apache.org/docs/ref_builtins_string.html#ref_builtin_json_string]) the single quotes need to be replaced with double quotes because single quotes will not be escaped. So why using this function and replace the quotes when we can use js_string?

chenejac commented 4 years ago

Andrew Woods said:

If 'js_string' solves the problem... let's do that.

chenejac commented 4 years ago

Dominik Feldschnieders said:

I added the function js_string at all places that seemed necessary to me. After some research and testing i figured out that there is no need to add js_string in html-tags and single #assign statements. In these cases the quotes will be automatically escaped.  There were still a lot of files that had to be changed. I think i got them all. I changed not only the ones that cause problems but all those that might cause problems in the future (with additional languages).

PR VIVO: [https://github.com/vivo-project/VIVO/pull/181] (with a short description for testing but i think this ticket as enough information for this) PR Vitro: [https://github.com/vivo-project/Vitro/pull/180] PR VIVO-languages: [https://github.com/vivo-project/VIVO-languages/pull/64] PR Vitro-languages: [https://github.com/vivo-project/Vitro-languages/pull/29]

chenejac commented 4 years ago

Dominik Feldschnieders said:

I have only added you (Andrew) as a reviewer, did not know if i should Brian as well.

chenejac commented 4 years ago

Andrew Woods said:

[~accountid:6113d5cd4e8d8d0069f2e486]: nicely done. JIRA only appears to provide for one "reviewer". However, [~accountid:557058:3f53c733-6ac2-4e92-9f12-e34e297283a3] gets notifications from both JIRA and GitHub... so we should be covered.

chenejac commented 4 years ago

Andrew Woods said:

One minor typo... then this should be good to go.

chenejac commented 4 years ago

Dominik Feldschnieders said:

i removed the typo

chenejac commented 4 years ago

Andrew Woods said:

Resolved with: https://github.com/vivo-project/VIVO/commit/bd35af354c5ea20fc3732a036e2fcb432d7811eb https://github.com/vivo-project/Vitro/commit/35375ed6988f42c9f9b0824b9d614332c49a9ba6 https://github.com/vivo-project/VIVO-languages/commit/0fcf24069a8eaa3b0f02214087ab95f6c9c29b14 https://github.com/vivo-project/Vitro-languages/commit/f58caf9c3f7aa15bf65516f6927b50293eb964d0

chenejac commented 3 years ago

Nicolas B Dickn said:

Similar fix to a fr_CA.ftl file : https://github.com/vivo-project/VIVO-languages/pull/88

chenejac commented 3 years ago

Andrew Woods said:

Follow-on resolution: https://github.com/vivo-project/VIVO-languages/commit/e13cf4213b88f30466d8db3ff6db74db937d57c2