Created attachment 96629 [details] [review] Bug 24305: Fix links to batch tools when report's columns do not contain integers If report's columns do not only contain integers, we should not point to batch tools. For instance, if the value is a link, the batch tools will not work as they will not retrieve the id (itemnnumber or biblionumber) properly Test plan: Create a report like: SELECT CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM items LIMIT 10; Execute it => There is not link to the batch tools Edit the report like: SELECT itemnumber FROM items LIMIT 10; Execute it => There is a link to the batch tools Edit the report like SELECT itemnumber, CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM items LIMIT 10; Execute it => There is link to the batch tools Try other combinations with biblionumber. I tested this and noticed something odd. The examples given in the test plan worked as expected:
Results from reports using only the CONCAT statement did not produce a link to to the batch tools.
Reports that did not use the CONCAT statement at all did produce a link to the batch tools.
Reports that had the CONCAT statement but also had results that were not concatenated did produce a link to the batch tools, but only for the results that were not concatenated. e.g
SELECT itemnumber, CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber
FROM items
LIMIT 10;
produced a link to the batch tools for the first instance of itemnumber.
However, I tried to create a report that used the CONCAT statement for both the biblionumber and the itemnumber and the results included a button to link to the batch tools, however it did not have a dropdown menu to select which tool to go to.
SELECT CONCAT('<a href=\"/cgi-bin/koha/catalogue/detail.pl?biblionumber=', biblionumber, '\">', biblionumber, '</a>' ) AS biblionumber, CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber
FROM items
LIMIT 10
I don't think the button should appear at all in this instance and it shouldn't appear if it is not going to link out to anything.
Best,
Holly
Thanks for testing Holly. This patch should fixes the issue you reported! Created attachment 97302 [details] [review] Bug 24305: Remove previous declaration of batch_biblionumbers We should not init batch_biblionumbers before. Also this patch deals with batch_cardnumbers. Created attachment 97829 [details] [review] Bug 24305: Remove previous declaration of batch_biblionumbers We should not init batch_biblionumbers before. Also this patch deals with batch_cardnumbers. Signed-off-by: Holly Cooper <hc@interleaf.ie> Marcel, they apply correctly for me, please try again. (In reply to Jonathan Druart from comment #6) > Marcel, they apply correctly for me, please try again. And yes, they apply! Not sure what happened here. Looking here now for QA Figured out how you should use the square brackets in a SQL query in Reporting. Well documented?
You can do:
SELECT [[items.biblionumber|bieb]]
This becomes SELECT items.biblionumber AS bieb
Not sure why we need it if using "AS" is just as easy but anyway.
Adding a few comment lines in the code with a follow-up.
Using this construct does not work with the == 'biblionumber' test. Adding one extra line in get_prepped_report for that as well. This is consistent with the headers filled by $sth->{NAME}.
Created attachment 97866 [details] [review] Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]] When you would use the construct, you wont have batch features unless we strip the table name. This is consistent with the $sth->{NAME} used to fill the headers by default. Test plan: Use [[items.biblionumber|bibno]] instead of items.biblionumber in a reporting query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 97867 [details] [review] Bug 24305: Fix links to batch tools when report's columns do not contain integers If report's columns do not only contain integers, we should not point to batch tools. For instance, if the value is a link, the batch tools will not work as they will not retrieve the id (itemnnumber or biblionumber) properly Test plan: Create a report like: SELECT CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM items LIMIT 10; Execute it => There is not link to the batch tools Edit the report like: SELECT itemnumber FROM items LIMIT 10; Execute it => There is a link to the batch tools Edit the report like SELECT itemnumber, CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM items LIMIT 10; Execute it => There is link to the batch tools Try other combinations with biblionumber. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 97868 [details] [review] Bug 24305: Remove previous declaration of batch_biblionumbers We should not init batch_biblionumbers before. Also this patch deals with batch_cardnumbers. Signed-off-by: Holly Cooper <hc@interleaf.ie> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 97869 [details] [review] Bug 24305: (follow-up) Add small comment for square brackets in get_prepped_report Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 97870 [details] [review] Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]] When you would use the construct, you wont have batch features unless we strip the table name. This is consistent with the $sth->{NAME} used to fill the headers by default. Test plan: Use [[items.biblionumber|bibno]] instead of items.biblionumber in a reporting query. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Still one question remaining. No blocker for me.
+ <textarea style="display:none" name="cardnumberlist" id="cardnumberlist">[% FOREACH result IN results %][% FOREACH cells IN result.cells %][% place = loop.index %][% IF header_row.$place.cell == 'cardnumber' || header_types.item(header_row.$place.cell) == 'cardnumber' && cells.cell.match('^(\d+)$') %][% SET batch_cardnumbers = 1 %]
Why do you check on only digits for cardnumber here? The field cardnumber is a varchar ?
that's wrong then :) Nice work everyone! Pushed to master for 20.05 (In reply to Marcel de Rooy from comment #14) > Still one question remaining. No blocker for me. > > + <textarea style="display:none" name="cardnumberlist" > id="cardnumberlist">[% FOREACH result IN results %][% FOREACH cells IN > result.cells %][% place = loop.index %][% IF header_row.$place.cell == > 'cardnumber' || header_types.item(header_row.$place.cell) == 'cardnumber' && > cells.cell.match('^(\d+)$') %][% SET batch_cardnumbers = 1 %] > > Why do you check on only digits for cardnumber here? The field cardnumber is > a varchar ? So... should we not have removed it? I think almost all our libraries would fail the cardnumber is numeric test... Created attachment 98057 [details] [review] Bug 24305: (follow-up) Remove previous declaration of batch_biblionumbers Revert change for cardnumbers, they are not always numbers. We may need to add a regex to remove special chars however, will be done on its own bug report if needed. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Followup pushed, thanks Jonathan for beating me to this. Backported to 19.11.x branch for 19.11.03 It appears like this does not work in 20.05, 21.05, or master (In reply to Lucas Gass from comment #22) > It appears like this does not work in 20.05, 21.05, or master What exactly? I've tried on master and it seems to work for me. (In reply to Jonathan Druart from comment #23) > (In reply to Lucas Gass from comment #22) > > It appears like this does not work in 20.05, 21.05, or master > > What exactly? I've tried on master and it seems to work for me. SELECT itemnumber, CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM items LIMIT 10; |
If you have a report with a CONCAT statement in it, and use that report to pass the item numbers to the batch item modification, the batch modification does not work. Title list does not show, just the edit item screen, and if you proceed, nothing is modified. To create, use a report such as: SELECT items.biblionumber,items.itemcallnumber,items.itype,biblio.author,biblio.title, biblio.copyrightdate, items.barcode, items.location,CONCAT('<a href=\"/cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=', biblionumber, '&itemnumber=', itemnumber, '#edititem', '\">', itemnumber, '</a>' ) AS itemnumber FROM biblio JOIN items USING (biblionumber) WHERE (items.location IS NULL OR items.location = '') ORDER by items.itemcallnumber ASC and see that the batch modification is blank. Remove the concat statement, and the batch item modification works as expected.