This fourth and last part will remove: - GetBranchName - GetBranchesLoop - GetBranches - GetBranch will move mybranch and onlymine to C4::Context and finally remove all occurrences of C4::Branch.
Created attachment 47746 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions
Created attachment 47747 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 47748 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 47749 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 47750 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 47751 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 47752 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 47753 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
Created attachment 47759 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 47760 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 47761 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 47762 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 47763 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 47764 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 47765 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
Applying: Bug 15758: Koha::Libraries - Remove GetBranchName fatal: sha1 information is lacking or useless (C4/Circulation.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 15758: Koha::Libraries - Remove GetBranchName
Created attachment 52723 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions
Created attachment 52724 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 52726 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 52727 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 52728 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 52729 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 52730 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 52731 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
Created attachment 52736 [details] [review] Bug 15758: Add missing use of Branches TT plugin in template
Hi Jonathan Sorry but patch fail in C4/XSLT When you make a search you obtain as follow: Software error: at <http://dev.koha:8080/cgi-bin/koha/catalogue/search.pl?q=au%3AMaxwell> Can't use string ("Midway") as a HASH ref while "strict refs" in use at /home/koha/kohaclone/C4/XSLT.pm line 308. Or when you try to see the detail page of a bib-record: Software error: <http://dev.koha:8080/cgi-bin/koha/catalogue/detail.pl?biblionumber=2> Can't use string ("Midway") as a HASH ref while "strict refs" in use at /home/koha/kohaclone/C4/XSLT.pm line 308. I think fails IMHO in this attachment <https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=52727&action=diff> So change bug status to "In Discussion" since I'm not a QA like you Hard work. Congrats! Regards
I think this should continue to be marked "needs signoff" because it requires so much testing.
I've made some progress on testing this. These are the pages I've tested which appear to be fine: admin/branches.pl admin/smart-rules.pl admin/patron-attr-types.pl admin/aqplan.pl admin/aqbudgets.pl admin/edi_ean_accounts.pl acqui/addorderiso2709.pl acqui/neworderbiblio.pl acqui/neworderempty.pl acqui/newordersubscription.pl acqui/newordersuggestion.pl acqui/basketgroup.pl acqui/basketheader.pl acqui/parcel.pl catalogue/issuehistory.pl catalogue/moredetail.pl circ/circulation.pl circ/reserveratios.pl circ/selectbranchprinter.pl circ/transferstoreceive.pl circ/view_holdsqueue.pl circ/waitingreserves.pl course_reserves/course-details.pl members/members-home.pl members/memberentry.pl suggestion/suggestion.pl These are the problems I encountered: admin/patron-attr-types.pl - Limiting by library works (data is saved) but correct library is not selected when editing that attribute again. admin/branch_transfer_limits.pl - "Undefined subroutine &main::mybranch called at /admin/branch_transfer_limits.pl line 43." admin/transport-cost-matrix.pl - Table rows display library names correctly, but table columns are blank admin/authorised_values.pl - Editing any authorized value "Not a HASH reference at /admin/authorised_values.pl line 61." admin/item_circulation_alerts.pl - No libraries listed in "select a library" dropdown admin/categories.pl - Editing any patron category - "Not a HASH reference at /admin/categories.pl line 62."" acqui/basket.pl?basketno=X - "Not a HASH reference at /acqui/basket.pl line 251."" opac-detail.pl and catalogue/detail.pl - Can't use string ( [my library's name] ) as a HASH ref while "strict refs" in use at /C4/XSLT.pm line 308. members/moremember.pl - sidebar: Logged-in branch is displayed instead of patron home library patron_lists/list.pl reserve/request.pl - Pickup library is saved correctly but not displayed correctly for existing holds circ/returns.pl - If I check in an item which belongs at another library, the holding library is shown as the destination library rather than the library where the item was checked in Several of those problems are blockers for this patch, but I think it's still worthwhile to keep this bug open to encourage others to test the areas which haven't been tested yet.
Created attachment 52788 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName
Created attachment 52789 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 52790 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 52791 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 52792 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 52793 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 52794 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 52795 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
(In reply to Owen Leonard from comment #28) Thanks! Everything is fixed, unless: > circ/returns.pl - If I check in an item which belongs at another library, > the holding library is shown as the destination library rather than the > library where the item was checked in It seems to be the behavior in master, could you confirm?
There is a conflict in catalogue/search.pl.
Created attachment 52913 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName
Created attachment 52914 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 52915 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 52916 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 52917 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 52918 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 52919 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 52920 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
Created attachment 53893 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions
Created attachment 53894 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 53895 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 53896 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 53897 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 53898 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 53899 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 53900 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
I've done a lot of testing now and have only run into small problems: These pages have a sidebar filter which includes a library selector but no libraries are listed: - acqui/invoices.pl - admin/aqbudgets.pl - opac/opac-topissues.pl - serials/serials-search.pl These pages don't show the patron's library in the sidebar: - members/discharge.pl - members/member-flags.pl - members/member-password.pl Other issues: - In both the staff client and OPAC, library names don't appear in the facets. Instead there is a list of asterisks. - admin/clone-rules.pl -- When you submit a library to clone rules to, the next step asks you to select a library. Your original selection seems to be getting lost. - circ/returns.pl -- When a hold is found, the line at the bottom of the dialog, "Hold at [library]" shows the library code instead of name. - members/paycollect.pl -- Doesn't show patron category description in the sidebar (maybe that's from Bug 15407?) - reports/guided_reports.pl -- When running an SQL report which uses the <<branches>> placeholder, the libraries are shown in order by code rather than name. - tools/holidays.pl -- The library name doesn't appear in the page heading.
Created attachment 53997 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions
Created attachment 53998 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 53999 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 54000 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 54001 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 54002 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 54003 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 54004 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
(In reply to Owen Leonard from comment #55) Excellent testing session Owen :) > I've done a lot of testing now and have only run into small problems: > > These pages have a sidebar filter which includes a library selector but no > libraries are listed: > > - acqui/invoices.pl > - admin/aqbudgets.pl > - opac/opac-topissues.pl > - serials/serials-search.pl > > These pages don't show the patron's library in the sidebar: > > - members/discharge.pl > - members/member-flags.pl > - members/member-password.pl Everything fixed. members/discharge.pl was actually moremember.pl > Other issues: > > - In both the staff client and OPAC, library names don't appear in the > facets. Instead there is a list of asterisks. Erk, I missed something in one rebase. Fixed now. > - admin/clone-rules.pl -- When you submit a library to clone rules to, the > next step asks you to select a library. Your original selection seems to be > getting lost. > > - circ/returns.pl -- When a hold is found, the line at the bottom of the > dialog, "Hold at [library]" shows the library code instead of name. Fixed! > - members/paycollect.pl -- Doesn't show patron category description in the > sidebar (maybe that's from Bug 15407?) In master, will fix it separately. > - reports/guided_reports.pl -- When running an SQL report which uses the > <<branches>> placeholder, the libraries are shown in order by code rather > than name. > > - tools/holidays.pl -- The library name doesn't appear in the page heading. Fixed!
(In reply to Jonathan Druart from comment #64) > > - members/paycollect.pl -- Doesn't show patron category description in the > > sidebar (maybe that's from Bug 15407?) > > In master, will fix it separately. See bug 17052.
Created attachment 54029 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions
Created attachment 54030 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop
Created attachment 54031 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context
Created attachment 54032 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches
Created attachment 54033 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library
Created attachment 54034 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch
Created attachment 54035 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin
Created attachment 54036 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks.
Created attachment 54037 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54038 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Remove GetBranchesLoop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54039 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Move mybranch to C4::Context Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54040 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Remove GetBranches Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54041 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54042 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54043 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54044 [details] [review] [SIGNED-OFF] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54861 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54862 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54863 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54864 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54865 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54866 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54867 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 54868 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
QA: On my list for tomorrow..
Created attachment 54908 [details] [review] Bug 15758: [QA Follow-up] Remove onlymine from Plugin/Branches.pm Hard to miss that one: Template process failed: plugin error - Bareword "C4::Branch::onlymine" not allowed while "strict subs" in use at Koha/Template/Plugin/Branches.pm line 59. Compilation failed in require at /usr/lib/perl5/Template/Plugins.pm line 206. Removed OnlyMine from Plugin/Branches. Replaced by a template var in cleanborrowers; it was used only once. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 54909 [details] [review] Bug 15758: [QA Follow-up] Some corrections related to GetBranchName [1] Acquisition.pm The lines filling $row in GetBasketGroupAsCSV may have side-effects when the library name is not found. This change restores former behavior. Just theoretically more safe. Note that it also contained a typo: $row->{deliveryplace} should have been $row->{$place}. [2] Auth.pm checkauth: $branchname = Koha::Libraries->find($branchcode)->branchname; Should normally be fine, but I rather have an empty string here than crashing on "Can't call method branchname on undefined value". Same for sub check_api_auth. Note that this holds for a larger number of calls, but I am adding a check here because it is checkauth. Also removed a duplicate use Koha::Libraries-statement. [3] Search.pm Also removed a duplicate use statement for Libraries. [4] svc/holds Added an (explicit) use statement for Koha::Libraries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
No not ready yet (by far..)
Impressive work, Jonathan. Some findings so far for first two patches. Looks like I will not finish this one today. === Patch 1 C4/Labels/Label.pm L100 require Koha::Libraries; Remember you failed patches before for that? :) L101 # FIXME Is this used?? Not sure. But can you use those fields when you define a label in the editor or something? One of the templates contains the string in text and implicitly seems to suggest that. t/db_dependent/Reserves.t still fails (cause somewhere else) === Patch 2 Auth.pm: use for Branches removed; but still called. (Will be resolved, yes) TODO in C4/Search.pm L337 koha-tmpl/intranet-tmpl/prog/en/modules/reports/borrowers_stats.tt: Exceptional [% l.branchname || 'UNKNOWN' %] Inconsistent? t/db_dependent/Holds/RevertWaitingStatus.t DBIx::Class::ResultSet::next(): [SQL::Abstract::__ANON__] Fatal: unexpected operator 'cancellationdate' with undef operand at /usr/share/koha/masterclone/Koha/Objects.pm line 168 Probably something similar as in Reserves.t
(In reply to Marcel de Rooy from comment #94) > === Patch 1 > C4/Labels/Label.pm > L100 require Koha::Libraries; > Remember you failed patches before for that? :) Yes :) But I have replaced an existing require with another one, not added a new one. > L101 # FIXME Is this used?? > Not sure. But can you use those fields when you define a label in the editor > or something? > One of the templates contains the string in text and implicitly seems to > suggest that. > t/db_dependent/Reserves.t still fails (cause somewhere else) Yes, see bug 16942. > === Patch 2 > Auth.pm: use for Branches removed; but still called. (Will be resolved, yes) > TODO in C4/Search.pm L337 Yes it's a TODO, for later. > koha-tmpl/intranet-tmpl/prog/en/modules/reports/borrowers_stats.tt: > Exceptional [% l.branchname || 'UNKNOWN' %] Inconsistent? No, the UNKNOWN comes from the pl. Now it's translatable. > t/db_dependent/Holds/RevertWaitingStatus.t > DBIx::Class::ResultSet::next(): [SQL::Abstract::__ANON__] Fatal: unexpected > operator 'cancellationdate' with undef operand at > /usr/share/koha/masterclone/Koha/Objects.pm line 168 > Probably something similar as in Reserves.t Same as before, see bug 16942. Thanks for QAing, really much appreciated!
Created attachment 54921 [details] [review] Bug 15758: [QA Follow-up] Corrections related to GetBranches [1] C4/Search A call to Koha::Libraries is added to routine pazGetRecords, but the results of that call are not used. So removing it again. [2] catalogue/itemsearch.pl Although A=>B=>C=>D works, we'd better use here A=>B, C=>D. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Some more tiny comments from the last big patch: You say that this (horrible??) category key within the branches hash is not used anymore due to bug 15295 ? I think so too. More evidence to present? FIXME in transferbook is fine with me. Just leave it. This PE stuff should perhaps be removed, but not on this report. circ/branchtransfers.pl + $item{'tobrname'} = $tobranchcd; and + $item{'tobrname'} = $tobcd; With a corresponding: <td class="tf-destination">[% Branches.GetName( trsfitemloo.tobrname ) %]</td> Do not really look good.. Name versus code? L194 Do we still need this warn BTW ? koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt + <p class="problem">Please return item to: [% Branches.GetName( errmsgloo.msg ) %]</p> Hmm. I will finish it later next week. I guess that splitting up this patch set in two parts would have been better.
(In reply to Marcel de Rooy from comment #97) > Some more tiny comments from the last big patch: > > You say that this (horrible??) category key within the branches hash is not > used anymore due to bug 15295 ? I think so too. More evidence to present? Everywhere it was used, Koha::LibraryCategory is now used > FIXME in transferbook is fine with me. Just leave it. This PE stuff should > perhaps be removed, but not on this report. Definitely not :) > circ/branchtransfers.pl > + $item{'tobrname'} = $tobranchcd; > and > + $item{'tobrname'} = $tobcd; > With a corresponding: <td class="tf-destination">[% Branches.GetName( > trsfitemloo.tobrname ) %]</td> > Do not really look good.. Name versus code? Yes the var name is wrong. Patch is coming. > L194 Do we still need this warn BTW ? No idea. In doubt, let's keep it. > koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt > + <p class="problem">Please return item to: [% > Branches.GetName( errmsgloo.msg ) %]</p> > Hmm. Ugly but correct. > I will finish it later next week. I guess that splitting up this patch set > in two parts would have been better. Yes it's what I did for C4::Members. But here for libraries it looked easier to get 1 big test than having to test everything for each patch.
Created attachment 54925 [details] [review] Bug 15758: Fix variable name, it's a branchcode
Created attachment 55061 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55062 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55063 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55064 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55065 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55066 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55067 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55068 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55069 [details] [review] Bug 15758: [QA Follow-up] Remove onlymine from Plugin/Branches.pm Hard to miss that one: Template process failed: plugin error - Bareword "C4::Branch::onlymine" not allowed while "strict subs" in use at Koha/Template/Plugin/Branches.pm line 59. Compilation failed in require at /usr/lib/perl5/Template/Plugins.pm line 206. Removed OnlyMine from Plugin/Branches. Replaced by a template var in cleanborrowers; it was used only once. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55070 [details] [review] Bug 15758: [QA Follow-up] Some corrections related to GetBranchName [1] Acquisition.pm The lines filling $row in GetBasketGroupAsCSV may have side-effects when the library name is not found. This change restores former behavior. Just theoretically more safe. Note that it also contained a typo: $row->{deliveryplace} should have been $row->{$place}. [2] Auth.pm checkauth: $branchname = Koha::Libraries->find($branchcode)->branchname; Should normally be fine, but I rather have an empty string here than crashing on "Can't call method branchname on undefined value". Same for sub check_api_auth. Note that this holds for a larger number of calls, but I am adding a check here because it is checkauth. Also removed a duplicate use Koha::Libraries-statement. [3] Search.pm Also removed a duplicate use statement for Libraries. [4] svc/holds Added an (explicit) use statement for Koha::Libraries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55071 [details] [review] Bug 15758: [QA Follow-up] Corrections related to GetBranches [1] C4/Search A call to Koha::Libraries is added to routine pazGetRecords, but the results of that call are not used. So removing it again. [2] catalogue/itemsearch.pl Although A=>B=>C=>D works, we'd better use here A=>B, C=>D. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55072 [details] [review] Bug 15758: Fix variable name, it's a branchcode Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55073 [details] [review] Bug 15758: [QA Follow-up] Final touch? From qa tools: reports/cat_issues_top.pl: pod: =over on line 39 without closing =back Note: qa tools does also report "options_for_libraries: not found" on two template includes. Considering this as a false alarm, since the block is defined in another include loaded in doc-head-open. [1] koha-tmpl/intranet-tmpl/prog/en/includes/subscriptions-search.inc [2] koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-topissues.inc Found a template where a USE Branches was missing: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Easy to spot since the libraries combo was empty. Tried to find similar occurrences with few greps, but couldn't find any. I did find duplicate USE Branches though in: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt koha-tmpl/intranet-tmpl/prog/en/modules/serials/checkexpiration.tt This patch removes them. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Final comments: Good job, Jonathan. Code looks good to me. Hard to test every corner. Hopefully we tackled most issues now. See also comments in last commit message. RM: Please push 15407 + this one 15758 now early to have some more test coverage and prevent rebasing. Passed QA
Created attachment 55346 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchName This is the fourth and last patch set to remove C4::Branch. The real purpose of this patch is to standardise and refactor some code which is related to the libraries selection/display. Its unconfessed purpose is to remove the C4::Branch package. Before this patch set, only 6 subroutines still existed in the C4::Branch package: - GetBranchName - GetBranchesLoop - mybranch - onlymine - GetBranches - GetBranch GetBranchName basically returns the branchname for a given branchcode. The branchname is only used for a display purpose and we don't need to retrieve it in package or pl scripts (unless for a few exceptions). We have a `Branches` template plugin with a `GetName` method which does exactly this job. To achieve this removal, we will use this template plugin and delete the GetBranchName from pl and pm files. The `Branches.all()` will now select the library of the logged in user if no `selected` parameter has been passed. This new behavior could cause regressions, for instance there are some places where we do not want an option preselected (batch item modification for instance), keep that in mind when testing. GetBranchesLoop took 3 parameters: $branch and $onlymine. The first one was used to set a "selected" flag, for a display purpose: select an option in the libraries dropdown lists. The second one was useless: If not passed or set to 0, the `C4::Branch::onlymine` subroutine was called. This onlymine flag was use to know if the logged in user was able to see other libraries infos. A patron can see the infos from other libraries if IndependentBranches is not set OR if he has the superlibrarian permission. Prior to this patch set, the "onlymine test" was done on different places (neworderempty.pl, additem.pl, holidays.pl, etc.), including the Branches TT plugin. In this patch set, this test is only done on one place (C4::Context::only_my_library, code moved from C4::Branch::onlymine). To accomplish the same job as this subroutine, we just need to call the `Branches.all()` method from the `Branches` TT plugin. It already accepts a `selected` parameter to set a flag on the option to select. To avoid the repetitive [% IF selected %]<option selected="selected">[% ELSE %]<option>[% END %] pattern, a new `html_helpers` TT include file has been created, it defines an `options_for_libraries` block, which takes a `selected` parameter. We could imagine to use this include file for other selects. The 'mybranch` and `onlymine` subroutines of the C4::Branch package have been moved to C4::Context. onlymine has been renamed with only_my_library. There are only 4 occurrences of it, against 11 before this patch set. There 2 subroutines are Context-centric and it makes sense to put them in `C4::Context` (at least it's the least worst place!) GetBranches is the tricky part of this patch set: It retrieves all the libraries, independently of the value of IndependentBranches. To keep the same way as the existing calls of `Branches.all()`, I have added a `unfiltered` parameter. If set, the `Branches.all()` will call a usual Koha::Libraries->search method, otherwise Koha::Libraries->search_filtered will be called. This new method will check if the logged in user is allowed to see other libraries or only its library. Note that this `GetBranches` subroutine also created a `category` key: it allowed to get the list of groups (of libraries) where this library existed. Thanks to a previous patch set (bug 15295), this value was not used anymore (I may have missed something!). Note that the only use of `GetBranch` was buggy (see bug 15746). Test plan (for the whole patch set): The best way to test this whole patch set is to test with 2 instances: 1 with the patch set applied, 1 using master, to be sure there is no regression. It would be good to test the same with `IndependentBranches` and the without `IndependentBranches`. No difference should be found. The tester must focus on the library dropdowns on as many forms as possible. You will notice changes in the order of the options: the libraries will now be ordered by branchname (instead of branchcode in some places). A special attention will be given to the following page: - acqui/neworderempty.pl - catalogue/search.pl - members/members-home.pl (header?) - opac/opac-topissues.pl - tools/holidays.pl - admin/branch_transfer_limits.pl - admin/item_circulation_alerts.pl - rotating_collections/transferCollection.pl - suggestion/suggestion.pl - tools/export.pl Notes for QA: - There are 2 FIXMEs in the patch set, I have kept the existing behavior, but I am not sure it's the good one. Feel free to open a bug report and I will fill a patch if you think it's not correct. Otherwise, remove the FIXME lines in a follow-up patch. - The whole patch set is huge and makes a lot of changes. But it finally will tremendously reduce the number of lines: 716 insertions for 1910 deletions Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55347 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranchesLoop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55348 [details] [review] Bug 15758: Koha::Libraries - Move mybranch to C4::Context Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55349 [details] [review] Bug 15758: Koha::Libraries - Remove GetBranches Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55350 [details] [review] Bug 15758: Koha::Libraries - Move onlymine to C4::Context::only_my_library Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55351 [details] [review] Bug 15758: Koha::Libraries - Ultimate duel for C4::Branch Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55352 [details] [review] Bug 15758: Koha::Libraries - Add tests for the Branches TT plugin Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55353 [details] [review] Bug 15758: Koha::Libraries - Do not select an option if selected is defined This will certainly need a few more tweaks. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 55354 [details] [review] Bug 15758: [QA Follow-up] Remove onlymine from Plugin/Branches.pm Hard to miss that one: Template process failed: plugin error - Bareword "C4::Branch::onlymine" not allowed while "strict subs" in use at Koha/Template/Plugin/Branches.pm line 59. Compilation failed in require at /usr/lib/perl5/Template/Plugins.pm line 206. Removed OnlyMine from Plugin/Branches. Replaced by a template var in cleanborrowers; it was used only once. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55355 [details] [review] Bug 15758: [QA Follow-up] Some corrections related to GetBranchName [1] Acquisition.pm The lines filling $row in GetBasketGroupAsCSV may have side-effects when the library name is not found. This change restores former behavior. Just theoretically more safe. Note that it also contained a typo: $row->{deliveryplace} should have been $row->{$place}. [2] Auth.pm checkauth: $branchname = Koha::Libraries->find($branchcode)->branchname; Should normally be fine, but I rather have an empty string here than crashing on "Can't call method branchname on undefined value". Same for sub check_api_auth. Note that this holds for a larger number of calls, but I am adding a check here because it is checkauth. Also removed a duplicate use Koha::Libraries-statement. [3] Search.pm Also removed a duplicate use statement for Libraries. [4] svc/holds Added an (explicit) use statement for Koha::Libraries. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55356 [details] [review] Bug 15758: [QA Follow-up] Corrections related to GetBranches [1] C4/Search A call to Koha::Libraries is added to routine pazGetRecords, but the results of that call are not used. So removing it again. [2] catalogue/itemsearch.pl Although A=>B=>C=>D works, we'd better use here A=>B, C=>D. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 55357 [details] [review] Bug 15758: Fix variable name, it's a branchcode
Created attachment 55358 [details] [review] Bug 15758: [QA Follow-up] Final touch? From qa tools: reports/cat_issues_top.pl: pod: =over on line 39 without closing =back Note: qa tools does also report "options_for_libraries: not found" on two template includes. Considering this as a false alarm, since the block is defined in another include loaded in doc-head-open. [1] koha-tmpl/intranet-tmpl/prog/en/includes/subscriptions-search.inc [2] koha-tmpl/opac-tmpl/bootstrap/en/includes/opac-topissues.inc Found a template where a USE Branches was missing: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Easy to spot since the libraries combo was empty. Tried to find similar occurrences with few greps, but couldn't find any. I did find duplicate USE Branches though in: koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt koha-tmpl/intranet-tmpl/prog/en/modules/serials/checkexpiration.tt This patch removes them. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Yet another rebase
Pushed to master for 16.11, thanks Jonathan, Marcel!
Created attachment 55445 [details] [review] Bug 15758: (QA followup) Remove leftover reference to C4::Branch Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Kyle M Hall from comment #128) > Pushed to master for 16.11, thanks Jonathan, Marcel! hi there.. this patchset seems to cause the t/00-load.t test to fail? root@xen1:/home/mason/g/k/master# koha-shell -c 'prove' mast3 ... Too late to run INIT block at /usr/share/koha/lib/C4/External/BakerTaylor.pm line 42. t/00-load.t ......................... 117/? # Failed test 'use Koha::Libraries;' # at t/00-load.t line 65. # Tried to use 'Koha::Libraries'. # Error: Bareword "C4::Context::only_my_library" not allowed while "strict subs" in use at Koha/Libraries.pm line 52. # Compilation failed in require at t/00-load.t line 65. # BEGIN failed--compilation aborted at t/00-load.t line 65. Bailout called. Further testing stopped: ***** PROBLEMS LOADING FILE 'Koha::Libraries' FAILED--Further testing stopped: ***** PROBLEMS LOADING FILE 'Koha::Libraries' root@xen1:/home/mason/g/k/master#
(In reply to Mason James from comment #130) > (In reply to Kyle M Hall from comment #128) > > Pushed to master for 16.11, thanks Jonathan, Marcel! > > hi there.. > this patchset seems to cause the t/00-load.t test to fail? % prove t/00-load.t t/00-load.t .. 1/? DBIx::Class::Storage::DBI::sql_maker(): Your storage class (DBIx::Class::Storage::DBI::Mock) does not set sql_limit_dialect and you have not supplied an explicit limit_dialect in your connection_info. DBIC will attempt to use the GenericSubQ dialect, which works on most databases but can be (and often is) painfully slow. Please file an RT ticket against 'DBIx::Class::Storage::DBI::Mock' at Koha/Objects.pm line 83 t/00-load.t .. ok All tests successful. Files=1, Tests=523, 2 wallclock secs ( 0.08 usr 0.00 sys + 1.98 cusr 0.06 csys = 2.12 CPU) Result: PASS The C4::Context::only_my_library call is at line 48, not 52 on current master: 48 if ( C4::Context::only_my_library ) {
(In reply to Jonathan Druart from comment #131) > (In reply to Mason James from comment #130) > > (In reply to Kyle M Hall from comment #128) > > > Pushed to master for 16.11, thanks Jonathan, Marcel! > > > > hi there.. > > this patchset seems to cause the t/00-load.t test to fail? > > % prove t/00-load.t > .. > Result: PASS > thanks Jonathan, I confirm it's working now too (ps: I think I forgot to export my dev $PERL5LIB previously)
(In reply to Kyle M Hall from comment #128) > Pushed to master for 16.11, thanks Jonathan, Marcel! Enhancement, skipping for 16.05.x
This patch introduces a regression to the branch limit in the staff client advanced search. (The OPAC masthead search is ok and I think OPAC advanced search is too, but need to double-check.) More details at Bug 18884...