Bug 11714 - GetBudgetHierarchy needs optimization
Summary: GetBudgetHierarchy needs optimization
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25499 14649
  Show dependency treegraph
 
Reported: 2014-02-07 16:57 UTC by Fridolin Somers
Modified: 2020-05-14 10:19 UTC (History)
4 users (show)

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


Attachments
Bug 11714 - GetBudgetHierarchy needs optimization (6.43 KB, patch)
2014-02-07 17:39 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11714 - GetBudgetHierarchy needs optimization (6.63 KB, patch)
2014-02-15 17:25 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 11714 - GetBudgetHierarchy needs optimization (7.27 KB, patch)
2014-07-11 11:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11714: A fund is a top-level fund if it does not have any parent (1.08 KB, patch)
2014-08-18 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11714 - GetBudgetHierarchy needs optimization (7.54 KB, patch)
2014-08-18 15:02 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 11714: A fund is a top-level fund if it does not have any parent (1.16 KB, patch)
2014-08-18 15:03 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 11714 - GetBudgetHierarchy needs optimization (7.38 KB, patch)
2014-08-22 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11714: A fund is a top-level fund if it does not have any parent (1.20 KB, patch)
2014-08-22 15:35 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2014-02-07 16:57:34 UTC
The method C4::Budgets::GetBudgetHierarchy() retreives from database budgets in an array containing a tree of budgets (parent -> children -> children, ...).
The code generating this tree with the SQL results needs optimization because when a lot of budgets exists, it can run during several minutes.

Have a look at code resume : 
while (1) {
  foreach my $r (@res) {
    foreach my $r2 (@res) {
    }
  }
}
With 1000 budgets there can be 1000x1000x1000 loops ...
Comment 1 Fridolin Somers 2014-02-07 17:39:09 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2014-02-15 17:25:46 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-03-18 11:18:51 UTC
Please provide unit tests.
Comment 4 Jonathan Druart 2014-07-11 11:00:07 UTC
I don't reproduce the issue on master. Certainly fixed with bug 12168.

*** This bug has been marked as a duplicate of bug 12168 ***
Comment 5 Jonathan Druart 2014-07-11 11:11:06 UTC
(In reply to Jonathan Druart from comment #4)
> I don't reproduce the issue on master. Certainly fixed with bug 12168.
> 
> *** This bug has been marked as a duplicate of bug 12168 ***

Sorry, Fridolin wanted to optimize the routine, not fixing a bug.
Comment 6 Jonathan Druart 2014-07-11 11:14:49 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2014-07-11 11:15:47 UTC
I rebased the patch and added some changes in order to fix conflicts with bug 12168 and bug 11578.
Comment 8 Jonathan Druart 2014-07-11 11:31:46 UTC
This is not a major anymore, back to normal.
Comment 9 Paola Rossi 2014-08-18 11:40:49 UTC
I've applied the patch aginst master 3.17.00.014

The totals shown in the "Period allocated" row are wrong. 
Instead of being:
1,000.00  4.00  996.00
they are:
1,110.00 10.00 1,100.00

So I pass the patch to "Failed QA" status.
Comment 10 Jonathan Druart 2014-08-18 12:48:52 UTC Comment hidden (obsolete)
Comment 11 Paola Rossi 2014-08-18 15:01:30 UTC
I've applied the patches against master 3.17.00.014

I pass the patches to "Signed Off" status.
Comment 12 Paola Rossi 2014-08-18 15:02:26 UTC Comment hidden (obsolete)
Comment 13 Paola Rossi 2014-08-18 15:03:13 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-08-22 15:34:26 UTC
Created attachment 31103 [details] [review]
[PASSED QA] Bug 11714 - GetBudgetHierarchy needs optimization

The method C4::Budgets::GetBudgetHierarchy() retreives from database budgets in an array containing a tree of budgets (parent -> children -> children, ...).
The code generating this tree with the SQL results needs optimization because when a lot of budgets exists, it can run during several minutes.

This patch rewites the code using a recurive method.

Test plan :
- Create a active budget "MyBudget" with 1000
- Click "Add found" on this budget
- Create a found "Parent" with 1000, set you has owner
- Click "Add child found" on found "Parent"
- Create a found "Child" with 100, set you has owner
- Click "Add child found" on found "Child"
- Create a found "Grand-child" with 10, set you has owner
|
- Create a new acquisition basket
- Add a new order with "Child budget"
- Select "Child" found and set all costs to 2
- Save order
- Add a new order with "Grand-Child budget"
- Select "Child" found and set all costs to 2
- Save order
- Close basket
- Perform the receive of the two orders
|
- Go to founds of "MyBudget"
=> You see a table with 3 founds
- in "Fund filters", select no library and uncheck "Show my funds only" and click on "Go"
=> You see a table with "Parent" found
- Click on small arrow left of the fund code of "Parent"
=> You see a new line with "Child" found
- Click on small arrow left of the fund code of "Child"
=> You see a new line with "Grand-Child" found
|
=> You see in "Grand-Child" row "Base-level spent" = 2 and "Total sublevels spent" = 2
=> You see in "Child" row "Base-level spent" = 2 and "Total sublevels spent" = 4
This confirms the founds are used in a hierarchie.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall 2014-08-22 15:35:18 UTC
Created attachment 31104 [details] [review]
[PASSED QA] Bug 11714: A fund is a top-level fund if it does not have any parent

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Tomás Cohen Arazi 2014-10-31 19:42:35 UTC
Patches pushed to master.

Thanks Fridolin and Jonathan!