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.
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.
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>
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.
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.
Since I edited the entire loop I am resetting this to needs sign-off
I don't understand what was wrong in the current code.
(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
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.
(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
Lucas, adjust the naming for "item" then we are good to go here. Any other improvements must go on its own bug report.
Looking here now
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).
Thank you Marcel!
Created attachment 135286 [details] [review] Bug 30747: (follup-up) Add missing otherholdings_datelastborrowed column in settings See Bug 24176
Pushed to master for 22.05, thanks to everybody involved [U+1F984]