On Wed, 2009-04-29 at 15:55 -0400, Galen Charlton wrote:
Hi Michael,
The patch generally looks OK. I have a couple comments, one of which I think requires that the patch be changed and resubmitted:
On Tue, Apr 28, 2009 at 12:18 PM, Michael Hafen <mdhafen@tech.washk12.org> wrote:
+ my $branch = (C4::Context->preference('CircControl') eq 'PickupLibrary') ? C4::Context->userenv->{'branch'} : + (C4::Context->preference('CircControl') eq 'PatronLibrary') ? $borrower->{'branchcode'} : + $item->{'homebranch'}; # fallback to item's homebranch
It's time for there to be a helper function that determines the applicable branch based on CircControl, the patron, and the item - this bit of code has been copied and pasted in a couple other places in C4::Circulation.
I agree with you here. It could also check the HomeOrHolding sys pref.
[snip]
+ $issuingimpossible{INVALID_DATE} = $duedate->output('syspref') unless ( $duedate && $duedate->output('iso') ge C4::Dates->today('iso') ); + }
Shouldn't this be moved outside the conditional block? Sometimes a due date is passed to CanBookBeIssued(), and it presumably ought to be checked as well.
This is an interesting question. On the one hand we have the desire to ensure valid data. On the other we can safely assume that the librarians know what they're doing. If they enter a date that is in the past it may be that they want it that way. In programming we usually don't trust the user that much, but given the possibility I'd want to change this from an error to a warning if we do decide to check user input too. So the question is: should librarians be able to enter an issue date in the past?
Regards,
Galen -- Michael Hafen Systems Analyst and Programmer Washington County School District Utah, USA
for Koha checkout http://development.washk12.org/gitweb/ or git://development.washk12.org/koha