Bug 12164

Created attachment 27911 [details]
script for testing this feature

To test this feature, you can use this patch to create and delete a fund structure and orders (received and unreceived).

The structure will be:
budget_1 (1 pending, 0 ordered)
> budget_11 (3 pending, 4 ordered)
>> budget_111 (2 pending, 1 ordered)
> budget_12 (4 pending, 3 ordered)
budget_2 (1 pending, 0 ordered)
> budget_21 (0 pending, 0 ordered)

1 order is 2 items (cost: 10 each).

Create the data: perl bug_12164.pl create
Delete the data: perl bug_12164.pl delete

Note: The delete action does *not* remove created items.
Important note: Backup your database before using this script!

Note: To complete this feature, a CSV export should be available on the report view.

I try to apply against master 3.17.00.007

Applying: Bug 12164: Move the budget period clone logic into C4::Budgets
fatal: sha1 information is lacking or useless (C4/Budgets.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 12164: Move the budget period clone logic into C4::Budgets

So I pass the patch to "Patch doesn't' apply" status.

I apply against master 3.17.00.007

A) I run the script. The only funds in the database are the expected ones described below. 
But, on main Acquisitions' page [Manual 3.16, See Budget/Fund Tracking: Clicking on the linked amounts under spent or ordered will show you a summary of the titles ordered/received on that budget.], after selecting the cells of the column "Spent" of the table "All available funds for Centerville", I see "No data available in table" even when some orders are expected in the selected cell (not zero).
[Perhaps does the script build the received orders in a not entirely correctly way?]

B) At 6-th step: "Verify the number of Unreceived orders is correct ...",
I see this error:
the numbers in "Unreceived orders"'s column are not the expected ones: they're the total numbers of received and unreceived orders.

C) At 6-th step, koha says:
"Choose the funds you want to move unreceived orders:"
but no checkbox is given to mark which funds I want to choose.

D) At 6-th step:
I've selected the budget, but, after selecting the "Move unreceived orders" button, koha says:
"You have chosen to move all unreceived orders from 'test_clone' to 'null'."
So the name of the new budget is not shown correctly.
[If I've already changed the name of the target budget, 'null' still remains as an error.]

E) I prefer to change the name of the "new" budget period, the target. I call it "test_clone_new". [it's the 2015's budget]. At 7-th step, the report shows:
Report after moving unreceived orders from budget test_clone_new (01/01/2015 - 12/31/2015) to test_clone_new (01/01/2015 - 12/31/2015)
So the old budget (name and dates) is not shown correctly. See also G) below.

F) Now the old budget is inactive and closed. It only has already received orders (OK). But I can close it again, marking the original budget as inactive (?), and so on. I choose the same target budget: no orders shall be moved (OK ...), but the base-level allocated, the  base-level available and the total available of the target budget period are increased again. See Nota bene 1 below.

G) I duplicate the "test_clone" budget into "test_2015".
Then I duplicate the same "test_clone" again in "test_2016".
Now I close the "test_clone" onto "test_2016", the last one. 
The report says:
"Report after moving unreceived orders from budget test_2015 (01/01/2015 - 12/31/2016) to test_2016 (01/01/2016 - 12/31/2016)"
The part "test_2015 (01/01/2015 - 12/31/2016)" is wrong. 
It should be instead:
test_clone (01/01/2013 - 12/31/2013)
----------------------------------------------------------------------
"Closing" a budget period is new in koha: 
NB.1: Perhaps would the option be absent for a budget period without unreceived orders? Or for a budget period already closed?
NB.2: How can I know that a budget period has been already closed? I don't see it [in "Edit"ing a budget period, f.i.].
----------------------------------------------------------------------

So I pass the patch to "Failed QA" status.
-----------------------------------------

(In reply to Paola Rossi from comment #16)

Hello Paola, thanks for testing!

> A) I run the script. The only funds in the database are the expected ones
> described below. 
> But, on main Acquisitions' page [Manual 3.16, See Budget/Fund Tracking:
> Clicking on the linked amounts under spent or ordered will show you a
> summary of the titles ordered/received on that budget.], after selecting the
> cells of the column "Spent" of the table "All available funds for
> Centerville", I see "No data available in table" even when some orders are
> expected in the selected cell (not zero).
> [Perhaps does the script build the received orders in a not entirely
> correctly way?]

Hum yes, not exactly sure what is wrong here, but the script does not depend on the AcqCreateItem pref. I don't think it is useful for testing.

> B) At 6-th step: "Verify the number of Unreceived orders is correct ...",
> I see this error:
> the numbers in "Unreceived orders"'s column are not the expected ones:
> they're the total numbers of received and unreceived orders.

Will be fixed.

> C) At 6-th step, koha says:
> "Choose the funds you want to move unreceived orders:"
> but no checkbox is given to mark which funds I want to choose.

I changed the sentence. No checkbox should appear.

> D) At 6-th step:
> I've selected the budget, but, after selecting the "Move unreceived orders"
> button, koha says:
> "You have chosen to move all unreceived orders from 'test_clone' to 'null'."
> So the name of the new budget is not shown correctly.
> [If I've already changed the name of the target budget, 'null' still remains
> as an error.]

Will be fixed.

> E) I prefer to change the name of the "new" budget period, the target. I
> call it "test_clone_new". [it's the 2015's budget]. At 7-th step, the report
> shows:
> Report after moving unreceived orders from budget test_clone_new (01/01/2015
> - 12/31/2015) to test_clone_new (01/01/2015 - 12/31/2015)
> So the old budget (name and dates) is not shown correctly. See also G) below.

Will be fixed.

> F) Now the old budget is inactive and closed. It only has already received
> orders (OK). But I can close it again, marking the original budget as
> inactive (?), and so on. I choose the same target budget: no orders shall be
> moved (OK ...), but the base-level allocated, the  base-level available and
> the total available of the target budget period are increased again. See
> Nota bene 1 below.

If you try to close an already "closed" budget, you should not be able to continue. You should arrive on a page with the following message:
  "There is no unreceived orders for this budget. Back"
I just tried and it seems to work. Could you detail please?

> G) I duplicate the "test_clone" budget into "test_2015".
> Then I duplicate the same "test_clone" again in "test_2016".
> Now I close the "test_clone" onto "test_2016", the last one. 
> The report says:
> "Report after moving unreceived orders from budget test_2015 (01/01/2015 -
> 12/31/2016) to test_2016 (01/01/2016 - 12/31/2016)"
> The part "test_2015 (01/01/2015 - 12/31/2016)" is wrong. 

This should be fixed with previous changes.

> ----------------------------------------------------------------------
> "Closing" a budget period is new in koha: 
> NB.1: Perhaps would the option be absent for a budget period without
> unreceived orders? Or for a budget period already closed?

Actually I don't know what could be the difference between a "closed" budget and an "inactive" budget. It is why it is recommended to mark as inactive a budget when you duplicate it. I don't want to display/hide the "close" link on  on admin/aqbudgetperiods.pl because the way to know is bad for performances.

> NB.2: How can I know that a budget period has been already closed? I don't
> see it [in "Edit"ing a budget period, f.i.].

No, you can't. This information is not stored in the DB.

I apply against master 3.17.00.007

Everything's OK.

I pass the patch to "Signed Off" status.

Started QA on this.

Hi Jonathan, I am still testing this, so far no bigger problems found. Some small issues and a question:

1) Some small typos found during code review:

+    There is no unreceived orders for this budget.
There are no unreceived oreders for this budget.

+          <input type="submit" value="Move unreceive orders" />
Move unreceived orders

+        <td>Informations</td>
Information, Details

+                <td>There was no unreceived orders for this fund.</td>
There were no...


2) The report shown after moving funds shows the amounts with quite a ot of zeros :)

99 200.000000


3) Question: If you decide to roll over the funds with the orders into the next year - should the funds be removed from last year's budget? I am not sure about this, so it's really a question to libraries I think.

(In reply to Katrin Fischer from comment #28)
> Hi Jonathan, I am still testing this, so far no bigger problems found. Some
> small issues and a question:
> 
> 1) Some small typos found during code review:

Patch coming.

> 2) The report shown after moving funds shows the amounts with quite a ot of
> zeros :)
> 
> 99 200.000000

I don't reproduce. Where it's happen? I just see "Moved!" or a message without amount in the "Informations, details" column.

> 3) Question: If you decide to roll over the funds with the orders into the
> next year - should the funds be removed from last year's budget? I am not
> sure about this, so it's really a question to libraries I think.

Do you mean *completely* removed? The fund could have been used to reveive orders, so we don't want to remove them.

> > 3) Question: If you decide to roll over the funds with the orders into the
> > next year - should the funds be removed from last year's budget? I am not
> > sure about this, so it's really a question to libraries I think.
> 
> Do you mean *completely* removed? The fund could have been used to reveive
> orders, so we don't want to remove them.

I had wondered if the money added to the new budget for the unreceived orders should be subtracted from the old budget, so that you end up with a 'balanced' old budget (no available money, no commited, only spent). But after some survey between my coworkers it seems ok the way it is :)

Nice new feature!

Jonathan, could you please rebase this to current master? I'm willing to push it, but I didn't notice it before, and now it doesn't apply :-(

I tried myself but it wasn't a trivial one.

Created attachment 29733 [details] [review]
Bug 12164: Move the budget period clone logic into C4::Budgets

Note the typo InsertInTable "aqcudgets", note sure about the existing
behavior before this patch!

This patch adds a link "Duplicate" in the action list for budget
periods (budgets).

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29734 [details] [review]
Bug 12164: On cloning budget period, mark original budget as inactive

This patch adds a checkbox "mark original budget as inactive" (budget
period).
If it is checked, the original budget will be marked as inactive.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29735 [details] [review]
Bug 12164: On cloning budget periods, add a "reset all funds" option

This patch adds a checkbox "reset all funds" (budgets).
If it is checked, the new created budgets (funds) will be reset.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29736 [details] [review]
Bug 12164: Close a budget period (budget)

This is the main patch.

On closing a budget period, all unreceived orders are moved from the
old/previous fiscal year into the new fiscal year.

You can rollover funds unused in the previous fiscal year to the new
fiscal year.

This patch set is based on bug 12168 (bugfix) and can be tested on top
of bug 11578 (easier to see the fund structure).

The patch set is cut in 6 main patches:

- Move the budget period clone logic into C4::Budgets
  The code is moved from the pl to Budgets.pm and unit tests are provided.
  The original code should certainly be buggy since a typo existed.
- On cloning budget period, mark original budget as inactive
  Cloning a budget period is already possible in Koha, this patch adds a
  checkbox to mark as inactive the original budget. That avoids to edit
  the budget and click the "inactive" checkbox. Both do the same action.
- On cloning budget periods, add a "reset all funds" option
  Same as before, a new checkbox is added on cloning a budget period. If
  you check it, all fund amounts will be set to 0. Otherwise, no change
  compared to the existing behavior.
- Close a budget period (budget)
  The goal of this patch set is to move unreceived orders from a budget to
  another. This patch adds a C4::Budgets::MoveOrders routine which does
  this job.
  This action is only possible if the fund structure is the same for both
  budgets, the budget_code field should be the same.
- On closing budget period, move unspent amount
  Unspent amount will be move from the previous budget structure to the
  new one.
- Add UI report
  This patch only adds a report when closing a budget is done.

Test plan:
Wording: below, budget is a "budget period" and fund is a "budget".
Prerequisite: Having 1 active budget with some funds (with different
levels and different amounts). Order and receive some orders (not all).
1/ Go on the budgets administration page (admin/aqbudgetperiods.pl) and
duplicate the structure of this budget ("Duplicate" link in the
"Actions" column).
2/ Enter start and end date for this budget and mark the original budget
as inactive.
3/ Note that a new budget is created, with the same fund structures (and
same value) and that the old one is marked as inactive (see
admin/aqbudgets.pl page with patches from bug 11578).
4/ Try to close the new budget: it is not possible, there is no
unreceived orders for this budget.
5/ You can close the inactive budget ("Close" link in the "Actions"
column).
6/ Verify the number of "Unreceived orders" is correct and select the
new budget in the budget list. Click on the "Move remaining unspent
funds" if you want to move unspent amounts.
7/ A report view is displayed and show you the ordernumber which have
been impacted (grouped by fund).
8/ Try different configuration, depending on the selected checkboxes.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29737 [details] [review]
Bug 12164: On closing budget period, move unspent amount

On closing a budget (budget period), the user will be presented with an
option to move remaining unspent funds from the previous budget to the
newly created one - adding to the amounts already entered in those
funds.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29738 [details] [review]
Bug 12164: Add UI report

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29739 [details] [review]
Bug 12164: follow-up: various fixes

This follow-up fixes issues raised on bug 12164 comment 17.

1/ The unreceived orders columns should only display "pending" orders
2/ Fix some typo from_budget_period_id vs budget_period_id vs
to_budget_period_id

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29740 [details] [review]
Bug 12164: Fix some typos

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 29741 [details] [review]
Bug 12164: Fix decimal number display

If the funds are not moved, the amount is display like 123.000000.
This patch hardcodes the number of decimal numbers to display (2).

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes QA script and tests, especially all acquisition tests.

New routines in Budgets.pm:
  - CloneBudgetPeriod
  - CloneBudgetHierarchy
  - MoveOrders

OK t/db_dependent/Budgets.t

Some of the tests done:

1) Duplicate an existing budget

- OK Existing budget is active, duplicate it and mark it inactive
- OK Existing budget is inactive, duplicate and mark it inactive

2) Close budget
- OK Budget was created without funds - no changes are made
- OK Budget was created with funds, all orders moved correctly
  - moving the available money to the new budget is optional
- OK Budget was created with funds, but one fund deleted,
     all orders moved correctly, one missing fund reported,
     orders unchanged

3) Partial receiced undone
- OK Partial receive from last budget is undone, order lines
     are correctly recombined and fund is set to the current budget

(In reply to Tomás Cohen Arazi from comment #44)
> Jonathan, could you please rebase this to current master? I'm willing to
> push it, but I didn't notice it before, and now it doesn't apply :-(
> 
> I tried myself but it wasn't a trivial one.

The last patch set fixes the conflict with bug 12482.
It was not so easy and I hope I did not forget something.
I retested a big part of the feature and all looks good.

Created attachment 30043 [details] [review]
Bug 12164: (followup) MoveOrders description corrected

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Created attachment 30044 [details] [review]
Bug 12164: (followup) keys expects a hash parameter

As of http://perldoc.perl.org/functions/keys.html we should avoid passing
a scalar parameter as it is not backwards compatible with squeezze's Perl
version (5.10).

Cite:
"Starting with Perl 5.14, keys can take a scalar EXPR, which must contain a
reference to an unblessed hash or array. The argument will be dereferenced
automatically. This aspect of keys is considered highly experimental. The
exact behaviour may change in a future version of Perl."

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Patches pushed to master.

Thanks Jonathan and everyone involved in testing this new feature.

Note for the QA people: read the last patch's comment, because we should filter patches including experimental features.