Bug 19457 - If CheckPrevCheckout is set to "Do", then checkouts are blocked at the SIPServer
Summary: If CheckPrevCheckout is set to "Do", then checkouts are blocked at the SIPServer
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 6906
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-13 09:44 UTC by Jonathan Field
Modified: 2019-06-14 20:42 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer (1.34 KB, patch)
2018-02-25 20:37 UTC, Liz Rea
Details | Diff | Splinter Review
Signed off amended patch (1.49 KB, patch)
2018-02-26 15:24 UTC, Colin Campbell
Details | Diff | Splinter Review
revision to signed off patch (1.50 KB, patch)
2018-03-13 12:44 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer (1.57 KB, patch)
2018-04-09 11:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19457: If CheckPrevCheckout is set to Do, then checkouts are blocked at the SIPServer (1.34 KB, patch)
2018-09-11 19:02 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) (1.36 KB, patch)
2018-09-12 16:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) (1.40 KB, patch)
2019-05-13 16:51 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 19457: Handle 'PREVISSUE' return by CanBookBeIssued from SIP (CheckPrevCheckout) (1.50 KB, patch)
2019-05-31 07:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Field 2017-10-13 09:44:07 UTC
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.
Comment 1 Liz Rea 2018-02-25 20:37:58 UTC
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
Comment 2 Colin Campbell 2018-02-26 15:24:37 UTC
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
Comment 3 Katrin Fischer 2018-03-12 15:35:27 UTC
Is this specific to 16.11?
Comment 4 Colin Campbell 2018-03-12 16:08:07 UTC
(In reply to Katrin Fischer from comment #3)
> Is this specific to 16.11?

No I think that is 16.11 and greater
Comment 5 Katrin Fischer 2018-03-12 22:01:00 UTC
Thx Colin, changing to master (bugs get backported from new to old)
Comment 6 Colin Campbell 2018-03-13 12:44:59 UTC
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
Comment 7 Kyle M Hall 2018-04-09 11:18:54 UTC
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>
Comment 8 Jonathan Druart 2018-04-10 18:35:44 UTC
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?
Comment 9 Katrin Fischer 2018-04-12 07:58:01 UTC
(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.
Comment 10 Jonathan Druart 2018-04-12 13:27:42 UTC
(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.
Comment 11 Katrin Fischer 2018-04-12 13:31:18 UTC
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?
Comment 12 Magnus Enger 2018-04-23 20:52:35 UTC
(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.
Comment 13 Liz Rea 2018-04-23 21:14:36 UTC
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
Comment 14 Liz Rea 2018-04-23 21:15:07 UTC
They didn't care about blocking from the self check, at all.
Comment 15 Katrin Fischer 2018-04-23 21:36:01 UTC
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.
Comment 16 Matthias Meusburger 2018-08-22 09:55:15 UTC
(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.
Comment 17 Matthias Meusburger 2018-09-11 19:02:37 UTC
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>
Comment 18 Matthias Meusburger 2018-09-11 19:03:45 UTC
Updated with Jonathan's modification.
Comment 19 Jonathan Druart 2018-09-12 16:43:27 UTC
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.
Comment 20 Liz Rea 2019-05-13 16:51:43 UTC
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>
Comment 21 Marcel de Rooy 2019-05-31 07:12:27 UTC
QA: Looking here
Comment 22 Marcel de Rooy 2019-05-31 07:32:14 UTC
> - 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 ;)
Comment 23 Marcel de Rooy 2019-05-31 07:33:01 UTC
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>
Comment 24 Martin Renvoize 2019-06-04 09:07:59 UTC
Nice work!

Pushed to master for 19.11.00
Comment 25 Fridolin SOMERS 2019-06-10 14:22:26 UTC
Pushed to 19.05.x for 19.05.01
Comment 26 Lucas Gass 2019-06-14 20:42:21 UTC
backported to 18.11.x for 18.11.07