[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

--- Comment #12 from Julian Maurice <julian.maurice at 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.