Bug 38895 - In advanced editor, the fixed data helpers put '#' instead of space in record content
Summary: In advanced editor, the fixed data helpers put '#' instead of space in record...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low trivial
Assignee: Janusz Kaczmarek
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-01-14 21:23 UTC by Janusz Kaczmarek
Modified: 2025-01-31 14:58 UTC (History)
3 users (show)

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


Attachments
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content (2.84 KB, patch)
2025-01-15 14:07 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content (2.89 KB, patch)
2025-01-15 17:31 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content (2.89 KB, patch)
2025-01-15 17:46 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content (2.99 KB, patch)
2025-01-31 09:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2025-01-14 21:23:47 UTC
When editing a bibliographic record in advanced editor, there are helpers for fixed data coded fields (like 008).  Like in the standard editor, empty value (space) is represented by a hash sign.  But unlike in the standard editor, the hash sign is not converted into a space but remains as '#' in the saved record.  This is obviously wrong.

For the standard editor there is a line:
valNode = valNode.replace("#", " ");
in xmlControlfield.js

An analogous replace('#',' ') is missing when constructing the <select> in cateditor/widget.js (line 164).
Comment 1 Janusz Kaczmarek 2025-01-15 14:07:35 UTC
Created attachment 176574 [details] [review]
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content

When editing a bibliographic record in advanced editor, there are
helpers for fixed data coded fields (like 008).  Like in the standard
editor, empty value (space) is represented by a hash sign.  But unlike
in the standard editor, the hash sign is not converted into a space but
remains as '#' in the saved record.  This is obviously wrong.

Test plan:
==========
1. Ensure to have EnableAdvancedCatalogingEditor set to 'enable'.
2. Create of edit a record, choosing for one of one-letter codes
   an empty value (represented by '#' in the helper).
3. Save the record, control in 'MARC preview' that you got a hash sign
   in the 008 field instead of a space.
4. Apply the patch ; restart all.
5. Open a record in advanced editor, refresh the page to reload the java
   script (Shift+Ctrl+R or another appropriate shortcut).  Edit the 008
   field again choosing for one of one-letter codes an empty value.
6. Save the record, control in 'MARC preview' that there is a space
   and not the hash sign.

Sponsored-by: Ignatianum University in Cracow
Comment 2 Roman Dolny 2025-01-15 17:31:30 UTC
Created attachment 176581 [details] [review]
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content

When editing a bibliographic record in advanced editor, there are
helpers for fixed data coded fields (like 008).  Like in the standard
editor, empty value (space) is represented by a hash sign.  But unlike
in the standard editor, the hash sign is not converted into a space but
remains as '#' in the saved record.  This is obviously wrong.

Test plan:
==========
1. Ensure to have EnableAdvancedCatalogingEditor set to 'enable'.
2. Create of edit a record, choosing for one of one-letter codes
   an empty value (represented by '#' in the helper).
3. Save the record, control in 'MARC preview' that you got a hash sign
   in the 008 field instead of a space.
4. Apply the patch ; restart all.
5. Open a record in advanced editor, refresh the page to reload the java
   script (Shift+Ctrl+R or another appropriate shortcut).  Edit the 008
   field again choosing for one of one-letter codes an empty value.
6. Save the record, control in 'MARC preview' that there is a space
   and not the hash sign.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 ByWater Sandboxes 2025-01-15 17:46:41 UTC
Created attachment 176582 [details] [review]
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content

When editing a bibliographic record in advanced editor, there are
helpers for fixed data coded fields (like 008).  Like in the standard
editor, empty value (space) is represented by a hash sign.  But unlike
in the standard editor, the hash sign is not converted into a space but
remains as '#' in the saved record.  This is obviously wrong.

Test plan:
==========
1. Ensure to have EnableAdvancedCatalogingEditor set to 'enable'.
2. Create of edit a record, choosing for one of one-letter codes
   an empty value (represented by '#' in the helper).
3. Save the record, control in 'MARC preview' that you got a hash sign
   in the 008 field instead of a space.
4. Apply the patch ; restart all.
5. Open a record in advanced editor, refresh the page to reload the java
   script (Shift+Ctrl+R or another appropriate shortcut).  Edit the 008
   field again choosing for one of one-letter codes an empty value.
6. Save the record, control in 'MARC preview' that there is a space
   and not the hash sign.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: esther <esther@bywatersolutions.com>
Comment 4 Jonathan Druart 2025-01-23 12:08:30 UTC
Maybe I don't understand the problem, but for me your patch introduces a regression.

I put a "#" on purpose (maybe it's silly, I was just trying to break your patch!).

Before the patch:
Editor: 008 250123b        ||||#||| |||| 00| 0 eng d
Marc detail:     @ 250123b ||||#||| |||| 00| 0 eng d 

After the patch:
Editor: 008 250123b        |||| ||| |||| 00| 0 eng d  The # disappeared here.
Marc defailt:    @ 250123b ||||#||| |||| 00| 0 eng d 

I didn't find differences when not using "#".
Comment 5 Janusz Kaczmarek 2025-01-23 15:33:33 UTC
(In reply to Jonathan Druart from comment #4)
> Maybe I don't understand the problem, but for me your patch introduces a
> regression.
> 
> I put a "#" on purpose (maybe it's silly, I was just trying to break your
> patch!).
> 
> Before the patch:
> Editor: 008 250123b        ||||#||| |||| 00| 0 eng d
> Marc detail:     @ 250123b ||||#||| |||| 00| 0 eng d 
> 
> After the patch:
> Editor: 008 250123b        |||| ||| |||| 00| 0 eng d  The # disappeared here.
> Marc defailt:    @ 250123b ||||#||| |||| 00| 0 eng d 
> 
> I didn't find differences when not using "#".

Well, depends of what are you starting with. 

If you already have # in 008, it will not show in the editor, like any other character put at the position where it is not legal (= not present at the list).  For instance, if you had 008/29 = '2' in the record you would see _ in the editor.

The main point is not to introduce '#' in 008, because this is not legal character. There should be an empty character (= space) where in the MARC 21 documentation (https://loc.gov/marc/bibliographic/bd008b.html) is used '#' ('#' is conventional character in MARC 21 doc for a blank).

This is what our "traditional" editor does - it shows '#' in the legend but puts ' ' (a blank) into 008 and this is right.

The advanced editor before the patch puts '#' in the content of the 008 which is undoubtedly wrong. 

Please start with an empty record without and with the patch, modify 008 using '#' in the advanced editor, save it and look at the data to see the effect.
Comment 6 Janusz Kaczmarek 2025-01-23 16:11:12 UTC
(In reply to Jonathan Druart from comment #4)
> After the patch:
> Editor: 008 250123b        |||| ||| |||| 00| 0 eng d  The # disappeared here.
> Marc defailt:    @ 250123b ||||#||| |||| 00| 0 eng d 

BTW there may be another bug to fill that the advanced editor does not show (and maybe highlights) the illegal values that possibly may be present in 008 (the 'traditional' 008 plugin highlights it in yellow): here '#', but to give more obvious example: 008/29 = '2' (not legal at this position).
Comment 7 Marcel de Rooy 2025-01-24 10:45:57 UTC
+                    if ( end - start > 1 ) {

What is the purpose of this test?
Should it be >0 btw ?
end index is not inclusive in the substring call

What about other places in the script where we do a similar call?
Comment 8 Janusz Kaczmarek 2025-01-24 12:09:36 UTC
(In reply to Marcel de Rooy from comment #7)
> +                    if ( end - start > 1 ) {
> 
> What is the purpose of this test?
> Should it be >0 btw ?
> end index is not inclusive in the substring call
> 
> What about other places in the script where we do a similar call?

This is to not delete spaces from one-character-long positions in 008. Without this change activating select drop-down looses the focus, because the value becomes an empty string instead of remaining ' '.  But for longer input fields, where end - start > 1 (e.g. like 008/7-10, 008/11-14 for dates) we want not to have spaces, but have the cursor placed at the beginning of the input field.
Comment 9 Marcel de Rooy 2025-01-31 09:29:34 UTC
Created attachment 177373 [details] [review]
Bug 38895: In advanced editor, the fixed data helpers put '#' instead of space in record content

When editing a bibliographic record in advanced editor, there are
helpers for fixed data coded fields (like 008).  Like in the standard
editor, empty value (space) is represented by a hash sign.  But unlike
in the standard editor, the hash sign is not converted into a space but
remains as '#' in the saved record.  This is obviously wrong.

Test plan:
==========
1. Ensure to have EnableAdvancedCatalogingEditor set to 'enable'.
2. Create of edit a record, choosing for one of one-letter codes
   an empty value (represented by '#' in the helper).
3. Save the record, control in 'MARC preview' that you got a hash sign
   in the 008 field instead of a space.
4. Apply the patch ; restart all.
5. Open a record in advanced editor, refresh the page to reload the java
   script (Shift+Ctrl+R or another appropriate shortcut).  Edit the 008
   field again choosing for one of one-letter codes an empty value.
6. Save the record, control in 'MARC preview' that there is a space
   and not the hash sign.

Sponsored-by: Ignatianum University in Cracow
Signed-off-by: esther <esther@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Katrin Fischer 2025-01-31 14:58:11 UTC
Pushed for 25.05!

Well done everyone, thank you!