Bug 25761

Summary: Implementation of too_many_overdue has unintended consequences
Product: Koha Reporter: Kyle M Hall <kyle>
Component: SIP2Assignee: Kyle M Hall <kyle>
Status: Pushed to master --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrew, cbrannon, colin.campbell, magnus, martin.renvoize, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Text to go in the release notes:
This bugfix allows the circulation rules that prevent checkouts if a user has reached a maximum number of overdue to be overridden at the SIP login level. This is especially useful for ebook lending services where you may want this block to be disabled. ***New SIP config option***: `overdues_block_checkout` defaults to `1`
Version(s) released in:
20.11.00
Bug Depends on: 24449    
Bug Blocks:    
Attachments: Bug 25761: Implementation of too_many_overdue has unintended consequences
Bug 25761: Implementation of too_many_overdue has unintended consequences
Bug 25761: Implementation of too_many_overdue has unintended consequences
Bug 25761: Implementation of too_many_overdue has unintended consequences
Bug 25761: (QA follow-up) Consistent flag names
Bug 25761: (QA follow-up) Consistent flag names
Bug 25761: Implementation of too_many_overdue has unintended consequences
Bug 25761: (QA follow-up) Consistent flag names

Description Kyle M Hall 2020-06-15 16:12:57 UTC
Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the patron status flags ). This has had unintended consequences as many library use SIP2 for validation to eresource vendors ( Overdrive, Hoopla, etc ), but do not want overdue checkouts to block the use of these resources.
Comment 1 Kyle M Hall 2020-06-15 16:31:23 UTC
Created attachment 105889 [details] [review]
Bug 25761: Implementation of too_many_overdue has unintended consequences

Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the
patron status flags ). This has had unintended consequences as
many library use SIP2 for validation to eresource vendors ( Overdrive,
Hoopla, etc ), but do not want overdue checkouts to block the
use of these resources.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Find a patron with overdue items
4) Run a SIP2 patron information request
5) Note the "too many overdue" flag is set
6) Set the new SIP config option "disable_too_many_overdue" for the
account you used for testing
7) Restart the SIP server
8) Run the same patron information request
9) Notice the "too many overdue" flag is no longer set!
Comment 2 Christopher Brannon 2020-06-15 20:59:21 UTC
Thank you for making it an option.  Our library WANTS sip users blocked when they have overdues.  Especially now that we don't charge fines.
Comment 3 Kyle M Hall 2020-09-08 11:08:05 UTC
Created attachment 109747 [details] [review]
Bug 25761: Implementation of too_many_overdue has unintended consequences

Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the
patron status flags ). This has had unintended consequences as
many library use SIP2 for validation to eresource vendors ( Overdrive,
Hoopla, etc ), but do not want overdue checkouts to block the
use of these resources.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Find a patron with overdue items
4) Run a SIP2 patron information request
5) Note the "too many overdue" flag is set
6) Set the new SIP config option "disable_too_many_overdue" for the
account you used for testing
7) Restart the SIP server
8) Run the same patron information request
9) Notice the "too many overdue" flag is no longer set!
Comment 4 Andrew Fuerste-Henry 2020-09-08 17:47:23 UTC
With patch applied, changing the flag in the SIP config file has no impact on response.
Comment 5 Kyle M Hall 2020-09-09 10:15:50 UTC
Created attachment 109786 [details] [review]
Bug 25761: Implementation of too_many_overdue has unintended consequences

Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the
patron status flags ). This has had unintended consequences as
many library use SIP2 for validation to eresource vendors ( Overdrive,
Hoopla, etc ), but do not want overdue checkouts to block the
use of these resources.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Find a patron with overdue items
4) Run a SIP2 patron information request
5) Note the "too many overdue" flag is set
6) Set the new SIP config option "disable_too_many_overdue" for the
account you used for testing
7) Restart the SIP server
8) Run the same patron information request
9) Notice the "too many overdue" flag is no longer set!
Comment 6 Martin Renvoize 2020-10-02 14:19:28 UTC
For consistency with bug 25541, I wonder if we ought to name the flag 'overdues_block_circ' or something along those lines?
Comment 7 Martin Renvoize 2020-10-02 14:21:16 UTC
As an aside, debian/templates/SIPconfig.xml is getting really out of sync these days.. I think there's a bug open to bring it back in sync.. but we should probably be catching cases where we add new config options and ensuring we add them to those templates too ;)
Comment 8 Victor Grousset/tuxayo 2020-11-12 20:27:34 UTC
Hi, not enough experience with SIP so I have two questions

> 5) Note the "too many overdue" flag is set

> READ: 24              00120201112    201630AEEdna Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha. |AOCPL|

Which one is it? I couldn't in the spec where is the patron status field.

> 6) Set the new SIP config option "disable_too_many_overdue" for the
> account you used for testing

Where does one do that?
Comment 9 Kyle M Hall 2020-11-13 11:53:54 UTC
(In reply to Victor Grousset/tuxayo from comment #8)
> Hi, not enough experience with SIP so I have two questions
> 
> > 5) Note the "too many overdue" flag is set
> 
> > READ: 24              00120201112    201630AEEdna Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha. |AOCPL|
> 
> Which one is it? I couldn't in the spec where is the patron status field.

It is the 6th character of the patron status flags, the 8th character of the entire message.

24              00120201112    201630AEEdna
       ^
       This one. Y means patron has too many overdues, blank means they don't.
> 
> > 6) Set the new SIP config option "disable_too_many_overdue" for the
> > account you used for testing
> 
> Where does one do that?

Find your SIPconfig.xml, usually in /etc/koha/sites/<instance>/SIPconfig.xml, then add as you can see here at the bottom of the page for the SIP account you want to use for testing: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=109786&action=diff
Comment 10 Martin Renvoize 2020-11-13 12:16:06 UTC
Created attachment 113595 [details] [review]
Bug 25761: Implementation of too_many_overdue has unintended consequences

Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the
patron status flags ). This has had unintended consequences as
many library use SIP2 for validation to eresource vendors ( Overdrive,
Hoopla, etc ), but do not want overdue checkouts to block the
use of these resources.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Find a patron with overdue items
4) Run a SIP2 patron information request
5) Note the "too many overdue" flag is set
6) Set the new SIP config option "disable_too_many_overdue" for the
account you used for testing
7) Restart the SIP server
8) Run the same patron information request
9) Notice the "too many overdue" flag is no longer set!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-11-13 12:16:10 UTC
Created attachment 113596 [details] [review]
Bug 25761: (QA follow-up) Consistent flag names

This follow-up updates the config flag to be consistent with bug 24449
and updates the debian template file to match too.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Kyle M Hall 2020-11-13 12:54:33 UTC
(In reply to Martin Renvoize from comment #11)
> Created attachment 113596 [details] [review] [review]
> Bug 25761: (QA follow-up) Consistent flag names
> 
> This follow-up updates the config flag to be consistent with bug 24449
> and updates the debian template file to match too.
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Thanks for the followup Martin!
Comment 13 Kyle M Hall 2020-11-13 12:55:34 UTC
Created attachment 113598 [details] [review]
Bug 25761: (QA follow-up) Consistent flag names

This follow-up updates the config flag to be consistent with bug 24449
and updates the debian template file to match too.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Victor Grousset/tuxayo 2020-11-13 16:54:19 UTC
(In reply to Kyle M Hall from comment #9)
> It is the 6th character of the patron status flags, the 8th character of the
> entire message.

READ: 24              00120201113    154013AEEdna Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha. |AOCPL|

overdue item +  OverduesBlockCirc == "Block" + add disable_too_many_overdue="0" to account koha + patches + restart_all

READ: 24Y             00120201113    163742AEEdna Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha.  -- Patron has overdues|AOCPL|


Somehow I get the charge privileges denied flag, is that expected?
And I don't get the disable_too_many_overdue flag.

Is the right config file this one? /etc/koha/sites/kohadev/SIPconfig.xml
(in koha-testing-docker)
Comment 15 Martin Renvoize 2020-11-13 18:28:32 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> (In reply to Kyle M Hall from comment #9)
> > It is the 6th character of the patron status flags, the 8th character of the
> > entire message.
> 
> READ: 24              00120201113    154013AEEdna
> Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha. |AOCPL|
> 
> overdue item +  OverduesBlockCirc == "Block" + add
> disable_too_many_overdue="0" to account koha + patches + restart_all
> 
> READ: 24Y             00120201113    163742AEEdna
> Acosta|AA23529001000463|BLY|CQN|AFGreetings from Koha.  -- Patron has
> overdues|AOCPL|
> 
> 
> Somehow I get the charge privileges denied flag, is that expected?
> And I don't get the disable_too_many_overdue flag.
> 
> Is the right config file this one? /etc/koha/sites/kohadev/SIPconfig.xml
> (in koha-testing-docker)

Sorry Victor, I should have made that clearer. My followup changes "disable_too_many_overdues" to "overdues_block_circ" to make it more consistent with some other configs we've introduced.

When enabled (the default) having too many overdues  (according to the circulation rules) should set that return flag to 'Y' as your seeing.. if you disable it by passing in that config Param with '0' then you should not see the 'Y'.

You are looking at the right file, but only if that after having run make to refresh it from the templates in the source directories.

Hope that helps
Comment 16 Victor Grousset/tuxayo 2020-11-13 19:45:22 UTC
Created attachment 113630 [details] [review]
Bug 25761: Implementation of too_many_overdue has unintended consequences

Bug 24449 implemented the too_many_overdue SIP flag ( position 6 of the
patron status flags ). This has had unintended consequences as
many library use SIP2 for validation to eresource vendors ( Overdrive,
Hoopla, etc ), but do not want overdue checkouts to block the
use of these resources.

Test Plan:
1) Apply this patch
2) Restart all the things!
3) Find a patron with overdue items
4) Run a SIP2 patron information request
5) Note the "too many overdue" flag is set
6) Set the new SIP config option "disable_too_many_overdue" for the
account you used for testing
7) Restart the SIP server
8) Run the same patron information request
9) Notice the "too many overdue" flag is no longer set!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2020-11-13 19:45:26 UTC
Created attachment 113631 [details] [review]
Bug 25761: (QA follow-up) Consistent flag names

This follow-up updates the config flag to be consistent with bug 24449
and updates the debian template file to match too.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2020-11-13 19:45:53 UTC
> having too many overdues  (according to the circulation rules)

Ok!!!!

Wait which setting of the circulation rules? I can't find it.

Anyway, I managed to get the expected result with OverduesBlockCirc



Thanks I learned useful stuff about SIP and the config files :)

BTW why do we have lpl-sc-beacock user in the config file if it's not in the sample data?
Comment 19 Martin Renvoize 2020-11-14 08:05:29 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > having too many overdues  (according to the circulation rules)
> 
> Ok!!!!
> 
> Wait which setting of the circulation rules? I can't find it.
> 
> Anyway, I managed to get the expected result with OverduesBlockCirc
> 
> 
> 
> Thanks I learned useful stuff about SIP and the config files :)
> 
> BTW why do we have lpl-sc-beacock user in the config file if it's not in the
> sample data?

Oops, indeed, it's OverduesBlockCirc syspref, my mistake.

No idea about the user in the config.. likely a legacy thing.
Comment 20 Victor Grousset/tuxayo 2020-11-14 10:43:09 UTC
> No idea about the user in the config.. likely a legacy thing.

Shall I do a followup and swap it with the koha user? or maybe with koha_sip (login term1)
As we add config to lpl-sc-beacock, it might as well match an account of sample data.
Comment 21 Jonathan Druart 2020-11-16 12:49:53 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 22 Andrew Fuerste-Henry 2020-12-08 17:55:58 UTC
Merge conflicts for 20.05.x, please rebase if needed