GetBudgetPeriod, when passed a fund ID (budget_period_id), returns a hashref of an individual budget, consistent with other functions of its sort. However, it also has a special mode -- if you pass 0 as its parameter, it returns a hashref of the first active budget. This is a problem for several reasons: [1] Nothing in Koha requires that there be only one active budget. Consequently, a function that returns "the" active one is working from an incorrect assumption (albeit, maybe one that used to be true when acquisitions support was originally added?). [2] Even the assumption were true that there can be only one active budget at a time, it is still bad practice for the one parameter of a function to either be an ID or a flag. [3] There is no code, other than a test case, that actually calls GetBudgetPeriod(0).
Created attachment 19578 [details] [review] bug 10577: improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Galen, I think I found a problem with this - you said it's never used, but from the description I thought about the funds page and spotted a problem there: - Apply the patch - Go to the funds page - The heading is missing the budget information now - it shows: Funds for '' I think this is a design issue of the funds page really. Maybe we just should add a budget column to the table and reword the heading so it can deal with funds from multiple budgets? But not sure how that would affect other functionality on that page.
(In reply to Katrin Fischer from comment #2) > Galen, I think I found a problem with this - you said it's never used, but > from the description I thought about the funds page and spotted a problem > there: > > - Apply the patch > - Go to the funds page > - The heading is missing the budget information now - it shows: > Funds for '' > > I think this is a design issue of the funds page really. Maybe we just > should add a budget column to the table and reword the heading so it can > deal with funds from multiple budgets? But not sure how that would affect > other functionality on that page. Good catch. The funds page appears to still partially reflect an (old?) assumption that there would be only one active budget at a time. That assumption is nonsense nowadays, so I still believe that I'm on the right track with the change I made to GetBudgetPeriod(). Of course, it does need to be accompanied by a follow-up so that we don't have the Funds for '' situation. My proposal: - add a column for the parent budget (should it always be displayed, or only when no specific budget_period_id is passed?) - adjust the code that generates the hover to make sure that the parent budget is supplied.
This is still the case but I think the issue with it that Katrin found was resolved in bug 11578, setting back to NSO
Created attachment 86563 [details] [review] bug 10577: improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86564 [details] [review] Bug 10577: (QA follow-up) Remove bad call signature Whilst working through this patch for signoff I found a case where the calling signature of GetBudgetPeriod was incorrect. I removed the superflous passed parameter in this followup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Patch rescued and tested.. Signing off.
Created attachment 86565 [details] [review] Bug 10577: Improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton <gmc@esilibrary.com> Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 86566 [details] [review] Bug 10577: (QA follow-up) Remove bad call signature Whilst working through this patch for signoff I found a case where the calling signature of GetBudgetPeriod was incorrect. I removed the superflous passed parameter in this followup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87009 [details] [review] Bug 10577: Improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton <gmc@esilibrary.com> Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87010 [details] [review] Bug 10577: (QA follow-up) Remove bad call signature Whilst working through this patch for signoff I found a case where the calling signature of GetBudgetPeriod was incorrect. I removed the superflous passed parameter in this followup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 87080 [details] [review] Bug 10577: Improve semantics of GetBudgetPeriod() Remove the option to pass zero to this function in order to get "the" active budget. This was a problem in three ways: - Koha doesn't require that there be only one active budget at a time, so the concept of "the" active budget doesn't make sense. - Having the single parameter be either an ID or a flag based on its value is poor function design. - No callers of GetBudgetPeriod() were actually using this modality. This patch also improves the DB-dependent tests for budgets by - wrapping the test in a transaction - counting budgets correctly To test: [1] Apply the patch. [2] Verify that prove -v t/db_dependent/Budgets.t passes [3] Verify in the staff interface that: - the budget hierarchy displays correctly - you can add and modify a budget Signed-off-by: Galen Charlton <gmc@esilibrary.com> Rescued-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 87081 [details] [review] Bug 10577: (QA follow-up) Remove bad call signature Whilst working through this patch for signoff I found a case where the calling signature of GetBudgetPeriod was incorrect. I removed the superflous passed parameter in this followup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 87082 [details] [review] Bug 10577: (QA follow-up) Remove tab chars Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 19.05
Enhancement will not be backported to 18.11.x series.