Bug 30232 - Acquisition logged in user performance
Summary: Acquisition logged in user performance
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 7923
  Show dependency treegraph
 
Reported: 2022-03-05 01:08 UTC by Fridolin Somers
Modified: 2022-09-03 08:27 UTC (History)
1 user (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 30232: Logged in user performance for CanUserManageBasket() (2.71 KB, patch)
2022-03-05 02:03 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30232: Logged in user performance for CanUserManageBasket() (3.59 KB, patch)
2022-03-05 02:26 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30232: Logged in user performance for CanUserUseBudget() (6.79 KB, patch)
2022-03-05 02:50 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30232: Logged in user performance for CanUserManageBasket() (3.59 KB, patch)
2022-03-05 06:21 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 30232: Logged in user performance for CanUserUseBudget() and CanUserModifyBudget() (16.92 KB, patch)
2022-03-05 06:22 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2022-03-05 01:08:46 UTC
Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.
Comment 1 Fridolin Somers 2022-03-05 02:03:55 UTC
Created attachment 131391 [details] [review]
Bug 30232: Logged in user performance for CanUserManageBasket()

Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.

This patch optimises call for CanUserManageBasket().

You may benchmark on a vendor with a lot of baskets.

Test plan
1)
1.1) Log in to staff interface as user A with all acquisition permissions
1.2) Set system preference 'AcqViewBaskets' to 'created or managed by staff member'
1.3) Create a new basket
1.4) Copy URL, ie /cgi-bin/koha/acqui/basket.pl?basketno=2
1.5) Click on 'Baskets' tab, you see the basket in table
2)
2.1) Log in to staff interface as user B with acquisition permissions
   but not 'order_manage_all'
2.2) Go to URL => You don't have access to basket
2.3) Click on 'Baskets' tab => you don't see the basket in table
Comment 2 Fridolin Somers 2022-03-05 02:26:22 UTC
Created attachment 131392 [details] [review]
Bug 30232: Logged in user performance for CanUserManageBasket()

Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.

This patch optimises call for CanUserManageBasket().
Also removes a useless refetch of logged in patron.

You may benchmark on a vendor with a lot of baskets.

Test plan
1)
1.1) Log in to staff interface as user A with all acquisition permissions
1.2) Set system preference 'AcqViewBaskets' to 'created or managed by staff member'
1.3) Create a new basket
1.4) Copy URL, ie /cgi-bin/koha/acqui/basket.pl?basketno=2
1.5) Click on 'Baskets' tab, you see the basket in table
2)
2.1) Log in to staff interface as user B with acquisition permissions
   but not 'order_manage_all'
2.2) Go to URL => You don't have access to basket
2.3) Click on 'Baskets' tab => you don't see the basket in table
Comment 3 Fridolin Somers 2022-03-05 02:34:52 UTC
Patch "Logged in user performance for CanUserManageBasket()"

Page /cgi-bin/koha/acqui/booksellers.pl tested with 3000 baskets.
Time goes from 30s to 20s.
Comment 4 Fridolin Somers 2022-03-05 02:50:44 UTC
Created attachment 131393 [details] [review]
Bug 30232: Logged in user performance for CanUserUseBudget()

Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.

This patch optimises call for CanUserUseBudget();

You may benchmark on a user with a lot of budgets acess.

Test plan :
Play with impacted pages
and check budgets list is the same than wihtout patch
Comment 5 Fridolin Somers 2022-03-05 06:21:56 UTC
Created attachment 131395 [details] [review]
Bug 30232: Logged in user performance for CanUserManageBasket()

Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.

This patch optimises call for CanUserManageBasket().
Also removes a useless refetch of logged in patron.

You may benchmark on a vendor with a lot of baskets.

Test plan
1)
1.1) Log in to staff interface as user A with all acquisition permissions
1.2) Set system preference 'AcqViewBaskets' to 'created or managed by staff member'
1.3) Create a new basket
1.4) Copy URL, ie /cgi-bin/koha/acqui/basket.pl?basketno=2
1.5) Click on 'Baskets' tab, you see the basket in table
2)
2.1) Log in to staff interface as user B with acquisition permissions
   but not 'order_manage_all'
2.2) Go to URL => You don't have access to basket
2.3) Click on 'Baskets' tab => you don't see the basket in table
Comment 6 Fridolin Somers 2022-03-05 06:22:00 UTC
Created attachment 131396 [details] [review]
Bug 30232: Logged in user performance for CanUserUseBudget() and CanUserModifyBudget()

Some acquisition methods can take borrowernumber or a Koha::Patron->unblessed.
When called with logged in user, sometimes in a loop, performance will be better if fetching Koha::Patron of logged in user only once.

This patch optimises call for CanUserUseBudget() and CanUserModifyBudget();

You may benchmark on a user with a lot of budgets acess.

Test plan :
Play with impacted pages
and check budgets list is the same than wihtout patch
Comment 7 Fridolin Somers 2022-03-05 06:45:06 UTC
/cgi-bin/koha/acqui/acqui-home.pl with 300 funds :
From 13 seconds to 10 seconds
Comment 8 Joonas Kylmälä 2022-09-03 08:27:50 UTC
I think it would be better if you change those C4/Budgets.pm functions accept directly a Koha::Patron, reading the code there is now a lot of duplication regarding the unblessing of the patron. Haven't checked all cases but if you check the code for CanUserUseBudget it's perfectly fine to just use the Koha::Patron methods, just remove the curly braces from the current code basically to make it work. I'd like to mark this as FQA if you don't mind.