[Koha-bugs] [Bug 5911] Transport Cost Matrix of transporting an item between branches

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

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

Srdjan Jankovic <srdjan at catalyst.net.nz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |Needs Signoff

--- Comment #27 from Srdjan Jankovic <srdjan at catalyst.net.nz> ---
(In reply to comment #25)
> QA comments:
> * perlcritic problem:
> C4/HoldsQueue.pm: Don't modify $_ in list functions at line 571, column 27. 
> See page 114 of PBP.  (Severity: 5)

perlcritic can be such a pain...

> * why is this code in this patch?
> +sub _flush_preferences {
> +    %sysprefs = ();
> +}

It is a poor man's sysprefs flush as per the comment above the code:
# FIXME: running this under mod_perl will require a means of
# flushing the caching mechanism

I did not want to remove the comment because:
a) this solution was created in anger, so may not fit all
b) I did not address mod_perl/plack/ any other persistant env, just needed a
way to change sysprefs for the test

> 
> * we're supposed not to add anything to C4::, but use Koha:: instead. You're
> creating HoldsQueue.pm. As we haven't defined a clear organization for
> Koha::, I can't say where this code should be, it's probably fair to have it
> in C4. Plus I don't want Koha:: be filled with C4:: like scripts, so won't
> reject the patch for this reason

When I started this, I did not even know of Koha namespace. Now I know, but not
sure how to put things there :)

> 
> The line:
>     # XXX GetHoldsQueueItems() does not support $itemtypeslimit!
> 
> should be
>     # FIXME GetHoldsQueueItems() does not support $itemtypeslimit!

I was not sure if that needs addressing, just wanted to add a note. I can
change this if required.

> 
> Just a question, not related to QA: if the syspref is OFF, how are things
> sorted ? as before ? (if yes, good point you've set the value to 0 by
> default, it means there won't be any change for users)

Yes, if off it behaves as before. Default is 0. t/db_dependent/HoldsQueue.t
tests both cases.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are watching all bug changes.