Bug 7304 - Working on funds ergonomic display and funds management by multiple librarians
Summary: Working on funds ergonomic display and funds management by multiple librarians
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7305 (view as bug list)
Depends on:
Blocks: 7163 12956
  Show dependency treegraph
 
Reported: 2011-12-02 09:57 UTC by claire.hernandez@biblibre.com
Modified: 2014-09-18 10:37 UTC (History)
6 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
More permissions for budgets (71.28 KB, patch)
2012-05-21 14:44 UTC, Julian Maurice
Details | Diff | Splinter Review
screen capture (63.94 KB, image/jpeg)
2012-05-25 15:30 UTC, Koha Team University Lyon 3
Details
More permissions for budgets (72.83 KB, patch)
2012-05-29 15:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7304: More permissions for budgets (69.67 KB, patch)
2012-06-01 20:09 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7304: More permissions for budgets (72.90 KB, patch)
2012-06-20 09:56 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description claire.hernandez@biblibre.com 2011-12-02 09:57:07 UTC
"* 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)
Comment 1 Julian Maurice 2012-05-21 14:44:55 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-05-21 14:46:36 UTC
PS: This patch also add a 'budget_manage_all' permission which allow user to modify (and use) every budget, whatever the restrictions are.
Comment 3 Julian Maurice 2012-05-21 14:47:47 UTC
*** Bug 7305 has been marked as a duplicate of this bug. ***
Comment 4 Koha Team University Lyon 3 2012-05-25 15:27:32 UTC
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.
Comment 5 Koha Team University Lyon 3 2012-05-25 15:30:40 UTC
Created attachment 9783 [details]
screen capture
Comment 6 Julian Maurice 2012-05-29 15:19:09 UTC Comment hidden (obsolete)
Comment 7 Koha Team University Lyon 3 2012-06-01 10:54:55 UTC
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
Comment 8 Koha Team University Lyon 3 2012-06-01 12:34:23 UTC
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
Comment 9 Chris Cormack 2012-06-01 20:09:57 UTC Comment hidden (obsolete)
Comment 10 Chris Cormack 2012-06-01 20:11:27 UTC
Fixed merge conflict on updatedatabase.pl.

Works welll, and yay for more unit tests!
Comment 11 Marcel de Rooy 2012-06-11 11:33:52 UTC
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.
Comment 12 Julian Maurice 2012-06-11 15:01:20 UTC
(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.
Comment 13 Ian Walls 2012-06-14 19:19:47 UTC
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.
Comment 14 Jared Camins-Esakov 2012-06-14 19:54:10 UTC
(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.
Comment 15 claire.hernandez@biblibre.com 2012-06-15 08:09:07 UTC
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 ?
Comment 16 Marcel de Rooy 2012-06-15 11:05:04 UTC
(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.
Comment 17 Julian Maurice 2012-06-20 09:56:01 UTC
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.
Comment 18 Marcel de Rooy 2012-06-20 12:33:10 UTC
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
Comment 19 Paul Poulain 2012-06-27 14:54:21 UTC
Nice feature ! patch pushed
Comment 20 Julian Maurice 2012-07-06 08:34:44 UTC
Ok in master, marked as resolved.