|
Description
claire.hernandez@biblibre.com
2012-07-13 08:14:10 UTC
Created attachment 11742 [details] [review] Bug 8436: Add limit on branch in checkexpiration.pl This limit is only available for superlibrarians and patrons that have superserials permission. Other patrons will only see subscriptions of their branch. Created attachment 11746 [details] [review] Bug 8436: Add limit on branch in checkexpiration.pl This limit is only available for superlibrarians and patrons that have superserials permission. Other patrons will only see subscriptions of their branch. Patch does not apply; please rebase and resubmit. Created attachment 19262 [details] [review] Bug 8436: Add limit on branch in checkexpiration.pl This limit is only available for superlibrarians and patrons that have superserials permission. Other patrons will only see subscriptions of their branch. Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Patch applied cleanly, go forth and signoff Can I get a test plan on this please so I can signoff Julian, Can you send a testing plan for us to follow. -Jesse difficult to test this patch before solving bug 12452 Created attachment 35166 [details] [review] [SIGNED OFF] Bug 8436: Add limit on branch in checkexpiration.pl I've applied the patch against master 3.19.00.005 head 12868 If a subscription will expire in the future, it can be selected [and listed] by : /koha/serials/checkexpiration.pl Home › Serials › Check expiration either on master or after having applied this patch. So imo I can test this patch. After having applied this patch on checkexpiration.pl : 1) a superlibrarian could select all the subscriptions, just as required; 2) if IndependentBranches syspref is set to "Prevent", a (Staff) superlibrarian without "superserials" permission could select only the subscriptions of his own branch, just as required; 3) if IndependentBranches syspref is set to "Prevent", a "Patron" librarian without "superserials" permission (and with all the other serials' permissions + catalogue's permission) could select only the subscriptions of his own branch, just as required; 4) if IndependentBranches syspref is set to "Prevent", a librarian without "superserials" permission could select the subscriptions of "none" branch, just as required; 5) if IndependentBranches syspref is set to "Don't prevent", a superlibrarian without "superserials" permission could select all the subscriptions, just as required. So I pass the patch to "Signed Off" status. Julian, could you take a look at the search for the expiration date? I think I can see bug 12452. I have a serial subscription with expiration date on 10/09/2014. If I use the filter on the left side of the subscription search with 01/10/2014 the subscription shows up (as it should). But when I repeat the same search on the 'check expiration' page, the subscription is not found. Using iso formatted dates dd/mm/yyyy. (In reply to Katrin Fischer from comment #13) I answered on bug 12452. Can you please use C4::Branch::GetBranchesLoop rather than GetBranches with bespoke selection logic? Created attachment 36062 [details] [review] Bug 8436: Replace GetBranches with GetBranchesLoop The branch selection logic still exists in GetBranchesLoop, it should be used here. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Comment on attachment 35166 [details] [review] [SIGNED OFF] Bug 8436: Add limit on branch in checkexpiration.pl Review of attachment 35166 [details] [review]: ----------------------------------------------------------------- ::: serials/checkexpiration.pl @@ +106,5 @@ > } > + > +my $branchname; > +my @branches_loop; > +if ( $flags->{superlibrarian} I think we should be using C4::Context->IsSuperLibrarian() here - please check! Created attachment 41891 [details] [review] Bug 8436: Use C4::Context->IsSuperLibrarian() Created attachment 43646 [details] [review] [PASSED QA] Bug 8436: Add limit on branch in checkexpiration.pl This limit is only available for superlibrarians and patrons that have superserials permission. Other patrons will only see subscriptions of their branch. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 43647 [details] [review] [PASSED QA] Bug 8436: Replace GetBranches with GetBranchesLoop The branch selection logic still exists in GetBranchesLoop, it should be used here. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 43648 [details] [review] [PASSED QA] Bug 8436: Use C4::Context->IsSuperLibrarian() Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 43649 [details] [review] Bug 8436: QA Follow-up: Activate branch filter, if IndependentBranches is off The superserials permission is only in use in combination with IndependentBranches - so it makes sense to also show the branch filter when IndependentBranches is off. Permission description: superserials Manage subscriptions from any branch (only applies when IndependentBranches is used) Also adds a missing check for superlibraian. Note: the duplicated permission check in the code is not really nice - would be good to rewrite this to be more clean. Created attachment 43650 [details] [review] Bug 8436: QA Follow-up: Add Library to result list If we offer a library filter, we should also show the library in the result list, so that search options and result list match. Otherwise, for libraries with multiple branches, there is no way to see, which is the subscription of a specific branch in the list. - Add a Library column in the result list - Rename branch to library in the template I am guilty of 2 small follow-ups here - Julian or some other QA person - could you take a look? Created attachment 43750 [details] [review] [PASSED QA] Bug 8436: QA Follow-up: Activate branch filter, if IndependentBranches is off The superserials permission is only in use in combination with IndependentBranches - so it makes sense to also show the branch filter when IndependentBranches is off. Permission description: superserials Manage subscriptions from any branch (only applies when IndependentBranches is used) Also adds a missing check for superlibraian. Note: the duplicated permission check in the code is not really nice - would be good to rewrite this to be more clean. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 43751 [details] [review] [PASSED QA] Bug 8436: QA Follow-up: Add Library to result list If we offer a library filter, we should also show the library in the result list, so that search options and result list match. Otherwise, for libraries with multiple branches, there is no way to see, which is the subscription of a specific branch in the list. - Add a Library column in the result list - Rename branch to library in the template Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Patches pushed to master. Thanks Julian! |