Bug 18403

Summary: Hide patron information if not part of the logged in user library group
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: fiona.borthwick, gmcharlt, jonathan.druart, jonathan.field, jzairo, kyle.m.hall, kyle, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
URL: https://github.com/joubu/Koha/commits/bug_18403
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20214
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21255
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30019
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16735, 17829, 18402, 19456    
Bug Blocks: 25770, 28005, 18789, 20133, 20214, 20323, 20659, 20866, 20899, 20918, 20928, 20934, 20981, 21096, 21137, 21208, 21414, 21904, 21907, 23097, 24018, 25142, 28973, 29580    
Attachments: Bug 18403: Hide patron information if not part of the logged in user library group
Bug 18403: Add new method Koha::Library->library_group
Bug 18403: Add new method Koha::Library::Group->has_child
Bug 18403: Send logged_in_user to template from C4::Auth
Bug 18403: Add new method Koha::Patron->can_see_patron_info
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos
Bug 18403: Adapt patron search
Bug 18403: Only display libraries from group in dropdown lists
Bug 18403: Use patron-title.inc when hidepatronname is used
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]
Bug 18403: Batch patron modification tool
Bug 18403: output_and_exit_if_error for circulation.pl
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons
Bug 18403: Add tests for Koha::Patrons
Bug 18403: Add new method Koha::Patron->can_see_patrons_from
Bug 18403: Patron modification requests
Bug 18403: Patron reviews
Bug 18403: Patron discharges
Bug 18403: Article requests
Bug 18403: Guarantors
Bug 18403: REST API - patrons endpoint
Bug 18403: Deal with the DB user
Bug 18403: Fix patron creation
Bug 18403: Hide patron information if not part of the logged in user library group
Bug 18403: Add new method Koha::Library->library_group
Bug 18403: Add new method Koha::Library::Group->has_child
Bug 18403: Send logged_in_user to template from C4::Auth
Bug 18403: Add new method Koha::Patron->can_see_patron_info
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos
Bug 18403: Adapt patron search
Bug 18403: Only display libraries from group in dropdown lists
Bug 18403: Use patron-title.inc when hidepatronname is used
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]
Bug 18403: Batch patron modification tool
Bug 18403: output_and_exit_if_error for circulation.pl
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons
Bug 18403: Add tests for Koha::Patrons
Bug 18403: Add new method Koha::Patron->can_see_patrons_from
Bug 18403: Patron modification requests
Bug 18403: Patron reviews
Bug 18403: Patron discharges
Bug 18403: Article requests
Bug 18403: Guarantors
Bug 18403: REST API - patrons endpoint
Bug 18403: Deal with the DB user
Bug 18403: Fix patron creation
Bug 18403: Add POD for output_and_exit_if_error
Bug 18403: Add POD for output_and_exit_if_error
Bug 18403: Hide patron information if not part of the logged in user library group
Bug 18403: Add new method Koha::Library->library_group
Bug 18403: Add new method Koha::Library::Group->has_child
Bug 18403: Send logged_in_user to template from C4::Auth
Bug 18403: Add new method Koha::Patron->can_see_patron_info
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos
Bug 18403: Adapt patron search
Bug 18403: Only display libraries from group in dropdown lists
Bug 18403: Use patron-title.inc when hidepatronname is used
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]
Bug 18403: Batch patron modification tool
Bug 18403: output_and_exit_if_error for circulation.pl
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons
Bug 18403: Add tests for Koha::Patrons
Bug 18403: Add new method Koha::Patron->can_see_patrons_from
Bug 18403: Patron modification requests
Bug 18403: Patron reviews
Bug 18403: Patron discharges
Bug 18403: Article requests
Bug 18403: Guarantors
Bug 18403: REST API - patrons endpoint
Bug 18403: Deal with the DB user
Bug 18403: Fix error triggered by subgroups being added to group
Bug 18403: Add POD for output_and_exit_if_error
Bug 18403: Fix few errors found with hit_pages.t
Bug 18403: rename Koha::Patron->can to has_permission
Bug 18403: Fix perlcritic on Koha::Patron
Bug 18403: Fix POD for C4::Output::output_and_exit_if_error
Bug 18403: Prevent Modifications.t to fail randomly

Description Jonathan Druart 2017-04-07 21:40:52 UTC
Add the ability to hide patron record information from any other library (outside the group).
Comment 1 Jonathan Druart 2017-04-10 21:32:24 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2017-04-10 21:32:28 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2017-04-10 21:32:32 UTC
Created attachment 62012 [details] [review]
Bug 18403: Add new method Koha::Library::Group->has_child

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

IMPORTANT NOTE: At the moment the feature only works for 1 level depth, see
bug 15707 comment 166+ for the discussion

It means that if we have:
 root_group
     + groupA
         + groupA1
             + groupA1_library2
         + groupA_library1
         + groupA2
     + groupB
         + groupB_library1
groupA1_library2 is not considered a child of groupA1.
Note that this can change.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green
Comment 4 Jonathan Druart 2017-04-10 21:32:35 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2017-04-10 21:32:39 UTC
Created attachment 62014 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patron_info

Technical note:
This is the method that will be called on the logged_in_user variable sent to
the template. Moreover we will check that the logged in user can access patron'
information when access to members/* and some circulation scripts will be done.
Comment 6 Jonathan Druart 2017-04-10 21:32:44 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2017-04-10 21:32:50 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2017-04-10 21:32:54 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2017-04-10 21:33:03 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2017-04-10 21:33:08 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2017-04-10 21:33:13 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2017-04-10 21:33:17 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2017-04-10 21:33:20 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2017-04-10 21:33:24 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2017-04-10 21:33:27 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2017-04-10 21:33:31 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2017-04-10 21:33:34 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2017-04-10 21:33:38 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2017-04-10 21:33:42 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2017-04-10 21:33:46 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2017-04-10 21:33:50 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2017-04-10 21:33:54 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2017-04-10 21:33:58 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2017-04-10 21:34:02 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2017-04-10 21:37:00 UTC
There are too many dependencies on this one, please see the following remote branch for the whole stack:
https://github.com/joubu/Koha/commits/bug_18403
Comment 26 Jonathan Druart 2017-04-18 16:15:02 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2017-04-18 16:15:08 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2017-04-26 10:27:37 UTC
We're walking through testing this... as it's such a big piece we've split out the testing results into a shared spreadsheet available here:

https://docs.google.com/spreadsheets/d/11NVVHFDJavGFhnARYozmdR-FVdKWxDEjavJAZRtB0Ww/edit?usp=sharing
Comment 29 Martin Renvoize 2017-04-28 15:29:35 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2017-04-28 15:29:39 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2017-04-28 15:29:43 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2017-04-28 15:29:46 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2017-04-28 15:29:50 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2017-04-28 15:29:54 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2017-04-28 15:29:58 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2017-04-28 15:30:02 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize 2017-04-28 15:30:07 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2017-04-28 15:30:11 UTC Comment hidden (obsolete)
Comment 39 Martin Renvoize 2017-04-28 15:30:15 UTC Comment hidden (obsolete)
Comment 40 Martin Renvoize 2017-04-28 15:30:19 UTC Comment hidden (obsolete)
Comment 41 Martin Renvoize 2017-04-28 15:30:23 UTC Comment hidden (obsolete)
Comment 42 Martin Renvoize 2017-04-28 15:30:27 UTC Comment hidden (obsolete)
Comment 43 Martin Renvoize 2017-04-28 15:30:31 UTC Comment hidden (obsolete)
Comment 44 Martin Renvoize 2017-04-28 15:30:35 UTC Comment hidden (obsolete)
Comment 45 Martin Renvoize 2017-04-28 15:30:39 UTC Comment hidden (obsolete)
Comment 46 Martin Renvoize 2017-04-28 15:30:43 UTC Comment hidden (obsolete)
Comment 47 Martin Renvoize 2017-04-28 15:30:47 UTC Comment hidden (obsolete)
Comment 48 Martin Renvoize 2017-04-28 15:30:51 UTC Comment hidden (obsolete)
Comment 49 Martin Renvoize 2017-04-28 15:30:55 UTC Comment hidden (obsolete)
Comment 50 Martin Renvoize 2017-04-28 15:30:59 UTC Comment hidden (obsolete)
Comment 51 Martin Renvoize 2017-04-28 15:31:03 UTC Comment hidden (obsolete)
Comment 52 Martin Renvoize 2017-04-28 15:31:08 UTC Comment hidden (obsolete)
Comment 53 Martin Renvoize 2017-04-28 15:31:12 UTC Comment hidden (obsolete)
Comment 54 Martin Renvoize 2017-04-28 15:31:16 UTC Comment hidden (obsolete)
Comment 55 Martin Renvoize 2017-04-28 15:33:30 UTC
It's your friendly git gopher again.. 

Uploaded signed off patches on behalf of Jon McGowan at PTFS Europe.  He and Jonathan Field have both rigorously tested this and you can see the feedback and testing results in the spreadsheet linked to above.

Signed Off.
Comment 56 Jonathan Druart 2017-06-02 16:02:19 UTC
I have rebased the remote branch and added the signed-off-by line.
Comment 57 Jonathan Druart 2017-06-02 16:06:24 UTC
Obsoleted the patches here to avoid confusions. The remote branch must be used.
Comment 58 Jonathan Druart 2017-12-05 17:36:49 UTC
The remote branch has been rebased.
Comment 59 Kyle M Hall 2018-02-09 14:08:31 UTC
Passed QA! Since this a git branch I'll just change the bug status. You can add me s/o when the commits are pushed!
Comment 60 Jonathan Druart 2018-02-09 19:59:17 UTC
Created attachment 71392 [details] [review]
Bug 18403: Add POD for output_and_exit_if_error
Comment 61 Jonathan Druart 2018-02-09 20:03:40 UTC
Created attachment 71394 [details] [review]
Bug 18403: Add POD for output_and_exit_if_error
Comment 62 Jonathan Druart 2018-02-12 18:47:34 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 63 Jonathan Druart 2018-02-12 19:33:14 UTC
Created attachment 71500 [details] [review]
Bug 18403: Hide patron information if not part of the logged in user library group

This patchset adds a new feature that will allow libraries inside a
single Koha installation to restrict access to information of patrons
that

The group of libraries feature is introduced by bug 15707, see this bug for more
information.

Let's imagine that 2 groups G1 and G2 are defined and that they include 2 libraries
each G1a, G1b and G2c, G2d: logged in users attached to G1a will only see patron's
information from G1a and G1b.
To add more flexibility, a new user permission named 'view_borrower_infos_from_any_libraries'
will drive this behavior. If set, the patron will be able to see patron's information
of any libraries.

If the restriction is set, the logged in user will not be able to search, show, edit,
delete patron's information of patrons attached to groups of libraries outside his
own group.
In situations we need to refer to a patron, for holds and checkouts for instance,
and his information cannot be viewed, a text "A patron from library G1A" will be
displayed.

Considered unecessary or outside the scope of this bug report:
* The report module is not affected by this feature for obvious reasons
* The firstname and surname of guarantors, basket (acq) managers, patrons linked
to orders are still displayed.
* Log viewer: Can only be staff
* patron list: you cannot add patrons from another group of librairies, but can
see/delete from list (too much rewrite, or we can test for patron one by one?).
* "Patron card creator" tool is not impacted by this feature.
* Upload patron images is not impacted by this patch, should it be?
* Tools:
  - Upload patrons
  - Clean borrowers tool (This can can done easily updating Koha::Patrons->search
with Koha::Patrons->search_limited in search_upcoming_membership_expires and
search_patrons_to_anonymise but we will need to move GetBorrowersToExpunge to
Koha::Patrons first)
We can discuss these different points but will be other bug reports not to add
more complexity to this first patchset.

Test plan:
You will find a test plan in the following commit messages.
Start by creating different group of libraries and patrons with and without the
new permission. Open different browser sessions to ease the tests.
Note that all patches have to be applied to test the different test plans.

Technical notes:
For QAers (and others) a techical note will be added to the commit messages of this
patchset. I would recommend you to read them one by one to understand the different
steps of this development.

+ Special attention should be payed to the REST api changes
+ Should we restrict the logged in user to libraries from his group when
he wants to set his library (Home › Circulation › Set library)?

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 64 Jonathan Druart 2018-02-12 19:33:22 UTC
Created attachment 71501 [details] [review]
Bug 18403: Add new method Koha::Library->library_group

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 65 Jonathan Druart 2018-02-12 19:33:28 UTC
Created attachment 71502 [details] [review]
Bug 18403: Add new method Koha::Library::Group->has_child

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

IMPORTANT NOTE: At the moment the feature only works for 1 level depth, see
bug 15707 comment 166+ for the discussion

It means that if we have:
 root_group
     + groupA
         + groupA1
             + groupA1_library2
         + groupA_library1
         + groupA2
     + groupB
         + groupB_library1
groupA1_library2 is not considered a child of groupA1.
Note that this can change.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 66 Jonathan Druart 2018-02-12 19:33:33 UTC
Created attachment 71503 [details] [review]
Bug 18403: Send logged_in_user to template from C4::Auth

Technical note:
To ease future changes we are passing a logged_in_user variable to templates.
It contains the Koha::Patron object representing the logged in patron.
This will be very useful for this patch and even after (for instance we will be
able to replace easily loggedinusername and loggedinusernumber).

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 67 Jonathan Druart 2018-02-12 19:33:39 UTC
Created attachment 71504 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patron_info

Technical note:
This is the method that will be called on the logged_in_user variable sent to
the template. Moreover we will check that the logged in user can access patron'
information when access to members/* and some circulation scripts will be done.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 68 Jonathan Druart 2018-02-12 19:33:44 UTC
Created attachment 71505 [details] [review]
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'

Test plan:
Login with a patron that only have the 'edit_borrowers' permission.
You should be able to access patron's information of patrons inside of your group.

Technical note:
Before this patchset the borrowers permission module contains only 1 permission 'edit_borrowers'.
That meant
  borrowers => 1
and
  borrowers => '*'
had the same behavior.
Moreover, now that we have 2 permissions, 'CAN_user_borrowers' is set when all
permissions of 'borrowers' are set.
We need to update the different occurrences of these tests.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 69 Jonathan Druart 2018-02-12 19:33:51 UTC
Created attachment 71506 [details] [review]
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos

Test plan:
Login with a patron that is not allowed to see patron's information for patrons
outside of his group. Try to access patron's information from scripts of the patron
module (members/*) and circ/circulation.pl.
You should be able to access patron's information of patrons outside of your group
and get "You are not allowed to see the information of this patron."
If you try and access a patron page with a borrowernumber that does not exist, you
should get "This patron does not exist"

Technical note:
A new C4::Output subroutine is created in this patch: "output_and_exit_if_error"
Executed at the beginning of the script it will permit not to copy/paste all the
different checks to know if the logged in user is authorised to see patron's information.
The design here can be discussed, but I did not find an alternative with as less changes.
On the way I refactor what we did with 'unknowuser' previously: it will now work with all
patron pages, not only the few that used it.
Note that the 'or die "Not logged in";' part should not be needed, but... who trusts
C4::Auth?
I think it could be used as a safeguard later. I am willing to sed and remove them
if required.

Changes in discharge.pl are mainly indentation changes.

With this patch we should now have a $patron variable that refer to the patron we
want to access. That will be very useful to remove plenty of code in members/* and
only pass this variable to the template (instead of 1 variable per patron's attribute).

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 70 Jonathan Druart 2018-02-12 19:33:56 UTC
Created attachment 71507 [details] [review]
Bug 18403: Adapt patron search

This patch modifies the patron search code to limit the libraries to the
ones
the logged in user is allowed to access

Test plan:
Search for patrons
You should not see patrons you are not allowed to see.

Technical note:
I am really glad to have refactored all the patron searches before
having to
write this patch. It tooks me ~40 l to acchieve this job and affect all
patron searches.
Thanks refactoring!

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 71 Jonathan Druart 2018-02-12 19:34:01 UTC
Created attachment 71508 [details] [review]
Bug 18403: Only display libraries from group in dropdown lists

From where patrons it's about patrons, we do not want to display the libraries
from all the system, but only the ones from the group.

Test plan:
- See the overdues (circ/overdue.pl) and make sure you can only see overdues from
patrons part of your group (do not forget to test the CSV export).
- Search for patrons, the 'library' filters (headers and left side) should only
display libraries from your group
- Search for article request by patron's library: only the libraries from your
group should be displayed

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 72 Jonathan Druart 2018-02-12 19:34:06 UTC
Created attachment 71509 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used

There is already a HidePatronName syspref to hide patron's information
on bibliographic
record detail pages and the hold list.

Test plan:
With the HidePatronName enabled, make sure the patron's information are
hidden from
the catalogue and hold list pages. If the logged in user is not allowed
to see the
patron's info, no link and no cardnumber will be displayed
With he HidePatronName disabled, make sure the patron's information are
displayed
if the logged in user is allowed to see the patron's info.

Technical note:
This patch improves the existing patron-title.inc include file to
display patron's
information. Using it everywhere patron's details are displayed will
permit to
homogenise the way they are displayed. The file takes now a patron
object (what
should be, in the future, the only way to use it), that way we can call
the new
method on it to know if patron's information can be shown by the logged
in used.

NOTE: I am not sure this syspref makes sense anymore. Should not we
remove it?

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 73 Jonathan Druart 2018-02-12 19:34:11 UTC
Created attachment 71510 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]

On this page we do not have the patron object sent to the template,
let's pass it!

Test plan:
Go on the checkout history of a bibliographic record
(catalogue/issuehistory.pl)
You should not see patron's information that are not part of your group
if you
are not allowed to see them.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 74 Jonathan Druart 2018-02-12 19:34:15 UTC
Created attachment 71511 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]

Same that the previous patch but for the holds queue

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 75 Jonathan Druart 2018-02-12 19:34:20 UTC
Created attachment 71512 [details] [review]
Bug 18403: Batch patron modification tool

Do not allow a logged in staff user to modify patrons that are not part of his
group if he is not allowed.

Test plan:
Make sure you are not allowed to modify patrons that are not part of your group
from the batch patron modification tool

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 76 Jonathan Druart 2018-02-12 19:34:24 UTC
Created attachment 71513 [details] [review]
Bug 18403: output_and_exit_if_error for circulation.pl

This is a follow-up for a previous patch, changes have been tested
already

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 77 Jonathan Druart 2018-02-12 19:34:28 UTC
Created attachment 71514 [details] [review]
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed

Most of the time when we search for patrons we do not want to search for all patrons,
but just the ones the logged in user is allowed to see the information.
This patch takes care of that by adding a new search_limited method to Koha::Patrons.
When called this method only search for patrons that the logged in user is allowed
to see.

Test plan:
Patron autocomplete search should be limited

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 78 Jonathan Druart 2018-02-12 19:34:33 UTC
Created attachment 71515 [details] [review]
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons

Technical note:
Here we are just refactoring a code that have been copied into 3 different places.
libraries_where_can_see_patrons is a terrible method's name, feel free to suggest
something better. The method return a list of branchcodes to be more efficient,
instead of Koha::Libraries

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 79 Jonathan Druart 2018-02-12 19:34:37 UTC
Created attachment 71516 [details] [review]
Bug 18403: Add tests for Koha::Patrons

A bit late but here are the tests for
 Koha::Patron->libraries_where_can_see_patrons
 Koha::Patron->can_see_patron_infos
 Koha::Patron->search_limited

Test plan:
  prove t/db_dependent/Koha/Patrons.t
should return green

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 80 Jonathan Druart 2018-02-12 19:34:42 UTC
Created attachment 71517 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patrons_from

Technical note:
Sometimes we do not have the patron object, for instance for the patron modifications
we will need to know if the logged in user can modify patron's from a given library.
This new subroutine 'can_see_patrons_from' will then be useful

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 81 Jonathan Druart 2018-02-12 19:34:48 UTC
Created attachment 71518 [details] [review]
Bug 18403: Patron modification requests

Limit patron's modifications based on logged in patron permissions.

Test plan:
Create some patron's modification requests at the OPAC
Make sure the logged in staff user see (or not) the modification depending his
permissions.
The number of modification displayed on the mainpage should be correct as well.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 82 Jonathan Druart 2018-02-12 19:34:53 UTC
Created attachment 71519 [details] [review]
Bug 18403: Patron reviews

This patch adds a new method Koha::Reviews->search_limited to return the
reviews
a logged in user is allowed to see depending his permissions.

Test plan:
Create some reviews at the OPAC and make sure a staff user is limited
(or not) to approve
or decline it.
The number of reviews displayed on the mainpage should be correct as
well.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 83 Jonathan Druart 2018-02-12 19:34:58 UTC
Created attachment 71520 [details] [review]
Bug 18403: Patron discharges

This patch deals with patron's discharges.

Test plan:
Same as previously you will need to request dischages at the OPAC.
On the staff interface the logged in user should not be allowed to see
discharge
from patrons outside his library group.
The number of discharges waiting displayed on the mainpage should be
correct as well.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Bug 18403: (follow-up) Patron discharges

Fix QA issue:
forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 150)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 84 Jonathan Druart 2018-02-12 19:35:02 UTC
Created attachment 71521 [details] [review]
Bug 18403: Article requests

Same as previously but for article requests.

Test plan:
Test article requests and make sure you do not need the requests for
patrons that
are attached to a group that is not part of your library's group

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 85 Jonathan Druart 2018-02-12 19:35:07 UTC
Created attachment 71522 [details] [review]
Bug 18403: Guarantors

Technically a kid from your library group could have a guarantor
attached to another
group of library, let's deal with this case.

Test plan:
- Create a kid from your library group
- With a superlibrarian staff user create a guarantor that is outside of
the group of
libraries of the kid
- Login with a limited staff user and confirm that on the patron detail
page you do not
see the link to the guarantor detail page.

Note that you see the firstname and surname of the guarantor
Q. should it be hidden?

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 86 Jonathan Druart 2018-02-12 19:35:13 UTC
Created attachment 71523 [details] [review]
Bug 18403: REST API - patrons endpoint

There is something wrond here, the userenv is no set and so we cannot
user search_limited.
Should we set the userenv or filter on the libraries using
libraries_where_can_see_patrons?
WAITING FOR FEEDBACK HERE.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 87 Jonathan Druart 2018-02-12 19:35:18 UTC
Created attachment 71524 [details] [review]
Bug 18403: Deal with the DB user

On first login, Koha explodes before the logged in user does not exist
in DB.
This patch deals with that by adding several checks when it's needed.

Test plan:
Use the DB user to create a superlibrarian user.
The DB user should no be allowed to do anything else.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Bug 18403: Fix patron creation

memberentry.pl can be called to create a new patron, in that case the
patron does not exist yet.

Signed-off-by: Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 88 Jonathan Druart 2018-02-12 19:35:24 UTC
Created attachment 71525 [details] [review]
Bug 18403: Fix error triggered by subgroups being added to group

With one root group and two child groups and it works fine.
But if I create any groups under *those* groups, not only does it not work,
I get the following error:
Template process failed: undef error - SQL::Abstract::puke(): [SQL::Abstract::__ANON__]
Fatal: SQL::Abstract before v1.75 used to generate incorrect SQL when the -IN operator was
given an undef-containing list: !!!AUDIT YOUR CODE AND DATA!!! (the
upcoming Data::Query-based version of SQL::Abstract will
emit the logically correct SQL instead of raising this
exception) at /home/vagrant/kohaclone/Koha/Objects.pm line 269

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 89 Jonathan Druart 2018-02-12 19:35:29 UTC
Created attachment 71526 [details] [review]
Bug 18403: Add POD for output_and_exit_if_error

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 90 Jonathan Druart 2018-02-12 19:35:34 UTC
Created attachment 71527 [details] [review]
Bug 18403: Fix few errors found with hit_pages.t

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 91 Jonathan Druart 2018-02-12 20:41:57 UTC
Created attachment 71528 [details] [review]
Bug 18403: rename Koha::Patron->can to has_permission

I do not exactly why but there is a conflict in the name of the method

prove t/db_dependent/api/v1/patrons.t failed with
[Mon Feb 12 17:13:16 2018] [error] Can't use string ("TO_JSON") as a
HASH ref while "strict refs" in use at
/home/vagrant/kohaclone/C4/Auth.pm line 2053.
Comment 92 Jonathan Druart 2018-02-12 20:42:12 UTC
Created attachment 71529 [details] [review]
Bug 18403: Fix perlcritic on Koha::Patron

Perl::Critic found these violations in "Koha/Patron.pm":
"return" statement followed by "sort" at line 798, column 5.  Behavior
is undefined if called in scalar context.  (Severity: 5)
Comment 93 Jonathan Druart 2018-02-12 20:44:41 UTC
Created attachment 71530 [details] [review]
Bug 18403: Fix POD for C4::Output::output_and_exit_if_error

xt/author/podcorrectness.t
C4/Output.pm (310): Expected text after =item, not a bullet
Comment 94 Nick Clemens 2018-02-16 01:40:12 UTC
Awesome work all! New feature, not backported to stable.
Comment 95 Jonathan Druart 2018-02-16 13:51:02 UTC
Created attachment 71767 [details] [review]
Bug 18403: Prevent Modifications.t to fail randomly

#   Failed test 'pending_count() correctly returns 2 if filtered by library'
    #   at t/db_dependent/Koha/Patron/Modifications.t line 355.
    #          got: '0'
    #     expected: '2'

    #   Failed test 'pending_count() correctly returns 2'
    #   at t/db_dependent/Koha/Patron/Modifications.t line 360.
    #          got: '0'
    #     expected: '2'

    #   Failed test 'pending_count() correctly returns 1'
    #   at t/db_dependent/Koha/Patron/Modifications.t line 365.
    #          got: '0'
    #     expected: '1'
    # Looks like you failed 3 tests of 16.

 #   Failed test 'pending_count() and pending() tests'
 #   at t/db_dependent/Koha/Patron/Modifications.t line 374.
 # Looks like you failed 1 test of 6.
 [19:45:28] t/db_dependent/Koha/Patron/Modifications.t

The logged in patron did not have permissions to view patron's info from other branches.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 96 Jonathan Druart 2018-02-16 15:09:28 UTC
(In reply to Jonathan Druart from comment #95)
> Created attachment 71767 [details] [review] [review]
> Bug 18403: Prevent Modifications.t to fail randomly

Pushed to master.