Bug 29511

Summary: While editing MARC records, blank subfields appear in varying order
Product: Koha Reporter: Valerie <valerie>
Component: CatalogingAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, fridolin.somers, kyle, m.de.rooy, martin.renvoize, nick, victor
Version: 21.05Keywords: release-notes-needed
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.03,21.05.11
Attachments: 245 blank subfields not in order
245 blank subfields in different order
Bug 29511: Sort hash keys
Bug 29511: Sort hash keys
Bug 29511: Sort hash keys
Bug 29511: Sort hash keys

Description Valerie 2021-11-17 21:54:57 UTC
Created attachment 127762 [details]
245 blank subfields not in order

Edit the MARC record 245. Blank subfields appear in inconsistent order. 

Expected order:
245 a,b,c,h

Default framework order:
245 a,b,c,h

Different records with only a subfield a have the following order:
245 a,c,h,b
245 a,h,c,b
245 a,b,c,h

Record with subfield a,c may appear as:
245 a,c,b,h
245 a,a,h,b

Field 300 is also noted to have different blank subfield orders for different records.
Comment 1 Valerie 2021-11-17 21:55:45 UTC
Created attachment 127763 [details]
245 blank subfields in different order
Comment 2 Nick Clemens 2021-11-23 13:44:08 UTC
Created attachment 127955 [details] [review]
Bug 29511: Sort hash keys

This code exists in authorities/authorities.pl and the hash keys are sorted
I just do the same here

To test:
1 - Find/create a record with 245a 245h populated
2 - Load/reload the record several times and note that fields c and b are ordered randomly
3 - Apply patch
4 - Reload and note c and b are ordered alphabetically
5 - Move subfield h before subfield c
6 - Save and reload
7 - Confirm that existing/filled fields retain order in the MARC record
Comment 3 Hayley Pelham 2021-12-22 00:58:16 UTC
Created attachment 128856 [details] [review]
Bug 29511: Sort hash keys

This code exists in authorities/authorities.pl and the hash keys are sorted
I just do the same here

To test:
1 - Find/create a record with 245a 245h populated
2 - Load/reload the record several times and note that fields c and b are ordered randomly
3 - Apply patch
4 - Reload and note c and b are ordered alphabetically
5 - Move subfield h before subfield c
6 - Save and reload
7 - Confirm that existing/filled fields retain order in the MARC record

Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Comment 4 Martin Renvoize 2022-01-19 10:54:20 UTC
Created attachment 129598 [details] [review]
Bug 29511: Sort hash keys

This code exists in authorities/authorities.pl and the hash keys are sorted
I just do the same here

To test:
1 - Find/create a record with 245a 245h populated
2 - Load/reload the record several times and note that fields c and b are ordered randomly
3 - Apply patch
4 - Reload and note c and b are ordered alphabetically
5 - Move subfield h before subfield c
6 - Save and reload
7 - Confirm that existing/filled fields retain order in the MARC record

Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-01-19 10:58:34 UTC
I'm slightly torn here.. but am going to PQA as the code works and is an improvement.. I'll let the RM make the final decision.

So.. reasoning.  Random hash ordering causes this, and having the keys completely randomly ordered is indeed annoying and inconsistent.. it is considerably better to have the consistent between runs.

My minor issue with the alpha sort is that I though, at least some, MARC21 fields have a specified subfield order so it might be 'weird' that they're coming out alphanumerically.  I can't see any easy way of doing this however.. and can't even find a definitive reference for subfield ordering of any particular fields.. just mentions in various docs saying they aren't always alpha sorted.

In short.. PQA
Comment 6 Katrin Fischer 2022-01-20 20:23:10 UTC
You can now reorder the fields using drag & drop and it should keep this order if you edit the record again.

You can also modify the order of subfields permanently by drag&dropping the tabs in the frameworks.

We can order 'unordered' alphabetically, but we should not do it in general.
Comment 7 Katrin Fischer 2022-01-20 20:34:53 UTC
(In reply to Katrin Fischer from comment #6)
> You can now reorder the fields using drag & drop and it should keep this
> order if you edit the record again.
> 
> You can also modify the order of subfields permanently by drag&dropping the
> tabs in the frameworks.
> 
> We can order 'unordered' alphabetically, but we should not do it in general.

I tested the patch, and the configured order of subfields is still correct when adding a new record and editing a record with reordered fields works too. Just got wondering when reading Martin's comment about sequence.
Comment 8 Katrin Fischer 2022-01-22 18:04:19 UTC
Created attachment 129713 [details] [review]
Bug 29511: Sort hash keys

This code exists in authorities/authorities.pl and the hash keys are sorted
I just do the same here

To test:
1 - Find/create a record with 245a 245h populated
2 - Load/reload the record several times and note that fields c and b are ordered randomly
3 - Apply patch
4 - Reload and note c and b are ordered alphabetically
5 - Move subfield h before subfield c
6 - Save and reload
7 - Confirm that existing/filled fields retain order in the MARC record

Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Fridolin Somers 2022-01-29 07:51:30 UTC
From my tests, cataloguing looks better with this patch :D
Random order is way worse than a forced alphabetical order.
Comment 10 Fridolin Somers 2022-01-29 07:54:05 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 11 Martin Renvoize 2022-01-31 10:04:07 UTC
Thanks for the double check Katrin, that was really helpful :) Really happy to see this one in, and also that it doesn't break an existing ordering too :)
Comment 12 Kyle M Hall 2022-02-07 16:07:03 UTC
Pushed to 21.11.x for 21.11.03
Comment 13 Andrew Fuerste-Henry 2022-02-21 15:26:13 UTC
Pushed to 21.05.x for 21.05.11
Comment 14 Victor Grousset/tuxayo 2022-02-24 22:32:55 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.