Bug 24305 - Batch Item modification via item number in reports does not work with CONCAT in report
Summary: Batch Item modification via item number in reports does not work with CONCAT ...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 19233
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-23 21:53 UTC by Donna
Modified: 2020-02-10 23:03 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03


Attachments
Bug 24305: Fix links to batch tools when report's columns do not contain integers (5.57 KB, patch)
2019-12-26 10:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24305: Remove previous declaration of batch_biblionumbers (3.88 KB, patch)
2020-01-13 11:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24305: Remove previous declaration of batch_biblionumbers (3.92 KB, patch)
2020-01-23 16:27 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]] (1.48 KB, patch)
2020-01-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24305: Fix links to batch tools when report's columns do not contain integers (5.66 KB, patch)
2020-01-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24305: Remove previous declaration of batch_biblionumbers (4.02 KB, patch)
2020-01-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24305: (follow-up) Add small comment for square brackets in get_prepped_report (1.09 KB, patch)
2020-01-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24305: (QA follow-up) Strip table name in [[table.field|alias]] (1.48 KB, patch)
2020-01-24 11:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24305: (follow-up) Remove previous declaration of batch_biblionumbers (2.04 KB, patch)
2020-01-29 10:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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