Bug 10577

Summary: C4::Budgets::GetBudgetPeriod has inappropriate overloading of its behavior
Product: Koha Reporter: Galen Charlton <gmcharlt>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: minor    
Priority: P5 - low CC: dcook, josef.moravec, katrin.fischer, m.de.rooy, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11578
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Part of the ongoing effort to improve the maintainability of our codebase.
Version(s) released in:
Bug Depends on:    
Bug Blocks: 10535    
Attachments: bug 10577: improve semantics of GetBudgetPeriod()
bug 10577: improve semantics of GetBudgetPeriod()
Bug 10577: (QA follow-up) Remove bad call signature
Bug 10577: Improve semantics of GetBudgetPeriod()
Bug 10577: (QA follow-up) Remove bad call signature
Bug 10577: Improve semantics of GetBudgetPeriod()
Bug 10577: (QA follow-up) Remove bad call signature
Bug 10577: Improve semantics of GetBudgetPeriod()
Bug 10577: (QA follow-up) Remove bad call signature
Bug 10577: (QA follow-up) Remove tab chars

Description Galen Charlton 2013-07-11 16:25:08 UTC
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).
Comment 1 Galen Charlton 2013-07-11 16:47:12 UTC
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>
Comment 2 Katrin Fischer 2013-07-11 20:50:47 UTC
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.
Comment 3 Galen Charlton 2013-07-22 23:28:45 UTC
(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.
Comment 4 Martin Renvoize 2019-03-13 10:35:11 UTC
This is still the case but I think the issue with it that Katrin found was resolved in bug 11578, setting back to NSO
Comment 5 Martin Renvoize 2019-03-13 11:17:45 UTC
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>
Comment 6 Martin Renvoize 2019-03-13 11:17:48 UTC
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>
Comment 7 Martin Renvoize 2019-03-13 11:18:22 UTC
Patch rescued and tested.. Signing off.
Comment 8 Martin Renvoize 2019-03-13 11:19:23 UTC
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>
Comment 9 Martin Renvoize 2019-03-13 11:19:26 UTC
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>
Comment 10 Martin Renvoize 2019-03-26 11:40:54 UTC
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>
Comment 11 Martin Renvoize 2019-03-26 11:40:58 UTC
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>
Comment 12 Josef Moravec 2019-03-27 16:05:16 UTC
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>
Comment 13 Josef Moravec 2019-03-27 16:05:19 UTC
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>
Comment 14 Josef Moravec 2019-03-27 16:05:23 UTC
Created attachment 87082 [details] [review]
Bug 10577: (QA follow-up) Remove tab chars

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Nick Clemens 2019-03-28 13:28:07 UTC
Awesome work all!

Pushed to master for 19.05
Comment 16 Martin Renvoize 2019-04-01 11:07:42 UTC
Enhancement will not be backported to 18.11.x series.