Bug 38164 - Translation process is broken
Summary: Translation process is broken
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: I18N/L10N (show other bugs)
Version: 24.05
Hardware: All All
: P5 - low blocker
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Jonathan Druart
URL:
Keywords: RM_priority
Depends on: 37303
Blocks:
  Show dependency treegraph
 
Reported: 2024-10-14 07:20 UTC by Katrin Fischer
Modified: 2024-10-23 22:52 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.05
Circulation function:


Attachments
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version" (9.60 KB, patch)
2024-10-14 13:06 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38164: Skip fuzzy translations (1.85 KB, patch)
2024-10-14 13:06 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version" (9.66 KB, patch)
2024-10-14 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 38164: Skip fuzzy translations (1.91 KB, patch)
2024-10-14 20:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version" (9.65 KB, patch)
2024-10-14 20:32 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38164: Skip fuzzy translations (1.94 KB, patch)
2024-10-14 20:32 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version" (9.71 KB, patch)
2024-10-14 20:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38164: Skip fuzzy translations (2.00 KB, patch)
2024-10-14 20:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38164: Update yarn.lock for removal of po2json.js (6.58 KB, patch)
2024-10-14 23:53 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version" (9.79 KB, patch)
2024-10-15 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38164: Skip fuzzy translations (2.07 KB, patch)
2024-10-15 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-10-14 07:20:42 UTC
As discussed on bug 37303 we do now require yarn for running the translation scripts because of po2json.

We need to fix this urgently as it means that production installations updating (and with security releases they should) can no longer install and update languages. 

We already had several reports in Mattermost and on the mailing list.
Comment 1 Magnus Enger 2024-10-14 07:29:53 UTC
Which versions will be affected by this?
Comment 2 Magnus Enger 2024-10-14 07:30:39 UTC
(In reply to Magnus Enger from comment #1)
> Which versions will be affected by this?

Ah, probably the ones that have bug 37303? So 24.11.00 and 24.05.04?
Comment 3 Katrin Fischer 2024-10-14 08:49:38 UTC
(In reply to Magnus Enger from comment #2)
> (In reply to Magnus Enger from comment #1)
> > Which versions will be affected by this?
> 
> Ah, probably the ones that have bug 37303? So 24.11.00 and 24.05.04?

That should be correct. I am setting keywords.
Comment 4 Tomás Cohen Arazi (tcohen) 2024-10-14 13:06:36 UTC
Created attachment 172714 [details] [review]
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version"

This reverts commit 4eb981635453871fa2a33396391f3f75a6baa8b1.
Comment 5 Tomás Cohen Arazi (tcohen) 2024-10-14 13:06:39 UTC
Created attachment 172715 [details] [review]
Bug 38164: Skip fuzzy translations

This patch was envisioned by Julian Maurice.

Test plan:
1. Go to `misc/translator/po`
2. es-ES-messages-js.po
3. Mark "Edit" string as fuzzy (around line 48). It should look like:

```
msgid "Edit"
msgstr "Editar"
```

4. Edit ./intranet-main.tt and add the following lines at the bottom,
   inside the `$(document).ready` block:

```javascript
  console.log(_("Edit"));
  console.log(__("Edit"));
```

5. Install the templates
  k$ koha-translate --install es-ES --dev kohadev && restart_all
6. Enable *es-ES* by searching for `language` in the sysprefs and switch
   to it for the staff interface.
7. Go to the Koha home page, open the browser console
=> FAIL: Notice that the second log in the console is displaying the
fuzzy string (i.e. is being translated when it shouldn't)
8. Apply this patch
9. Re-install the translated templates:
  k$ koha-translate --update es-ES --dev kohadev && restart_all
10. Repeat 7
=> SUCCESS: With this patch applied both logs show the English version of the
string.
11. Remove the fuzzy flag on `es-ES-messages-js.po`
12. Repeat 9 and 10
=> SUCCESS: The results are similar to step 7, but in this case they are
expected as the string is not marked fuzzy.
13. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Aleisha Amohia 2024-10-14 20:28:38 UTC
Created attachment 172756 [details] [review]
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version"

This reverts commit 4eb981635453871fa2a33396391f3f75a6baa8b1.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 7 Aleisha Amohia 2024-10-14 20:28:41 UTC
Created attachment 172757 [details] [review]
Bug 38164: Skip fuzzy translations

This patch was envisioned by Julian Maurice.

Test plan:
1. Go to `misc/translator/po`
2. es-ES-messages-js.po
3. Mark "Edit" string as fuzzy (around line 48). It should look like:

```
msgid "Edit"
msgstr "Editar"
```

4. Edit ./intranet-main.tt and add the following lines at the bottom,
   inside the `$(document).ready` block:

```javascript
  console.log(_("Edit"));
  console.log(__("Edit"));
```

5. Install the templates
  k$ koha-translate --install es-ES --dev kohadev && restart_all
6. Enable *es-ES* by searching for `language` in the sysprefs and switch
   to it for the staff interface.
7. Go to the Koha home page, open the browser console
=> FAIL: Notice that the second log in the console is displaying the
fuzzy string (i.e. is being translated when it shouldn't)
8. Apply this patch
9. Re-install the translated templates:
  k$ koha-translate --update es-ES --dev kohadev && restart_all
10. Repeat 7
=> SUCCESS: With this patch applied both logs show the English version of the
string.
11. Remove the fuzzy flag on `es-ES-messages-js.po`
12. Repeat 9 and 10
=> SUCCESS: The results are similar to step 7, but in this case they are
expected as the string is not marked fuzzy.
13. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 8 Victor Grousset/tuxayo 2024-10-14 20:32:42 UTC Comment hidden (obsolete)
Comment 9 Victor Grousset/tuxayo 2024-10-14 20:32:45 UTC Comment hidden (obsolete)
Comment 10 Victor Grousset/tuxayo 2024-10-14 20:34:32 UTC
Created attachment 172760 [details] [review]
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version"

This reverts commit 4eb981635453871fa2a33396391f3f75a6baa8b1.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 11 Victor Grousset/tuxayo 2024-10-14 20:34:35 UTC
Created attachment 172761 [details] [review]
Bug 38164: Skip fuzzy translations

This patch was envisioned by Julian Maurice.

Test plan:
0. Apply the 1st patch (the revert)
1. Go to `misc/translator/po`
2. es-ES-messages-js.po
3. Mark "Edit" string as fuzzy (around line 48). It should look like:

```
msgid "Edit"
msgstr "Editar"
```

4. Edit ./intranet-main.tt and add the following lines at the bottom,
   inside the `$(document).ready` block:

```javascript
  console.log(_("Edit"));
  console.log(__("Edit"));
```

5. Install the templates
  k$ koha-translate --install es-ES --dev kohadev && restart_all
6. Enable *es-ES* by searching for `language` in the sysprefs and switch
   to it for the staff interface.
7. Go to the Koha home page, open the browser console
=> FAIL: Notice that the second log in the console is displaying the
fuzzy string (i.e. is being translated when it shouldn't)
8. Apply this patch
9. Re-install the translated templates:
  k$ koha-translate --update es-ES --dev kohadev && restart_all
10. Repeat 7
=> SUCCESS: With this patch applied both logs show the English version of the
string.
11. Remove the fuzzy flag on `es-ES-messages-js.po`
12. Repeat 9 and 10
=> SUCCESS: The results are similar to step 7, but in this case they are
expected as the string is not marked fuzzy.
13. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 12 Victor Grousset/tuxayo 2024-10-14 20:39:01 UTC
Testing notes: 1st apply the reverting patch.

> 7.
I get
- Editar
- Editar
ok, that's the bug

> 10. «With this patch applied both logs show the
> English version of the string.»
I get
- Editar
- Edit 
????? :o

> 12.
I get
- Editar
- Editar 
ok


I looked at the test plan of bug 37303 and it says «second log in the console is displaying the fuzzy string» 😵‍💫 😵‍💫 😵‍💫 😵‍💫 
(the order of _() and __() was the same)

It seems the thing is working and there is uncertainly of behavior of __() and _() regarding which is/are being affected by the initial issue.

Anyway, it's the same output on main with po2json JS installed.
As far as not needing yarn on production it works :)
(there is still something suspicious about _() outputting fuzzy strings)
Comment 13 Victor Grousset/tuxayo 2024-10-14 20:55:04 UTC
Thanks Aleisha for the signoff, I merged both.


As for QA, the yarn.lock file isn't reverted or updated.
So we have po2json not in package.json anymore but still in the yarn.lock.
And po2json isn't listed as a dependency of anything else.
So it's almost certain it's an issue.

But xt/verify-yarnlock.t is passing ?!?!?!?!?!?!?!
Comment 14 Victor Grousset/tuxayo 2024-10-14 23:53:29 UTC
Created attachment 172762 [details] [review]
Bug 38164: Update yarn.lock for removal of po2json.js
Comment 15 Victor Grousset/tuxayo 2024-10-14 23:57:58 UTC
Apparently to sync yarn.lock with package.json on can just run `yarn`

In our setup in KTD that's:
sudo yarn --modules-folder ../node_modules/
(maybe there is a better way to currently use yarn)

KTD start works. Translating work.
Comment 16 Jonathan Druart 2024-10-15 08:49:57 UTC
(In reply to Victor Grousset/tuxayo from comment #12)
> (there is still something suspicious about _() outputting fuzzy strings)

_("") picks from -staff-prog.po, not -messages-js.po
Comment 17 Jonathan Druart 2024-10-15 08:55:02 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> Created attachment 172762 [details] [review] [review]
> Bug 38164: Update yarn.lock for removal of po2json.js

Skipped this patch, this is a task for RM/RMaints.
Comment 18 Jonathan Druart 2024-10-15 08:57:22 UTC
Created attachment 172766 [details] [review]
Bug 38164: Revert "Bug 37303: Replace po2json with a JS version"

This reverts commit 4eb981635453871fa2a33396391f3f75a6baa8b1.

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Jonathan Druart 2024-10-15 08:57:25 UTC
Created attachment 172767 [details] [review]
Bug 38164: Skip fuzzy translations

This patch was envisioned by Julian Maurice.

Test plan:
0. Apply the 1st patch (the revert)
1. Go to `misc/translator/po`
2. es-ES-messages-js.po
3. Mark "Edit" string as fuzzy (around line 48). It should look like:

```
msgid "Edit"
msgstr "Editar"
```

4. Edit ./intranet-main.tt and add the following lines at the bottom,
   inside the `$(document).ready` block:

```javascript
  console.log(_("Edit"));
  console.log(__("Edit"));
```

5. Install the templates
  k$ koha-translate --install es-ES --dev kohadev && restart_all
6. Enable *es-ES* by searching for `language` in the sysprefs and switch
   to it for the staff interface.
7. Go to the Koha home page, open the browser console
=> FAIL: Notice that the second log in the console is displaying the
fuzzy string (i.e. is being translated when it shouldn't)
8. Apply this patch
9. Re-install the translated templates:
  k$ koha-translate --update es-ES --dev kohadev && restart_all
10. Repeat 7
=> SUCCESS: With this patch applied both logs show the English version of the
string.
11. Remove the fuzzy flag on `es-ES-messages-js.po`
12. Repeat 9 and 10
=> SUCCESS: The results are similar to step 7, but in this case they are
expected as the string is not marked fuzzy.
13. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2024-10-15 08:57:58 UTC
IMO we are only postponing the problem to the next time we will need yarn.

It would be better to try with alternative approaches. But I do understand that we want to provide a quick solution now, however I also know that we won't deal with the yarn issue as we are lacking manpower and so will hit this one again.
Comment 21 Jonathan Druart 2024-10-15 09:02:41 UTC
A note regarding node-po2json deb package: we don't want it I think, see bug 37303 comment 21 => we don't want to be stuck with v0.4.5
Comment 22 Tomás Cohen Arazi (tcohen) 2024-10-15 12:46:50 UTC
(In reply to Jonathan Druart from comment #20)
> IMO we are only postponing the problem to the next time we will need yarn.

I think this patch should target 24.05. And we should find a solution for 24.11
Comment 23 Katrin Fischer 2024-10-15 12:49:18 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #22)
> (In reply to Jonathan Druart from comment #20)
> > IMO we are only postponing the problem to the next time we will need yarn.
> 
> I think this patch should target 24.05. And we should find a solution for
> 24.11

That means I would not push it to main, but the RMaints pick it up directly from here. Correct?
Comment 24 Tomás Cohen Arazi (tcohen) 2024-10-15 12:50:32 UTC
(In reply to Katrin Fischer from comment #23)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #22)
> > (In reply to Jonathan Druart from comment #20)
> > > IMO we are only postponing the problem to the next time we will need yarn.
> > 
> > I think this patch should target 24.05. And we should find a solution for
> > 24.11
> 
> That means I would not push it to main, but the RMaints pick it up directly
> from here. Correct?

Exactly. This was a stopgap for stable.
Comment 25 Katrin Fischer 2024-10-15 13:07:31 UTC
Handing over to @ping Lucas then!

Should we open a separate bug for the 24.11 fix? We need to keep sight of it.
Comment 26 Victor Grousset/tuxayo 2024-10-16 20:39:00 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Victor Grousset/tuxayo from comment #14)
> > Created attachment 172762 [details] [review] [review] [review]
> > Bug 38164: Update yarn.lock for removal of po2json.js
> 
> Skipped this patch, this is a task for RM/RMaints.

Right, that makes sense especially with backports and changes due to yarn versions. Like SCSS and DBIC. Not a problem then.
Still something to keep in mind because having to sync yarn.lock after package.json changes isn't mentioned.
https://wiki.koha-community.org/wiki/Release_management
https://wiki.koha-community.org/wiki/Release_maintenance

Especially since xt/verify-yarnlock.t doesn't cover this case (opened Bug 38170 for that)


-----


(In reply to Tomás Cohen Arazi (tcohen) from comment #22)
> (In reply to Jonathan Druart from comment #20)
> > IMO we are only postponing the problem to the next time we will need yarn.
> 
> I think this patch should target 24.05. And we should find a solution for
> 24.11


Are we talking about finding a solution about fuzzy strings or "the next time we will need yarn" as in the next time a node package would be of use for us in production?

It seems you both weren't in the same issue.

As for the fuzzy strings thing, the fixed po2json.pl might be enough.
VS po2json.js which has a similar amount of code as our .pl but even 1.0.0-beta-3 has dependencies on fixed versions
https://npmgraph.js.org/?q=po2json%401.0.0-beta-3

po2json.pl has also dependencies on stuff that isn't on fixed version but that doesn't look much maintained:
https://metacpan.org/pod/JSON
https://metacpan.org/pod/Locale::PO
Maybe it's ok, it's just interfaces for reasonably simple file formats.

Anyway both are not in a great shape themselves and the dependencies they use (.pl => the dependencies themselves, .js => the fact that they are fixed).

The biggest difference is po2json.pl can be maintained by us without having to fork it (it already is) and is easier to integrate into Koha (it already is) than a node lib.

If we find a non trivial advantage for one, that could easily supersede the above IHMO. Especially if we are sure we will end up having to use node libs in production.
Comment 27 David Cook 2024-10-17 02:39:36 UTC
(In reply to Jonathan Druart from comment #20)
> IMO we are only postponing the problem to the next time we will need yarn.
> 
> It would be better to try with alternative approaches. But I do understand
> that we want to provide a quick solution now, however I also know that we
> won't deal with the yarn issue as we are lacking manpower and so will hit
> this one again.

We shouldn't need "yarn" with a deployed/installed Koha. 

In the case of bug 37303, "yarn run po2json" wouldn't work anyway, even if "yarn" were installed, since po2json isn't installed in the deployed/installed Koha environment.

Plus "yarn run po2json" is just running the "bin/po2json" file which is just being executed using node, which is also not available with a deployed/installed Koha. 

So at the end of the day what we're really trying to do is run "node <path_to_po2json_proj>/bin/po2json".

When we're developing, we need to be thinking more about the deployment environment and not just the development environment.
Comment 28 David Cook 2024-10-17 02:44:59 UTC
At this point, I don't know if the Javascript "po2json" is our only option or not.

If it is, then the question should be... do we make NodeJS an additional prod (not just dev) dependency of Koha? (Keeping in mind we already have a million dependencies.)

For what it's worth, I have a small PHP project where I have added NodeJS as a dependency, because there is some functionality that I can only access using Nodejs. So I do understand the merit of doing that.

But... if we did do that... we'd need to think a lot about how we structure the source code. We'd be supporting another complete ecosystem in prod. That's big.

If we do this, I'd recommend that we use our existing package.json to *bundle po2json* (or our own customized po2json) into a single script, so that in prod we'd just have 1 file to manage.
Comment 29 David Cook 2024-10-17 02:51:47 UTC
Tomas has pointed out bug 38179 to me, so I'll move my discussion over to there, and not clog up this backporting work.
Comment 30 Katrin Fischer 2024-10-18 12:39:40 UTC
Udpated the bug slightly. We decided on bug 38179 to also use this fix for main. 

It is also an URGENT bug fix for 24.05, but not below.
Comment 31 Katrin Fischer 2024-10-18 15:45:53 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 32 Lucas Gass (lukeg) 2024-10-23 22:51:44 UTC
Backported to 24.05.x for upcoming 24.05.05