Bug 35386

Created attachment 159278 [details] [review]
Bug 35386: Add RESTAPIRenewalBranch system preference

This patch adds a new system prefernce, RESTAPIRenewalBranch, analogous to the existing OpacRenewalBranch system preference.

The preference allows choosing how the renewal branch is recorded in the statistics table.
In order ot preserve existing behaviour, the default is to use the api user's branch.

To test:
1 - Checkout some items to a patron
2 - Add an API user account with circulation permissions and a different homebranch
3 - POST a renewal to: http://localhost:8080/api/v1/checkouts/{checkout_id}/renewal
4 - Check statistics table and confirm the api users branch was used
5 - Apply patches, restart all
6 - Repeat API renewal, confirm same branch used
7 - Change the RESTAPIRenewal syspref
8 - Repeat API renewal and confirm specified branch is used
9 - Confirm the syspref works for all settings

Created attachment 159282 [details] [review]
Bug 35386: Add RESTAPIRenewalBranch system preference

This patch adds a new system prefernce, RESTAPIRenewalBranch, analogous to the existing OpacRenewalBranch system preference.

The preference allows choosing how the renewal branch is recorded in the statistics table.
In order ot preserve existing behaviour, the default is to use the api user's branch.

To test:
1 - Checkout some items to a patron
2 - Add an API user account with circulation permissions and a different homebranch
3 - POST a renewal to: http://localhost:8080/api/v1/checkouts/{checkout_id}/renewal
4 - Check statistics table and confirm the api users branch was used
5 - Apply patches, restart all
6 - Repeat API renewal, confirm same branch used
7 - Change the RESTAPIRenewal syspref
8 - Repeat API renewal and confirm specified branch is used
9 - Confirm the syspref works for all settings

When I tested this out I confirmed before the patch the branch recorded for the renewal is the API user's homebranch.

When I applied the patch I noticed the system preference RESTAPIRenewalBranch defaults to 'APIRenew' and when I made a post to {{base_url}}/api/v1/checkouts/1/renewal
the branch recorded in the statistics table as 'API Renew' According to the test plan it should default to the branch of the api user.


Also one of the options for RESTAPIRenewalBranch is NULL, which did work to record the branch in the statistics as NULL, but I'm not sure if that is an intended option.

The other options I tested worked as expected for:
the branch of the api user
the library the item was checked out from
the items home library
NULL
the patron's home library

(In reply to Brendan Lawlor from comment #3)
> Also one of the options for RESTAPIRenewalBranch is NULL, which did work to
> record the branch in the statistics as NULL, but I'm not sure if that is an
> intended option.

That's a good question! I'd like to mention that the author provided tests for that particular use case so I assume it is what they wanted it to be like.

(In reply to Nick Clemens from comment #0)
> When renewing items via the API Koha records the branch of the renewal as
> the branch of the account that is signed in to the API.
> 
> For outside service (Aspen, Communico) they may have a single API account
> that represents all user interactions.
> 
> While this behavior may not be technically incorrect, it should be
> configurable like OpacRenewalBranch - allowing the library to decide which
> branch is recorded in the statistics table.

We can currently specify the current branch by using the `x-koha-library` header (available since bug 24003 => v19.11.07):

https://api.koha-community.org/Development.html#section/Special-headers

I think decoupling the default from the OPAC would be correct, though. So I'd rename the bug.

Out of curiosity, I tested this when also using the header x-koha-library. 

The library specified in the header takes precedence over the system preference so  it won't negatively affect any implementations that use that header.

I think it's a good idea to have the default configurable by the library with the setting in addition to being able to use the header so I think this is the correct behavior.

(In reply to Brendan Lawlor from comment #3)
> When I tested this out I confirmed before the patch the branch recorded for
> the renewal is the API user's homebranch.
> 
> When I applied the patch I noticed the system preference
> RESTAPIRenewalBranch defaults to 'APIRenew' and when I made a post to
> {{base_url}}/api/v1/checkouts/1/renewal
> the branch recorded in the statistics table as 'API Renew' According to the
> test plan it should default to the branch of the api user.

I only see this if I look at the preference before running updatedatabase
If the update has not been run to set the default then the interface will display the first option in the list

> Also one of the options for RESTAPIRenewalBranch is NULL, which did work to
> record the branch in the statistics as NULL, but I'm not sure if that is an
> intended option.

This was intended to match the OPacRenewalBranch options

(In reply to Tomás Cohen Arazi from comment #5)
> I think decoupling the default from the OPAC would be correct, though. So
> I'd rename the bug.

Renamed


(In reply to Brendan Lawlor from comment #6)
> I think it's a good idea to have the default configurable by the library
> with the setting in addition to being able to use the header so I think this
> is the correct behavior.

Agreed :-)


This should be ready for sign off

Created attachment 161244 [details] [review]
Bug 35386: Add RESTAPIRenewalBranch system preference

This patch adds a new system prefernce, RESTAPIRenewalBranch, analogous to the existing OpacRenewalBranch system preference.

The preference allows choosing how the renewal branch is recorded in the statistics table.
In order ot preserve existing behaviour, the default is to use the api user's branch.

To test:
1 - Checkout some items to a patron
2 - Add an API user account with circulation permissions and a different homebranch
3 - POST a renewal to: http://localhost:8080/api/v1/checkouts/{checkout_id}/renewal
4 - Check statistics table and confirm the api users branch was used
5 - Apply patches, restart all
6 - Repeat API renewal, confirm same branch used
7 - Change the RESTAPIRenewal syspref
8 - Repeat API renewal and confirm specified branch is used
9 - Confirm the syspref works for all settings

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Created attachment 161428 [details] [review]
Bug 35386: Add RESTAPIRenewalBranch system preference

This patch adds a new system prefernce, RESTAPIRenewalBranch, analogous to the existing OpacRenewalBranch system preference.

The preference allows choosing how the renewal branch is recorded in the statistics table.
In order ot preserve existing behaviour, the default is to use the api user's branch.

To test:
1 - Checkout some items to a patron
2 - Add an API user account with circulation permissions and a different homebranch
3 - POST a renewal to: http://localhost:8080/api/v1/checkouts/{checkout_id}/renewal
4 - Check statistics table and confirm the api users branch was used
5 - Apply patches, restart all
6 - Repeat API renewal, confirm same branch used
7 - Change the RESTAPIRenewal syspref
8 - Repeat API renewal and confirm specified branch is used
9 - Confirm the syspref works for all settings

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Tidied and fixed QA complaints inline.. all working as expected

Passing QA

Please all, keep terminology in mind. Small fixes made inline:
* api => API
* branch > library (in system preference description at least)

Pushed for 24.05!

Well done everyone, thank you!

Not backported to 23.11.x