http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7304
--- Comment #12 from Julian Maurice <[email protected]> --- (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. _______________________________________________ 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/
