If a library uses the system preference CheckPrevCheckout (Check for a previous issue) then, at the staff client, the issue is blocked with a dialogue asking if the checkout should be allowed or not, this is correct. However, for the sipserver, if CheckPrevCheckout is set to "Do" then the checkout is blocked with a generic "1" code and the checkout is refused by the self check. There is an argument that this is correct but, it is more likely that libraries would want to allow the checkout in this scenario as there is no explanation to the block and, most likely, it would be a genuine transaction. The sipserver needs an exception to handle this scenario.
Created attachment 72187 [details] [review] Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: Set CheckPrevCheckout to Do, or Do, with confirmation Verify the SIP server is blocking checkouts with this setting set Apply this patch Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before.y
Created attachment 72216 [details] [review] Signed off amended patch Signed off patch - works as advertised Fixed a typo there was an extra ) after the screen message indented the conditional to match preceding and break on the conditions
Is this specific to 16.11?
(In reply to Katrin Fischer from comment #3) > Is this specific to 16.11? No I think that is 16.11 and greater
Thx Colin, changing to master (bugs get backported from new to old)
Created attachment 72771 [details] [review] revision to signed off patch Amended the yes value to hardyes - this reflects the values of the syspref as in releases from 16.11 to current. 'yes' was the value in the test db I was using (and I assume Liz's) probably from an earlier release of the patch that added the syspref. - Tested against production as well
Created attachment 73858 [details] [review] Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: Set CheckPrevCheckout to Do, or Do, with confirmation Verify the SIP server is blocking checkouts with this setting set Apply this patch Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before.y Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I need a confirmation here. The test plan says "Set CheckPrevCheckout to Do, or Do, with confirmation", but it's "unless overriden, do". That means we should check the patron category and patron's settings as well. Finally it sounds like this flag is set when the error/warning must be displayed, and maybe the following code would be enough: } elsif ($confirmation eq 'PREVISSUE') { $self->screen_msg("This item was previously checked out by you"); last; } Could someone confirm or tell me what I am missing?
(In reply to Jonathan Druart from comment #8) > I need a confirmation here. > > The test plan says "Set CheckPrevCheckout to Do, or Do, with confirmation", > but it's "unless overriden, do". That means we should check the patron > category and patron's settings as well. > > Finally it sounds like this flag is set when the error/warning must be > displayed, and maybe the following code would be enough: > > } elsif ($confirmation eq 'PREVISSUE') { > $self->screen_msg("This item was previously checked out by you"); > last; > } > > Could someone confirm or tell me what I am missing? There is no setting on patron category here - so this is alright. The pref has 4 values that seem a little confusing. https://bywatersolutions.com/2017/10/09/koha-tutorial-check-previous-checkouts/ I feel the patch is correct.
(In reply to Katrin Fischer from comment #9) > (In reply to Jonathan Druart from comment #8) > > I need a confirmation here. > > > > The test plan says "Set CheckPrevCheckout to Do, or Do, with confirmation", > > but it's "unless overriden, do". That means we should check the patron > > category and patron's settings as well. > > > > Finally it sounds like this flag is set when the error/warning must be > > displayed, and maybe the following code would be enough: > > > > } elsif ($confirmation eq 'PREVISSUE') { > > $self->screen_msg("This item was previously checked out by you"); > > last; > > } > > > > Could someone confirm or tell me what I am missing? > > There is no setting on patron category here - so this is alright. The pref > has 4 values that seem a little confusing. No it's not confusing, it's a great way to behave (inheritance). The patron's category can set this value is pref is set to relevant value (softyes, softno). I still think the tests are not necessary, waiting for an answer from patch's author or testers.
You are right, the setting on category only appears when you set a 'do' setting. Which is a little confusing. :( The important bis is that it should not block the circulation when set to softyes/hardyes. So apart from the message, we need to do something about the flag that blocks?
(In reply to Jonathan Druart from comment #10) > I still think the tests are not necessary, waiting for an answer from > patch's author or testers. Consider this a friendly "ping". :-) Would be nice to have a fix for this.
The use case this patch was originally developed for was display a message if the item was previously issued, if the setting was either hard or soft yes. If you tell me what you're requiring, I'll be happy to fix it in whatever way you want. Cheers, Liz
They didn't care about blocking from the self check, at all.
Hi Liz, you check the preference only, but this can be overwritten for each patron category. I missed it at first too - you need to switch the pref to do. Also take a look at comment#8.
(In reply to Katrin Fischer from comment #15) > Hi Liz, you check the preference only, but this can be overwritten for each > patron category. I think that the possible override is already taken into account here: C4/SIP/ILS/Transaction/Checkout.pm "do_checkout" calls "_can_we_issue" which calls "CanBookBeIssued" which calls "wants_check_for_previous_checkout" which checks for sypref, patron, and patron category. No need to re-check, right? (In reply to Jonathan Druart from comment #8) > Finally it sounds like this flag is set when the error/warning must be displayed, and maybe the following code would be enough: > > } elsif ($confirmation eq 'PREVISSUE') { > $self->screen_msg("This item was previously checked out by you"); > last; > } I guess you're right, PREVISSUE is set after the syspref, patron and patron category were already checked by wants_check_for_previous_checkout in CanBookBeIssued, so no need to check again here. Imho, with Jonathan's modification, this patch is ok.
Created attachment 78567 [details] [review] Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: Set CheckPrevCheckout to Do, or Do, with confirmation Verify the SIP server is blocking checkouts with this setting set Apply this patch Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before.y Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Updated with Jonathan's modification.
Created attachment 78597 [details] [review] Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: - Do not apply this patch - Set CheckPrevCheckout to "Do", or "Unless overridden, do" Verify the SIP server is blocking checkouts with this setting set - Apply this patch - Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before.
Created attachment 89674 [details] [review] Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: - Do not apply this patch - Set CheckPrevCheckout to "Do", or "Unless overridden, do" Verify the SIP server is blocking checkouts with this setting set - Apply this patch - Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before. Signed-off-by: Liz Rea <wizzyrea@gmail.com>
QA: Looking here
> - Note that the checkout is now allowed, with a screen message telling > the borrower they've issued the item before. Not the item, but the biblio ;)
Created attachment 90216 [details] [review] Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer To test: - Do not apply this patch - Set CheckPrevCheckout to "Do", or "Unless overridden, do" Verify the SIP server is blocking checkouts with this setting set - Apply this patch - Note that the checkout is now allowed, with a screen message telling the borrower they've issued the item before. Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work! Pushed to master for 19.11.00
Pushed to 19.05.x for 19.05.01
backported to 18.11.x for 18.11.07
How is this system preference being checked? I don't see any code that directly references "CheckPrevCheckout"? I just see templates. I must be missing something painfully obvious, but I don't see it.
See comment #16, CheckPrevCheckout is checked by wants_check_for_previous_checkout in Koha/Patron.pm
(In reply to Matthias Meusburger from comment #28) > See comment #16, CheckPrevCheckout is checked by > wants_check_for_previous_checkout in Koha/Patron.pm Thanks, Matt. The reason I didn't see it before was that it checks "checkPrevCheckout" rather than "CheckPrevCheckout". My grep search was case sensitive and I totally missed it heh.
Hello, I have just dicovered this BZ and I'm really surprised and not agree this this choice. In our library, we want to prevent users who are using the selfcheckout system to checkout an item they have just check-in. They didn't really matter the warning and they can keep a book for all the time. For some title, there's a real pressure becaus they are recommanded by the teachers and we can't have one for all the students. We should have the choice to prevent or warn the check out at the selfchekout sytem and it's not the cas here if I have well understood. Sonia
So I guess we need a pref to control that. Sonia, can you open a new bug report?
Done : https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26591