Bug 39679

Summary: Missing space before barcode in holds table with item-specific hold
Product: Koha Reporter: Andrew Fuerste-Henry <andrew>
Component: Hold requestsAssignee: Andrew Fuerste-Henry <andrew>
Status: Pushed to main --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, gmcharlt, jonathan.druart, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
This fixes the holds table for a record: 1. There is now a space between "Only item" and the barcode for specific item holds, for example: "Only item 3999...". 2. Where there is no barcode for the item, it now shows as "Item only [No barcode]" to improve consistency with other areas of Koha (instead of "Only item No barcode").
Version(s) released in:
25.05.00
Circulation function:
Attachments: Bug 39679: Add space before barcode in holds table
Bug 39679: Add space before barcode in holds table
Bug 39679: Add space before barcode in holds table
Bug 39679: Add space before barcode in holds table
Bug 39679: Add space before barcode in holds table
Bug 39679: Add space before barcode in holds table
Bug 39679: (follow-up) Improve consistency
Bug 39679: (QA follow-up) Translation friendly approach

Description Andrew Fuerste-Henry 2025-04-18 14:24:12 UTC
To recreate:
1 : place an item-level hold for an item with a barcode
2 : in the holds list on request.pl, see that the Details column for your hold lists "Only item[barcode]" in the dropdown without a space before the barcode
3 : place a second item-level hold for the same patron
4 : back on the holds list, see that the Details column isn't a dropdown anymore but still says "Only item[barcode]" without a space
Comment 1 Andrew Fuerste-Henry 2025-04-18 14:27:14 UTC Comment hidden (obsolete)
Comment 2 Andrew Fuerste-Henry 2025-04-18 14:31:49 UTC Comment hidden (obsolete)
Comment 3 Andrew Fuerste-Henry 2025-04-18 14:32:36 UTC
Oops, actually the space was not missing when the hold is not able to be changed from item- to bib-level. Updated to remove extraneous edit.
Comment 4 David Nind 2025-04-19 08:56:20 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize (ashimema) 2025-05-08 15:20:17 UTC
Sorry Andrew, <span> are not allowed inside <option> according to the html spec.

I'll post an alternative shortly.
Comment 6 Martin Renvoize (ashimema) 2025-05-08 15:26:44 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize (ashimema) 2025-05-08 15:27:06 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize (ashimema) 2025-05-08 15:28:23 UTC
Any chance of a retest here David.. if someone confirms this resolves the spacing issue then I think we can go strait to PQA here.

I do wonder what it looks like for an item without a barcode.. feels like it would read `Only itemNo barcode` to me.
Comment 9 David Nind 2025-05-08 20:46:34 UTC
Created attachment 182120 [details] [review]
Bug 39679: Add space before barcode in holds table

To test:
1) Place an item-level hold for an item with a barcode
2) In the holds list on request.pl, see that the Details
   column for your hold lists "Only item[barcode]" in the
   dropdown without a space before the barcode
3) Apply patch, restart_all, reload page
4) Confirm your hold has a space before the barcode

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-05-08 20:47:58 UTC
(In reply to Martin Renvoize (ashimema) from comment #8)
> Any chance of a retest here David.. if someone confirms this resolves the
> spacing issue then I think we can go strait to PQA here.
> 
> I do wonder what it looks like for an item without a barcode.. feels like it
> would read `Only itemNo barcode` to me.

It does resolve the spacing.

For an item with no barcode, it shows as "Only item No barcode" before and after the patch.
Comment 11 Martin Renvoize (ashimema) 2025-05-09 10:28:10 UTC
Nice one, thanks David :)
Comment 12 Owen Leonard 2025-05-09 18:38:04 UTC
This could also be solved in a similar way to Bug 39688:

<option selected="selected" value="">
    [%- IF ( hold.barcodenumber ) -%]
        Only item [%- hold.barcodenumber | html -%]
    [%- ELSE -%]
        Only item [No barcode]
    [%- END -%]
</option>
Comment 13 David Nind 2025-05-09 20:50:02 UTC
Created attachment 182228 [details] [review]
Bug 39679: (follow-up) Improve consistency

This improves the consistency with other areas of Koha:
1. A space before the barcode: "Only item 3999....".
2. Where there is no barcode: "Only item [No barcode]".

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2025-05-09 20:52:11 UTC
(In reply to Owen Leonard from comment #12)
> This could also be solved in a similar way to Bug 39688:
> 
> <option selected="selected" value="">
>     [%- IF ( hold.barcodenumber ) -%]
>         Only item [%- hold.barcodenumber | html -%]
>     [%- ELSE -%]
>         Only item [No barcode]
>     [%- END -%]
> </option>

I'm all for consistency!

I've added a follow-up with Owen's suggested changes.

Have left as Passed QA, QA script is happy. 

Feel free to change status if it needs another sign off.
Comment 15 Andrew Fuerste-Henry 2025-05-09 21:05:32 UTC
Thanks everybody for fixing this properly :)
Comment 16 Jonathan Druart 2025-05-12 07:27:24 UTC
The correct solution here is to use the I18 TT plugin

<option selected="selected" value="">
    [%- IF ( hold.barcodenumber ) -%]
        [% tx("Only item {barcode}", {barcode: hold.barcodenumber}) %]
    [%- ELSE -%]
        [% t("Only item [No barcode]") %]
    [%- END -%]
</option>
Comment 17 Martin Renvoize (ashimema) 2025-05-12 08:45:28 UTC
Created attachment 182253 [details] [review]
Bug 39679: (QA follow-up) Translation friendly approach

As discussed at length on (mattermost)[https://chat.koha-community.org/koha-community/pl/wiopmtr7j3835df3yfi6q64ooy] we need to use the I18N library to properly handle this otherwise we will end up with TT tags inside the translatable strings.

We can not use <span> here as we are inside an <option> tag and nesting
<span> insdie <option> is against the html standard.

Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Comment 18 Katrin Fischer 2025-05-12 16:32:30 UTC
Pushed for 25.05!

Well done everyone, thank you!