[Bug 26963] New: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Bug ID: 26963 Summary: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #1 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 113282 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113282&action=edit Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups. Test Plan: 1) Create a record with 100 items 2) Create 100 holds that are item level 3) You should start to see slow page loads as you create more holds 4) Apply this patch 5) Restart all the things! 6) Note the hold page now loads faster 7) Subsequent loads should be even faster due to the caching mechanism -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113282|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 113283 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113283&action=edit Bug 26963: Koha::Template::Plugin::Branches::pickup_locations is very inefficient, causing timeouts on records with large numbers of holds/items If a record has a large number of holds ( > 50 I'd say ), and a large number of items, checking the pickup locations takes so long the script often times out. We need to find a way to cache the results so we don't have to spend so much time on expensive lookups. Test Plan: 1) Create a record with 100 items 2) Create 100 holds that are item level 3) You should start to see slow page loads as you create more holds 4) Apply this patch 5) Restart all the things! 6) Note the hold page now loads faster 7) Subsequent loads should be even faster due to the caching mechanism -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #3 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Just to understand : $cache_key = "I" . $item->homebranch . "-" . $patron->branchcode The use of homebranch and branchcode in key are logic on actual code in Koha::Items::pickup_locations(). But this code may change... Couldn't we cache inside Koha::Items::pickup_locations() ? -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #4 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113329 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113329&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113329|0 |1 is obsolete| | --- Comment #5 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113330 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113330&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113330|0 |1 is obsolete| | --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113333 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113333&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113333|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113339 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113339&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113340 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113340&action=edit Bug 26963: Cache pickup locations for biblio level holds in the template When determining pickup locations for holds that have not specified an itemnumber we will get the same results for each hold. This patch simply places the list of libraries into a template variable and reuses it for each hold To test: 1 - Have several title level holds with varying pickup locations 2 - Have some libraries that are not pickup locations 3 - View the holds table for the biblio with the holds 4 - Observe the dropdowns load correctly 5 - Apply patch 6 - Verify there are no changes -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Attachment #113283|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |nick@bywatersolutions.com |ity.org | Priority|P5 - low |P3 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26988 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26988 [Bug 26988] Don't load hold pickup locations unless dropdown is selected -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113339|0 |1 is obsolete| | --- Comment #9 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 113419 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113419&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113340|0 |1 is obsolete| | --- Comment #10 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 113420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113420&action=edit Bug 26963: Cache pickup locations for biblio level holds in the template When determining pickup locations for holds that have not specified an itemnumber we will get the same results for each hold. This patch simply places the list of libraries into a template variable and reuses it for each hold To test: 1 - Have several title level holds with varying pickup locations 2 - Have some libraries that are not pickup locations 3 - View the holds table for the biblio with the holds 4 - Observe the dropdowns load correctly 5 - Apply patch 6 - Verify there are no changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113419|0 |1 is obsolete| | --- Comment #11 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113422&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113420|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113423 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113423&action=edit Bug 26963: Cache pickup locations for biblio level holds in the template When determining pickup locations for holds that have not specified an itemnumber we will get the same results for each hold. This patch simply places the list of libraries into a template variable and reuses it for each hold To test: 1 - Have several title level holds with varying pickup locations 2 - Have some libraries that are not pickup locations 3 - View the holds table for the biblio with the holds 4 - Observe the dropdowns load correctly 5 - Apply patch 6 - Verify there are no changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113422|0 |1 is obsolete| | --- Comment #13 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 113424 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113424&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #14 from Andrew Fuerste-Henry <andrew@bywatersolutions.com> --- Created attachment 113425 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113425&action=edit Bug 26963: Cache pickup locations for biblio level holds in the template When determining pickup locations for holds that have not specified an itemnumber we will get the same results for each hold. This patch simply places the list of libraries into a template variable and reuses it for each hold To test: 1 - Have several title level holds with varying pickup locations 2 - Have some libraries that are not pickup locations 3 - View the holds table for the biblio with the holds 4 - Observe the dropdowns load correctly 5 - Apply patch 6 - Verify there are no changes Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This appears to work in testing.. but a few things worry me slightly. I'm not seeing great unit tests for the branchlimits handling of the pickup_locations method and as we've not got new tests for the introduced private method I'd have to fail QA on that front. As this fixes a bug I think we should write the tests to prove branchlimits side and probably go with the code as is.. however, as I dug into it, the code feels a bit manic in this area I feel like a bit of golfing down the line would be great. I'd love to see the methods that are currently returning arrayrefs of Koha::Objects actually return proper Koha::Object sets which could then be chained properly. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113423|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 AspenCat Team <aspencatteam@clicweb.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aspencatteam@clicweb.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113424|0 |1 is obsolete| | Attachment #113425|0 |1 is obsolete| | --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113451 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113451&action=edit Bug 26963: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113452 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113452&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113451|0 |1 is obsolete| | Attachment #113452|0 |1 is obsolete| | --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113453 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113453&action=edit Bug 26963: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113454 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113454&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #20 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113455 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113455&action=edit Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #21 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Martin Renvoize from comment #15)
I'm not seeing great unit tests for the branchlimits handling of the pickup_locations method
Tests added
As this fixes a bug I think we should write the tests to prove branchlimits side and probably go with the code as is.. however, as I dug into it, the code feels a bit manic in this area I feel like a bit of golfing down the line would be great.
I'd love to see the methods that are currently returning arrayrefs of Koha::Objects actually return proper Koha::Object sets which could then be chained properly.
I agree strongly, but would like to see this on a new bug, and happy to help on that front. This bug is a major issue and causing severe problems for our larger consortia and I would love to see it move forward -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=26988 Blocks|26988 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26988 [Bug 26988] Don't load hold pickup locations unless dropdown is selected -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org Depends on| |22284 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22284 [Bug 22284] Add ability to define groups of locations for hold pickup -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #22 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Martin Renvoize from comment #15)
I'd love to see the methods that are currently returning arrayrefs of Koha::Objects actually return proper Koha::Object sets which could then be chained properly.
I don't understand. Are you suggesting that Koha::Item->pickup_locations should return the Koha::Item object? If it's a "getter" method I see no reason not to return what you had asked for :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113480 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113480&action=edit Bug 26963: [GOLF] Convert to ResultSets This patch removes the previously introduced private method by converting the arrayref returns to ResultSets appropriately and inlining the filter search queries. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113481 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113481&action=edit Bug 26963: [GOLF] Migrate unit tests into pickup_location We wrote unit tests for _can_pickup_locations as part of this patchset, but then I inlined the method whilst golfing. This patch moves those tests into the existing pickup_locations test so we more thoroughly cover the case where branch transfer limits are in play. NOTE: The tests all assume that all items have an effective_itemtype and ccode. I'm pretty sure items all have a type at this point, but I'm less sure we enforce collection codes? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- OK, So golfing was the best way for me to wrap my head around the code here :). I intend to move those GOLF patches into their own bug and not hold up proceedings here... but I wanted to post them to show my working ;). QA Question.. I've lost myself a little here.. can an item exist without a collection code? If so, I think the tests should also test for this case.. which they currently don't do (And would currently fail for I believe). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Koha::Template::Plugin::Bra |Koha::Item::pickup_location |nches::pickup_locations is |s is very inefficient, |very inefficient, causing |causing timeouts on records |timeouts on records with |with large numbers of |large numbers of |holds/items |holds/items | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #26 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113483&action=edit Bug 26963: [GOLF] Don't delete existing data before tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113498&action=edit Bug 26963: [GOLF] Fix cases where we expected a list -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113453|0 |1 is obsolete| | --- Comment #28 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113503 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113503&action=edit Bug 26963: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113454|0 |1 is obsolete| | --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113504 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113504&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113455|0 |1 is obsolete| | --- Comment #30 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113505 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113505&action=edit Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113480|0 |1 is obsolete| | --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113506 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113506&action=edit Bug 26963: [GOLF] Convert to ResultSets This patch removes the previously introduced private method by converting the arrayref returns to ResultSets appropriately and inlining the filter search queries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113481|0 |1 is obsolete| | --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113507 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113507&action=edit Bug 26963: [GOLF] Migrate unit tests into pickup_location We wrote unit tests for _can_pickup_locations as part of this patchset, but then I inlined the method whilst golfing. This patch moves those tests into the existing pickup_locations test so we more thoroughly cover the case where branch transfer limits are in play. NOTE: The tests all assume that all items have an effective_itemtype and ccode. I'm pretty sure items all have a type at this point, but I'm less sure we enforce collection codes? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113483|0 |1 is obsolete| | --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113508 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113508&action=edit Bug 26963: [GOLF] Don't delete existing data before tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #34 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113509 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113509&action=edit Bug 26963: [GOLF] Update mocked return of pickup_locations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113498|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113510 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113510&action=edit Bug 26963: [GOLF] Fix cases where we expected a list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #36 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113511 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113511&action=edit Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved. To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB Signed-off-by: Nick Clemens <nick@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Jonathan Druart from comment #22)
(In reply to Martin Renvoize from comment #15)
I'd love to see the methods that are currently returning arrayrefs of Koha::Objects actually return proper Koha::Object sets which could then be chained properly.
I don't understand. Are you suggesting that Koha::Item->pickup_locations should return the Koha::Item object?
If it's a "getter" method I see no reason not to return what you had asked for :)
It is indeed a getter.. but to me it clearly sounds like a relation getter.. i.e that it should return a related set.. i.e. a Koha::Libraries object as apposed to an ARRAREF of Koha::Library objects... with a proper Koha::Libraries object you can further refine the results by chaining additional ->search calls for example.. which is the exact case we're running into here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113511|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113512 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113512&action=edit Bug 26963: [GOLF] Fix up tests and cover case of undefined ccode While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved. To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113503|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113513 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113513&action=edit Bug 26963: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113504|0 |1 is obsolete| | --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113514 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113514&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113505|0 |1 is obsolete| | --- Comment #41 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113515 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113515&action=edit Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113516 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113516&action=edit Bug 26963: (QA follow-up) Convert to ResultSets This patch removes the previously introduced private method by converting the arrayref returns to ResultSets appropriately and inlining the filter search queries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #43 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113517 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113517&action=edit Bug 26963: (QA follow-up) Migrate unit tests into pickup_location We wrote unit tests for _can_pickup_locations as part of this patchset, but then I inlined the method whilst golfing. This patch moves those tests into the existing pickup_locations test so we more thoroughly cover the case where branch transfer limits are in play. NOTE: The tests all assume that all items have an effective_itemtype and ccode. I'm pretty sure items all have a type at this point, but I'm less sure we enforce collection codes? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113518 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113518&action=edit Bug 26963: (QA follow-up) Don't delete existing data before tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #45 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113519 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113519&action=edit Bug 26963: (QA follow-up) Update mocked return of pickup_locations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #46 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113520 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113520&action=edit Bug 26963: (QA follow-up) Fix cases where we expected a list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #47 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 113521 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113521&action=edit Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved. To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113506|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113507|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113508|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113509|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113510|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113512|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113513|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113514|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113515|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113516|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113517|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113518|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113519|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113520|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #113521|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #48 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113523 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113523&action=edit Bug 26963: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #49 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113524 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113524&action=edit Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #50 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113525 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113525&action=edit Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #51 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113526 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113526&action=edit Bug 26963: (QA follow-up) Convert to ResultSets This patch removes the previously introduced private method by converting the arrayref returns to ResultSets appropriately and inlining the filter search queries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #52 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113527 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113527&action=edit Bug 26963: (QA follow-up) Migrate unit tests into pickup_location We wrote unit tests for _can_pickup_locations as part of this patchset, but then I inlined the method whilst golfing. This patch moves those tests into the existing pickup_locations test so we more thoroughly cover the case where branch transfer limits are in play. NOTE: The tests all assume that all items have an effective_itemtype and ccode. I'm pretty sure items all have a type at this point, but I'm less sure we enforce collection codes? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #53 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113528 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113528&action=edit Bug 26963: (QA follow-up) Don't delete existing data before tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #54 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113529 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113529&action=edit Bug 26963: (QA follow-up) Update mocked return of pickup_locations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #55 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113530&action=edit Bug 26963: (QA follow-up) Fix cases where we expected a list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #56 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 113531 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113531&action=edit Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved. To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Koha::Item::pickup_location |Improve |s is very inefficient, |Koha::Item::pickup_location |causing timeouts on records |s performance |with large numbers of | |holds/items | QA Contact|testopia@bugs.koha-communit |tomascohen@gmail.com |y.org | Text to go in the| |Koha::Item::pickup_location release notes| |s is very inefficient, | |causing timeouts on records | |with large numbers of | |holds/items. | | | |This | |development refactors the | |underlying implementation, | |and also makes the method | |return a resultset, to | |delay as much as possible | |the DB access, and thus | |allowing for further | |filtering on the callers, | |through chaining. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27002 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27002 [Bug 27002] Make Koha::Biblio->pickup_locations return a Koha::Libraries resultset -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wizzyrea@gmail.com --- Comment #57 from Liz Rea <wizzyrea@gmail.com> --- To answer a question from yonks ago, an item can most certainly exist without a collection code. They are Authorised values and some libraries do not use them at all. Liz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #58 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I think we should keep the number of tests, don't you? I wrote a similar patch than "Don't delete existing data before tests" today: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113551 I will drop mine, but I think this part makes sense: + next + unless grep { $pickup_location eq $_ } @branchcodes; Do we agree that this change can be considered an enhancement? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #59 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #58)
I think we should keep the number of tests, don't you?
I wrote a similar patch than "Don't delete existing data before tests" today: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113551
I will drop mine, but I think this part makes sense: + next + unless grep { $pickup_location eq $_ } @branchcodes;
Do we agree that this change can be considered an enhancement?
I do keep the number of tests, I just calculate it based on the number of branches - I think the line you mention makes that unecessary though, so yes, that line should be copied here This should be considered a bug - not an enhancement. The current code can crash a system with the number of DB requests generated and should be backported to avoid issues on any large systems -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #60 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 113586 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113586&action=edit Bug 26963: Ignore existing libraries -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #61 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Nick Clemens from comment #59)
This should be considered a bug - not an enhancement. The current code can crash a system with the number of DB requests generated and should be backported to avoid issues on any large systems
Ok, but we are working on 2 things at the same time, modifying the same code. So which one goes first? This one or bug 26988? We need a dependency between both, otherwise it does not work... https://snipboard.io/u4TGrK.jpg [2020/11/13 10:28:48] [ERROR] GET /api/v1/holds/8/pickup_locations: unhandled exception (Mojo::Exception)<<Not an ARRAY reference at /kohadevbox/koha/Koha/REST/V1/Holds.pm line 422.>> Koha::REST::Plugin::Exceptions::__ANON__ /kohadevbox/koha/Koha/REST/Plugin/Exceptions.pm (73) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |26988 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26988 [Bug 26988] Defer loading the hold pickup locations until the dropdown is selected -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |20.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #62 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 20.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #63 from Lucas Gass <lucas@bywatersolutions.com> --- I would think this is needed in 20.05 but the patchset does not apply clean, any chance we can get a rebase? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 --- Comment #64 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 113729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=113729&action=edit Bug 26963: [20.05.x] Bug 26963: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: Don't call 'can_be_transferred' for each possible library for each item Currently When calling Koha::Template::Plugin::Branches::pickup_locations we call pickup_location for each item of the bib, and for each item we get a list of possible branches, we then check those branches against the transfer limits, this is inefficent Given a system with 100 branches, and each branch having an item attached to one bib (100 items) we will call can_be_transferred ~10000 times - and that will happen for each hold placed on the bib For me this patch reduced load time from 77 seconds to 18 seconds To test: 1 - Find a bib 2 - Place 4 title level holds 3 - Add some branches and items for this bib to your system: INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"A"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),CONCAT(branchname,"B"),pickup_location FROM branches; INSERT INTO branches (branchcode,branchname,pickup_location) SELECT CONCAT(branchcode,"D"),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; INSERT INTO items (biblionumber,biblioitemnumber,barcode,itype,homebranch,holdingbranch) SELECT 1,1,CONCAT("test-",branchcode),'BKS',branchcode,branchcode FROM branches; 4 - Set systempreferences: UseBranchTransferLimits = 'enforce' BranchTransferLimitsType = 'item type' 5 - Find the bib and click the holds tab 6 - Wait for a long time, it shoudl eventually load 7 - Apply patch and restart al the things 8 - Load the page again, it should be much faster Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Bob Bennhoff <bbennhoff@clicweb.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (follow-up) Change subroutine name for QA tools It didn't like the ending _at Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Convert to ResultSets This patch removes the previously introduced private method by converting the arrayref returns to ResultSets appropriately and inlining the filter search queries. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Migrate unit tests into pickup_location We wrote unit tests for _can_pickup_locations as part of this patchset, but then I inlined the method whilst golfing. This patch moves those tests into the existing pickup_locations test so we more thoroughly cover the case where branch transfer limits are in play. NOTE: The tests all assume that all items have an effective_itemtype and ccode. I'm pretty sure items all have a type at this point, but I'm less sure we enforce collection codes? Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Don't delete existing data before tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Update mocked return of pickup_locations Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Fix cases where we expected a list Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: (QA follow-up) Fix up tests and cover case of undefined ccode While this technically shouldn't happen, if a library creates itemtype limits, then flips the pref, those rules are still in the db until a ccode rule is saved. To be extra safe, when checking for rules of one type, we should make sure the other type is undef - i.e. if looking for ccode rules, we should confirm the itype is undef for those rules Additionally, we shouldn't set the barcode now that we are not deleting all items, so we use copynumber for our item identification field as it doesn't need to be unique in the DB Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 26963: Ignore existing libraries -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.11.00 |20.11.00, 20.05.06 released in| | Status|Pushed to master |Pushed to stable --- Comment #65 from Lucas Gass <lucas@bywatersolutions.com> --- backported to 20.05.x for 20.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |RESOLVED Resolution|--- |FIXED CC| |aleisha@catalyst.net.nz --- Comment #66 from Aleisha Amohia <aleisha@catalyst.net.nz> --- missing dependencies, not backported to 19.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26963 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27062 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27062 [Bug 27062] pickup_location tests don't deal correctly with existing libraries -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org