http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304
M. de Rooy <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #11 from M. de Rooy <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
