Bug 17556

Summary: Move GetHideLostItemsPreference to Koha::Patron
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: gveranis, josef.moravec, katrin.fischer, kyle, liz, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19870
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17555    
Bug Blocks: 16846, 17764    
Attachments: Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference
[SIGNED-OFF] Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference

Description Jonathan Druart 2016-11-04 15:24:39 UTC
The subroutine C4::Members::GetHideLostItemsPreference can easily be replaced with Koha::Patron->find(42)->category->hidelostitems
Comment 1 Jonathan Druart 2016-11-04 15:33:12 UTC
Created attachment 57215 [details] [review]
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference

The subroutine C4::Members::GetHideLostItemsPreference can easily be
replaced with Koha::Patron->find(42)->category->hidelostitems

Test plan:
Create 2 patron categories, 1 with "Lost items in staff client" set to
"shown" and another one to "Hidden by default"
Create 2 patrons using them
On the result search page, the detail page of a record, the item list
page and the page to place a hold, make sure the lost items are
shown/hidden as expected
Comment 2 Josef Moravec 2016-11-04 19:59:21 UTC
In reserve request, if I tried to hold through "Search to hold" functionality, everything works OK, but when I tried just "hold" link (on search result page and detail) i get "Can't call method "category" on an undefined value at /home/vagrant/kohaclone/reserve/request.pl line 417."
Comment 3 Jonathan Druart 2016-11-07 08:55:36 UTC
Created attachment 57246 [details] [review]
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference

The subroutine C4::Members::GetHideLostItemsPreference can easily be
replaced with Koha::Patron->find(42)->category->hidelostitems

Test plan:
Create 2 patron categories, 1 with "Lost items in staff client" set to
"shown" and another one to "Hidden by default"
Create 2 patrons using them
On the result search page, the detail page of a record, the item list
page and the page to place a hold, make sure the lost items are
shown/hidden as expected
Comment 4 Jonathan Druart 2016-11-07 08:56:03 UTC
(In reply to Josef Moravec from comment #2)
> In reserve request, if I tried to hold through "Search to hold"
> functionality, everything works OK, but when I tried just "hold" link (on
> search result page and detail) i get "Can't call method "category" on an
> undefined value at /home/vagrant/kohaclone/reserve/request.pl line 417."

Sorry about that, last patch should fix that.
Comment 5 Josef Moravec 2016-11-09 07:06:10 UTC
Created attachment 57361 [details] [review]
[SIGNED-OFF] Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference

The subroutine C4::Members::GetHideLostItemsPreference can easily be
replaced with Koha::Patron->find(42)->category->hidelostitems

Test plan:
Create 2 patron categories, 1 with "Lost items in staff client" set to
"shown" and another one to "Hidden by default"
Create 2 patrons using them
On the result search page, the detail page of a record, the item list
page and the page to place a hold, make sure the lost items are
shown/hidden as expected

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 6 Marcel de Rooy 2016-12-02 08:03:36 UTC
Please add a test :)
Comment 7 Jonathan Druart 2016-12-02 09:36:35 UTC
(In reply to Marcel de Rooy from comment #6)
> Please add a test :)

I'd like you to reconsider this, the searchResults massive subroutine is not tested and it would be very time consuming to cover this change.

Basically the change is:
- SELECT hidelostitems FROM borrowers,categories WHERE borrowers.categorycode = categories.categorycode AND borrowernumber = ?
+ Koha::Patrons->find($borrowernumber)->category->hidelostitems

Which sounds quite safe :)
Comment 8 Marcel de Rooy 2016-12-02 11:46:58 UTC
Created attachment 57922 [details] [review]
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference

The subroutine C4::Members::GetHideLostItemsPreference can easily be
replaced with Koha::Patron->find(42)->category->hidelostitems

Test plan:
Create 2 patron categories, 1 with "Lost items in staff client" set to
"shown" and another one to "Hidden by default"
Create 2 patrons using them
On the result search page, the detail page of a record, the item list
page and the page to place a hold, make sure the lost items are
shown/hidden as expected

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2016-12-02 11:50:31 UTC
I think that there is still room for improvement in the combination of the preference hidelostitems and the category column.
This could be moved from the scripts into the object.
Comment 10 Kyle M Hall 2016-12-09 18:53:58 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Liz Rea 2016-12-12 22:30:05 UTC
Not that it particularly matters, because you shouldn't do this anyway - this patch breaks searching in the staff client for the database user superlibrarian. 

Only leaving this note in case someone else sees 

Can't call method "category" on an undefined value at /home/liz/koha-src/koha/C4/Search.pm line 2091.

and traces it back to this patch.
Comment 12 Katrin Fischer 2016-12-12 22:33:04 UTC
This won't get backported to 16.11.x as it is an enhancement.