[Patch] advanced-portfolio report redux (phew!)

[Andrew Sackville-West]

On Sun, 05 Mar 2006 16:23:54 -0500
Mike Alexander <mta at umich.edu> wrote:

THanks for your input. Can't look at the code right this second, but comments below.


> --On March 2, 2006 11:22:41 AM -0800 Andrew Sackville-West 
> <andrew at farwestbilliards.com> wrote:
> 
> > 3. should properly exchange currencies, provided there is  proper
> > multi-currency pricedb. I don't use multiple currencies and frankly,
> > don't understand them well, so it needs testing in this regard. If
> > someone could provide a properly setup test file with multiple
> > currencies, stock buys and sells etc., I would be grateful.
> 
> We're getting our house painted so I don't have a lot of time to work 
> on this right now, but I took a quick look.  It looks pretty good, but 
> there are few problems still.  Here are a few things I noticed from 
> reading the code.
> 
> The setting of ugain around line 461 isn't correct if "currency" and 
> "commod-currency" aren't the same since "value" is in commod-currency 
> and you're forcing ugain to be in currency without doing any 
> conversion.  You need to use exchange-fn for this.

yup. 

> 
> Around line 509 you're using getpair to get the value in "currency" 
> from "moneyincoll" which won't work.  This will only get the value that 
> happens to already be in that currency (if any) and will ignore all the 
> values in other currencies.  You need to use 
> gnc:sum-collector-commodity to get the value in the right currency. 
> You already have the negative of this value in "moneyin" set around 
> line 456 so you probably just need to change line 509 to use 
> (gnc:gnc-monetary-amount moneyin).

this was the existing method, and I agree, will fix it.

> 
> It looks to me like sum-total-gain and sum-total-ugain are both 
> monetary amounts (in spite of the fact that they are initialized to 
> gnc:numeric-zero) but sum-total-both-gains is a numeric value.  This 
> probably works, but is confusing (assuming I'm reading the code right).
> 
> You're using getpair around line 674 to get the value in "currency" 
> from "total-moneyin".  This needs to use something like
> 
> (gnc:gnc-monetary-amount
>   (gnc:sum-collector-commodity total-moneyin currency exchange-fn))

right. okay.

> 
> I have a simple file that could be used to test this, but it's a bit 
> too simple since it doesn't have enough transactions to test many of 
> the features of this report.  I'll try to beef it up a little bit and 
> send you a copy if you want.

that would be great. As I've said, I don't use this thing, so I'm guessing about a lot of it. A real test file would help immensely, even if it is fairly simple.

I don't see anything above that would be unreasonable to implement. I hope to have a reworked copy again in a few days. I'm currently fixing the price-fn (and therefore, exchange-fn) to get a most recent price BEFORE report date, and that change will be included as well.

Thanks again Mike!

A
> 
> -- 
> Mike Alexander           mta at umich.edu
> Ann Arbor, MI            PGP key ID: BEA343A6
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.gnucash.org/pipermail/gnucash-devel/attachments/20060305/a3693ce7/attachment.bin