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.