Add the ability to hide patron record information from any other library (outside the group).
Created attachment 62010 [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)?
Created attachment 62011 [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
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
Created attachment 62013 [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).
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.
Created attachment 62015 [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.
Created attachment 62016 [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).
Created attachment 62017 [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!
Created attachment 62018 [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
Created attachment 62019 [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?
Created attachment 62020 [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.
Created attachment 62021 [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
Created attachment 62022 [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
Created attachment 62023 [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
Created attachment 62024 [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
Created attachment 62025 [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
Created attachment 62026 [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
Created attachment 62027 [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
Created attachment 62028 [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.
Created attachment 62029 [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.
Created attachment 62030 [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.
Created attachment 62031 [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
Created attachment 62032 [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?
Created attachment 62033 [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.
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
Created attachment 62302 [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.
Created attachment 62303 [details] [review] Bug 18403: Fix patron creation memberentry.pl can be called to create a new patron, in that case the patron does not exist yet.
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
Created attachment 62830 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62831 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62832 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62833 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62834 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62835 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62836 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62837 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62838 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62839 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62840 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62841 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62842 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62843 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62844 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62845 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62846 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62847 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62848 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62849 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62850 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62851 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62852 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62853 [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.
Created attachment 62854 [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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Created attachment 62855 [details] [review] 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: Jon McGowan <jon.mcgowan@ptfs-europe.com>
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.
I have rebased the remote branch and added the signed-off-by line.
Obsoleted the patches here to avoid confusions. The remote branch must be used.
The remote branch has been rebased.
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!
Created attachment 71392 [details] [review] Bug 18403: Add POD for output_and_exit_if_error
Created attachment 71394 [details] [review] Bug 18403: Add POD for output_and_exit_if_error
Pushed to master for 18.05, thanks to everybody involved!
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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.
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)
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
Awesome work all! New feature, not backported to stable.
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>
(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.