Bug 7112

Summary: Having two prices in 020$c causes basket creation to fail from staged marc import
Product: Koha Reporter: Thatcher Rea <thatcher.rea>
Component: AcquisitionsAssignee: Kyle M Hall <kyle.m.hall>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: major    
Priority: P3 CC: chris, jcamins, katrin.fischer, koha.sekjal, kyle.m.hall, mathsabypro, paul.poulain, thatcher.rea, wizzyrea
Version: 3.8   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7516
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Sample MARC file
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import

Description Thatcher Rea 2011-10-28 03:26:19 UTC
While attempting to create a basket from a staged MARC record import, If 020$c has two prices - ie, $5.99 ($7.75 CAN) this will cause the basket creation to fail with the following message:

Only one decimal separator permitted at
/home/koha/kohaclone/acqui/addorderiso2709.pl line 210
Comment 1 Liz Rea 2011-10-28 03:32:35 UTC
Created attachment 6087 [details]
Sample MARC file

Here is a record for testing purposes that should cause the order add to fail from a staged import
Comment 2 Thatcher Rea 2011-11-17 21:11:05 UTC
After further testing it appears that the period (.) is the offending character here, since this also occurs on single prices in the 020$c:  

18.95 (U.S.)
Comment 3 Kyle M Hall 2012-02-08 17:28:57 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-02-08 17:45:44 UTC
As an aside, perhaps GetMarcPrice should do some regex magic to pull the first valid looking number from the price field. For example, if the price field contains "$5.99 ($7.75 CAN)", it should return "5.99". If the field contained "18.95 (U.S.)", then it would "18.95".
Comment 5 Katrin Fischer 2012-02-08 20:35:35 UTC
I was wondering if it works for prices with decimal commas? I should test that :)
Comment 6 Kyle M Hall 2012-02-09 11:16:04 UTC
That's a good question? I didn't even think of that! I suppose a first question would be do other currency fields in Koha handle decimal commas? If yes, then the question should be asked here as well.

(In reply to comment #5)
> I was wondering if it works for prices with decimal commas? I should test that
> :)
Comment 7 Katrin Fischer 2012-02-09 11:29:08 UTC
Other fields do currently not handle commas. But I think this is a different problem - you can control what you enter in Koha and train staff accordingly.
This bug is about making use of records from a vendor or third party, so it might be interesting to make it recognize other currency/number formatting as well and convert to a form that the acquisitions module can work with.
Perhaps it's worth keeping this bug open with a note, if it's currently not working.
Comment 8 Kyle M Hall 2012-02-09 14:52:43 UTC
(In reply to comment #7)

I see what you mean. I feel that maybe we should have a custom subroutine that does its best to make sense of any give price field and attempt to get a single number out of it. Then GetMarcPrice would run the price through this before passing back returning the price value. I think I will make a separate bug report for it so we can close this one once it has been signed off and pushed to master.

> Other fields do currently not handle commas. But I think this is a different
> problem - you can control what you enter in Koha and train staff accordingly.
> This bug is about making use of records from a vendor or third party, so it
> might be interesting to make it recognize other currency/number formatting as
> well and convert to a form that the acquisitions module can work with.
> Perhaps it's worth keeping this bug open with a note, if it's currently not
> working.
Comment 9 Kyle M Hall 2012-02-09 19:15:49 UTC
*** Bug 7516 has been marked as a duplicate of this bug. ***
Comment 10 Kyle M Hall 2012-02-09 19:16:39 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2012-02-09 19:18:10 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2012-02-09 19:24:57 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-02-09 19:28:55 UTC
I think this one is a keeper. I marked bug 7516 that I opened for MungeMarcPrice as a duplicate and merged it into this patch. I did so because it will actually fix this problem, rather than only fixing one special case ( having more than one period in the price field ).
Comment 14 Jared Camins-Esakov 2012-03-17 09:41:02 UTC
Created attachment 8250 [details] [review]
Bug 7112 - Having two prices in 020$c causes basket creation to fail from staged marc import

The root problem here is that the price is being pulled from the MARC record
and is then run through Number::Format::unformat_number. This routine is
really being misused, and should only be used to reverse the effects of
Number::Format on a number string. We are apparently using it to strip
out currency characters and the like.

Number::Format::unformat_number will choke if there is more than one period (.)
in the price field. MARC standards do not limit this field to a single period,
so unless there is only one period, we should skip number unformatting.
Examples of that break unformat_number include '18.95 (U.S.)', and
'$5.99 ($7.75 CAN)', both of which are perfectly valid.

This commit adds the function MungeMarcPrice that will better handle
find a real price value in a given price field. It does a very good
job at finding a price in any currency format, and attempts to find
a price in whichever currency is active before falling back to
the first valid price it can find.

The variable $price may fail to have an actual price, in which case
the price then defaults to '0.00', which would be rarely if ever the
correct price. To combat this, I have added highlighting to any
price in the Order Details table that begins with 0 ( i.e. '0.00' ).

Also, fixed the incomplete table footer, adding a new td with a
span of 3 to fill in the nonexistant cells.

Signed-off-by: Jared Camins-Esakov <jcamins@cpbibliography.com>
Comment 15 Ian Walls 2012-03-19 15:57:58 UTC
Cleverly picks the best price based on active currency.  Minimal changes to C4/Biblio, little chance of regression.  Fixes template table issue.  Marking as Passed QA.
Comment 16 Paul Poulain 2012-03-20 13:56:47 UTC
patch pushed (i haven't tested it myself, but it's a small patch, so trusting others)
Comment 17 Jared Camins-Esakov 2012-05-24 00:02:01 UTC
This does not appear to have been pushed. Resetting to Passed QA.
Comment 18 Chris Cormack 2012-05-24 21:31:33 UTC
Bugfix, pushed to stable will be in 3.8.2
Comment 19 Mathieu Saby 2013-01-09 15:57:12 UTC
Very cosmetic problem introduced by this patch :

In a basket, if some prices are between 0 and 1, they are displayed in red, with class = "number error".
For example, a VAT of 0.89 euros is displayed in red.
In don't think it is normal.
I have made a new bug : http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9374

M. Saby