Bug 18861

Summary: branch_transfer_limits needs keys
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: System AdministrationAssignee: Fridolin Somers <fridolin.somers>
Status: Failed QA --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: alexbuckley, gmcharlt, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26633
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7923    
Bug Blocks:    
Attachments: Bug 18861 - branch_transfer_limits needs keys
Bug 18861 - branch_transfer_limits needs keys
Bug 18861 - branch_transfer_limits needs keys

Description Fridolin Somers 2017-06-27 12:50:56 UTC
The branch transfers limits are edited in : /cgi-bin/koha/admin/branch_transfer_limits.pl
This page calls a loop on IsBranchTransferAllowed with all destination branches and codes (ccodes or itemtypes).

This can takes a huge time because table branch_transfer_limits has no keys.
Comment 1 Fridolin Somers 2017-06-27 13:04:18 UTC
Created attachment 64676 [details] [review]
Bug 18861 - branch_transfer_limits needs keys

The branch transfers limits are edited in : /cgi-bin/koha/admin/branch_transfer_limits.pl
This page calls a loop on IsBranchTransferAllowed with all destination branches and codes (ccodes or itemtypes).

This can takes a huge time because table branch_transfer_limits has no keys.

Since ccode or itemtype may be used, this patch adds a key on both combinaison : ccode/fromBranch/toBranch and itemtype/fromBranch/toBranch.

Tested on a table with 66000 entries, time whent from 26 second to less than 1 second.

Test plan :
- Go to Administration > Library transfer limits
- Check page load is faster
- Uncheck a checkbox
- Save
=> Verify save is OK
- Change branch and same tests
Comment 2 Alex Buckley 2017-07-01 22:02:30 UTC
Created attachment 64764 [details] [review]
Bug 18861 - branch_transfer_limits needs keys

The branch transfers limits are edited in : /cgi-bin/koha/admin/branch_transfer_limits.pl
This page calls a loop on IsBranchTransferAllowed with all destination branches and codes (ccodes or itemtypes).

This can takes a huge time because table branch_transfer_limits has no keys.

Since ccode or itemtype may be used, this patch adds a key on both combinaison : ccode/fromBranch/toBranch and itemtype/fromBranch/toBranch.

Tested on a table with 66000 entries, time whent from 26 second to less than 1 second.

Test plan :
- Go to Administration > Library transfer limits
- Check page load is faster
- Uncheck a checkbox
- Save
=> Verify save is OK
- Change branch and same tests

Followed test plan which worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 3 Alex Buckley 2017-07-01 22:05:32 UTC
Hi Frindolin 

Your patch works well, one thing I noticed was that it would be logical for the checkbox of the current branch not to be displayed.

i.e. if I am looking at the Fairfield checkin and transfer policy then the fairfield checkbox should not be displayed because when you uncheck the fairfield box then save the page it reloads with fairfield checked again.

Perhaps this could be a future patch :)
Comment 4 Tomás Cohen Arazi 2017-07-03 17:40:55 UTC
What is the status of this bug? Signed off?
Comment 5 Fridolin Somers 2017-07-06 09:20:12 UTC
(In reply to Alex Buckley from comment #3)
> Perhaps this could be a future patch :)
Indeed, another bug report.
Comment 6 Fridolin Somers 2020-11-16 15:07:40 UTC
Back to signed off
Comment 7 Nick Clemens (kidclamp) 2020-12-23 16:25:20 UTC
Created attachment 114686 [details] [review]
Bug 18861 - branch_transfer_limits needs keys

The branch transfers limits are edited in : /cgi-bin/koha/admin/branch_transfer_limits.pl
This page calls a loop on IsBranchTransferAllowed with all destination branches and codes (ccodes or itemtypes).

This can takes a huge time because table branch_transfer_limits has no keys.

Since ccode or itemtype may be used, this patch adds a key on both combinaison : ccode/fromBranch/toBranch and itemtype/fromBranch/toBranch.

Tested on a table with 66000 entries, time whent from 26 second to less than 1 second.

Test plan :
- Go to Administration > Library transfer limits
- Check page load is faster
- Uncheck a checkbox
- Save
=> Verify save is OK
- Change branch and same tests

Followed test plan which worked as intended
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 8 Nick Clemens (kidclamp) 2020-12-23 16:26:16 UTC
I updated the patch, but in testing I didn't see any difference.

I added many types and branches:
INSERT INTO itemtypes (itemtype,description) SELECT CONCAT(itemtype,"A"),CONCAT(description,"A") FROM itemtypes;
INSERT INTO itemtypes (itemtype,description) SELECT CONCAT(itemtype,"B"),CONCAT(description,"B") FROM itemtypes;
INSERT INTO itemtypes (itemtype,description) SELECT CONCAT(itemtype,"C"),CONCAT(description,"C") FROM itemtypes;
INSERT INTO itemtypes (itemtype,description) SELECT CONCAT(itemtype,"D"),CONCAT(description,"D") FROM itemtypes;
INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"A"),CONCAT(branchname,"A"),pickup_location FROM branches;
INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"B"),CONCAT(branchname,"B"),pickup_location FROM branches;
INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"C"),CONCAT(branchname,"C"),pickup_location FROM branches;
INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;

Before and after the indexes it took about 30 seconds for the page to load
Comment 9 Fridolin Somers 2020-12-28 14:52:57 UTC
(In reply to Nick Clemens from comment #8)
> I updated the patch, but in testing I didn't see any difference.
> 
> I added many types and branches:
> INSERT INTO itemtypes (itemtype,description) SELECT
> CONCAT(itemtype,"A"),CONCAT(description,"A") FROM itemtypes;
> INSERT INTO itemtypes (itemtype,description) SELECT
> CONCAT(itemtype,"B"),CONCAT(description,"B") FROM itemtypes;
> INSERT INTO itemtypes (itemtype,description) SELECT
> CONCAT(itemtype,"C"),CONCAT(description,"C") FROM itemtypes;
> INSERT INTO itemtypes (itemtype,description) SELECT
> CONCAT(itemtype,"D"),CONCAT(description,"D") FROM itemtypes;
> INSERT INTO branches (branchcode,branchname,pickup_location) SELECT
> CONCAT(branchcode,"A"),CONCAT(branchname,"A"),pickup_location FROM branches;
> INSERT INTO branches (branchcode,branchname,pickup_location) SELECT
> CONCAT(branchcode,"B"),CONCAT(branchname,"B"),pickup_location FROM branches;
> INSERT INTO branches (branchcode,branchname,pickup_location) SELECT
> CONCAT(branchcode,"C"),CONCAT(branchname,"C"),pickup_location FROM branches;
> INSERT INTO branches (branchcode,branchname,pickup_location) SELECT
> CONCAT(branchcode,"D"),CONCAT(branchname,"D"),pickup_location FROM branches;
> 
> Before and after the indexes it took about 30 seconds for the page to load

How many entries do you have in table branch_transfer_limits ?