Bug 29802

Summary: biblionumber in OPACHiddenItems breaks opac lists
Product: Koha Reporter: Nick Clemens <nick>
Component: OPACAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, m.de.rooy
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:
22.05.00,21.11.05
Bug Depends on: 24254    
Bug Blocks:    
Attachments: Bug 29802: add 'me' to filter_by_visible_in_opac query
Bug 29802: add 'me' to filter_by_visible_in_opac query
Bug 29802: Unit tests
Bug 29802: add 'me' to filter_by_visible_in_opac query
Bug 29802: Unit tests
Bug 29802: add 'me' to filter_by_visible_in_opac query

Description Nick Clemens 2022-01-05 19:26:43 UTC
When biblionumber is in the opachiddenitems preference the query to find hidden items dies:
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'biblionumber' in where clause is ambiguous [for Statement "SELECT `me`.`itemnumber`, `me`.`biblionumber`, `me`.`biblioitemnumber`, `me`.`barcode`, `me`.`dateaccessioned`, `me`.`booksellerid`, `me`.`homebranch`, `me`.`price`, `me`.`replacementprice`, `me`.`replacementpricedate`, `me`.`datelastborrowed`, `me`.`datelastseen`, `me`.`stack`, `me`.`notforloan`, `me`.`damaged`, `me`.`damaged_on`, `me`.`itemlost`, `me`.`itemlost_on`, `me`.`withdrawn`, `me`.`withdrawn_on`, `me`.`itemcallnumber`, `me`.`coded_location_qualifier`, `me`.`issues`, `me`.`renewals`, `me`.`reserves`, `me`.`restricted`, `me`.`itemnotes`, `me`.`itemnotes_nonpublic`, `me`.`holdingbranch`, `me`.`timestamp`, `me`.`location`, `me`.`permanent_location`, `me`.`onloan`, `me`.`cn_source`, `me`.`cn_sort`, `me`.`ccode`, `me`.`materials`, `me`.`uri`, `me`.`itype`, `me`.`more_subfields_xml`, `me`.`enumchron`, `me`.`copynumber`, `me`.`stocknumber`, `me`.`new_status`, `me`.`exclude_from_local_holds_priority`, `branchtransfers`.`branchtransfer_id`, `branchtransfers`.`itemnumber`, `branchtransfers`.`daterequested`, `branchtransfers`.`datesent`, `branchtransfers`.`frombranch`, `branchtransfers`.`datearrived`, `branchtransfers`.`datecancelled`, `branchtransfers`.`tobranch`, `branchtransfers`.`comments`, `branchtransfers`.`reason`, `branchtransfers`.`cancellation_reason`, `reserves`.`reserve_id`, `reserves`.`borrowernumber`, `reserves`.`reservedate`, `reserves`.`biblionumber`, `reserves`.`branchcode`, `reserves`.`desk_id`, `reserves`.`notificationdate`, `reserves`.`reminderdate`, `reserves`.`cancellationdate`, `reserves`.`cancellation_reason`, `reserves`.`reservenotes`, `reserves`.`priority`, `reserves`.`found`, `reserves`.`timestamp`, `reserves`.`itemnumber`, `reserves`.`waitingdate`, `reserves`.`expirationdate`, `reserves`.`lowestPriority`, `reserves`.`suspend`, `reserves`.`suspend_until`, `reserves`.`itemtype`, `reserves`.`item_level_hold`, `reserves`.`non_priority` FROM `items` `me` LEFT JOIN `branchtransfers` `branchtransfers` ON `branchtransfers`.`itemnumber` = `me`.`itemnumber` LEFT JOIN `reserves` `reserves` ON `reserves`.`itemnumber` = `me`.`itemnumber` WHERE ( ( ( `biblionumber` NOT IN ( ?, ?, ?, ?, ?, ? ) OR `biblionumber` IS NULL ) AND `itemlost` = ? AND ( `itype` NOT IN ( ?, ? ) OR `itype` IS NULL ) AND ( `location` NOT IN ( ?, ? ) OR `location` IS NULL ) AND `me`.`biblionumber` = ? AND 1=1 ) ) ORDER BY `me`.`itemnumber`"
Comment 1 Nick Clemens 2022-01-06 16:18:57 UTC
Created attachment 129101 [details] [review]
Bug 29802: add 'me' to filter_by_visible_in_opac query

This patch prefixes all of the fields in OpacHiddenItems with
"me." before searching.

Unit tests added to cover this case1

To test:
1 - Create a public list
2 - Set OpacHiddenItems to:
    biblionumber: [1]
3 - Attempt to view list in OPAC
4 - Booom
5 - Aply patch
6 - Reload list
7 - Success
Comment 2 Fridolin Somers 2022-02-25 07:15:25 UTC
Indeed, this comes from C4::XSLT::buildKohaItemsNamespace :

$items_rs->search( $query, { prefetch => [ 'branchtransfers', 'reserves' ] } );
Comment 3 Fridolin Somers 2022-02-25 07:24:01 UTC
Created attachment 131099 [details] [review]
Bug 29802: add 'me' to filter_by_visible_in_opac query

This patch prefixes all of the fields in OpacHiddenItems with
"me." before searching.

Unit tests added to cover this case1

To test:
1 - Create a public list
2 - Set OpacHiddenItems to:
    biblionumber: [1]
3 - Attempt to view list in OPAC
4 - Booom
5 - Aply patch
6 - Reload list
7 - Success

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 4 Marcel de Rooy 2022-03-18 08:06:50 UTC
Please show that it breaks in your test. And that your change resolves that.

And tell me what this does:
+    my $mocked_context = Test::MockModule->new('C4::Context');
+    $mocked_context->mock( 'yaml_preference', sub {
+        return { biblionumber => [ $biblio2->biblionumber ] };
+    });

I think it is useless code.
Comment 5 Nick Clemens 2022-03-18 14:28:15 UTC
Created attachment 131911 [details] [review]
Bug 29802: Unit tests

prove -v t/db_dependent/Koha/Items.t
prove -v t/db_dependent/XSLT.t
Comment 6 Nick Clemens 2022-03-18 14:28:18 UTC
Created attachment 131912 [details] [review]
Bug 29802: add 'me' to filter_by_visible_in_opac query

This patch prefixes all of the fields in OpacHiddenItems with
"me." before searching.

Unit tests added to cover this case1

To test:
1 - Create a public list
2 - Set OpacHiddenItems to:
    biblionumber: [1]
3 - Attempt to view list in OPAC
4 - Booom
5 - Aply patch
6 - Reload list
7 - Success

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 7 Nick Clemens 2022-03-18 14:29:26 UTC
(In reply to Marcel de Rooy from comment #4)
> Please show that it breaks in your test. And that your change resolves that.

Tests updated and moved to own patch
> 
> And tell me what this does:
> +    my $mocked_context = Test::MockModule->new('C4::Context');
> +    $mocked_context->mock( 'yaml_preference', sub {
> +        return { biblionumber => [ $biblio2->biblionumber ] };
> +    });
> 
> I think it is useless code.

Looks like you are correct - copied from Items.t - seems to have been done for convenience there, mocking pref directly works
Comment 8 Jonathan Druart 2022-03-21 13:54:52 UTC
Created attachment 131970 [details] [review]
Bug 29802: Unit tests

prove -v t/db_dependent/Koha/Items.t
prove -v t/db_dependent/XSLT.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2022-03-21 13:54:57 UTC
Created attachment 131971 [details] [review]
Bug 29802: add 'me' to filter_by_visible_in_opac query

This patch prefixes all of the fields in OpacHiddenItems with
"me." before searching.

Unit tests added to cover this case1

To test:
1 - Create a public list
2 - Set OpacHiddenItems to:
    biblionumber: [1]
3 - Attempt to view list in OPAC
4 - Booom
5 - Aply patch
6 - Reload list
7 - Success

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2022-03-21 13:57:26 UTC
Sorry Marcel, I didn't you were assigned as QA contact here. Feel free to switch it back to NQA if you want to have another look.
Comment 11 Marcel de Rooy 2022-03-21 14:11:04 UTC
(In reply to Jonathan Druart from comment #10)
> Sorry Marcel, I didn't you were assigned as QA contact here. Feel free to
> switch it back to NQA if you want to have another look.

No reason to :)
Comment 12 Fridolin Somers 2022-03-23 20:30:45 UTC
Whaoo what a complex programing paradox ;)
great job and great unit tests
Comment 13 Fridolin Somers 2022-03-23 20:50:54 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 14 Kyle M Hall 2022-03-25 13:57:53 UTC
Pushed to 21.11.x for 21.11.05