[Bug 7304] New: Working on funds ergonomic display and funds management by multi librarians
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Bug #: 7304 Summary: Working on funds ergonomic display and funds management by multi librarians Classification: Unclassified Change sponsored?: Sponsored Product: Koha Version: rel_3_8 Platform: All OS/Version: All Status: NEW Severity: enhancement Priority: P5 - low Component: Acquisitions AssignedTo: claire.hernandez@biblibre.com ReportedBy: claire.hernandez@biblibre.com QAContact: ian.walls@bywatersolutions.com Blocks: 7163 "* 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) -- Configure bugmail: http://bugs.koha-community.org/bugzilla3/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|claire.hernandez@biblibre.c |julian.maurice@biblibre.com |om | --- Comment #1 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 9681 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9681&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #2 from Julian Maurice <julian.maurice@biblibre.com> --- PS: This patch also add a 'budget_manage_all' permission which allow user to modify (and use) every budget, whatever the restrictions are. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #3 from Julian Maurice <julian.maurice@biblibre.com> --- *** Bug 7305 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@univ-lyon3.fr --- Comment #4 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #5 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- Created attachment 9783 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9783&action=edit screen capture -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9681|0 |1 is obsolete| | --- Comment #6 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 9837 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9837&action=edit 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 ?
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Koha Team Lyon 3 <koha@univ-lyon3.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #7 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #8 from Koha Team Lyon 3 <koha@univ-lyon3.fr> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #9 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 9896 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9896&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9837|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off CC| |chris@bigballofwax.co.nz Version|rel_3_8 |master --- Comment #10 from Chris Cormack <chris@bigballofwax.co.nz> --- Fixed merge conflict on updatedatabase.pl. Works welll, and yay for more unit tests! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl QA Contact|koha.sekjal@gmail.com |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #11 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Ian Walls <koha.sekjal@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jcamins@cpbibliography.com, | |koha.sekjal@gmail.com --- Comment #13 from Ian Walls <koha.sekjal@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #14 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #15 from claire.hernandez@biblibre.com <claire.hernandez@biblibre.com> --- 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 ? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 --- Comment #16 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #9896|0 |1 is obsolete| | --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 10373 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=10373&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #18 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |paul.poulain@biblibre.com Version|master |rel_3_10 --- Comment #19 from Paul Poulain <paul.poulain@biblibre.com> --- Nice feature ! patch pushed -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #20 from Julian Maurice <julian.maurice@biblibre.com> --- Ok in master, marked as resolved. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Working on funds ergonomic |Working on funds ergonomic |display and funds |display and funds |management by multi |management by multiple |librarians |librarians -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |12956 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org