Bug 18861 - branch_transfer_limits needs keys
Summary: branch_transfer_limits needs keys
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 7923
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-27 12:50 UTC by Fridolin Somers
Modified: 2021-01-21 11:20 UTC (History)
4 users (show)

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


Attachments
Bug 18861 - branch_transfer_limits needs keys (2.14 KB, patch)
2017-06-27 13:04 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 18861 - branch_transfer_limits needs keys (2.25 KB, patch)
2017-07-01 22:02 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18861 - branch_transfer_limits needs keys (3.15 KB, patch)
2020-12-23 16:25 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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 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 ?