Bug 24305

Summary: Batch Item modification via item number in reports does not work with CONCAT in report
Product: Koha Reporter: Donna <bwsdonna>
Component: CatalogingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: 1joynelson, hc, jonathan.druart, katrin.fischer, lucas, m.de.rooy, martin.renvoize
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03
Bug Depends on: 19233    
Bug Blocks:    
Attachments: Bug 24305: Fix links to batch tools when report's columns do not contain integers
Bug 24305: Remove previous declaration of batch_biblionumbers
Bug 24305: Remove previous declaration of batch_biblionumbers
Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]]
Bug 24305: Fix links to batch tools when report's columns do not contain integers
Bug 24305: Remove previous declaration of batch_biblionumbers
Bug 24305: (follow-up) Add small comment for square brackets in get_prepped_report
Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]]
Bug 24305: (follow-up) Remove previous declaration of batch_biblionumbers

Description Donna 2019-12-23 21:53:15 UTC
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.
Comment 1 Jonathan Druart 2019-12-26 10:17:24 UTC
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.
Comment 2 Holly 2020-01-09 15:49:16 UTC
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
Comment 3 Jonathan Druart 2020-01-13 11:16:52 UTC
Thanks for testing Holly. This patch should fixes the issue you reported!
Comment 4 Jonathan Druart 2020-01-13 11:22:00 UTC
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.
Comment 5 ByWater Sandboxes 2020-01-23 16:27:38 UTC
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>
Comment 6 Jonathan Druart 2020-01-24 08:52:23 UTC
Marcel, they apply correctly for me, please try again.
Comment 7 Marcel de Rooy 2020-01-24 09:10:24 UTC
(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
Comment 8 Marcel de Rooy 2020-01-24 11:26:39 UTC
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}.
Comment 9 Marcel de Rooy 2020-01-24 11:27:23 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2020-01-24 11:27:42 UTC
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>
Comment 11 Marcel de Rooy 2020-01-24 11:27:46 UTC
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>
Comment 12 Marcel de Rooy 2020-01-24 11:27:50 UTC
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>
Comment 13 Marcel de Rooy 2020-01-24 11:27:54 UTC
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>
Comment 14 Marcel de Rooy 2020-01-24 11:29:25 UTC
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 ?
Comment 15 Jonathan Druart 2020-01-24 11:46:24 UTC
that's wrong then :)
Comment 16 Martin Renvoize 2020-01-24 15:21:16 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Katrin Fischer 2020-01-24 22:40:45 UTC
(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?
Comment 18 Katrin Fischer 2020-01-24 22:41:25 UTC
I think almost all our libraries would fail the cardnumber is numeric test...
Comment 19 Jonathan Druart 2020-01-29 10:17:44 UTC
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>
Comment 20 Martin Renvoize 2020-01-29 10:20:36 UTC
Followup pushed, thanks Jonathan for beating me to this.
Comment 21 Joy Nelson 2020-02-10 23:03:26 UTC
Backported to 19.11.x branch for 19.11.03
Comment 22 Lucas Gass 2021-11-19 15:31:58 UTC
It appears like this does not work in 20.05, 21.05, or master
Comment 23 Jonathan Druart 2021-11-22 10:59:40 UTC
(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.
Comment 24 Jonathan Druart 2021-11-22 10:59:57 UTC
(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;