Bug 35392 - HTML in translatable string
Summary: HTML in translatable string
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Pedro Amorim
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 34587
Blocks:
  Show dependency treegraph
 
Reported: 2023-11-23 20:30 UTC by Caroline Cyr La Rose
Modified: 2024-04-05 15:40 UTC (History)
8 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.05.00


Attachments
Bug 35392: Avoid HTML in translatable content (2.28 KB, patch)
2023-11-26 16:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35392: Move usage-reports store into erm store (11.44 KB, patch)
2023-11-27 11:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: New getDialoJobEnqueueMessage (5.37 KB, patch)
2023-11-27 11:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Add missing file upload dialog cypress test (1.61 KB, patch)
2023-11-27 11:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Avoid HTML in translatable content (2.28 KB, patch)
2023-11-27 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Move usage-reports store into erm store (11.44 KB, patch)
2023-11-27 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: New getDialoJobEnqueueMessage (5.72 KB, patch)
2023-11-27 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Add missing file upload dialog cypress test (1.61 KB, patch)
2023-11-27 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: (QA follow-up): (1.30 KB, patch)
2024-01-03 10:48 UTC, Pedro Amorim
Details | Diff | Splinter Review
label demo (112.57 KB, image/jpeg)
2024-01-03 10:49 UTC, Pedro Amorim
Details
Bug 35392: Fix HTML in translatable string (3.76 KB, patch)
2024-01-04 12:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Update tests (2.07 KB, patch)
2024-01-04 12:01 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35392: Fix HTML in translatable string (3.81 KB, patch)
2024-04-05 10:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 35392: Update tests (2.12 KB, patch)
2024-04-05 10:59 UTC, David Nind
Details | Diff | Splinter Review
Bug 35392: Fix HTML in translatable string (3.89 KB, patch)
2024-04-05 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35392: Update tests (2.19 KB, patch)
2024-04-05 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35392: Use the same selector as everywhere else for .message (2.23 KB, patch)
2024-04-05 13:17 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 Caroline Cyr La Rose 2023-11-23 20:30:40 UTC
This string appears as is in Weblate to be translated, HTML tags and all

<li>Job for uploaded file has been queued, <a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">click here</a> to check its progress.</li>

Usually, HTML tags don't appear. I'm not sure what is different for this one. Probably something to do with JS or Vue?

For reference, the line is

koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersFileImport.vue:93

Furthermore, while we are there to correct it, we should also correct the "click here" link. I understand it is not an accessible way of making links.

All other job queue messages look like this

H1 Title: The job has been enqueued!
Text: It will be processed as soon as possible.
Link: View detail of the enqueued job

See for example https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt#L78
Comment 1 Caroline Cyr La Rose 2023-11-23 20:43:41 UTC
I found the same type of string again

<li>Job for report type <strong>%s</strong> has been queued, <a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">click here</a> to check its progress.</li>

at

koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersList.vue:249
Comment 2 Jonathan Druart 2023-11-24 06:19:15 UTC
(In reply to Caroline Cyr La Rose from comment #1)
> I found the same type of string again
> 
> <li>Job for report type <strong>%s</strong> has been queued, <a
> href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s"
> target="_blank">click here</a> to check its progress.</li>
> 
> at
> 
> koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/
> UsageStatisticsDataProvidersList.vue:249

Yes, given the code that's expected:
248                                         message += this.$__(
249                                             '<li>Job for report type <strong>%s</strong> has been queued, <a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">click here</a> to check     its progress.</li>'
250                                         ).format(job.report_type, job.job_id)

Could be improved however.
Comment 3 Tomás Cohen Arazi 2023-11-26 16:10:31 UTC
Created attachment 159256 [details] [review]
Bug 35392: Avoid HTML in translatable content
Comment 4 Pedro Amorim 2023-11-27 11:01:14 UTC
Created attachment 159263 [details] [review]
Bug 35392: Move usage-reports store into erm store

This is all ERM, no need to have it be separate store files
Comment 5 Pedro Amorim 2023-11-27 11:01:17 UTC
Created attachment 159264 [details] [review]
Bug 35392: New getDialoJobEnqueueMessage

DRY the dialog message shown when a background job is enqueued in ERM

This takes Tomas work in UsageStatisticsDataProvidersList.vue, DRYs it
and also applies it to UsageStatisticsDataProvidersFileImport.vue to address
the issue mentioned by Caroline.

To test:
1) Do a regular harvest,
2) Do a file upload
3) Notice in both cases the message is shown as before
Comment 6 Pedro Amorim 2023-11-27 11:01:20 UTC
Created attachment 159265 [details] [review]
Bug 35392: Add missing file upload dialog cypress test
Comment 7 Jonathan Druart 2023-11-27 16:24:55 UTC
Pedro, I think those patches should be on another bug.
Comment 8 Jonathan Druart 2023-11-27 16:27:01 UTC
Well, maybe not, they seem related. 

I am getting a conflict on the second patch however

Applying: Bug 35392: Move usage-reports store into erm store
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/vue/stores/usage-reports.js).
Comment 9 Jonathan Druart 2023-11-27 16:27:22 UTC
But really, it's too late for such changes. We should only fix the translation issue here.
Comment 10 Jonathan Druart 2023-11-27 16:31:06 UTC
Not tested, but maybe this is better:

"Job for report type %s has been queued, %sclick here%s to check its progress".format('<strong>%s</strong>'.format(job.report_type), '<a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">'.format(job.job_id), '</a>')

What do you think?
Comment 11 Jonathan Druart 2023-11-27 16:33:18 UTC
(In reply to Jonathan Druart from comment #8)
> Well, maybe not, they seem related. 
> 
> I am getting a conflict on the second patch however
> 
> Applying: Bug 35392: Move usage-reports store into erm store
> error: sha1 information is lacking or useless
> (koha-tmpl/intranet-tmpl/prog/js/vue/stores/usage-reports.js).

It's actually on the third patch

Applying: Bug 35392: New getDialoJobEnqueueMessage
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersList.vue
Falling back to patching base and 3-way merge...                                                                                                                                                                     
Auto-merging koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersList.vue                                                                                                                 
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/UsageStatisticsDataProvidersList.vue
Comment 12 Pedro Amorim 2023-11-27 16:44:40 UTC
Created attachment 159279 [details] [review]
Bug 35392: Avoid HTML in translatable content
Comment 13 Pedro Amorim 2023-11-27 16:44:43 UTC
Created attachment 159280 [details] [review]
Bug 35392: Move usage-reports store into erm store

This is all ERM, no need to have it be separate store files
Comment 14 Pedro Amorim 2023-11-27 16:44:47 UTC
Created attachment 159281 [details] [review]
Bug 35392: New getDialoJobEnqueueMessage

DRY the dialog message shown when a background job is enqueued in ERM

This takes Tomas work in UsageStatisticsDataProvidersList.vue, DRYs it
and also applies it to UsageStatisticsDataProvidersFileImport.vue to address
the issue mentioned by Caroline.

To test:
1) Do a regular harvest,
2) Do a file upload
3) Notice in both cases the message is shown as before
Comment 15 Pedro Amorim 2023-11-27 16:44:49 UTC
Created attachment 159283 [details] [review]
Bug 35392: Add missing file upload dialog cypress test
Comment 16 Pedro Amorim 2023-11-27 16:48:39 UTC
(In reply to Jonathan Druart from comment #9)
> But really, it's too late for such changes. We should only fix the
> translation issue here.

Hey, thanks for looking! Rebased. I wanted to make it easier to maintain going forward, whilst addressing Caroline's comment about the file upload, but I understand, please make obsolete if you don't agree!
Comment 17 Pedro Amorim 2023-12-22 15:08:47 UTC
@Joubu, are we able to move on here? 
I don't want my patches to be the cause of holding off this bug.
Please advise.

If you decide to obsolete my patches please note that they're also fixing Tomas' original patch:
1) Has a typo "$this."
2) Only addresses UsageStatisticsDataProvidersList.vue. Does not address UsageStatisticsDataProvidersFileImport.vue
Comment 18 Katrin Fischer 2023-12-30 19:53:39 UTC
The translation is one issue, but there is another: 
For usability and accessibility we should always put the link on meaningful words, so no links like 'here' or 'click here'. People know that they can click links and screen readers will be happier reading something that reveals the purpose of a link.

Maybe this would work?

'<li><span>Job for uploaded file has been queued.</span><a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">Check progress</a></li>'

The span I think would break this into 2 strings for translation.

Do we have another translation issue here?

+            { short: "Jan", description: "January", value: 1, active: true },
+            { short: "Feb", description: "February", value: 2, active: true },
+            { short: "Mar", description: "March", value: 3, active: true },
+            { short: "Apr", description: "April", value: 4, active: true },
Comment 19 Pedro Amorim 2024-01-02 11:29:24 UTC
(In reply to Katrin Fischer from comment #18)
> The translation is one issue, but there is another: 
> For usability and accessibility we should always put the link on meaningful
> words, so no links like 'here' or 'click here'. People know that they can
> click links and screen readers will be happier reading something that
> reveals the purpose of a link.
> 
> Maybe this would work?
> 
> '<li><span>Job for uploaded file has been queued.</span><a
> href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s"
> target="_blank">Check progress</a></li>'
> 
> The span I think would break this into 2 strings for translation.
> 
> Do we have another translation issue here?
> 
> +            { short: "Jan", description: "January", value: 1, active: true
> },
> +            { short: "Feb", description: "February", value: 2, active: true
> },
> +            { short: "Mar", description: "March", value: 3, active: true },
> +            { short: "Apr", description: "April", value: 4, active: true },

Katrin, these are very good points, however can we please open one (or two?) new bugs for these issues you mention?

The original problem here has to do with HTML being present in a translatable string, whereas I believe what you're referring to are possibly different problems:
1) Usability: "Click here" link in ERM usage statistics background job dialog should be handled differently
2) Possible problem with month names translatability in ERM usage statistics reports (please provide a test plan)
Comment 20 Katrin Fischer 2024-01-02 22:12:43 UTC
I would need to test this, but I think it's not unproblematic for translations:

+                '" target="_blank">' +
+                __("click here") +
+                "</a> " +
+                __("to check its progress.") +
+                "</li>"

This will make it 2 separate strings in the po file. This is often quite hard to translate, maybe nicer with Weblate that keeps more of the sequence, but they could end up in 2 different spots of a big file. That's why I was suggesting to rephrase AND get rid of the click here the same time by making the link something like "Check progress".

This bug is about improving translatability for a specific string, so I think not totally out of scope.

> +            { short: "Mar", description: "March", value: 3, active: true },
> +            { short: "Apr", description: "April", value: 4, active: true },

For the second: I have no idea where it appears in the ERM module, so hard to provide a test plan right now. The code just looked very suspicious. But totally agree it should be on a separate bug.
Comment 21 Pedro Amorim 2024-01-03 10:48:13 UTC
Created attachment 160450 [details] [review]
Bug 35392: (QA follow-up):

Fix 'click here' and 'to check its progress' being 2 seperate translatable strings
Comment 22 Pedro Amorim 2024-01-03 10:49:52 UTC
Created attachment 160451 [details]
label demo

(In reply to Katrin Fischer from comment #20)
> This will make it 2 separate strings in the po file. This is often quite
> hard to translate, maybe nicer with Weblate that keeps more of the sequence,
> but they could end up in 2 different spots of a big file. That's why I was
> suggesting to rephrase AND get rid of the click here the same time by making
> the link something like "Check progress".

Fair point, I've submitted a follow patch please take a look when convenient.
Submitting a picture here of how it looks in the UI for both situations (new harvest job and file upload job).
Comment 23 Jonathan Druart 2024-01-04 08:45:14 UTC
Can we get a simple version of this fix for backport, please?
Comment 24 Pedro Amorim 2024-01-04 12:01:46 UTC
Created attachment 160512 [details] [review]
Bug 35392: Fix HTML in translatable string
Comment 25 Pedro Amorim 2024-01-04 12:01:49 UTC
Created attachment 160513 [details] [review]
Bug 35392: Update tests

Run:
yarn cypress run --spec "t/cypress/integration/ERM/DataProviders_spec.ts"
Comment 26 David Nind 2024-04-05 10:59:21 UTC
Created attachment 164466 [details] [review]
Bug 35392: Fix HTML in translatable string

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 David Nind 2024-04-05 10:59:23 UTC
Created attachment 164467 [details] [review]
Bug 35392: Update tests

Run:
yarn cypress run --spec "t/cypress/integration/ERM/DataProviders_spec.ts"

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 David Nind 2024-04-05 11:00:47 UTC
Testing notes (using KTD):

1. Apply patch
2. yarn build
3. Run tests: yarn cypress run --spec "t/cypress/integration/ERM/DataProviders_spec.ts"
4. Tests pass!
Comment 29 Jonathan Druart 2024-04-05 13:17:11 UTC
Created attachment 164473 [details] [review]
Bug 35392: Fix HTML in translatable string

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2024-04-05 13:17:15 UTC
Created attachment 164474 [details] [review]
Bug 35392: Update tests

Run:
yarn cypress run --spec "t/cypress/integration/ERM/DataProviders_spec.ts"

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 31 Jonathan Druart 2024-04-05 13:17:18 UTC
Created attachment 164475 [details] [review]
Bug 35392: Use the same selector as everywhere else for .message

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 32 Katrin Fischer 2024-04-05 15:40:41 UTC
Pushed for 24.05!

Well done everyone, thank you!