[Koha-bugs] [Bug 7304] Working on funds ergonomic display and funds management by multi librarians

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304

M. de Rooy <m.de.rooy at rijksmuseum.nl> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Failed QA

--- Comment #11 from M. de Rooy <m.de.rooy at 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.