Bug 36836

Created attachment 170929 [details] [review]
Bug 36836: Fix translation issues in ERM module

This tries to fix multiple translation issues in the ERM module.
This includes:

* Making strings translatable
* Using double quotes instead of single around strings according to
  coding guidelines
* Fixing some translations where the variable was marked for translation,
  but not the strings themselves:  ...>{{ $__(item.tab_name) }}</a

To test:
* Apply patch
* Run yarn build
* Make sure you have a clean work space in git
* Run gulp po:update lang de-DE (or another language)
* Verify the strings from the patch set now appear in the po files

There are some more issues I found but failed to fix.

Maybe someone more knowledgeable about vue can help with these?


1) koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue

All 4 alert(...) messages

2) koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersCounterLogs.vue
                    render: function (data, type, row, meta) {
                        const importer = row.borrowernumber
                            ? `<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=${row.borrowernumber}">Borrowernumber ${row.borrowernumber}</a>`
                            : __("Cronjob")
                        return importer
                    },

I also suspect some other things to not translate, but might be easier to see in the GUI later and report separately.

(In reply to Katrin Fischer from comment #2)
> There are some more issues I found but failed to fix.
> 
> Maybe someone more knowledgeable about vue can help with these?
> 
> 
> 1)
> koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> UsageStatisticsReportBuilder.vue
> 
> All 4 alert(...) messages

Try
  this.$__("your string"))

> 2)
> koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> UsageStatisticsDataProvidersCounterLogs.vue
>                     render: function (data, type, row, meta) {
>                         const importer = row.borrowernumber
>                             ? `<a
> href="/cgi-bin/koha/members/moremember.pl?borrowernumber=${row.
> borrowernumber}">Borrowernumber ${row.borrowernumber}</a>`
>                             : __("Cronjob")
>                         return importer
>                     },
> 
> I also suspect some other things to not translate, but might be easier to
> see in the GUI later and report separately.

"Borrowernumber" won't be translatable I'd say.

Try
  "<a href="/cgi-bin/koha/members/moremember.pl?borrowernumber=%s">%s</a>".format(${row.borrowernumber}, __("Borrowernumber %s").format(${row.borrowernumber}))

But I am not sure we want to use "Borrowernumber" here...

(In reply to Jonathan Druart from comment #3)
> (In reply to Katrin Fischer from comment #2)
> > There are some more issues I found but failed to fix.
> > 
> > Maybe someone more knowledgeable about vue can help with these?
> > 
> > 
> > 1)
> > koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> > UsageStatisticsReportBuilder.vue
> > 
> > All 4 alert(...) messages
> 
> Try
>   this.$__("your string"))

I already tried that I think and it didn't pick up the translations. I am a bit confused as to when to use what and mostly looked at other spots "similar" to what I was trying to do. Some notes on the wiki would be quite helpful.


> > 2)
> > koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> > UsageStatisticsDataProvidersCounterLogs.vue
> >                     render: function (data, type, row, meta) {
> >                         const importer = row.borrowernumber
> >                             ? `<a
> > href="/cgi-bin/koha/members/moremember.pl?borrowernumber=${row.
> > borrowernumber}">Borrowernumber ${row.borrowernumber}</a>`
> >                             : __("Cronjob")
> >                         return importer
> >                     },
> > 
> > I also suspect some other things to not translate, but might be easier to
> > see in the GUI later and report separately.
> 
> "Borrowernumber" won't be translatable I'd say.
> 
> Try
>   "<a
> href="/cgi-bin/koha/members/moremember.pl?borrowernumber=%s">%s</a>".
> format(${row.borrowernumber}, __("Borrowernumber
> %s").format(${row.borrowernumber}))
> 
> But I am not sure we want to use "Borrowernumber" here...

Similar for me, but I thought that could be a discussion for later.

Maybe if this patch is OK we can move them to a separate bug for later, as I am not sure when I can work on the follow-up (already had this patch sitting for a few weeks).

(In reply to Katrin Fischer from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > (In reply to Katrin Fischer from comment #2)
> > > There are some more issues I found but failed to fix.
> > > 
> > > Maybe someone more knowledgeable about vue can help with these?
> > > 
> > > 
> > > 1)
> > > koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> > > UsageStatisticsReportBuilder.vue
> > > 
> > > All 4 alert(...) messages
> > 
> > Try
> >   this.$__("your string"))
> 
> I already tried that I think and it didn't pick up the translations. I am a
> bit confused as to when to use what and mostly looked at other spots
> "similar" to what I was trying to do. Some notes on the wiki would be quite
> helpful.

<template> part
  {{ $__("This way") }}

<script> part
  this.$__("This way")

The only exception is when we are trying to translate a string that will be displayed in a DataTable (as we are not using a proper Vue component for that), we need to access the global __() function (see js/vue/i18n/index.js) and so use the regular way:
  __("This way")

I have noticed that strings from UsageStatisticsReportBuilder.vue are not picked by the translation script.

The problem is ES6 template literals, same as bug 35374.

(In reply to Jonathan Druart from comment #6)
> I have noticed that strings from UsageStatisticsReportBuilder.vue are not
> picked by the translation script.

Not all the strings, the ones after the first backticks (in getData)

I have split out the translation issue for the alerts into a separate bug report:
Bug 37805 - ERM module partially untranslatable

So we can move on with this patch which I believe covers what was working in my tests and have the other problems separate.

Created attachment 173261 [details] [review]
Bug 36836: Fix translation issues in ERM module

This tries to fix multiple translation issues in the ERM module.
This includes:

* Making strings translatable
* Using double quotes instead of single around strings according to
  coding guidelines
* Fixing some translations where the variable was marked for translation,
  but not the strings themselves:  ...>{{ $__(item.tab_name) }}</a

To test:
* Apply patch
* Run yarn build
* Make sure you have a clean work space in git
* Run gulp po:update lang de-DE (or another language)
* Verify the strings from the patch set now appear in the po files

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Some observations thought I don't think blockers:
- $__("(EBSCO)") should this be translatable as it's the name of a service?
- $__("(local)") should this not leave the parenthesis out of the translate string?
- $__("Uploaded on:") should this not leave the colon out of the translate string?
- $__("SUSHI credentials") maybe leave 'SUSHI' out of this and consider just the word 'credentials' for translation?

(In reply to Pedro Amorim from comment #10)
> Some observations thought I don't think blockers:
> - $__("(EBSCO)") should this be translatable as it's the name of a service?

We could leave it out, but I think it doesn't hurt to have it. I am not sure if EBSCO is used somewhere where they would spell it in Kyrillic or another script, but we keep the option that way.

> - $__("(local)") should this not leave the parenthesis out of the translate

We could totally do that, but I was not sure how - dabbling for the first time in this area. I think it's not a blocker for translation.

> string?
> - $__("Uploaded on:") should this not leave the colon out of the translate
> string?

Again, we could and it might be nice, but I was not sure about how to do a proper concatenation here. There is no general rule and you will find tons of strings with : in the po files.

> - $__("SUSHI credentials") maybe leave 'SUSHI' out of this and consider just
> the word 'credentials' for translation?

In some languages the sequence might be different or, like in German, you need to add a hyphen between words: SUSHI-Logindaten. So no, don't try to make strings more granular, context is important.

(In reply to Katrin Fischer from comment #11)
> (In reply to Pedro Amorim from comment #10)
> > - $__("Uploaded on:") should this not leave the colon out of the translate
> > string?
> 
> Again, we could and it might be nice, but I was not sure about how to do a
> proper concatenation here. There is no general rule and you will find tons
> of strings with : in the po files.

Not from .vue files.

I am not sure about the changes in koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersShow.vue

-                        >{{ $__(item.tab_name) }}</a
+                        >{{ $item.tab_name }}</a

So not using the translated version. Then later:

+                {
+                    test: "TR",
+                    data_type: "title",
+                    tab_name: this.$__("Titles"),
+                },

Is this correct?

In koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsReportBuilder.vue

-                                >{{ $__(prop.name) }}:</label
+                                >{{ $prop.name }}:</label

-                                    {{ $__(month.short) }}
+                                    {{ $month.short }}

Extra '$' here?

Created attachment 174839 [details] [review]
Bug 36836: Fix translation issues in ERM module

This tries to fix multiple translation issues in the ERM module.
This includes:

* Making strings translatable
* Using double quotes instead of single around strings according to
  coding guidelines
* Fixing some translations where the variable was marked for translation,
  but not the strings themselves:  ...>{{ $__(item.tab_name) }}</a

To test:
* Apply patch
* Run yarn build
* Make sure you have a clean work space in git
* Run gulp po:update lang de-DE (or another language)
* Verify the strings from the patch set now appear in the po files

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Amended-by: Jonathan Druart
-                                        {{ $__("Uploaded on:") }}
+                                        {{ $__("Uploaded on") }}:
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

(In reply to Jonathan Druart from comment #13)
> I am not sure about the changes in
> koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> UsageStatisticsDataProvidersShow.vue
> 
> -                        >{{ $__(item.tab_name) }}</a
> +                        >{{ $item.tab_name }}</a
> 
> So not using the translated version. Then later:
> 
> +                {
> +                    test: "TR",
> +                    data_type: "title",
> +                    tab_name: this.$__("Titles"),
> +                },
> 
> Is this correct?

It's been a while since I tested this. 

To my knowledge translation tools will not work with variables? 

> -                        >{{ $__(item.tab_name) }}</a

So this should not work as the tools cannot figure out where the strings are to be found.

So removed there and added the translation mark-up to the strings that tab_name can be.

(In reply to Jonathan Druart from comment #14)
> In
> koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> UsageStatisticsReportBuilder.vue
> 
> -                                >{{ $__(prop.name) }}:</label
> +                                >{{ $prop.name }}:</label
> 
> -                                    {{ $__(month.short) }}
> +                                    {{ $month.short }}
> 
> Extra '$' here?

I am really not good with the vue syntax yet, I am more guessing than knowing - can you fix?

Created attachment 174887 [details] [review]
Bug 36836: Amend translation fixes