Re: [Koha-devel] Due Date check in C4::Circulation::CanBookBeIssued?
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
Hi, On Wed, Apr 29, 2009 at 5:05 PM, Michael Hafen <mdhafen@tech.washk12.org> wrote:
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?
It depends. I can certainly envision cases where one might need to be able to manually enter such loans to fix problems or to enter loans that didn't get recorded in Koha for some reason. But outside of a possible scenario where you use offline circulation to handle bookmobile loans and for some reason upload the offline circulation files infrequently, I don't think this is something you'd want to do very often. Even if a *library* may need to do this occasionally, it's probably not something that you'd want your average circulation clerk or student worker to be able to do without authorization -- for one thing, being able to do this means being able to add automatic overdue penalties to a patron record -- so this question also ties into staff permissions. I think CanBookBeIssued() should always check the supplied due date. If clients of CanBookBeIssued() need to ignore the date check under certain circumstances, they should be responsible for dealing with it. Regards, Galen -- Galen Charlton VP, Research & Development, LibLime galen.charlton@liblime.com p: 1-888-564-2457 x709 skype: gmcharlt
On Wed, 2009-04-29 at 17:20 -0400, Galen Charlton wrote:
Hi,
On Wed, Apr 29, 2009 at 5:05 PM, Michael Hafen <mdhafen@tech.washk12.org> wrote:
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?
It depends. I can certainly envision cases where one might need to be able to manually enter such loans to fix problems or to enter loans that didn't get recorded in Koha for some reason. But outside of a possible scenario where you use offline circulation to handle bookmobile loans and for some reason upload the offline circulation files infrequently, I don't think this is something you'd want to do very often. Even if a *library* may need to do this occasionally, it's probably not something that you'd want your average circulation clerk or student worker to be able to do without authorization -- for one thing, being able to do this means being able to add automatic overdue penalties to a patron record -- so this question also ties into staff permissions.
I checked offline circulation, and process_koc.pl doesn't call CanBookBeIssued. It goes staight to AddIssue. Don't need to worry about that I guess. You point about staff permissions is a good one. I don't know if I want to get into making a sub-permission for entering due dates in the past. That's a bit much for a rarely, if ever, used feature. I guess I'll go ahead and move the check out of the block unless someone here asks otherwise. Thanks Galen.
I think CanBookBeIssued() should always check the supplied due date. If clients of CanBookBeIssued() need to ignore the date check under certain circumstances, they should be responsible for dealing with it.
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
On Wed, Apr 29, 2009 at 5:05 PM, Michael Hafen <mdhafen@tech.washk12.org>wrote:
So the question is: should librarians be able to enter an issue date in the past?
Effectively, I think they already can, if they upload offline circ. For example, you circulate items for 5 days, but don't upload your offline circ until a week later. The due date should rightly fall in the past. Note that the question was about due date, not issue date, but this example demonstrates that *both* could be in the past. And unless there is something else "internal" to the logic, this isn't a question for this C4::* function to decide. Validation of the librarian's duedate input must happen at the script level. I checked offline circulation, and process_koc.pl doesn't call
CanBookBeIssued. It goes staight to AddIssue.
Yes, this is because of a serious bug: process_koc.pl has no error handling. That needs to change. Offline circ *should* call CanBookBeIssued, and for that to happen it has to allow at least some mechanism for incorporating past dates. If it cannot call CanBookBeIssued, it will need a nearly identical copy to perform the same action. --Joe
Ok, given that offline circ will call CanBookBeIssued, we need to proceed carefully with error handling. The next question I think is will offline circ call GetIssueLength and do it's own due date calculation while processing a file. If it doesn't then some step needs to be taken. If it does then we will have to take the first version of this patch with only calculated due dates being checked. We could also take the stance that a librarian must know when a due date, whether entered or calculated, is invalid. In this case I think the best solution would be to code process_koc.pl to ignore the INVALID_DATE error. On the other hand, if the librarian doesn't need to know whether the entered or calculated due date is invalid, then we have to take the third option of leaving the code as it is. Which is without checking the validity of the due date. It's likely that different possibilities will be better for different people, so there is the fourth option of having a syspref control where in the code due date checking is done. I'm partial to having the due date checked, as that will fix the bug report. So either option of having process_koc.pl ignore INVALID_DATE or having CanBookBeIssued only check calculated dates is good for me. I think I'd go more for the former, having process_koc.pl ignore the INVALID_DATE error, so librarians can't punk people with overdue fines as Galen pointed out. That in mind I think it's a matter of decided between having process_koc.pl ignore the INVALID_DATE error and coding in a sys pref to control where the due date check is. On Wed, 2009-04-29 at 19:23 -0400, Joe Atzberger wrote:
On Wed, Apr 29, 2009 at 5:05 PM, Michael Hafen <mdhafen@tech.washk12.org> wrote:
So the question is: should librarians be able to enter an issue date in the past?
Effectively, I think they already can, if they upload offline circ. For example, you circulate items for 5 days, but don't upload your offline circ until a week later. The due date should rightly fall in the past. Note that the question was about due date, not issue date, but this example demonstrates that *both* could be in the past.
And unless there is something else "internal" to the logic, this isn't a question for this C4::* function to decide. Validation of the librarian's duedate input must happen at the script level.
I checked offline circulation, and process_koc.pl doesn't call CanBookBeIssued. It goes staight to AddIssue.
Yes, this is because of a serious bug: process_koc.pl has no error handling. That needs to change. Offline circ *should* call CanBookBeIssued, and for that to happen it has to allow at least some mechanism for incorporating past dates. If it cannot call CanBookBeIssued, it will need a nearly identical copy to perform the same action.
--Joe -- 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
participants (3)
-
Galen Charlton -
Joe Atzberger -
Michael Hafen