Bug 30747

Summary: Column settings on otherholdings table in detail.tt doesnt work
Product: Koha Reporter: Lucas Gass <lucas>
Component: Staff interfaceAssignee: Lucas Gass <lucas>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, m.de.rooy, oleonard
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24176
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00
Bug Depends on: 29648    
Bug Blocks: 29282    
Attachments: Bug 30747: Make column settings work for otherholdings table
Bug 30747: Make column settings work for otherholdings table
Bug 30747: Make column settings work for otherholdings table using forEach loop
Bug 30747: Make column settings work for otherholdings table using forEach loop
Bug 30747: (follup-up) Add missing otherholdings_datelastborrowed column in settings

Description Lucas Gass 2022-05-13 14:06:19 UTC
If you turn on the system preference SeparateHoldings you will get a otherholdings table on detail.tt. Table settings cannot be applied to this table. The the Columns icon is not present in the top pager. 

The reason is because of the JS:

if ( !has_images[i] ) { // remove the cover_image column
     table_settings.splice(1,1);
}


table_settings.splice(1,1) is removing the table settings that control the otherholdings table columns.
Comment 1 Lucas Gass 2022-05-13 15:16:46 UTC
Created attachment 134979 [details] [review]
Bug 30747: Make column settings work for otherholdings table

To test:
1. Turn on SeparateHoldings and find a record that has some items at different home libraries.
2. Look at the Other holdings(x) tab. Notice there is no Columns tool (gear icon).
3. Go to Table Settings, try hiding columns from the otherholdings table. You cannot.
4. Apply patch.
5. Now under the Other holdings(x) tab you should see the Columns button, it should work.
6. Go back to Table settings and try hiding columns, it should work.
7. Now try uploading an item level cover image, make sure all columns are still properly hidden and the Columns tool works
8 Try steps 6 & 7 but with the regular holdings table. Confirm that all colums can properly hide.
Comment 2 Owen Leonard 2022-05-17 14:10:41 UTC
Created attachment 135073 [details] [review]
Bug 30747: Make column settings work for otherholdings table

To test:

1. Turn on SeparateHoldings and find a record that has some items at
   different home libraries.
2. Look at the Other holdings(x) tab. Notice there is no Columns tool
   (gear icon).
3. Go to Table Settings, try hiding columns from the otherholdings
   table. You cannot.
4. Apply patch.
5. Now under the Other holdings(x) tab you should see the Columns
   button, it should work.
6. Go back to Table settings and try hiding columns, it should work.
7. Now try uploading an item level cover image, make sure all columns
   are still properly hidden and the Columns tool works
8. Try steps 6 & 7 but with the regular holdings table. Confirm that all
   colums can properly hide.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Marcel de Rooy 2022-05-18 11:37:18 UTC
            var ids = ['holdings_table', 'otherholdings_table'];
            var table_settings = [ [% TablesSettings.GetTableSettings('catalogue', 'detail','holdings_table','json') | $raw %], [% TablesSettings.GetTableSettings('catalogue', 'detail','otherholdings_table','json')  | $raw %] ];
            var has_images = ["[% itemloop_has_images | html %]", "[% otheritemloop_has_images | html %]"];
            for (var i in ids) {
                var id = ids[i];

This code does not look good to me.
Note: for...in should not be used to iterate over an Array where the index order is important.
Comment 4 Lucas Gass 2022-05-18 14:13:52 UTC
Created attachment 135119 [details] [review]
Bug 30747: Make column settings work for otherholdings table using forEach loop

To test:

1. Turn on SeparateHoldings and find a record that has some items at
   different home libraries.
2. Look at the Other holdings(x) tab. Notice there is no Columns tool
   (gear icon).
3. Go to Table Settings, try hiding columns from the otherholdings
   table. You cannot.
4. Apply patch.
5. Now under the Other holdings(x) tab you should see the Columns
   button, it should work.
6. Go back to Table settings and try hiding columns, it should work.
7. Now try uploading an item level cover image, make sure all columns
   are still properly hidden and the Columns tool works
8. Try steps 6 & 7 but with the regular holdings table. Confirm that all
   colums can properly hide.
Comment 5 Lucas Gass 2022-05-18 14:15:09 UTC
Since I edited the entire loop I am resetting this to needs sign-off
Comment 6 Jonathan Druart 2022-05-18 14:22:33 UTC
I don't understand what was wrong in the current code.
Comment 7 Marcel de Rooy 2022-05-18 14:24:19 UTC
(In reply to Jonathan Druart from comment #6)
> I don't understand what was wrong in the current code.

for..in is for objects not regular arrays
Comment 8 Fridolin Somers 2022-05-18 19:55:15 UTC
Hi, good catch

+            ids.forEach( function(item, index) {

Ich, using 'item' for something else than catalogue (item of a biblio record) is not a good idea I think.
Maybe 'table_id' would be better.
Comment 9 Lucas Gass 2022-05-18 20:40:21 UTC
(In reply to Fridolin Somers from comment #8)
> Hi, good catch
> 
> +            ids.forEach( function(item, index) {
> 
> Ich, using 'item' for something else than catalogue (item of a biblio
> record) is not a good idea I think.
> Maybe 'table_id' would be better.

Yes, good point. I am still unsure how Jonathan and Marcel would like this implemented. Will the forEach() work? If so, I will change item to something better! Thanks Frido
Comment 10 Jonathan Druart 2022-05-19 04:51:56 UTC
Lucas, adjust the naming for "item" then we are good to go here. Any other improvements must go on its own bug report.
Comment 11 Marcel de Rooy 2022-05-19 05:51:23 UTC
Looking here now
Comment 12 Marcel de Rooy 2022-05-19 06:10:55 UTC
Created attachment 135190 [details] [review]
Bug 30747: Make column settings work for otherholdings table using forEach loop

To test:

1. Turn on SeparateHoldings and find a record that has some items at
   different home libraries.
2. Look at the Other holdings(x) tab. Notice there is no Columns tool
   (gear icon).
3. Go to Table Settings, try hiding columns from the otherholdings
   table. You cannot.
4. Apply patch.
5. Now under the Other holdings(x) tab you should see the Columns
   button, it should work.
6. Go back to Table settings and try hiding columns, it should work.
7. Now try uploading an item level cover image, make sure all columns
   are still properly hidden and the Columns tool works
8. Try steps 6 & 7 but with the regular holdings table. Confirm that all
   colums can properly hide.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Included a rename to table_name(s).
Comment 13 Lucas Gass 2022-05-19 15:16:50 UTC
Thank you Marcel!
Comment 14 Fridolin Somers 2022-05-23 18:48:52 UTC
Created attachment 135286 [details] [review]
Bug 30747: (follup-up) Add missing otherholdings_datelastborrowed column in settings

See Bug 24176
Comment 15 Fridolin Somers 2022-05-23 18:53:48 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄