If the translations can use template literals, then you could switch back to a template literal.

However, a better idea is to build the HTML using DOM objects. 

Something like...

let para = document.createElement('p');
let strong = document.createElement('strong');
strong.textContent = __("Error:");
let err_msg = document.createTextNode(errorMessage);
para.appendChild(strong);
para.appendChild(err_msg);
document.getElementById('barcode-container').appendChild(para);

--

I get that it's a lot longer to write, but it's a billion times safer than using innerHTML. 

Look at "Cross Site Scripting Prevention Cheat Sheet" by OWASP. Specifically "Safe Sinks".

I'll probably look at this after an upcoming meeting...

Created attachment 170744 [details] [review]
Bug 37742: Fix error display

This change fixes the display of the error message on "My virtual card"
on the OPAC.

Test plan:
0. Apply the patch and enable syspref "OPACVirtualCard"
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add an asterisk (*) to the end of the card number
3. Go to http://localhost:8080/cgi-bin/koha/opac-virtual-card.pl
4. Note that the error message appears as follows:
Code 39 must contain only digits, capital letters, spaces and the symbols -.$/+%

I think you are correct about that we should not show a technical error to the user. But it might help debug library staff and I assume they would test this before keeping it active for public?

(In reply to Katrin Fischer from comment #4)
> I think you are correct about that we should not show a technical error to
> the user. But it might help debug library staff and I assume they would test
> this before keeping it active for public?

Perhaps we should have a patron barcode on the staff side too, and then they could troubleshoot it from that side instead?

The tough thing with something like this is that there can be a lot of variation in cardnumbers. It might work for a lot of borrowers but not all, and then only a minority are seeing the weirdness.

OK, so we need to decide here if we want to adapt the error to be generic or not.

(In reply to David Cook from comment #1)
> If the translations can use template literals, then you could switch back to
> a template literal.

What exactly is a template literal? 

AFAIK our tools can parse HTML in HTML pages, but not HTML that is constructed in JS.

I'd love to get some quick opinion on the message, so we can fix this fast (yes, feeling slightly guilty :) )

(In reply to Katrin Fischer from comment #6)
> OK, so we need to decide here if we want to adapt the error to be generic or
> not.

Happy to follow your lead here. My opinion is we should show something more user-friendly but that's just my opinion.

(In reply to Katrin Fischer from comment #7)
> (In reply to David Cook from comment #1)
> > If the translations can use template literals, then you could switch back to
> > a template literal.
> 
> What exactly is a template literal? 

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals

(In reply to Katrin Fischer from comment #8)
> I'd love to get some quick opinion on the message, so we can fix this fast
> (yes, feeling slightly guilty :) )

<3

Instead of using "errorMessage" here I think we could use __("Unavailable to generate barcode").

It should be translatable, and I think anyone with developer support should be able to figure out the problem. If they don't have support, they can come online, and I'll probably end up helping them anyway :p

(In reply to David Cook from comment #10)
> (In reply to Katrin Fischer from comment #7)
> > (In reply to David Cook from comment #1)
> > > If the translations can use template literals, then you could switch back to
> > > a template literal.
> > 
> > What exactly is a template literal? 
> 
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Template_literals

I don't think our tools can parse these currently. You'd also not want translators faced with the HTML bits included in these, but only the text.


>Instead of using "errorMessage" here I think we could use __("Unavailable to >generate barcode").

I'd maybe make it:

Uanble to generate barcode

Or:

The barcode image could not be generated. Please get in touch with library staff.

(In reply to Katrin Fischer from comment #12)
> I don't think our tools can parse these currently. You'd also not want
> translators faced with the HTML bits included in these, but only the text.

It looks like xgettext supports it (bug 24725), and we removed a prohibition against them. 

But I see your point about not wanting translators faced with the HTML/Javascript bits. Although maybe xgettext deals with that? Unfortunately, I don't know enough about the translatability topic.

Scanning through koha-tmpl, it looks like they're just used for non-translateable bits of JS code.

Might be worth adding a coding guideline for that I reckon...

> >Instead of using "errorMessage" here I think we could use __("Unavailable to >generate barcode").
> 
> I'd maybe make it:
> 
> Uanble to generate barcode
> 
> Or:
> 
> The barcode image could not be generated. Please get in touch with library
> staff.

Patch incoming...

Created attachment 170850 [details] [review]
Bug 37742: Fix error display

This change fixes the display of the error message on "My virtual card"
on the OPAC.

Test plan:
0. Apply the patch and enable syspref "OPACVirtualCard"
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add an asterisk (*) to the end of the card number
3. Go to http://localhost:8080/cgi-bin/koha/opac-virtual-card.pl
4. Note that the error message appears as follows:
Code 39 must contain only digits, capital letters, spaces and the symbols -.$/+%

Created attachment 171159 [details] [review]
Bug 37742: Fix error display

This change fixes the display of the error message on "My virtual card"
on the OPAC.

Test plan:
0. Apply the patch and enable syspref "OPACVirtualCard"
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add an asterisk (*) to the end of the card number
3. Go to http://localhost:8080/cgi-bin/koha/opac-virtual-card.pl
4. Note that the error message appears as follows:
Code 39 must contain only digits, capital letters, spaces and the symbols -.$/+%

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

Created attachment 173355 [details] [review]
Bug 37742: Fix error display

This change fixes the display of the error message on "My virtual card"
on the OPAC.

Test plan:
0. Apply the patch and enable syspref "OPACVirtualCard"
1. Go to http://localhost:8081/cgi-bin/koha/members/memberentry.pl
?op=edit_form&destination=circ&borrowernumber=51
2. Add an asterisk (*) to the end of the card number
3. Go to http://localhost:8080/cgi-bin/koha/opac-virtual-card.pl
4. Note that the error message appears as follows:
Code 39 must contain only digits, capital letters, spaces and the symbols -.$/+%

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Pushed for 24.11!

Well done everyone, thank you!