"* aqbudgets.pl displays funds the librarians can use. There is a « show all funds » check box to access every funds. * on the fund level, you can define owner, users and/or library and then restrict access to : none of these or to the owner only or to the ower + users o to the ower + users + librarians from a specific library. We based the restriction on the library a librarian is connected to. These restrictions take effect on : acqui/acqui-home.pl acqui/booksellers.pl acqui/lateorders.pl acqui/neworderempty.pl acqui/parcel.pl admin/aqbudgets.pl admin/aqplan.pl" (BibLibre MT5557)
Created attachment 9681 [details] [review] More permissions for budgets - Possibility to add users to a budget - Restrictions changed to: - None - Owner - Owner and users - Owner, users and library - Restricted users cannot spent on these budgets (they cannot modify them either) Modified pages: - admin/aqbudgets.pl - admin/aqplan.pl - suggestion/suggestion.pl - acqui/acqui-home.pl - acqui/addorderiso2709.pl - acqui/neworderempty.pl Unit tests in t/Budgets/CanUserUseBudget.t and t/Budgets/CanUserModifyBudget.t --- Restriction rules: If restriction is 'None', every users with basic acquisition permission can spent on this budget, and every users with budget_modify permission can modify it If 'owner', only the owner can spent on it and modify it (only with 'budget_modify' permission). If there is no owner, we are in the same case as 'None' If 'owner and users' only owner and users can spent on it and modify it (with 'budget_modify'). If no owner, same as 'None'. If 'owner, users and library', owner, users and borrowers connected to the budget library can spent on it and modify it. If no owner or no library is set for the budget, same as 'None'. TO TEST: In admin/aqbudgets.pl, users cannot click on 'Edit' button for restricted budgets In admin/aqplan.pl, users cannot modify planning for restricted budgets In other modified pages, the restricted budgets are simply not displayed.
PS: This patch also add a 'budget_manage_all' permission which allow user to modify (and use) every budget, whatever the restrictions are.
*** Bug 7305 has been marked as a duplicate of this bug. ***
Test on a sandbox : - I create a budget with a owner and a user - I choose restriction = Owner and users - I log to koha with my authorised user : When i tried to go to a basket, there's no budget available : You can't create any orders unless you first define a budget and a fund. (cf attachments) two comments : 1. To add a new user to a budget, it doesn't work if this user have only 'order_manage' in permission : it will be nice to permit user to be added only to commande on this budget and not necesserly manage this budget. 2. On page : http://pro.test1.biblibre.com/cgi-bin/koha/acqui/acqui-home.pl in the table "All available funds for xxx library", in the column "fund", we expect to read the name of the budget not his number.
Created attachment 9783 [details] screen capture
Created attachment 9837 [details] [review] More permissions for budgets - Possibility to add users to a budget - Restrictions changed to: - None - Owner - Owner and users - Owner, users and library - Restricted users cannot spent on these budgets (they cannot modify them either) Modified pages: - admin/aqbudgets.pl - admin/aqplan.pl - suggestion/suggestion.pl - acqui/acqui-home.pl - acqui/addorderiso2709.pl - acqui/basket.pl - acqui/neworderempty.pl Unit tests in t/Budgets/CanUserUseBudget.t and t/Budgets/CanUserModifyBudget.t -- (In reply to comment #4) > Test on a sandbox : > - I create a budget with a owner and a user > - I choose restriction = Owner and users > - I log to koha with my authorised user : When i tried to go to a basket, > there's no budget available : You can't create any orders unless you first > define a budget and a fund. (cf attachments) Only budgets with amount greater than 0 are taken into account, so you have to be sure that there is at least one budget with positive amount. However this new patch do not filter budgets on owner and branchcode anymore (unless budget restriction request it) so the problem should not occurs anymore. > 1. To add a new user to a budget, it doesn't work if this user have only > 'order_manage' in permission : it will be nice to permit user to be added > only to commande on this budget and not necesserly manage this budget. Ok fixed. All users with at least one of acquisition sub-permissions can be added to the budget users list. > 2. On page : http://pro.test1.biblibre.com/cgi-bin/koha/acqui/acqui-home.pl > in the table "All available funds for xxx library", in the column "fund", we > expect to read the name of the budget not his number. As far as I remember, acqui-home.pl was always displaying the budget codes and not the budget names, and this patch do not modify that. Maybe this request can become a new bug ?
The patch doesn't apply anymore.... git bz apply 7304 Bug 7304 - Working on funds ergonomic display and funds management by multi librarians More permissions for budgets Apply? [yn] y Applying: Bug 7304: More permissions for budgets error: patch failed: installer/data/mysql/updatedatabase.pl:5284 error: installer/data/mysql/updatedatabase.pl: patch does not apply Patch failed at 0001 Bug 7304: More permissions for budgets When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/More-permissions-for-budgets-wqIY11.patch
Patch doesn't apply... git bz apply 7304 Bug 7304 - Working on funds ergonomic display and funds management by multi librarians More permissions for budgets Apply? [yn] y Applying: Bug 7304: More permissions for budgets error: patch failed: installer/data/mysql/updatedatabase.pl:5284 error: installer/data/mysql/updatedatabase.pl: patch does not apply Patch failed at 0001 Bug 7304: More permissions for budgets When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/More-permissions-for-budgets-zwG92l.patch
Created attachment 9896 [details] [review] Bug 7304: More permissions for budgets - Possibility to add users to a budget - Restrictions changed to: - None - Owner - Owner and users - Owner, users and library - Restricted users cannot spent on these budgets (they cannot modify them either) Modified pages: - admin/aqbudgets.pl - admin/aqplan.pl - suggestion/suggestion.pl - acqui/acqui-home.pl - acqui/addorderiso2709.pl - acqui/basket.pl - acqui/neworderempty.pl Unit tests in t/Budgets/CanUserUseBudget.t and t/Budgets/CanUserModifyBudget.t Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Fixed merge conflict on updatedatabase.pl. Works welll, and yay for more unit tests!
QA Comment: Larger patch. Generally looks good. Nice feature. Some points still need to be addressed: (Not blocking:) Is there (perhaps) a way to combine CanUserModifyBudget with CanUserUseBudget to eliminate duplicate code? If you could do so in a followup, you are welcome.. (Please clarify:) acqui/acqui-home.pl You change line: my $budget_arr = GetBudgetHierarchy; All parameters are removed in the call. Do you ask for all budgets now and check each budget in the loop? If so, is it economical to do so? Could some of this work already be done in a better phrased sql call? (BTW Seems to be that quite some calls in this loop are repeated for the same borrower/branch, but that goes outside the scope of this report.) [Same point repeated for the other acqui scripts.] (typo level:) admin/aqbudget_user_search.pl comment lines: script to find a guarantor? copy-and-paste.. Since you rename the file, could you please correct that too? (Please clarify/correct:) table aqbudgetborrowers (kohastructure and updatedatabase): Do you also need a UPDATE/DELETE clause on the constraints for budget id and borrower? unit tests: Great to have them! You introduce a new dependency however: Test::MockModule for testing both budget functions. I would rather suggest to get consensus from the dev list for adding dependencies (especially for limited testing only) than just adding them to a unit test. Note that a new dependency must be added to several files (install, packaging, etc.) Is there a simple way to achieve the same result without adding the dependency? (More theoretically: how much does the value of a unit test decrease if you mock function calls?) (Please clarify:) Since there is a "empty" budget test in t and a non-empty one in t/db_dependent, why do you add tests to t instead of t/db ? Please provide some answers on questions raised. Especially the dependency remark still holds me back from marking it ok right now. I will move it to Failed QA for now, but you could also choose for In Discussion while referring it to the list.
(In reply to comment #11) > QA Comment: > Larger patch. Generally looks good. Nice feature. Some points still need to > be addressed: > > (Not blocking:) Is there (perhaps) a way to combine CanUserModifyBudget with > CanUserUseBudget to eliminate duplicate code? If you could do so in a > followup, you are welcome.. As CanUserModifyBudget already uses CanUserUseBudget. I don't know which piece of code you're talking about. Are you talking about this? if (not ref $borrower) { $borrower = C4::Members::GetMember(borrowernumber => $borrower); } ... It's duplicated in both subs, but I don't know where to place it otherwise. I'm open to any suggestions :) > > (Please clarify:) acqui/acqui-home.pl > You change line: my $budget_arr = GetBudgetHierarchy; > All parameters are removed in the call. Do you ask for all budgets now and > check each budget in the loop? If so, is it economical to do so? Could some > of this work already be done in a better phrased sql call? (BTW Seems to be > that quite some calls in this loop are repeated for the same > borrower/branch, but that goes outside the scope of this report.) > [Same point repeated for the other acqui scripts.] Hmm, I don't know. But since tests to do depend on budget_permission, it will require a sql query like this: SELECT ... FROM aqbudgets WHERE (budget_permission = 0) OR (budget_permission = 1 AND budget_owner_id...) OR (budget_permission = 2 ...) -- require a join with aqbudgetborrowers for 2 and 3 OR ... Is it possible to do a "conditionnal join" ? Is this sample query the good way to do this? > > (typo level:) admin/aqbudget_user_search.pl > comment lines: script to find a guarantor? copy-and-paste.. Since you rename > the file, could you please correct that too? > Ok, will fix that. > (Please clarify/correct:) table aqbudgetborrowers (kohastructure and > updatedatabase): > Do you also need a UPDATE/DELETE clause on the constraints for budget id and > borrower? > Will fix that too. > unit tests: > Great to have them! > You introduce a new dependency however: Test::MockModule for testing both > budget functions. > I would rather suggest to get consensus from the dev list for adding > dependencies (especially for limited testing only) than just adding them to > a unit test. Note that a new dependency must be added to several files > (install, packaging, etc.) > Is there a simple way to achieve the same result without adding the > dependency? (More theoretically: how much does the value of a unit test > decrease if you mock function calls?) > (Please clarify:) Since there is a "empty" budget test in t and a non-empty > one in t/db_dependent, why do you add tests to t instead of t/db ? I use Test::MockModule to not be "database-dependent". And that's why unit tests are not in t/db_dependent. About dependencies files, should a module only used for unit tests be listed in Koha dependencies? It is not required at all to run Koha and it's only useful to developers. Is there something like a 'dependency level' (required/recommended/...) in those files ? > > Please provide some answers on questions raised. > Especially the dependency remark still holds me back from marking it ok > right now. I will move it to Failed QA for now, but you could also choose > for In Discussion while referring it to the list. Thanks for the review.
There is a way of marking Perl modules as optional or required in C4/Installer/PerlDependencies.pm. If it's not required, one adds a note about what feature or context it could be used in. Looks like this module is indeed packaged for Debian (or at least old Ubuntu), so it wouldn't give us too many problems there. But I'd like to avoid new dependencies wherever possible, as it makes it that much harder to install Koha, and adds one more piece of the puzzle that we don't have direct control over. What's the goal of Test::MockModule? What is it doing, and how? jcamins is working on a testing suite using HTML::Recorder and Test::WWW::Mechanize... is there any overlap here? I encourage discussion here.
(In reply to comment #13) > There is a way of marking Perl modules as optional or required in > C4/Installer/PerlDependencies.pm. If it's not required, one adds a note > about what feature or context it could be used in. > > Looks like this module is indeed packaged for Debian (or at least old > Ubuntu), so it wouldn't give us too many problems there. But I'd like to > avoid new dependencies wherever possible, as it makes it that much harder to > install Koha, and adds one more piece of the puzzle that we don't have > direct control over. I would like to see Test::MockModule as an optional dependency, at least, and I think there is a strong argument for making it a required dependency. My plan for the Test::WWW::Mechanize tests is to add a t/www directory for all the tests that require a configured installation. If there is a concern with Test::MockModule, perhaps we should have a t/mock directory for those tests? > What's the goal of Test::MockModule? What is it doing, and how? jcamins is > working on a testing suite using HTML::Recorder and Test::WWW::Mechanize... > is there any overlap here? I encourage discussion here. Test::MockModule is used for selectively overriding methods for testing. This could be used to write unit tests that test both MARC21 and UNIMARC in one run, for example.
I think this discussion about tests (good practices, wich perl library for what etc.) would have more sense here http://wiki.koha-community.org/w/index.php?title=Talk:Unit_Tests&action=edit&redlink=1 or on koha-devel ?
(In reply to comment #15) Claire, you are right of course where this leads to a general discussion. Just to make a shortcut for this report: Ian: He only uses MockModule to redefine two routines in Budgets.pm. Julian:: In order to do this without a new dependency, just change the symbol table: *Module::routine= \&mockroutine; Note that if you imported routine, you should use *routine=.. I agree that this code is not really nice, but MockModule just does the same. (Note my earlier comment on mocking in a unit test; I am certainly not promoting mocking routines in unit tests.) I checked this call with perlcritic and it is not complaining.
Created attachment 10373 [details] [review] Bug 7304: More permissions for budgets Rebased patch Fixed typo at admin/aqbudget_user_search.pl Fixed updatedatabase.pl and kohastructure.pl so that foreign keys have ON DELETE/ON UPDATE clauses Removed Test::MockModule dependency.
The size of this patch and the relative small changes to the code (compared to last version) may justify keeping it in Signed-off state. Several points of QA comment have received attention. No dependency is added. Good to see that you also remove the warning on redefine. Although I still believe that some code improvements could be made here as referred to in my previous comments, they should not block this patch from getting through. Passed QA
Nice feature ! patch pushed
Ok in master, marked as resolved.