Acquisitions - Funds transfer always transfers entire fund, not specified amount

[Sharon Herbert]

v. 2.05. Fund Transfer Problem: (Admin-Server Administration-Acquisitions-Funds-Fund Details for any fund-Transfer Money) When you transfer an amount from one fund to another it doesn't matter what amount you input, the entire fund is transferred.
Confirmation from Sally Fortin, Equinox:
Yes, I see a bug. When I transfer money from one fund to another, the amount transferring is not the amount that I specified but the entire original amount that was placed in the fund into another fund.

[Sharon Herbert (sherbert-u)]

This bug seems to be intermittent. Worked correctly with initial testing, then a few weeks later the entire fund would transfer when tested instead of only the partial amount. Sally Fortin confirmed that this was the case when she tested in June as well. We have tried again in the past week and it is working again, but we have made no changes to the test server.

[James Fournie (jfournie)]

I have determined that the bug is in the database function somewhere, I have the following OpenSRF call in the log:

NOTE: db (pg 8.4) and evergreen (2.0.5) are on separate servers, timestamps are slightly off for some reason atm.

osrf_http_translator 2011-07-29 11:05:36 [ACT:23750:./osrf_http_translator.c:293:13118865092375037] [1.1.1.1.1] [] open-ils.acq open-ils.acq.funds.transfer_money "authkey", 2, 500, "14", null, ""
osrf_http_translator 2011-07-29 11:05:36 [INFO:23750:./osrf_http_translator.c:293:13118865092375037] [1.1.1.1.1] [] open-ils.acq open-ils.acq.funds.transfer_money "authkey", 2, 500, "14", null, ""

HOWEVER:

training=# select * FROM acq.fund_allocation where (fund = 2 or fund = 14) and allocator = 136;
 id | funding_source | fund | amount | allocator | note | create_time
----+----------------+------+-----------+-----------+----------------------+-------------------------------
 68 | 1 | 2 | -22025.00 | 136 | Transfer to fund 14 | 2011-07-29 11:03:56.415816-07
 69 | 1 | 14 | 22025.00 | 136 | Transfer from fund 2 | 2011-07-29 11:03:56.415816-07
(2 rows)

AND:

training=# select * FROM acq.fund_transfer where src_fund = 2 and dest_fund = 14;
 id | src_fund | src_amount | dest_fund | dest_amount | transfer_time | transfer_user | note | funding_source_credit
----+----------+------------+-----------+-------------+-------------------------------+---------------+------+-----------------------
 26 | 2 | 100.00 | 14 | 100.00 | 2011-07-29 11:03:56.415816-07 | 136 | | 6
(1 row)

[Bill Erickson (berick)]

If anyone can reliably reproduce this, it would help a lot. The transfer proc is sufficiently complex to require the addition of debug statements to track down the problem.

[Sharon Herbert (sherbert-u)]

Hi Bill,

We'll aim replicate it on our training server next week and send you
the coordinates. We'd do it sooner, but are doing an Acq demo on
Saturday for the Board and don't want to risk messing things up.

We really appreciate you having a look at this bug!

Thanks,
Sharon
Quoting Bill Erickson <email address hidden>:

> If anyone can reliably reproduce this, it would help a lot. The
> transfer proc is sufficiently complex to require the addition of debug
> statements to track down the problem.
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/800478
>
> Title:
> Acquisitions - Funds transfer always transfers entire fund, not
> specified amount
>
> Status in Evergreen - Open ILS:
> Confirmed
>
> Bug description:
> v. 2.05. Fund Transfer Problem: (Admin-Server
> Administration-Acquisitions-Funds-Fund Details for any fund-Transfer
> Money) When you transfer an amount from one fund to another it
> doesn't matter what amount you input, the entire fund is transferred.
> Confirmation from Sally Fortin, Equinox:
> Yes, I see a bug. When I transfer money from one fund to another,
> the amount transferring is not the amount that I specified but the
> entire original amount that was placed in the fund into another fund.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/800478/+subscriptions
>

--
Sharon Herbert
Project Manager, Sitka
Tel: [phone]
Email: <email address hidden>

[Jennifer Pringle (jpringle-u)]

So far in testing this bug no longer appears to be present on 2.4.0.

[Christine Burns (christine-burns)]

This bug is happening again

When I transfer money from one fund to another, the amount transferring is not the amount that I specified but the entire original amount that was placed in the fund into another fund.

[Bill Erickson (berick)]

Huzzah! I can reproduce it now. It's definitely in acq.fund_transfer(). I'm working on a test so others can confirm and then a fix.

[Liam Whalen (whalen-ld)]

The problem is with this line here:

 curr_old_amt := trunc( curr_old_amt, 2 );

When the next line is executed:

old_remaining := old_remaining - curr_old_amt;

old_remaining can be slightly larger than 0 depending on how the truncation works out.

This results in the next IF statement evaluating as true when it should not:

IF old_remaining > 0 THEN

Which moves the entire original allocation into a deduction.

I think the best solution is to fix how prorated amounts are stored in the database. These should probably be stored as 2 digit rounded numbers. This means the final prorated entry will have to be massaged, so that all the prorated values equal the total value for the item they are representing.

[Bill Erickson (berick)]

I've pushed a pgtap test which demonstrates the problem:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp800478-acq-fund-transfer-wrong-amounts

The scripts uses large, round numbers and still fails, so I don't believe this is a rounding problem. I think it's simply a logic error in the transfer code.

Will poke further.

[Bill Erickson (berick)]

Ha, beware my pgtap test is invalid when the transfer is successful. Fixing...

[Liam Whalen (whalen-ld)]

I was looking at transer_fund in the context of a roll over, so I was assuming it was related to the same thing I was looking at. I have some other things to work on here, but this bug is soon to be on my list. I will get in touch once I am on to it to make sure I am not duplicating your work.

Thank you for the link to the test.

Liam
On Feb 25, 2014, at 8:33 AM, Bill Erickson <email address hidden> wrote:

> I've pushed a pgtap test which demonstrates the problem:
>
> http://git.evergreen-
> ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp800478
> -acq-fund-transfer-wrong-amounts
>
> The scripts uses large, round numbers and still fails, so I don't
> believe this is a rounding problem. I think it's simply a logic error
> in the transfer code.
>
> Will poke further.
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/800478
>
> Title:
> Acquisitions - Funds transfer always transfers entire fund, not
> specified amount
>
> Status in Evergreen - Open ILS:
> Confirmed
>
> Bug description:
> v. 2.05. Fund Transfer Problem: (Admin-Server Administration-Acquisitions-Funds-Fund Details for any fund-Transfer Money) When you transfer an amount from one fund to another it doesn't matter what amount you input, the entire fund is transferred.
> Confirmation from Sally Fortin, Equinox:
> Yes, I see a bug. When I transfer money from one fund to another, the amount transferring is not the amount that I specified but the entire original amount that was placed in the fund into another fund.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/800478/+subscriptions

[Liam Whalen (whalen-ld)]

A quick thought,

If the old amount cannot be less than the funding source, then when this check is performed:

 IF curr_old_amt > allocated_amt THEN +
                         curr_old_amt := allocated_amt; +
                 END IF;

That will cause the same IF I noted below to evaluate to TRUE again.

It should probably set the value to the current_old_amt instead of orig_allocated_amt;

 IF old_remaining > 0 THEN +
                         -- +
                         -- In this case we're using the whole allocation, so use that +
                         -- amount directly instead of applying a currency translation +
                         -- and thereby inviting round-off errors. +
                         -- +
                         source_deduction := - orig_allocated_amt;
   Maybe this is what is needed?
    source_deduciton := - current_old_amt; +
                         RAISE EXCEPTION 'acq.transfer_fund: old_remaining is %. source_deduction is %', old_remaining, source_deduction;+

Liam

On Feb 25, 2014, at 8:33 AM, Bill Erickson <email address hidden> wrote:

> I've pushed a pgtap test which demonstrates the problem:
>
> http://git.evergreen-
> ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp800478
> -acq-fund-transfer-wrong-amounts
>
> The scripts uses large, round numbers and still fails, so I don't
> believe this is a rounding problem. I think it's simply a logic error
> in the transfer code.
>
> Will poke further.
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/800478
>
> Title:
> Acquisitions - Funds transfer always transfers entire fund, not
> specified amount
>
> Status in Evergreen - Open ILS:
> Confirmed
>
> Bug description:
> v. 2.05. Fund Transfer Problem: (Admin-Server Administration-Acquisitions-Funds-Fund Details for any fund-Transfer Money) When you transfer an amount from one fund to another it doesn't matter what amount you input, the entire fund is transferred.
> Confirmation from Sally Fortin, Equinox:
> Yes, I see a bug. When I transfer money from one fund to another, the amount transferring is not the amount that I specified but the entire original amount that was placed in the fund into another fund.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/800478/+subscriptions

[Bill Erickson (berick)]

Exactly! That's where I ended up, too. I should check my email more frequently ;) -- Ditto curr_new_amt for the destination fund allocation.

I've pushed a fix to my pgtap test and the stored procedure to the same branch. The pgtap test now passes. A simpler transfer appears to work as well. Further testing appreciated.

[Bill Erickson (berick)]

Adding pullrequest for additional attention / eyes, etc.

[Liam Whalen (whalen-ld)]

I am not sure if curr_new_amt is correct in all cases. It will be correct in the case where it is set to allocated amount. It might need to be orig_allocated_amt in other cases.

I do not know what the logic is trying to do when it checks for

IF old_remaining > 0 THEN

Liam
On Feb 25, 2014, at 9:30 AM, Bill Erickson <email address hidden> wrote:

> Exactly! That's where I ended up, too. I should check my email more
> frequently ;) -- Ditto curr_new_amt for the destination fund allocation.
>
> I've pushed a fix to my pgtap test and the stored procedure to the same
> branch. The pgtap test now passes. A simpler transfer appears to work
> as well. Further testing appreciated.
>
>
> ** Also affects: evergreen/2.5
> Importance: Undecided
> Status: New
>
> ** Also affects: evergreen/2.4
> Importance: Undecided
> Status: New
>
> ** Changed in: evergreen/2.5
> Milestone: None => 2.5.4
>
> ** Changed in: evergreen/2.4
> Milestone: None => 2.4.7
>
> ** Changed in: evergreen
> Milestone: None => 2.6.0-beta1
>
> ** Tags added: pullrequest
>
> --
> You received this bug notification because you are a bug assignee.
> https://bugs.launchpad.net/bugs/800478
>
> Title:
> Acquisitions - Funds transfer always transfers entire fund, not
> specified amount
>
> Status in Evergreen - Open ILS:
> Confirmed
> Status in Evergreen 2.4 series:
> New
> Status in Evergreen 2.5 series:
> New
>
> Bug description:
> v. 2.05. Fund Transfer Problem: (Admin-Server Administration-Acquisitions-Funds-Fund Details for any fund-Transfer Money) When you transfer an amount from one fund to another it doesn't matter what amount you input, the entire fund is transferred.
> Confirmation from Sally Fortin, Equinox:
> Yes, I see a bug. When I transfer money from one fund to another, the amount transferring is not the amount that I specified but the entire original amount that was placed in the fund into another fund.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/800478/+subscriptions

[Liam Whalen (whalen-ld)]

I have made some modifications to acq.transfer_fund in a separate branch.

I believe I have caught a few conversion problems, but I am not sure if all of may changes are correct, so I have created a new working branch rather than modify Bill's branch.

I have left my comments in my branch to help me figure out what is going on.

A few things I have done that I am not 100% on.

If the amount to transfer is negative, then I am swapping old_fund and new_fund and negating the amount to be transferred to make it positive. I believe the transfer_fund code is expecting a positive transfer value. Can anyone confirm this?

When adding a source_deduction or a source_addition, I am always converting to the currency of the funding source.

It seems that the values in acq.fund_allocation_total are assuming that this is the case.

After writing that comment, I see that I no longer need to have the separate function to calculate the total amount allocated to a fund. I will remove acq.current_fund_allocation and get the values from acq.fund_allocation_total.

There is a section of code that stops money from being credited to non-active accounts. Is there any case where a library might want to transfer funds to an non-active account?

I am considering adding the code in to specify a funding source to explicitly debit by allowing the funding_source_in argument to default to NULL. Is there any reason not to do this?

I believe that old_remaining can never be brought below 0. curr_old_amt is set to old_remaining and then curr_old_amt may be modified so that it is smaller than old_remaining, but never larger. So, when old_remaining is set to old_remaining - curr_old_amt, I think it is impossible for old_remaining to be less than 0. I am working on this assumption. If someone can see otherwise, please point it out to me.

I think there is a copy and paste error in the calculation of source_addition. The current code compares the currency of the funding source to the currency of the old_fund to determine if the source_addition needs to be converted. I believe this should be a comparison between the funding source and the currency of the new fund. Is this correct?

I believe the acq.exchange_rate table should be locked while this function executes to prevent the rates from fluctuating during a single transfer. Does that make sense?

I have not run this code yet, so there may be syntax errors, and there are most likely logic errors. But, I do think the current function needs to be modified a lot. I am not certain if I have made invalid modifications in this branch though.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/ldw/LP800478_fund_transfer

[Bill Erickson (berick)]

I'm glad this code is getting more eyes. It's a complicated bit of logic. Thanks for all the input and code, Liam.

Before I get into your code, can you demonstrate any specific problems solved by your branch? If fixing the problem at hand means restructuring the code (as opposed to my 2-line change), that's fine, but it would be good to have specifics to justify the effort. Also, I think I'll wait until you've installed and tested the code before I give it a whirl.

Regarding some of your other questions, we're getting a little far afield with some of the feature-like stuff (allowing the funding source to be passed in, etc.) for the purposes of this LP ticket. Granted, it's easy and you're in there, but we're muddying the waters. Maybe open a separate LP for some of the feature-like stuff?

I'm being a little rigid, because I know there are people waiting on a solution to the problem at hand and I'd rather not require that we back-port an entirely new version of the function unless it's warranted. Having said all of that, working code wins. Give me something that installs cleanly, works, and passes at least the pgtap tests I pushed (and preferably more), I'll gladly sign off.

Removing pullrequest for now.

[Liam Whalen (whalen-ld)]

I am working through the errors in my code now.

There are definite issues with the function as it is.

There is a check for same currencies that is supposed to check for equality between the funding source and the new fund, but it checks the old fund instead.

As well, sometimes values are added to acq.fund_acquistion without converting the value into the currency of the funding source if necessary.

I have been thinking about the case where funds are transferred into the void.

I believe this was added to allow fund to be added back to the funding source. Do you know if this is correct?

I think the best option is to pull out that functionality into a separate function (return_funds_to_source or something like that). That will cut down on a lot of the IF statements that are needed in the current function because it can be restricted to only transferring between funds, so there will have to be valid fund ids for both old and new fund.

I expect to be back on this work today in the evening.

[Bill Erickson (berick)]

killing_more_bugs++

Transferring funds to "void" is how we remove money from the system. For example, if the library is allotted $X for the fiscal year, but is expected to return the unspent portion, the money goes away and does not go back into the funding source.

[Liam Whalen (whalen-ld)]

Sitka support has told me that we need the funds to be returned to the funding source, and I believe that is what the function does now when it transfers into the void.

If there is a case where funds need to be removed without going somewhere, then we should use some sort of debit that goes into a void account.

[Liam Whalen (whalen-ld)]

I am going to remove the transfer to the void functionality from the transfer_fund function and put it in a return_funds_to_source function unless anyone objects to this?

[Bill Erickson (berick)]

I have no objection to code cleanup, as long as we aren't losing any functionality.

[Liam Whalen (whalen-ld)]

I have modified acq.fund_transfer a fair bit and moved the returning of funds into a new function acq.return_funds_to_source.

The change are here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/ldw/LP800478_fund_transfer

This fixes the following:

It locks the acq.exchange_rate table for the duration of a transfer to avoid fluctuating rates while transfering.

It checks to see if the amount being transferred is negative and if it does, it swaps the transferring and receiving funds and make the amount positive.

If removes the parameter that passed in the converted amount to be transferred to the new fund. This causes a problem if the rate used to convert outside of the function is different from the rate in acq.exchange_rate.

acq.transfer_fund now checks to make sure there is enough money left in the transferring fund before trying to transfer the amount to the new fund.

The new funciton acq.return_funds_to_source is essentially the same function without having to worry about a second fund.

This commit also adds two columns to acq.fund_allocation: fund_amount and conversion_ratio. fund_amount is the amount allocated in the currency of the fund receiving the allocation. This will allow the system to display the total value in the fund without that value changing due to changes in the exchange rate.

Currently, because the amount transferred is stored in the currency of the source, if the source and fund have different currencies then the VIEW acq.fund_allocation_total changes depending on the rate of exchange.

However, if 100 US allocated to a CDN account at 1 to 1 then that account has 100 CDN regardless of future rate changes. So that total needs to stay at 100.

As well, by adding the conversion_ratio we will allow the amounts transferred to be rolled back in a new INSERT if necessary, and for accounting to see what was done at what exchange rates.

[Bill Erickson (berick)]

On Mon, Mar 3, 2014 at 2:13 PM, Liam Whalen <email address hidden> wrote:
<snip>

> If removes the parameter that passed in the converted amount to be
> transferred to the new fund. This causes a problem if the rate used to
> convert outside of the function is different from the rate in
> acq.exchange_rate.
>

The purpose of this parameter is to allow staff to enter transfers where
the money has already been converted into the new currency. In this case,
the exact amounts are known in advance and the caller does not want to rely
on the configured exchange rate, particularly since the exchange rate used
for the conversion may no longer be applicable.

This seems like an important feature to keep, no? What problems was it
causing, exactly?

<snip>

> This commit also adds two columns to acq.fund_allocation: fund_amount
> and conversion_ratio. fund_amount is the amount allocated in the
> currency of the fund receiving the allocation. This will allow the
> system to display the total value in the fund without that value
> changing due to changes in the exchange rate.
>
> Currently, because the amount transferred is stored in the currency of
> the source, if the source and fund have different currencies then the
> VIEW acq.fund_allocation_total changes depending on the rate of
> exchange.
>

+1 to storing these.

>
> However, if 100 US allocated to a CDN account at 1 to 1 then that
> account has 100 CDN regardless of future rate changes. So that total
> needs to stay at 100.
>

Maybe we need a way to indicate when a fund_allocation is "final" or not.
 In the case of a physical transfer of money, the allocation would be
marked as "final" and fund_allocation.fund_amount would be used to
determine the true amount allocated (e.g. by acq.fund_allocation_total).
 For other allocations and transfers, where the currency conversion does
not occur until later, like when the money is spent, we continue relying on
fund_allocation.amount, since it tracks the current exchange rate.

-b

--
Bill Erickson
| Senior Software Developer
| phone: 877-OPEN-ILS (673-6457)
| email: <email address hidden>
| web: http://esilibrary.com
| Equinox Software, Inc. / The Open Source Experts

[Liam Whalen (whalen-ld)]

On Mar 3, 2014, at 12:39 PM, Bill Erickson <email address hidden> wrote:

> On Mon, Mar 3, 2014 at 2:13 PM, Liam Whalen <email address hidden> wrote:
> <snip>
>
>
>> If removes the parameter that passed in the converted amount to be
>> transferred to the new fund. This causes a problem if the rate used to
>> convert outside of the function is different from the rate in
>> acq.exchange_rate.
>>
>
> The purpose of this parameter is to allow staff to enter transfers where
> the money has already been converted into the new currency. In this case,
> the exact amounts are known in advance and the caller does not want to rely
> on the configured exchange rate, particularly since the exchange rate used
> for the conversion may no longer be applicable.
>
> This seems like an important feature to keep, no? What problems was it
> causing, exactly?
>
> <snip>
>
>

I was not aware that was the original purpose. Is that something that happens? I do not know how an acquisitions department manages its money as far as transferring the bank values between accounts when the ILS account is modified.

If staff need to specify an amount that was received, then I will add that functionality back in.

>> This commit also adds two columns to acq.fund_allocation: fund_amount
>> and conversion_ratio. fund_amount is the amount allocated in the
>> currency of the fund receiving the allocation. This will allow the
>> system to display the total value in the fund without that value
>> changing due to changes in the exchange rate.
>>
>> Currently, because the amount transferred is stored in the currency of
>> the source, if the source and fund have different currencies then the
>> VIEW acq.fund_allocation_total changes depending on the rate of
>> exchange.
>>
>
> +1 to storing these.
>
>
>>
>> However, if 100 US allocated to a CDN account at 1 to 1 then that
>> account has 100 CDN regardless of future rate changes. So that total
>> needs to stay at 100.
>>
>
> Maybe we need a way to indicate when a fund_allocation is "final" or not.
> In the case of a physical transfer of money, the allocation would be
> marked as "final" and fund_allocation.fund_amount would be used to
> determine the true amount allocated (e.g. by acq.fund_allocation_total).
> For other allocations and transfers, where the currency conversion does
> not occur until later, like when the money is spent, we continue relying on
> fund_allocation.amount, since it tracks the current exchange rate.
>

I think this last suggestion ties in with your first issue with my changes. So, in the case where the library knows how much goes into the receiving fund, then it should be marked as finalized, and the amount specified by the user should be entered into acq.fund_allocation.fund_amount.

In the case where a transfer is not finalized, how should the staff indicate that it is final once the funds have been changed? Should there be new interface for unfinalized transfers. When a transfer is finalized, should the user be prompted to enter the amount in the receiving fund for each finalization, should it default to acq.exchange_rate, or should a library setting determine the default? Shou...

[Bill Erickson (berick)]

My preference for now, to get the basic transfers working as expected, is to:

1. Use your (Liam) new code, but retain the ability to specify the destination amount for backwards compatibility. I haven't heard either way if people use it, so I'm hesitant to remove it just yet. Maybe later....

2. Move the discussion of "finalizing" to a new feature enhancement ticket, since it's adding functionality, requires new UI, etc.

Sound sane?

[Liam Whalen (whalen-ld)]

I have made more headway on this fix, but it still needs to be tested and tweaked before I push it to my working branch again.

1. My changes have added back the ability to specify a transfer amount.

2. I agree that we should defer the finalizing for now.

Liam

[Bill Erickson (berick)]

Excellent. Thanks for the update and continued efforts, Liam. When it's
ready for testing, I'll toss it onto a dev server and give it a good poke.
 -b

On Thu, Mar 13, 2014 at 10:30 AM, Liam Whalen <email address hidden> wrote:

> I have made more headway on this fix, but it still needs to be tested
> and tweaked before I push it to my working branch again.
>
> 1. My changes have added back the ability to specify a transfer amount.
>
> 2. I agree that we should defer the finalizing for now.
>
> Liam
>
> --
> You received this bug notification because you are a member of Evergreen
> Bug Wranglers, which is subscribed to Evergreen.
> https://bugs.launchpad.net/bugs/800478
>
> Title:
> Acquisitions - Funds transfer always transfers entire fund, not
> specified amount
>
> Status in Evergreen - Open ILS:
> Confirmed
> Status in Evergreen 2.4 series:
> New
> Status in Evergreen 2.5 series:
> New
>
> Bug description:
> v. 2.05. Fund Transfer Problem: (Admin-Server
> Administration-Acquisitions-Funds-Fund Details for any fund-Transfer Money)
> When you transfer an amount from one fund to another it doesn't matter what
> amount you input, the entire fund is transferred.
> Confirmation from Sally Fortin, Equinox:
> Yes, I see a bug. When I transfer money from one fund to another, the
> amount transferring is not the amount that I specified but the entire
> original amount that was placed in the fund into another fund.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/evergreen/+bug/800478/+subscriptions
>

--
Bill Erickson
| Senior Software Developer
| phone: 877-OPEN-ILS (673-6457)
| email: <email address hidden>
| web: http://esilibrary.com
| Equinox Software, Inc. / The Open Source Experts

[Liam Whalen (whalen-ld)]

I have my modifications working on Sitka's servers. I believe I have all the necessary changes included in this commit:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/ldw/LP800478_fund_transfer_and_year_end

Please let me know if this does not work because I have probably forgotten a necessary piece of my changes.

The current commit message is an amalgam of all my commits for Sitka. If these changes are sufficient for the community, then I will modify the commit message, so that it becomes more cogent.

[Evergreen Bug Maintenance (bugmaster)]

Added pullrequest and un-assigned, since this looked ready for testing. Please edit as needed if I am mistaken.

[Bill Erickson (berick)]

Finally getting a chance to review these changes. I have a few comments, Liam:

1. A more focused and community-generic commit message would definitely be appreciated as this is quite a lot to sift through.

2. In a previous comment you said you were planning to recover the ability to specify the destination fund amount (i.e. external currency conversions), but it looks like the latest code still removes that.

3. I'd suggest giving the SQL upgrade scripts prefixes which guarantee their install order (XXXA.foo.sql, XXXB.foo.sql, .. or some such). They may apply in the right order as-is, but it's nice to see at a glance the order is well defined.

4. The upgrade scripts should use the new "SELECT evergreen.upgrade_deps_block_check(‘XXXX’, :eg_version);" instead of inserting directly into config.upgrade_log.

5. Please leave my PgTap test commit as a standalone commit (with me as the author) just because that's the right thing to do.

6. FWIW it's not essential that these commits all be squashed into one. It's OK if they are, but if it's easier to keep them separate, that's fine too, as long as each commit is tagged with the LP#.

7. Finally and most importantly, when I reached the 3rd file in the upgrades (XXXX.schema.acq.current_fund_allocation.sql), I started getting syntax errors and was unable to proceed with applying upgrades, so I was unable to test the code.

[Bill Erickson (berick)]

Removing pullrequest pending repairs.

[Bill Erickson (berick)]

I'd like to take a step back on this ticket and offer a suggestion.

Since it may be some time before the pending issues with Liam's patch are addressed (and I have no power to make Liam work his finger's to the bone) and since Liam's patch represents a significant overhaul to the fund transfer code and then some, including functionality changes, I think we should open a new ticket targeting 2.next for managing the larger overhaul encompassed by Liam's patches.

For this ticket, at least for the problems that I have witnessed in the wild, I would like to once again propose my original 2-line fix + pgtap test. This code is ready to go (pending sign-off) and back-portable so that running production sites may apply it without a full upgrade. It does not fix all of the issues identified by Liam, but it is a step in the right direction and can offer some immediate relief.

I have rebased my original branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp800478-acq-fund-transfer-wrong-amounts

Re-adding pullrequest tag for this branch.

[Liam Whalen (whalen-ld)]

As Bill suggested, I have created a new ticket for my changes here:

https://bugs.launchpad.net/evergreen/+bug/1335668

Liam

[Liam Whalen (whalen-ld)]

I have signed off on this here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=5d58e0687e16ce03dd7603b1f810c1e0752fbfb1

Tested on 2.next
Opensrf 2.3.0
Ubuntu 12.04

The pgTap test works as well.

[Mike Rylander (mrylander)]

Committed to master for 2.7 inclusion. Thanks, Bill and Liam!

[Mike Rylander (mrylander)]

Also committed to rel_2_5 and rel_2_6. Thanks for the poke from Kathy Lussier.

[Jennifer Pringle (jpringle-u)]

Tested on master and I couldn't get it to transfer the total amount, just the amounts I had specified which is the desired behaviour.

[Ben Shum (bshum)]

Like bug 1322285, the upgrade script for this fix never made it to 2.7.0 properly and only exists in 2.6.2-2.6.3. Changing bug's status to unfixed and reassigning milestone to next maintenance release for 2.7 series.

[Ben Shum (bshum)]

Okay, pushed some changes to master and rel_2_7 to deal with this. Marking back to fix committed.