|
Description
Jonathan Druart
2023-04-20 06:46:18 UTC
*** Bug 23024 has been marked as a duplicate of this bug. *** I've pushed a remote branch as WIP/POC, see https://gitlab.com/joubu/Koha/-/commits/bug_33568 This is really awesome, please have a look! I let the git history to show how I made the changes, step by step. But we could decide to squash later if needed. There is still a lot of work to be done, and I would like to make sure I won't be alone on this one before spending more time on it. I think we really to do this move, it will help people with lot of items attached to a single biblio. I am expecting support with: * early code review (now, it's ready for that) * answer the FIXMEs (in git log and code), can be done inline in gitlab? * help with the remaining work * testing session (it will need to be fully tested before we push it) * keeping in mind that this is on its way and we should not work/push big changes to this script/template (!) It's either a candidate for an early push to 23.11, and a couple of you are with me, or I drop it now (read that I am not going to rebase it if the conflicts are getting tricky). Let me know what you think about it and if you are willing (and how) to help me here. (In reply to Jonathan Druart from comment #2) > I think we really to do this move, it will help people with lot of items > attached to a single biblio. I 100% agree. Since this is one of the big limitations I bump into with Koha these days, I should be able to find time to test at the very least. Oh wait... this is for the staff interface? Hmm... it's probably a more concern for us for the OPAC. Telling staff they have to wait a while isn't so bad in comparison to expecting public users to wait. I was looking at the REST API route /biblios/XXX/items for some discovery but realized it needed more for the "Status" column unless I wanted to re-implement all the logic that we've embedded in the template view layer... Wonder if there's something to be done with this one and bug 16826 and https://github.com/NatLibFi/koha-plugin-rest-di On (In reply to David Cook from comment #5) > I was looking at the REST API route /biblios/XXX/items for some discovery > but realized it needed more for the "Status" column unless I wanted to > re-implement all the logic that we've embedded in the template view layer... > > Wonder if there's something to be done with this one and bug 16826 and > https://github.com/NatLibFi/koha-plugin-rest-di That is totally outside the scope of this bug. (In reply to Jonathan Druart from comment #6) > That is totally outside the scope of this bug. Sure although it'll relate in the future one hopes. Remote branch rebased and a couple of fixes added. Setting status to Needs Signoff, this is ready to get feedback from testers. Checkbox selection should be preserve when a new request is made (pagination, filtering, etc.) Created attachment 151339 [details] [review] Bug 33568: Preserve item selection across pagination and filtering BTW Do we really need StaffDetailItemSelection? (In reply to Jonathan Druart from comment #8) > Remote branch rebased and a couple of fixes added. > > Setting status to Needs Signoff, this is ready to get feedback from testers. Is there a test plan? btw: it is not possible to test the patch in a sandbox http://staff-b33568.sandboxes.ptfs-europe.co.uk/ Error: Template process failed: undef error - C4::Koha::GetAuthorisedValues(): DBI Exception: DBD::mysql::st execute failed: 'koha_b32748.av.category' isn't in GROUP BY at /kohadevbox/koha/Koha/Template/Plugin/AuthorisedValues.pm line 39 at /kohadevbox/koha/C4/Templates.pm line 127 (In reply to Michaela Sieber from comment #11) > (In reply to Jonathan Druart from comment #8) > > Remote branch rebased and a couple of fixes added. > > > > Setting status to Needs Signoff, this is ready to get feedback from testers. > > > Is there a test plan? There is a lot to test here, so it's hard to provide an exhaustive list. The idea is to catch bugs I have missed. You need to play with: * datatable features (column visibility, sorting, filtering, show/hide filters) * sysprefs SeparateHoldings and SeparateHoldingsBranch * syspref StaffDetailItemSelection * the different action links (when you select at least one item) * item cover images * item groups (EnableItemGroups) * bundles of items (there is still a FIXME to display the 'Manage bundle' button) * display of other values (recall, status, holds, checkout and renewal count, URIs) * host records * course reserves * analytic (this is still a FIXME) * print label button (SpineLabelShowPrintOnBibDetails) * edit permissions will show the edit buttons * and the other things I have missed in this list? :) In the code there are still FIXMEs: * display library names instead of codes * desk name is not displayed anymore * checkout due date is not correctly formatted * other technical ones for QA. > btw: it is not possible to test the patch in a sandbox > http://staff-b33568.sandboxes.ptfs-europe.co.uk/ > > Error: > Template process failed: undef error - C4::Koha::GetAuthorisedValues(): DBI > Exception: DBD::mysql::st execute failed: 'koha_b32748.av.category' isn't in > GROUP BY at /kohadevbox/koha/Koha/Template/Plugin/AuthorisedValues.pm line 39 > at /kohadevbox/koha/C4/Templates.pm line 127 There is something wrong going on but it's not related to this patch. The same of the sandbox was 'b33568' but the error is refering to another instance 'b32748'. To test this change you need to pass the info to the remote branch (there was a patch that has been wrongly attached here): 1. Don't pass a bug number 2. Click "advanced options" 3. "Git remote & branch" should be https://gitlab.com/joubu/Koha and bug_33568 I've just tested and it is working, see https://staff-bug33568.sandboxes.biblibre.eu/cgi-bin/koha/catalogue/detail.pl?biblionumber=1 (In reply to Jonathan Druart from comment #12) > In the code there are still FIXMEs: > * display library names instead of codes Done in Bug 33568: Display library names instead of codes for recall Bug 33568: Display library names instead of codes for first hold Bug 33568: Display library names instead of codes for transfers > * desk name is not displayed anymore Done in Bug 33568: Display the desk name for the first hold > * checkout due date is not correctly formatted Done in Bug 33568: Use as_due_date to display due dates Remaining TODOs: * bundles of items (display the 'Manage bundle' button) * host records * analytic A new one: * Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523. (In reply to Jonathan Druart from comment #13) > Remaining TODOs: > * bundles of items (display the 'Manage bundle' button) Done in Bug 33568: Restore bundle items > * host records Done in Bug 33568: Restore host records > * analytic Done in Bug 33568: Restore analytics Remaning TODOs: * Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523. * Fix bug 33817 * Fix bug 33804 New branch pushed: * Fixed a bug about SeparateHoldings * Code moved to an include file * New patch for discussion: """ Bug 33568: Fix DT dom and remove DT info if not needed Was there a real need to deal with a specific dom? The "no filter" view was not really light, only the column filters were shown/hidden. This patch is suggesting to remove all the DT info in case the table is displaying all the items of the bibliographic record: Pagination, filtering, buttons, etc. """ A few minor things: - use "fa fa-filter" icon for the "Show filters" link - the bulk actions controls (delete, modify) appear if you click a checkbox but not if you click "Select all" - The checkbox column should not be sortable (an existing bug) I like the choice to hide all the table controls until you click "Show filters" but I am concerned that others might miss the quick filter box. I hope others will comment on that. (In reply to Owen Leonard from comment #16) > A few minor things: Thanks! > - use "fa fa-filter" icon for the "Show filters" link Commit "Restore filters" adjusted. > - the bulk actions controls (delete, modify) appear if you click a checkbox > but not if you click "Select all" Done in "Fix select/clear all links". > - The checkbox column should not be sortable (an existing bug) This seems trivial to fix but it's not. The problem is the StaffDetailItemSelection (I've let a comment somewhere, IMO we should remove it, I don't see how it can be useful). As it's an existing bug I would like to deal with this specific bug on a separate bug report. I have opened bug 33860. > I like the choice to hide all the table controls until you click "Show > filters" but I am concerned that others might miss the quick filter box. I > hope others will comment on that. There was actually a bug. I've adjusted "Fix DT dom and remove DT info if not needed". The idea is to display the search and pagination only if there is pagination. If all the items are displayed then we show a simple table and browser search ctrl+f can then be used to search on the displayed info. * Filters are operating on codes not values, 'FPL' works in 'Home library' but 'Fairview' doesn't * Location appears in 'Home library' column, but cannot be filtered on code or value * Status column not filterable or sortable * Date columns filters only work on SQL formats '2023-06-01' works '06/01/2023' doesn't * Click 'show filters', 'hide filters' and repeat X 2 - status column disappears? If you do choose to show all items, a sort action requires reloading the whole table - is it possible to sort/filter on the loaded data? (In reply to Nick Clemens from comment #18) > * Filters are operating on codes not values, 'FPL' works in 'Home library' > but 'Fairview' doesn't Yes, we should add dropdown lists for itemtype and libraries, like we do on the patron search. But for instance "home library" contains the location as well, that's not that easy :-/ > * Location appears in 'Home library' column, but cannot be filtered on code > or value See above. > * Status column not filterable or sortable Yes, that's no longer possible, and won't be possible, at least in a first iteration. That's not trivial it seems. See the following commit: Bug 33568: Status FIXME We need tests here! See the FIXME in the code, we are losing the ability to search on the status, but this is really impossible to implement if we are not storing the status (DB or search engine). > * Date columns filters only work on SQL formats '2023-06-01' works > '06/01/2023' doesn't I thought I fixed that earlier somewhere else. And I did, on https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=145096 But that's not nice. Not considering this high priority for now and considering outside the scope of this bug. > * Click 'show filters', 'hide filters' and repeat X 2 - status column > disappears? See the following commit: Bug 33568: Restore filters FIXME - If we click 'show filters' the 'columns' list does not contain all the columns. We are destroying the DT and the hidden columns are removed from the dom. I think there are 2 possible solutions here: 1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage to make this work correctly, because of the th filters) 2. Move the th definition in the JS init of DT > If you do choose to show all items, a sort action requires reloading the > whole table - is it possible to sort/filter on the loaded data? We could imagine a "freeze" button to remove the AJAX driven behaviour, if all data are loaded. But the behaviour will then be different. It will help to filter/sort on columns on which it's not possible server-side, but I am not sure it's a good idea... Remaining TODOs (in addition of the FIXMEs in the code): * Display the library name of the patron "A patron from LIBRARY NAME" if the patron's data must be hidden. This is related to bug 29523. * Fix bug 33817 * Fix bug 33804 * Add dropdown lists for itemtype, libraries (current library and library) and collection in the column filter row (like we do on the main patron search). However how do we deal with "home library" which contains the location as well? * Search by formatted date (see bug 32505) (In reply to Jonathan Druart from comment #19) > (In reply to Nick Clemens from comment #18) > > * Click 'show filters', 'hide filters' and repeat X 2 - status column > > disappears? > > See the following commit: > Bug 33568: Restore filters > > FIXME - If we click 'show filters' the 'columns' list does not contain > all the columns. > We are destroying the DT and the hidden columns are removed from the > dom. > > I think there are 2 possible solutions here: > 1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage > to make this work correctly, because of the th filters) > 2. Move the th definition in the JS init of DT Should be fixed now. Bug 33568: (folow-up) Restore filters *** Bug 29527 has been marked as a duplicate of this bug. *** (In reply to Jonathan Druart from comment #19) > * Search by formatted date (see bug 32505) Done on bug 34226. Remote branch rebased against master. This is working really well in all the tests I could think of. I identified two issues: 1. Item cover images are not showing up. 2. The "Select all" control doesn't trigger the display of the batch actions controls, and likewise the "Clear all" control doesn't hide them. Clicking the checkbox works as it should. (In reply to Owen Leonard from comment #24) Thanks a lot for your time on this one! :) > This is working really well in all the tests I could think of. I identified > two issues: > > 1. Item cover images are not showing up. See commit "Bug 33568: Fix local cover images" > 2. The "Select all" control doesn't trigger the display of the batch actions > controls, and likewise the "Clear all" control doesn't hide them. > Clicking the > checkbox works as it should. I cannot recreate this exact problem but I found a couple of problems with the checkbox selection and the show/hide filters, which have been fixed on "Bug 33568: Fix 'actions' links when filters are shown/hidden", so hopefully yours is fixed as well. Created attachment 153791 [details]
Cover image browser with images attached to three items
The cover image navigation seems to be getting duplicated.
(In reply to Jonathan Druart from comment #25) > See commit "Bug 33568: Fix local cover images" > "Bug 33568: Fix 'actions' links when filters are shown/hidden" These issues have indeed been resolved. (In reply to Owen Leonard from comment #26) > Created attachment 153791 [details] > Cover image browser with images attached to three items > > The cover image navigation seems to be getting duplicated. Hum, interesting :D Fixed (amended the patch)! - $(".cover-slider").each(function(){ + cover_sliders.each(function(){ 2 new patches added to the remote branch, for performance purpose: Bug 33568: Do not embed library names Bug 33568: Do not embed item types 1000 items generated for a given bibliographic record. 1000 items displayed: without last 2 patches: 28s with last 2 patches: 18s master: 15s (But lack on the UI as you see the 1000 items not paginated) 20 items displayed: without last 2 patches: 0.9s with last 2 patches: 0.7s master: 15s (cannot display 20 items, all displayed) I'm getting this error which breaks the loading of the table: Uncaught TypeError: can't access property "notforloan", row.item_type is undefined (In reply to Owen Leonard from comment #30) > I'm getting this error which breaks the loading of the table: > > Uncaught TypeError: can't access property "notforloan", row.item_type is > undefined Should be fixed now. Looks like I forgot to include one change in my previous push :-/ I noticed a couple more issues: - With SeparateHoldings enabled (set to "home library"), the tabs show up correctly and the contents of the tabs appear to be correct but the DataTables filters are enabled by default, though the control says "Show filters" Clicking "Show filters" makes the tables disappear. - I happened to look at a record from my own test data having items which had invalid item types. This caused a 500 error in the console and prevented the table from loading. (In reply to Owen Leonard from comment #32) > I noticed a couple more issues: > > - With SeparateHoldings enabled (set to "home library"), the tabs show up > correctly and the contents of the tabs appear to be correct but the > DataTables filters are enabled by default, though the control says "Show > filters" I don't see that. The filters are not there and the link is "Show filters". > Clicking "Show filters" makes the tables disappear. However I am seeing that. It should be fixed now (commit "Bug 33568: (folow-up) Restore filters" has been adjusted) > - I happened to look at a record from my own test data having items which > had invalid item types. This caused a 500 error in the console and prevented > the table from loading. This is actually "expected". Koha is not supposed to behave correctly if invalid item types are used. It's not directly related to this bug, see bug 33166. For the record, the error is "Can't call method "notforloan" on an undefined value at /kohadevbox/koha/Koha/Item.pm line 1510." (In reply to Jonathan Druart from comment #33) > I don't see that. The filters are not there and the link is "Show filters". Well, I'm not seeing it now either. I do see another issue with the SeparateHoldings preference: If you're looking at a record which has no items that belong to "your" library you'll see a tab for that library saying "No matching records" See http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=9 in the sample data for example (with SeparateHoldingsBranch set to "home library"). Before this patch Koha would display only a single "Holdings" tab showing all items. I'm not sure this is necessarily an unwanted change, but it is a difference. (In reply to Owen Leonard from comment #34) > (In reply to Jonathan Druart from comment #33) > > I don't see that. The filters are not there and the link is "Show filters". > > Well, I'm not seeing it now either. I do see another issue with the > SeparateHoldings preference: > > If you're looking at a record which has no items that belong to "your" > library you'll see a tab for that library saying "No matching records" See > http://127.0.0.1:8081/cgi-bin/koha/catalogue/detail.pl?biblionumber=9 in the > sample data for example (with SeparateHoldingsBranch set to "home library"). > > Before this patch Koha would display only a single "Holdings" tab showing > all items. I'm not sure this is necessarily an unwanted change, but it is a > difference. Nice one, thanks! Fixed in "Bug 33568: Hide holdings tab if empty (SeparateHoldings)" Add my signoff, Jonathan, I think I've tested everything I can think of! (In reply to Owen Leonard from comment #36) > Add my signoff, Jonathan, I think I've tested everything I can think of! Thanks a lot, Owen! I've added your signed-off-by lines to the remote branch! And rebased on top of bug 34913. Still looks good, no obvious errors. A bit late, tested on real data with record having more than 2500 items What was wrong in my test : 1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n \n \n \n display 2) no checkbox in Other holdings tab if there is the first "my library" tab (even as superlibrarian) 3) cannot place a hold, gateway timeout after having chosen the patron from the suggested list 4) with syspref StaffDetailItemSelection disabled : impossible to charge table (error 500) 5) EnableItemGroups : No item group column in Other holdings tab (In reply to Laurence Rault from comment #39) > A bit late, tested on real data with record having more than 2500 items > > What was wrong in my test : > 1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n > \n \n \n display > > 2) no checkbox in Other holdings tab if there is the first "my library" tab > (even as superlibrarian) > > 3) cannot place a hold, gateway timeout after having chosen the patron from > the suggested list > > 4) with syspref StaffDetailItemSelection disabled : impossible to charge > table (error 500) > > 5) EnableItemGroups : No item group column in Other holdings tab Can you provide me with the commit id on which you have tested? Using the English interface or French? (In reply to Laurence Rault from comment #39) Where have you tested, on a sandbox? > A bit late, tested on real data with record having more than 2500 items > > What was wrong in my test : > 1) each time I click show filter or hide filter : some \n \n \n \n \n \n \n > \n \n \n display Can you share a screenshot? > 2) no checkbox in Other holdings tab if there is the first "my library" tab > (even as superlibrarian) Same, I don't recreate, here is what I see: https://snipboard.io/pCzOst.jpg > 3) cannot place a hold, gateway timeout after having chosen the patron from > the suggested list I don't think it's caused by this patch. > 4) with syspref StaffDetailItemSelection disabled : impossible to charge > table (error 500) I do recreate this one! > 5) EnableItemGroups : No item group column in Other holdings tab Looks like this is totally broken with my last rebase. I don't manage to create a new item group (broken by bug 34913). (In reply to Jonathan Druart from comment #41) > (In reply to Laurence Rault from comment #39) > > 4) with syspref StaffDetailItemSelection disabled : impossible to charge > > table (error 500) > > I do recreate this one! Fixed on "Bug 33568: Fix ordering". It should also correct the default ordering. Tested on specific vm, on branch bug_33568. Branch updated, so now : 1) \n : solved 2) checkbox : solved 3) cannot place a hold, gateway timeout after having chosen the patron from the suggested list load of page request.pl : 40 s for 1000 items more than 60 s for 1600 items (and timeout at 60s) The patch does not change items display for this page ? 4) solved 5) items group broken as you said (In reply to Laurence Rault from comment #43) > 3) cannot place a hold, gateway timeout after having chosen the patron from > the suggested list > > load of page request.pl : > 40 s for 1000 items > more than 60 s for 1600 items (and timeout at 60s) > The patch does not change items display for this page ? Nope! Only catalogue/detail.pl (In reply to Jonathan Druart from comment #41) > (In reply to Laurence Rault from comment #39) > > 5) EnableItemGroups : No item group column in Other holdings tab > > Looks like this is totally broken with my last rebase. I don't manage to > create a new item group (broken by bug 34913). I fixed the creation of item groups on the separate bug ("Bug 34913: Move item groups table to kohaTable"). The patch is on the remote branch now. I don't recreate the problem. I am seeing the column is at least one item of the table is in an item group. As soon as I see 29523 move I'll come back to this.. it's right at the top of my pile but I'm struggling to juggle with that one still teetering on the edge. ;) Testing here... I am sorry, but my testing didn't go as well as I had hoped for :( Problems: 1) One of the commit messages needs to be fixed 2) When there is no barcode, it appears as 'null' in the column 3) 'Host records' column should not be available, when EasyAnalytics is turned off. 4) Table configuration doesn't appear to be used as expected. I checked: Catalog > holdings_table has only holdings_issues and holdings_renewals hidden by default. But the table keeps appearing with only a minimum number of columns and a lot of them hidden. Unfortunately the hidden column also includes much neded ones like callnumber. Any changes I make don't seem to stick. 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in small size modal. This also has the effect, that there is no way to close this window without printing. 6) The status column is no longer sortable. It's sortable in 22.11 and works well there. Alos: I can't activate a filter for this column and the normal filter doesn't find the contents of the column. There is no way to find the 'checked out' or 'ordered' items in that list. 7) When an item is checked out, there is no longer a link to the patron's account. 8) hidepatronname doesn't seem to have any effect. 9) Using the single search box on top of the table: "checked" doesn't find the checked out items. In 22.11 that works. 10) StaffDetailItemSelection: when disabled, not only the checkboxes are gone, but I can also no longer activate the column specific filters. In 22.11 this is still possible when the select feature is disabled. Notes/Questions: 1) Currently some column only show when there is data for them, like course reserves. this is very helpful in preserving space, but doesn't seem to be the case anymore. 2) Right now we see all items immediately in the table, one the page has loaded. Now we only see the first 20. Libraries might feel this is a too low default setting. It would be great if we could have that configurable in table settings from the start. Works well: 1) Date sorting with dd.mm.yyyy 2) Checkboxes between pages are preserved. 3) SeparateHoldings*: works as expected (In reply to Katrin Fischer from comment #48) > I am sorry, but my testing didn't go as well as I had hoped for :( > > Problems: > 1) One of the commit messages needs to be fixed Two were kept for history, I removed them from the branch. > 2) When there is no barcode, it appears as 'null' in the column Fixed in "Bug 33568: Do not display 'null' when no barcode" > 3) 'Host records' column should not be available, when EasyAnalytics is > turned off. Fixed in "Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off" > 4) Table configuration doesn't appear to be used as expected. I checked: > Catalog > holdings_table has only holdings_issues and holdings_renewals > hidden by default. But the table keeps appearing with only a minimum number > of columns and a lot of them hidden. Unfortunately the hidden column also > includes much neded ones like callnumber. Any changes I make don't seem to > stick. The table is hiding empty columns. So if the x items displayed on the page do not have callnumber, then the column will be hidden, even if other pages have items with callnumber. Is that what you are seeing? See commit message "Bug 33568: Hide empty columns". It is what we are doing already on master, but here we don't want to fetch all the items to know if data exist for each each column... > 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in > small size modal. This also has the effect, that there is no way to close > this window without printing. Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails" > 6) The status column is no longer sortable. It's sortable in 22.11 and works > well there. Alos: I can't activate a filter for this column and the normal > filter doesn't find the contents of the column. There is no way to find the > 'checked out' or 'ordered' items in that list. See commit message for "Bug 33568: Status" """ See the FIXME in the code, we are losing the ability to search on the status, but this is really impossible to implement if we are not storing the status (DB or search engine). """ > 7) When an item is checked out, there is no longer a link to the patron's > account. Fixed in "Bug 33568: Restore link to patron" > 8) hidepatronname doesn't seem to have any effect. I think this is a bug in js-patron-format.inc, it should be reported separately. > 9) Using the single search box on top of the table: "checked" doesn't find > the checked out items. In 22.11 that works. If we want that we need a filter at the top of the page. That should be a separate bug. > 10) StaffDetailItemSelection: when disabled, not only the checkboxes are > gone, but I can also no longer activate the column specific filters. In > 22.11 this is still possible when the select feature is disabled. Fixed in "Bug 33568: Restore column filters when StaffDetailItemSelection is off" > Notes/Questions: > 1) Currently some column only show when there is data for them, like course > reserves. this is very helpful in preserving space, but doesn't seem to be > the case anymore. "Preserving space"? See above. > 2) Right now we see all items immediately in the table, one the page has > loaded. Now we only see the first 20. Libraries might feel this is a too low > default setting. It would be great if we could have that configurable in > table settings from the start. It should be very easy to implement. I will do it when this reaches master. Hi Jonathan, this was a quick return :) There are at least 2 points that give me some headache here in that it changes behavior in a way that will hurt library workflows. (marked DISCUSS). I think they need more time/thought and eyes on them before we are ready here. Librarians: We need you to tell us how you use this table! > > Problems: > > 1) One of the commit messages needs to be fixed > Two were kept for history, I removed them from the branch. NOT OK - QA script still complains: * Commit title does not contain 'follow-up' correctly spelt - fd8d8f5ba2 (Bug 33568: (folow-up) Restore filters) * We also got some perltidy complaints on top. > > 2) When there is no barcode, it appears as 'null' in the column > Fixed in "Bug 33568: Do not display 'null' when no barcode" OK - Confirmed fixed. > > 3) 'Host records' column should not be available, when EasyAnalytics is > > turned off. > > Fixed in "Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off" OK - Confirmed fixed. > > 4) Table configuration doesn't appear to be used as expected. I checked: > > Catalog > holdings_table has only holdings_issues and holdings_renewals > > hidden by default. But the table keeps appearing with only a minimum number > > of columns and a lot of them hidden. Unfortunately the hidden column also > > includes much neded ones like callnumber. Any changes I make don't seem to > > stick. > > The table is hiding empty columns. So if the x items displayed on the page > do not have callnumber, then the column will be hidden, even if other pages > have items with callnumber. Is that what you are seeing? > See commit message "Bug 33568: Hide empty columns". > It is what we are doing already on master, but here we don't want to fetch > all the items to know if data exist for each each column... DISCUSS - Yes, confirmed - the column visibility is determined by the value savailable on the first visible page. I think this could be problematic, I'd really like to get more opinions, favourably from daily Koha users on this. > > 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in > > small size modal. This also has the effect, that there is no way to close > > this window without printing. > > Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails" NOT OK - I yarn build, restart_all... but the modal is not 'modaling' for me. I still opens full size in a new tab. > > 6) The status column is no longer sortable. It's sortable in 22.11 and works > > well there. Alos: I can't activate a filter for this column and the normal > > filter doesn't find the contents of the column. There is no way to find the > > 'checked out' or 'ordered' items in that list. > > See commit message for "Bug 33568: Status" > """ > See the FIXME in the code, we are losing the ability to search on the > status, > but this is really impossible to implement if we are not storing the > status (DB or search engine). > """ DISCUSS - So the problem here is that we are storing different numeric values and therefore cannot sort on anything useful... is that it? I believe that could be an issue for a lot of libraries, as it stands now for the before and after: Before * Can sort on status * Can search on status using column filter * Can search on status using the top table search field (overall search) After * Can't do any of that. It's definitely something that we need to get more eyes on again as it could be seen as quite disruptive. Crazy idea: could we collect the status appearing in the data (all of them) and provide a pull down for limiting somehow? A multi select maybe? > > 7) When an item is checked out, there is no longer a link to the patron's > > account. > > Fixed in "Bug 33568: Restore link to patron" OK - Confirmed fixed. > > 8) hidepatronname doesn't seem to have any effect. > > I think this is a bug in js-patron-format.inc, it should be reported > separately. I double-checked in current master: hide: Checked out to 42 : due 04.11.2023 don't hide: Checked out to koha (42) : due 04.11.2023 So it does look like it's related to this patch set somehow. Sorry. > > 9) Using the single search box on top of the table: "checked" doesn't find > > the checked out items. In 22.11 that works. > > If we want that we need a filter at the top of the page. That should be a > separate bug. I was talking about the filter on top of the table - are we talking about the same thing? > > 10) StaffDetailItemSelection: when disabled, not only the checkboxes are > > gone, but I can also no longer activate the column specific filters. In > > 22.11 this is still possible when the select feature is disabled. > > Fixed in "Bug 33568: Restore column filters when StaffDetailItemSelection is > off" OK - Confirmed fixed. 11) NEW: Column filters for item type and current library don't show as a pull down. Holding branch does. > > Notes/Questions: > > 1) Currently some column only show when there is data for them, like course > > reserves. this is very helpful in preserving space, but doesn't seem to be > > the case anymore. > > "Preserving space"? > See above. Yep, understood now. Hesitating. > > 2) Right now we see all items immediately in the table, one the page has > > loaded. Now we only see the first 20. Libraries might feel this is a too low > > default setting. It would be great if we could have that configurable in > > table settings from the start. > > It should be very easy to implement. I will do it when this reaches master. I filed it separately as: Bug 35191 - Make entries per page configurable for items table on staff detail page (In reply to Katrin Fischer from comment #50) > > > Problems: > > > 1) One of the commit messages needs to be fixed > > Two were kept for history, I removed them from the branch. > > NOT OK - QA script still complains: > * Commit title does not contain 'follow-up' correctly spelt - fd8d8f5ba2 > (Bug 33568: (folow-up) Restore filters) Indeed, I missed that one, fixed now. > * We also got some perltidy complaints on top. It's too late now. It is too time consuming to fix inline, and I don't like the big "tidy" patches... > > > 4) Table configuration doesn't appear to be used as expected. I checked: > > > Catalog > holdings_table has only holdings_issues and holdings_renewals > > > hidden by default. But the table keeps appearing with only a minimum number > > > of columns and a lot of them hidden. Unfortunately the hidden column also > > > includes much neded ones like callnumber. Any changes I make don't seem to > > > stick. > > > > The table is hiding empty columns. So if the x items displayed on the page > > do not have callnumber, then the column will be hidden, even if other pages > > have items with callnumber. Is that what you are seeing? > > See commit message "Bug 33568: Hide empty columns". > > It is what we are doing already on master, but here we don't want to fetch > > all the items to know if data exist for each each column... > > DISCUSS - Yes, confirmed - the column visibility is determined by the value > savailable > on the first visible page. > > I think this could be problematic, I'd really like to get more opinions, > favourably from daily Koha users on this. As explained, I didn't find an alternative. > > > 5) SpineLabelShowPrintOnBibDetails: the callnumber print on longer opens in > > > small size modal. This also has the effect, that there is no way to close > > > this window without printing. > > > > Fixed in "Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails" > > NOT OK - I yarn build, restart_all... but the modal is not 'modaling' for > me. I still opens > full size in a new tab. You don't need to rebuild. It seems ok for me however, the pop-up opens in a new window. Could you try to add a console.log there: diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc index acda1e9edd4..e24e6deb661 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers/tables/items/catalogue_detail.inc @@ -818,6 +818,7 @@ [% IF ( SpineLabelShowPrintOnBibDetails ) %] $(".print-label").on("click", function(e){ + console.log("click"); e.preventDefault(); link = $(this).attr("href"); openWindow(link,"Print spine label",400,400); open the console and see if you see the "click" when you click on the "Print label" button? > > > 6) The status column is no longer sortable. It's sortable in 22.11 and works > > > well there. Alos: I can't activate a filter for this column and the normal > > > filter doesn't find the contents of the column. There is no way to find the > > > 'checked out' or 'ordered' items in that list. > > > > See commit message for "Bug 33568: Status" > > """ > > See the FIXME in the code, we are losing the ability to search on the > > status, > > but this is really impossible to implement if we are not storing the > > status (DB or search engine). > > """ > > DISCUSS - So the problem here is that we are storing different numeric > values and therefore cannot sort on anything useful... is that it? > > I believe that could be an issue for a lot of libraries, as it stands now > for the before and after: > > Before > * Can sort on status > * Can search on status using column filter > * Can search on status using the top table search field (overall search) > > After > * Can't do any of that. > > It's definitely something that we need to get more eyes on again as it could > be seen as quite disruptive. Yes, it has been advertised since the beginning. > Crazy idea: could we collect the status appearing in the data (all of them) > and provide a pull down for limiting somehow? A multi select maybe? Maybe, but it's definitely not for now neither here. We need to move the "status" logic from the template to a module. That is something we need to do, but it is outside the scope of this bug. > > > 8) hidepatronname doesn't seem to have any effect. > > > > I think this is a bug in js-patron-format.inc, it should be reported > > separately. > > I double-checked in current master: > hide: Checked out to 42 : due 04.11.2023 > don't hide: Checked out to koha (42) : due 04.11.2023 > So it does look like it's related to this patch set somehow. Sorry. It is a bug in js-patron-format.inc, but we were not using it before (patron-title.inc). I finally decided to fix in a follow-up here. Fixed in "Bug 33568: Fix patron_to_html display". Read the commit message, there are more fixes in this one. > > > 9) Using the single search box on top of the table: "checked" doesn't find > > > the checked out items. In 22.11 that works. > > > > If we want that we need a filter at the top of the page. That should be a > > separate bug. > > I was talking about the filter on top of the table - are we talking about > the same thing? Yes. This is the same as the "status" problem. "checked" is not in a DB column. We can have this quite easily (without the other statuses however), but it's not a simple patch. > 11) NEW: Column filters for item type and current library don't show as a > pull down. Holding branch does. I do see a select there. Can you retest or provide more steps to recreate? This is what I am seeing after a reset_all on /catalogue/detail.pl?biblionumber=4, click "Show filters" https://snipboard.io/9E0VwH.jpg > > > 2) Right now we see all items immediately in the table, one the page has > > > loaded. Now we only see the first 20. Libraries might feel this is a too low > > > default setting. It would be great if we could have that configurable in > > > table settings from the start. > > > > It should be very easy to implement. I will do it when this reaches master. > > I filed it separately as: > Bug 35191 - Make entries per page configurable for items table on staff > detail page I will have a look. If I understand correctly, this is "in discussion". I need to know if people still wants it for 23.11, do I spend more time on it today or do we postpone the remaining work to next month/year? The problem I see is, that we have lot of libraries that won't need this change, they have only a few items per record and everything works fine. They might see some of this a a regression, also it seems we still have some things to iron out and we are running out of time. Then there is the big libraries that would be happy to have this as they experience problems with records with many items - they might be happy to accept the changes. I think rushing a decision between 3 people (you, me, RM) here would not be good right now. We should aim to have this on the Road map early next release and maybe we can get some more reactions (I am thinking of ways to reach out) OK. Not sure I am ready for another round of rebase however... I'd be keen to see this pushed pretty much immediately after release giving us a whole cycle to really work through the issues presented and making for a trivial backport for those libraries very keen. Moving forward please? (In reply to Katrin Fischer from comment #52) > I think rushing a decision between 3 people (you, me, RM) here would not be > good right now. We should aim to have this on the Road map early next > release and maybe we can get some more reactions (I am thinking of ways to > reach out) I'm in favour of this change conceptually, but haven't followed it very closely. What input is needed from a wider audience? Happy to provide targeted feedback if someone can tell me what they want feedback on :) I think we should put it on the road map if it isn't yet and I'd really like to give libraries a chance to get involved. With the holidays coming up, I mid-January might be a good time window to work on this. How can we get feedback from libraries if it is not pushed? This was supposed to be pushed early in the cycle. I agree with Joubu - this will become unwieldy to improve unless pushed. It will make some changes to the current display, and we can handle those individually on smaller bugs once we have this base. Remote branch rebased. We can take this for our libraries in Finland and maybe for Ukraine in our custom build for testing, until this "more or less complete". They asking for this feature. Remote branch rebased against master. (In reply to Andrii Nugged from comment #61) > We can take this for our libraries in Finland and maybe for Ukraine in our > custom build for testing, until this "more or less complete". They asking > for this feature. Is it being tested? Any feedback? I'll take a look at this early next week. I think I can get others from MCPL to test too - we have bibs with hundreds of items, and would definitely like to see performance improvements on that move forward! (In reply to Emily Lamancusa from comment #64) > I'll take a look at this early next week. I think I can get others from MCPL > to test too - we have bibs with hundreds of items, and would definitely like > to see performance improvements on that move forward! Thanks Emily, that would be awesome. Notes from my testing:
The majority of it seems to be working as it's supposed to. I ran into a few problems with filtering, though:
1. When using the general "Search" field at the top of the table, the filter often failed with the error message: “Something went wrong when loading the table: 500 Internal Server Error”. In this case, the displayed result set remained unchanged.
- always got the error if "Show entries" was set to a value other than "All" and I
searched for a value that would return 0 results
- always got the error if "Show entries" was set to a value other than "All" and I
searched for a value that exists within the item set but was not showing on the
current page
- inconsistently got the error if "Show entries" was set to a value other than "All"
and I searched for a value that exists within the items showing on the current page
- the above behavior would occur even if all of the items were showing (e.g. bib has
25 items and the table is set to show 50)
- never got the error if "Show entries" was set to "All", no matter what I searched
2. If I typed in a value and pressed the "Enter" key, in a case that would generate the above 500 error, it generated two errors.
First: “Something went wrong when loading the table: undefined : undefined”
Then: “Something went wrong when loading the table: 500 Internal Server Error”
(including this mostly in case it helps with troubleshooting #1)
3. If StaffDetailItemSelection and/or LocalCoverImages is turned off, the alignment of the drop-down filters for the other columns is offset by 1 for each of the two sysprefs that is disabled
Example: StaffDetailItemSelection is enabled; LocalCoverImages is disabled.
The first 5 columns are the selection column, Item Type, Current Library, Home Library, Collection.
Item Type has a text filter instead of a drop-down.
Current Library has a drop-down, but it contains Item Type values (and is thus useless).
Home Library has a drop-down containing branch names (but this is presumably still shifted from Current Library)
Collection has a drop-down containing branch names (again, useless)
** If the cover images column is hidden, but not disabled by syspref, the offset does not occur.
4. The general "Search" bar at the top of the table can only search on AV codes, not display names (is this intentionally left for follow-up since the column-specific filters were fixed by adding drop-downs?)
A couple of other minor things that do not need to be blockers:
1. The "Clear filter" button (next to the general "Search" box at the top of the table) is always disabled
2. Missing space after the transit date: “In transit from Springfield to Fairfield since 01/25/2024There is an item level hold on this item (priority = 0).”
**Note: I didn't test course reserves, analytics, or host records, as I'm not familiar with how those work generally in Koha.
Feedback from the MCPL staff who looked at the patch: Inability to sort/filter on status: Front-line staff don't often do this, so it's not a big deal for them. However, acquisitions staff do need to sort/filter by item status on bib records pretty frequently. It would be great if that could be prioritized as a follow-up, even if it's a filter that's more hard-coded than we'd like with a FIXME for later. Hiding columns based on whether the first page of results returns data: This doesn't come up often for us (most columns in our system either always have data or very rarely have data), though I see how this could potentially be an issue for other libraries. Finally, the reload time when changing the sort/filter was also a concern - if you do multiple sorts/filters on smaller bibs, this can actually increase the overall loading time. The potential follow-up that was discussed earlier, to block the Ajax reload when all items are showing, could definitely help with this, especially on smaller bibs. Hi Emily, thanks for the valueable testing and feedback! I believe, as you already did in your report, we need to differentiate between what needs fixing here and what needs to be fixed later, but at least the internal server errors deserve to be looked at. (In reply to Emily Lamancusa from comment #66) Thanks a lot! > Notes from my testing: > The majority of it seems to be working as it's supposed to. I ran into a few > problems with filtering, though: > > 1. When using the general "Search" field at the top of the table, the filter > often failed with the error message: “Something went wrong when loading the > table: 500 Internal Server Error”. In this case, the displayed result set > remained unchanged. This is driving me crazy and I am totally stuck. I have pushed some info to a separate branch bug_33568_wip https://gitlab.com/joubu/Koha/-/commit/c7969388f1a59ad83a2bf82df1d0abf9a8c065b3 > 3. If StaffDetailItemSelection and/or LocalCoverImages is turned off, the > alignment of the drop-down filters for the other columns is offset by 1 for > each of the two sysprefs that is disabled Good catch. Fixed in "Fix columns shift when pref are off". > 4. The general "Search" bar at the top of the table can only search on AV > codes, not display names (is this intentionally left for follow-up since the > column-specific filters were fixed by adding drop-downs?) Yes, that's a limitation of our DT/REST API for now. We need to deal with that but that's not for here. > A couple of other minor things that do not need to be blockers: > 1. The "Clear filter" button (next to the general "Search" box at the top of > the table) is always disabled Weird, I am sure I fixed that already. This is not related directly to this table (see the patron search for instance). It should be reported separately. > 2. Missing space after the transit date: “In transit from Springfield to > Fairfield since 01/25/2024There is an item level hold on this item (priority > = 0).” Done (fixed inline) (In reply to Emily Lamancusa from comment #67) > Feedback from the MCPL staff who looked at the patch: > > Inability to sort/filter on status: Front-line staff don't often do this, so > it's not a big deal for them. However, acquisitions staff do need to > sort/filter by item status on bib records pretty frequently. It would be > great if that could be prioritized as a follow-up, even if it's a filter > that's more hard-coded than we'd like with a FIXME for later. It can be done, but it is not trivial. We need to move the logic to perl. > Hiding columns based on whether the first page of results returns data: This > doesn't come up often for us (most columns in our system either always have > data or very rarely have data), though I see how this could potentially be > an issue for other libraries. As state in a previous comment this is impossible to resolve. > Finally, the reload time when changing the sort/filter was also a concern - > if you do multiple sorts/filters on smaller bibs, this can actually increase > the overall loading time. The potential follow-up that was discussed > earlier, to block the Ajax reload when all items are showing, could > definitely help with this, especially on smaller bibs. To be honest I have no idea how easy/hard it is to implement. But I will be happy to look into it. Don't hesitate to open new bug reports depending on this one, if you have enhancement ideas/requests. (In reply to Jonathan Druart from comment #69) > (In reply to Emily Lamancusa from comment #66) > > Thanks a lot! > > > Notes from my testing: > > The majority of it seems to be working as it's supposed to. I ran into a few > > problems with filtering, though: > > > > 1. When using the general "Search" field at the top of the table, the filter > > often failed with the error message: “Something went wrong when loading the > > table: 500 Internal Server Error”. In this case, the displayed result set > > remained unchanged. > > This is driving me crazy and I am totally stuck. I have pushed some info to > a separate branch bug_33568_wip > https://gitlab.com/joubu/Koha/-/commit/ > c7969388f1a59ad83a2bf82df1d0abf9a8c065b3 "Fixed" on "Bug 33568: Remove LPAD ordering" Remote branch rebased. (In reply to Jonathan Druart from comment #49) > > 4) Table configuration doesn't appear to be used as expected. I checked: > > Catalog > holdings_table has only holdings_issues and holdings_renewals > > hidden by default. But the table keeps appearing with only a minimum number > > of columns and a lot of them hidden. Unfortunately the hidden column also > > includes much neded ones like callnumber. Any changes I make don't seem to > > stick. > > The table is hiding empty columns. So if the x items displayed on the page > do not have callnumber, then the column will be hidden, even if other pages > have items with callnumber. Is that what you are seeing? > See commit message "Bug 33568: Hide empty columns". > > It is what we are doing already on master, but here we don't want to fetch > all the items to know if data exist for each each column... I think we should remove the feature of hiding empty columns entirely. It is a really small 'feature' that causes less harm than the benefits it brings. (In reply to Tomás Cohen Arazi from comment #72) > (In reply to Jonathan Druart from comment #49) > > > 4) Table configuration doesn't appear to be used as expected. I checked: > > > Catalog > holdings_table has only holdings_issues and holdings_renewals > > > hidden by default. But the table keeps appearing with only a minimum number > > > of columns and a lot of them hidden. Unfortunately the hidden column also > > > includes much neded ones like callnumber. Any changes I make don't seem to > > > stick. > > > > The table is hiding empty columns. So if the x items displayed on the page > > do not have callnumber, then the column will be hidden, even if other pages > > have items with callnumber. Is that what you are seeing? > > See commit message "Bug 33568: Hide empty columns". > > > > It is what we are doing already on master, but here we don't want to fetch > > all the items to know if data exist for each each column... > > I think we should remove the feature of hiding empty columns entirely. It is > a really small 'feature' that causes less harm than the benefits it brings. I see what you mean, but we need to test this maybe - it can "blow up" the table quite a bit. thinking of the notes fields (materials, internal note, OPAC note, enumchron...) that you would want to see when set and not hide via column settings by default. For libraries with only a few items per record, this could be seen as quite a regression. Would it be possible to only "activate" this behavior if we have a paged items table? Like check the count of items and then behave differently? (just brainstorming, we need to decide on something, I am aware) (In reply to Katrin Fischer from comment #73) > I see what you mean, but we need to test this maybe - it can "blow up" the > table quite a bit. thinking of the notes fields (materials, internal note, > OPAC note, enumchron...) that you would want to see when set and not hide > via column settings by default. > > For libraries with only a few items per record, this could be seen as quite > a regression. Would it be possible to only "activate" this behavior if we > have a paged items table? Like check the count of items and then behave > differently? (just brainstorming, we need to decide on something, I am aware) I've been thinking about it, and maybe what we need is some sort of hint/tooltip that displays some of that info? So they can safely hide the columns entirely, but still know there's some extra info. Trying to see from another angle to unlock this. I get it. I understand your concern, but it is too tiny compared to the benefits having this in master will bring. (In reply to Tomás Cohen Arazi from comment #74) > (In reply to Katrin Fischer from comment #73) > > I see what you mean, but we need to test this maybe - it can "blow up" the > > table quite a bit. thinking of the notes fields (materials, internal note, > > OPAC note, enumchron...) that you would want to see when set and not hide > > via column settings by default. > > > > For libraries with only a few items per record, this could be seen as quite > > a regression. Would it be possible to only "activate" this behavior if we > > have a paged items table? Like check the count of items and then behave > > differently? (just brainstorming, we need to decide on something, I am aware) > I've been thinking about it, and maybe what we need is some sort of > hint/tooltip that displays some of that info? So they can safely hide the > columns entirely, but still know there's some extra info. Trying to see from > another angle to unlock this. > > I get it. I understand your concern, but it is too tiny compared to the > benefits having this in master will bring. I think it's important we keep both use cases in mind. Big libraries with many items that could immensely profit from this, but also the huge number of small libraries where this might create problems. What about the suggestion I made about making the behavior dependent on number of items? I believe that could give us best of both worlds. * Show all columns if the display is paginated, like you suggested. * Keep the behavior Jonathan implemented for records with unpaginated display (number of items <= number of items per page). (In reply to Katrin Fischer from comment #75) > > I think it's important we keep both use cases in mind. Big libraries with > many items that could immensely profit from this, but also the huge number > of small libraries where this might create problems. My proposal is the way I keep both cases in mind :-D I just think some sacrifices will need to be made at the beginning and maybe we can later re-add things as people express their opinions. That said, > What about the suggestion I made about making the behavior dependent on > number of items? I believe that could give us best of both worlds. > > * Show all columns if the display is paginated, like you suggested. > * Keep the behavior Jonathan implemented for records with unpaginated > display (number of items <= number of items per page). this could work. It would be a matter of hearing from Jonathan as he's invested a lot of time and needs to tell us if he can adjust that needed bit. (In reply to Tomás Cohen Arazi from comment #72) > I think we should remove the feature of hiding empty columns entirely. It is > a really small 'feature' that causes less harm than the benefits it brings. Hm, maybe we should take a step back: Maybe Jonathan's solution is already good and we need a tooltip on the column configuration page instead? My feeling is of the 3 proposed options, "always showing all columns" is the least ideal for now. Besides the conversation about columns visibility, I found this odd behavior:
I picked `CGI programming with Perl` for my tests, and added some extra items:
```
perl -mt::lib::TestBuilder -e 'my $t = t::lib::TestBuilder->new; foreach my $i (1..100) { $t->build_sample_item({ biblionumber => 11 }); }'
```
And even though TestBuilder generated 100 random libraries for those items (checked on the DB) all the 'Home library' and 'Holding library' columns are displaying empty!
I'm reverting the FQA as I cannot reproduce anymore. Marked my comment as important as Jonathan might have some insight on what might have happened (I feel some caching got in the middle). (In reply to Tomás Cohen Arazi from comment #79) > I'm reverting the FQA as I cannot reproduce anymore. Marked my comment as > important as Jonathan might have some insight on what might have happened (I > feel some caching got in the middle). Right, t::lib::TestBuilder totally skips Koha::<Class>->store, and thus cache invalidation doesn't take place. Sorry for the noise! This is PQA in my opinion. Will wait until we hear from Jonathan about the columns display solution. There are really too many columns to display them all. I think the solution proposed by this patch is a good compromise and we need to hear from end users to adjust the behaviour if needed. (In reply to Jonathan Druart from comment #81) > There are really too many columns to display them all. I think the solution > proposed by this patch is a good compromise and we need to hear from end > users to adjust the behaviour if needed. I am OK with that. IIRC it would mean that not much changes for libraries with few items, which is good for now. What do you think about adding a little tool tip to the relevant section on the columns configuration page? Maybe something like: "Please note that if a field is not used on the first page of items int he holdings table, it will be automatically hidden from display and can be made visible on demand. You can still use the column configuration to permanently hide columns from the holdings table." Trying to minimize the surprise and manage expectations :) (In reply to Katrin Fischer from comment #82) > (In reply to Jonathan Druart from comment #81) > > There are really too many columns to display them all. I think the solution > > proposed by this patch is a good compromise and we need to hear from end > > users to adjust the behaviour if needed. > > I am OK with that. IIRC it would mean that not much changes for libraries > with few items, which is good for now. What do you think about adding a > little tool tip to the relevant section on the columns configuration page? > Maybe something like: > > "Please note that if a field is not used on the first page of items int he > holdings table, it will be automatically hidden from display and can be made > visible on demand. You can still use the column configuration to permanently > hide columns from the holdings table." > > Trying to minimize the surprise and manage expectations :) Added, see commit "Add hint on the table settings admin page". Cool. Tomas, ready to pass QA or do we miss something else? Created attachment 164308 [details] [review] Bug 33568: Remove debug_display This is useless, we could move it to its own bug report if needed. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164309 [details] [review] Bug 33568: Starting up Copying the same thead and making a new table using the REST API endpoint Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164310 [details] [review] Bug 33568: Display string version of coded values + format dates Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164311 [details] [review] Bug 33568: can_be_edited Not sure if this is ideal but at least the algorithm complexity is way better than before! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164312 [details] [review] Bug 33568: Display item type image Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164313 [details] [review] Bug 33568: Restore class names on th's Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164314 [details] [review] Bug 33568: Cover images Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164315 [details] [review] Bug 33568: Hide empty columns Instead of relying on the existence of data in at least one item we are going to hide empty columns. That could be confusing: it does not mean items on another page do not have data for this column. The user can still display the column using the "Columns" visibility button, and filter on the column. I think this is an acceptable behaviour. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164316 [details] [review] Bug 33568: Temporary set searchable to false where no data Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164317 [details] [review] Bug 33568: fix DataTables bKohaColumnsUseNames Looks like this bug does not exist in Koha, but we hit it here. Could be moved to its own bug. Compare with ../includes/columns_settings.inc (code that is used prior to this change) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164318 [details] [review] Bug 33568: Add columns visibility Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164319 [details] [review] Bug 33568: Remove visibility conditions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164320 [details] [review] Bug 33568: Fix search on item type Is this good? Cannot we return incorrect results here? Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164321 [details] [review] Bug 33568: Shelving location Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164322 [details] [review] Bug 33568: Item groups FIXME - we need tests, but I'd like to make sure there is no an easier way to do this many to many things. Tomas? Filter on the "Item group" column needs to be tested! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164323 [details] [review] Bug 33568: enumchron/volinfo Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164324 [details] [review] Bug 33568: Status FIXME We need tests here! See the FIXME in the code, we are losing the ability to search on the status, but this is really impossible to implement if we are not storing the status (DB or search engine). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164325 [details] [review] Bug 33568: Add link to moredetail Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164326 [details] [review] Bug 33568: URI Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164327 [details] [review] Bug 33568: Display stringified version of copy_number FIXME - should we apply the same test for other row._strings occurrences, in case they are not linked with an AV cat? Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164328 [details] [review] Bug 33568: public notes Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164329 [details] [review] Bug 33568: Host title See FIXMEs! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164330 [details] [review] Bug 33568: Print label button Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164331 [details] [review] Bug 33568: Restore action buttons Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164332 [details] [review] Bug 33568: Separate holdings This needs to be well tested! Sysprefs SeparateHoldings and SeparateHoldingsBranch As well as the "Lost items in staff interface" that can be defined at patron's category level. This patch is simply restoring the previous behaviour but I think we could do better. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164333 [details] [review] Bug 33568: Course reserves FIXME We need tests here FIXME/TODO - Isn't it possible to replace all this mess with the following Koha::Item method and a new DBIC rs? sub courses { my ($self) = @_; return Koha::Courses->search( { 'ci.itemnumber' => $self->itemnumber }, { join => { course_reserves => 'ci' } } ); } Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164334 [details] [review] Bug 33568: Escape all the things Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164335 [details] [review] Bug 33568: Restore filters FIXME - If we click 'show filters' the 'columns' list does not contain all the columns. We are destroying the DT and the hidden columns are removed from the dom. I think there are 2 possible solutions here: 1. don't destroy but adjust the 'dom' parameter of DT (I didn't manage to make this work correctly, because of the th filters) 2. Move the th definition in the JS init of DT Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164336 [details] [review] Bug 33568: Fix action links display Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164337 [details] [review] Bug 33568: Preserve item selection across pagination and filtering BTW Do we really need StaffDetailItemSelection? Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164338 [details] [review] Bug 33568: Use as_due_date to display due dates Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164339 [details] [review] Bug 33568: Display library names instead of codes for transfers Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164340 [details] [review] Bug 33568: Bug 33568: Display library names instead of codes for first hold Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164341 [details] [review] Bug 33568: Bug 33568: Display library names instead of codes for recall Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164342 [details] [review] Bug 33568: Display the desk name for the first hold Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164343 [details] [review] Bug 33568: Restore bundle items Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164344 [details] [review] Bug 33568: Restore host records Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164345 [details] [review] Bug 33568: Restore analytics FIXME We cannot have the usual _count/+count pattern for the REST API, I think it's acceptable FIXME We lost the sort on this column, not a big deal IMO Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164346 [details] [review] Bug 33568: Move code to a dedicated include file Not very nice but at least the code is isolated in a separate file and reduce the size of detail.tt We could also move the code related to the item bundle that is included inside this same table. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164347 [details] [review] Bug 33568: Fix DT dom and remove DT info if not needed Was there a real need to deal with a specific dom? The "no filter" view was not really light, only the column filters were shown/hidden. This patch is suggesting to remove all the DT info in case the table is displaying all the items of the bibliographic record: Pagination, filtering, buttons, etc. This patch is for discussion. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164348 [details] [review] Bug 33568: Fix select/clear all links Follow-up for "Bug 33568: Preserve item selection across pagination and filtering" FIXME We should deal with the items selection in a better way, but I would not consider it blocker, that can be done later. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164349 [details] [review] Bug 33568: Add search on home and holding library's names QUESTION - Do we prefer this or a dropdown list in the column filter header? Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164350 [details] [review] Bug 33568: Prevent 500 when filtering by item type (FIXMEs) See FIXMEs added by this patch Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164351 [details] [review] Bug 33568: (follow-up) Restore filters This is a bit ugly but fixes the problem raised in the previous commit, as well as others for instance comment 18 "* Click 'show filters', 'hide filters' and repeat X 2 - status column disappears?" Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164352 [details] [review] Bug 33568: Fix local cover images We need to reprocess the images when the table is refreshed, to do that we teach verify_cover_images to go through a given container instead of the whole document, and pass a call back function to build_items_table that will be called when DataTables will have redrawn the table (drawCallback). Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164353 [details] [review] Bug 33568: Fix 'actions' links when filters are shown/hidden It also fixes a problem when "Select all" was clicked, the checkbox selection was not kept when the table was refreshed (was comparing string and integer, parseInt needed) Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164354 [details] [review] Bug 33568: (bug 34226 follow-up) Add type=date to date columns Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164355 [details] [review] Bug 33568: Do not embed library names Performance for 1000 items from 30s to 22s It replaces the input in the column header filter with a dropdown list. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164356 [details] [review] Bug 33568: Do not embed item types Performance for 1000 items from 22 to 19s It replaces the input in the column header filter with a dropdown list. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164358 [details] [review] Bug 33568: follow-up for 29523 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164359 [details] [review] Bug 33568: Add some missing StaffDetailItemSelection conditionals Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164360 [details] [review] Bug 33568: Hide holdings tabs if empty (SeparateHoldings) See comment 34. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164361 [details] [review] Bug 33568: JSON-encode html output of build_table This prevents error with quotes coming from translations Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164362 [details] [review] Bug 33568: Fix ordering Use search_ordered for default ordering It also fixes a crash when StaffDetailItemSelection is turned off. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164363 [details] [review] Bug 33568: Do not display 'null' when no barcode Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164364 [details] [review] Bug 33568: Hide 'Host records' if EasyAnalyticalRecords is off Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164365 [details] [review] Bug 33568: Fix popup behaviour for SpineLabelShowPrintOnBibDetails Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164366 [details] [review] Bug 33568: Restore link to patron Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164367 [details] [review] Bug 33568: Restore column filters when StaffDetailItemSelection is off Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164368 [details] [review] Bug 33568: Fix patron_to_html display Take HidePatronName into account Display cardnumber Remove parenthesis around cardnumber if the name is hidden Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164369 [details] [review] Bug 33568: (follow-up) Fix ordering - WIP This is not working. We are still getting the error Unable to programatically derive a required group_by from the supplied order_by criteria. To proceed either add an explicit group_by, or simplify your order_by to only include plain columns (supplied order_by: 'lpad_copynumber'). To recreate the problem you can go to detail.pl and search for something Without search the query is working. The problem appears when the query has WHERE parameters added and "GROUP BY" is added. DBIC is adding to the GROUP BY what is added to the select, but does not handle this situation (not a plain column). You can play with: 1. Remove from Koha::Items->search_ordered 475 'lpad_copynumber', => all good 2. Remove the following block from /usr/share/perl5/DBIx/Class/Storage/DBIHacks.pm 584 # only consider real columns (for functions the user got to do an explicit group_by) 585 my $chunk_ci; 586 if ( 587 @{$order_by[$o_idx]} != 1 588 or 589 # only declare an unknown *plain* identifier as "leftover" if we are called with 590 # aliastypes to examine. If there are none - we are still in _resolve_attrs, and 591 # can just assume the user knows what they want 592 ( ! ( $chunk_ci = $colinfos->{$order_by[$o_idx][0]} ) and $attrs->{_aliastypes} ) 593 ) { 594 push @$leftovers, $order_by[$o_idx][0]; 595 } => all good I don't know if we are doing something wrong here, but I have no idea how we can build the group_by explicitely. Is there another way to do this? Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164370 [details] [review] Bug 33568: Fix columns shift when pref are off From comment 66: If StaffDetailItemSelection and/or LocalCoverImages is turned off, the alignment of the drop-down filters for the other columns is offset by 1 for each of the two sysprefs that is disabled Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164371 [details] [review] Bug 33568: Remove LPAD ordering This is coming from bug 3521 it seems. And we are going to deal with this later if people complains. kidclamp> the performance issue for sites with many copies is bigger than sorting issues imho kidclamp> drop the LPAD I say :-) Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164372 [details] [review] Bug 33568: serialitem => serial_item Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164373 [details] [review] Bug 33568: POD fixes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Created attachment 164374 [details] [review] Bug 33568: Add hint on the table settings admin page Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Thanks Tomas! I have added Laurence and Emily as well as yours. Laurence and Emily have done extended testing and I think they deserve their signed-off-by lines on this one. Thanks everyone! (In reply to Jonathan Druart from comment #151) > Thanks Tomas! > I have added Laurence and Emily as well as yours. > Laurence and Emily have done extended testing and I think they deserve their > signed-off-by lines on this one. Totally agree, thanks for thinking of it! My goal is to push this before leaving for Marseille. Created attachment 164472 [details] [review] Bug 33568: Fix false positive 'missing_filter' It was ok but the QA script complains and xt/find-missing-filters.t as well. I have given this priority as it's quite a big change and it will give us the chance to get a lot of eyes on it next week during the hackfest.
Some notes from my own review:
1) I notice quite a lot of remaining FIXMEs. I feel like some of those should filed as follow-up bugs, like the question of sorting serial issues here:
my $items_rs = $biblio->items;
+ # FIXME We need to order_by serial.publisheddate if we have _order_by=+me.serial_issue_number
+ # FIXME Do we always need host_items => 1 or depending on a flag?
+ # FIXME Should we prefetch => ['issue','branchtransfer']?
+ my $items_rs = $biblio->items( { host_items => 1 } )->search_ordered( {}, { join => 'biblioitem' } );
$items_rs = $items_rs->filter_by_bookable if $bookable_only;
+ # FIXME We need to order_by serial.publisheddate if we have _order_by=+me.serial_issue_number
my $items = $c->objects->search($items_rs);
2) Untranslatables
node += '<a href="/cgi-bin/koha/catalogue/image.pl?itemnumber=%s&imagenumber=%s" title="Local cover image">'.format(id, id);
node += '<img src="/cgi-bin/koha/catalogue/image.pl?thumbnail=1&imagenumber=%s" alt="Local cover image" data-link="/cgi-bin/koha/catalogue/imageviewer.pl?itemnumber=[% item.itemnumber | uri %
return '<a href="/cgi-bin/koha/catalogue/search.pl?idx=hi&q=%s">%s analytics</a>'.format(row.item_id, row.analytics_count);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
return '<a href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s&hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id);
nodes += ' <li><a href="/cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=%s&filetype=image"><i class="fa fa-upload"></i> Upload image</a></li>'.format(row.item_id);
nodes += '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=%s&itemnumber=%s#edititem"><i class="fa-solid fa-pencil"></i> Edit</a>'.format(row.biblio_id, row.item_id);
nodes += '<button class="btn btn-default btn-xs details-control"><i class="fa fa-folder"></i> Manage bundle (%s|%s)</button>'.format(escape_str(row.bundle_items_not_lost_count), escape_str(row.bundle_items_lost_count));
3) Unit tests
Actually you removed some :D We should add some for balance and the new methods in the various .pm.
4) Processing note
The processing note now always shows for a split second when doing an action like sorting columns. Not sure if that is an option, but could we make it only show if the action itself takeslike a little longer? It's just a fast blink now and you can hardly even read the "processing" (definitely for a separate bug of course)
IMPORTANT: When you provide follow-ups - could you please obsolete the already pushed patches? This will make it much easier to handle. (uncommenting 60+ patches for git bz apply with -i is no fun)
Pushed for 24.05! Well done everyone, thank you! Created attachment 164480 [details] [review] Bug 33568: Fix translatability issues (In reply to Katrin Fischer from comment #154) > I have given this priority as it's quite a big change and it will give us > the chance to get a lot of eyes on it next week during the hackfest. > > Some notes from my own review: > > 1) I notice quite a lot of remaining FIXMEs. I feel like some of those > should filed as follow-up bugs, like the question of sorting serial issues > here: > > my $items_rs = $biblio->items; > + # FIXME We need to order_by serial.publisheddate if we have > _order_by=+me.serial_issue_number > + # FIXME Do we always need host_items => 1 or depending on a flag? > + # FIXME Should we prefetch => ['issue','branchtransfer']? > + my $items_rs = $biblio->items( { host_items => 1 } > )->search_ordered( {}, { join => 'biblioitem' } ); > $items_rs = $items_rs->filter_by_bookable if $bookable_only; > + # FIXME We need to order_by serial.publisheddate if we have > _order_by=+me.serial_issue_number > my $items = $c->objects->search($items_rs); Yes, there were a lot of FIXMEs. We were supposed to discuss them. > 2) Untranslatables > > node += '<a > href="/cgi-bin/koha/catalogue/image.pl?itemnumber=%s&imagenumber=%s" > title="Local cover image">'.format(id, id); > node += '<img > src="/cgi-bin/koha/catalogue/image.pl?thumbnail=1&imagenumber=%s" > alt="Local cover image" > data-link="/cgi-bin/koha/catalogue/imageviewer.pl?itemnumber=[% > item.itemnumber | uri % > return '<a href="/cgi-bin/koha/catalogue/search.pl?idx=hi&q=%s">%s > analytics</a>'.format(row.item_id, row.analytics_count); > return '<a > href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s& > hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id); > return '<a > href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s& > hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id); > return '<a > href="/cgi-bin/koha/cataloguing/addbiblio.pl?hostbiblionumber=%s& > hostitemnumber=%s">Create analytics</a>'.format(row.biblio_id, row.item_id); > nodes += ' <li><a > href="/cgi-bin/koha/tools/upload-cover-image.pl?itemnumber=%s& > filetype=image"><i class="fa fa-upload"></i> Upload > image</a></li>'.format(row.item_id); > nodes += '<a class="btn btn-default btn-xs" > href="/cgi-bin/koha/cataloguing/additem. > pl?op=edititem&biblionumber=%s&itemnumber=%s#edititem"><i class="fa-solid > fa-pencil"></i> Edit</a>'.format(row.biblio_id, row.item_id); > nodes += '<button class="btn btn-default btn-xs details-control"><i > class="fa fa-folder"></i> Manage bundle > (%s|%s)</button>'.format(escape_str(row.bundle_items_not_lost_count), > escape_str(row.bundle_items_lost_count)); See last patch. > 3) Unit tests > > Actually you removed some :D We should add some for balance and the new > methods in the various .pm. Will do on bug 36535. > 4) Processing note > > The processing note now always shows for a split second when doing an action > like sorting columns. Not sure if that is an option, but could we make it > only show if the action itself takeslike a little longer? It's just a fast > blink now and you can hardly even read the "processing" (definitely for a > separate bug of course) This is not related to this patch, if we want it that needs to be done for all DT. Created attachment 164482 [details] [review] Bug 33568: Prevent api/v1/biblios.t to fail randomly koha_1 | # Failed test 'The items are returned' koha_1 | # at t/db_dependent/api/v1/biblios.t line 196. koha_1 | # Structures begin differing at: koha_1 | # $got->[0]{external_id} = 'th151O2JtgfGdVW6aiF' koha_1 | # $expected->[0]{external_id} = 'uxrMzUXrkHQy7MHOSfjK' koha_1 | # Looks like you failed 1 test of 11. koha_1 | koha_1 | # Failed test 'get_items() tests' koha_1 | # at t/db_dependent/api/v1/biblios.t line 206. Created attachment 164483 [details] [review] Bug 33568: (follow-up) Add search on home and holding library's names Silly typo! Caught by a test koha_1 | # Failed test 'Home branch code matches holdingbranch' koha_1 | # at t/db_dependent/Items.t line 581. koha_1 | # got: 'QHImNSEoNi' koha_1 | # expected: 'VY5y6p' koha_1 | # Looks like you failed 1 test of 7. koha_1 | koha_1 | # Failed test 'Koha::Item(s) tests' koha_1 | # at t/db_dependent/Items.t line 588. Created attachment 164485 [details] [review] Bug 33568: Prevent api/v1/biblios.t to fail randomly koha_1 | # Failed test 'The items are returned' koha_1 | # at t/db_dependent/api/v1/biblios.t line 196. koha_1 | # Structures begin differing at: koha_1 | # $got->[0]{external_id} = 'th151O2JtgfGdVW6aiF' koha_1 | # $expected->[0]{external_id} = 'uxrMzUXrkHQy7MHOSfjK' koha_1 | # Looks like you failed 1 test of 11. koha_1 | koha_1 | # Failed test 'get_items() tests' koha_1 | # at t/db_dependent/api/v1/biblios.t line 206. Created attachment 164528 [details] [review] Bug 33568: (follow-up) Fix duplicated key in item spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Follow-up was pushed to main. https://git.koha-community.org/Koha-community/Koha/commits/branch/main/search?q=33568 *** Bug 33930 has been marked as a duplicate of this bug. *** Whaou big change, great :D Will this enhancement rectify the limitations on how many items can be added to a bib record? (In reply to Christopher Brannon from comment #165) > Will this enhancement rectify the limitations on how many items can be added > to a bib record? It depends on which limitation you mean? Are you referring to a specific bug? This will fix a performance issue mostly, it would work with many items, but this will make it work much better. The MARCXML we use doesn't really limit the number of items. There can be indexing issues with Zebra and many items, but they could be resolved in configuration (upping a size limit). Not aware of indexing issues with Elasticsearch and record size so far. There are probably other spots where it will take a while to load records with many items. We had an issue where we could not put all our archived newspapers on the same record, but that was back in the day when we were on Zebra. Now that we are on ES, I'll have to see if it is still an issue. It would break the search for that item or cause the item to not load fully. We'll see. Obviously no backport to 23.11.x *** Bug 26802 has been marked as a duplicate of this bug. *** Not sure if I am commenting in the correct place. I first wanted to thank you for this development. The loading of bibs with many items is gloriously more speedy. I did want to add some feedback that when we try to edit the same bibs, we are now getting time outs and "Page Unresponsive" messages. We are almost unable to edit items, but can if we are very patient with click, aaaaand wait, type a character, and wait. If there is another bug to comment on let me know. (In reply to Dani Elder from comment #170) > Not sure if I am commenting in the correct place. I first wanted to thank > you for this development. The loading of bibs with many items is gloriously > more speedy. > > I did want to add some feedback that when we try to edit the same bibs, we > are now getting time outs and "Page Unresponsive" messages. We are almost > unable to edit items, but can if we are very patient with click, aaaaand > wait, type a character, and wait. > > If there is another bug to comment on let me know. Dani, can you give more context? I have a record with 403 items, and when I click to edit the bib or the items, both load almost instantly. So, I am not seeing what you have described. How many items are in your bib? Are you just going to the top of the record and clicking edit --> edit record, or are you going in some other way? Are you using the regular editor or the advanced editor? I'm not sure how the bib editing would be affected, since it is not loading the items associated with it. It might be something unrelated. Sorry, I meant item editing. Possibly unrelated? Have I mentioned we are a law library and probably stretching the bounds of what is possible to expect with speedy loading/editing? Some slowness is expected. We have, several hundred items attached, sometimes in the thousands on some bibs (after being split). When we go to edit we go from the bib detail page to > Edit items. We are just trying to add new barcodes and such. The display loads instantly and that is a thing of beauty. We now can't filter or search at the moment on location (maybe another ticket) and when we try to edit or add an items is when the slowness hits. We haven't got that "page unresponsive" error before. (In reply to Dani Elder from comment #172) > Sorry, I meant item editing. Possibly unrelated? > > Have I mentioned we are a law library and probably stretching the bounds of > what is possible to expect with speedy loading/editing? Some slowness is > expected. We have, several hundred items attached, sometimes in the > thousands on some bibs (after being split). > > When we go to edit we go from the bib detail page to > Edit items. We are > just trying to add new barcodes and such. The display loads instantly and > that is a thing of beauty. We now can't filter or search at the moment on > location (maybe another ticket) and when we try to edit or add an items is > when the slowness hits. We haven't got that "page unresponsive" error before. Hi Dani, thanks for clarifying! Item editing should not have been affected by this patch, but it could be another change that did. Out of interest: do you usually use the edit links in the item table on the detail page or do you use the "edit items" from the toolbar and then pick the right item on the editing page? I am running out of time to check, but it might be worth filing a separate bug for that, if we don't have one yet. It could be another area that could benefit from the API work. (In reply to Katrin Fischer from comment #173) > (In reply to Dani Elder from comment #172) > > Sorry, I meant item editing. Possibly unrelated? [...] > > When we go to edit we go from the bib detail page to > Edit items. We are > > just trying to add new barcodes and such. The display loads instantly and > > that is a thing of beauty. We now can't filter or search at the moment on > > location (maybe another ticket) and when we try to edit or add an items is > > when the slowness hits. We haven't got that "page unresponsive" error before. Hi Dani, we just talked about this on Mattermost and a new bug has been filed: Bug 37591 - Moredetail.pl page is opening very slowly Katrin asked: Out of interest: do you usually use the edit links in the item table on the detail page or do you use the "edit items" from the toolbar and then pick the right item on the editing page? Good question, different staff do things differently. We mainly do edit from bib detail page but not always.
> Good question, different staff do things differently. We mainly do edit from
> bib detail page but not always.
Both are on the big detail page, that's why I was asking. It it's the general loading time of the page, it probably doesn't make a difference tho.
|
