Bug 17556 - Move GetHideLostItemsPreference to Koha::Patron
Summary: Move GetHideLostItemsPreference to Koha::Patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 17555
Blocks: 16846 17764
  Show dependency treegraph
 
Reported: 2016-11-04 15:24 UTC by Jonathan Druart
Modified: 2017-12-22 01:34 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference (6.18 KB, patch)
2016-11-04 15:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference (6.20 KB, patch)
2016-11-07 08:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference (6.25 KB, patch)
2016-11-09 07:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17556: Koha::Patrons - Remove GetHideLostItemsPreference (6.33 KB, patch)
2016-12-02 11:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

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