Bug 35386 - Add ability to configure renewal branch when not specified in API request
Summary: Add ability to configure renewal branch when not specified in API request
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-11-22 16:34 UTC by Nick Clemens
Modified: 2024-01-25 15:31 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35386: Add RESTAPIRenewalBranch system preference (10.43 KB, patch)
2023-11-27 16:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35386: Add RESTAPIRenewalBranch system preference (10.43 KB, patch)
2023-11-27 16:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 35386: Add RESTAPIRenewalBranch system preference (10.47 KB, patch)
2024-01-22 14:39 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35386: Add RESTAPIRenewalBranch system preference (16.61 KB, patch)
2024-01-25 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-11-22 16:34:56 UTC
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.
Comment 1 Nick Clemens 2023-11-27 16:41:15 UTC
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
Comment 2 Nick Clemens 2023-11-27 16:44:49 UTC
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
Comment 3 Brendan Lawlor 2023-12-11 14:34:59 UTC
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
Comment 4 Tomás Cohen Arazi 2023-12-11 18:47:40 UTC
(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.
Comment 5 Tomás Cohen Arazi 2023-12-11 20:40:01 UTC
(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.
Comment 6 Brendan Lawlor 2023-12-13 21:47:09 UTC
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.
Comment 7 Nick Clemens 2024-01-19 19:33:08 UTC
(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
Comment 8 Nick Clemens 2024-01-19 19:33:47 UTC
(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
Comment 9 Brendan Lawlor 2024-01-22 14:39:21 UTC
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>
Comment 10 Martin Renvoize 2024-01-25 15:31:20 UTC
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>
Comment 11 Martin Renvoize 2024-01-25 15:31:58 UTC
Tidied and fixed QA complaints inline.. all working as expected

Passing QA