Created attachment 15269 [details] filetest.elc When importing a record from a staged file, sometimes, the price is set to 0 in the Vendor price field (Accounting details block) even if initially present. it occurs when the price is written without decimals : example : 19 EUR. In this case it's not captured by the regular expression in MungeMarcPrice routine (Biblio.pm) : ( $price ) = $price =~ m/([\d\,\.]+[[\,\.]\d\d]?)/; Same thing if the currency symbol was used after the digits, the result would be the same because of this : $price = $parts[1]; If I got it well, seems that the whole routine can be replaced by 2 regular expressions (may be we could even go up to a single one) integrated in GetMarcprice : the first part (symbol treatment) is unusefull as it can be treated directly by the regular expression that's following it. The part concerning groups of more than 2 digits ( if I got it, it 's come out to delete the eventual separator between thousands and hundreds) can be replaced by another regular expression. Test Plan : 1) get on your pc, the joined example file (testfile.elc). That's the kind of file most currently used in France for our acquisitions. Most of the prices in it match the pattern '[digit][digit] EUR' except 2 titles out of them : La méthodologie appliquée du commentaire de texte (14,20 EUR) Le tournant artiste de la littérature française (54,80 EUR) 2) Go to Tools/Stage MARC records for import and stage it in koha. 3) Go to Acquisition module, and an open basket of your choice and click on 'From a staged file' 4) Choose testfile.elc and add it to the basket 5) Click on the first individual 'add order' link : the price might beeing set to 00.00 in the 'Vendor price' field of Accounting details block. 6) Now go back with go back function of your browser : 7) Apply the patch 8) Click again on the same link 9) The price is now present (I hope so) 10 Sign off (I hope so again)
Created attachment 15270 [details] [review] Bug 9593 Prices not imported correctly from a staged file
Hello Lyon 3 I noticed the problem last week. I have worked on a solution of my own, and our vendor (Biblibre) has been working on it too... So I set the bug In Discussion. Mathieu Saby Rennes 2 university
My solution was changing these lines in MungeMarcPrice routine: - ( $price ) = $price =~ m/([\d\,\.]+[[\,\.]\d\d]?)/; + ( $price ) = $price =~ m/([\d\,\.]+[\,|\.\d\d]?)/; ## Split price into array on periods and commas my @parts = split(/[\,\.]/, $price); ## If the last grouping of digits is more than 2 characters, assume there is no decimal value and put it back. my $decimal = pop( @parts ); - if ( length( $decimal ) > 2 ) { + if ((scalar @parts == 0) or ( length( $decimal ) > 2 )) { M. Saby
(En réponse au commentaire 3) > My solution was changing these lines in MungeMarcPrice routine: > > - ( $price ) = $price =~ m/([\d\,\.]+[[\,\.]\d\d]?)/; > + ( $price ) = $price =~ m/([\d\,\.]+[\,|\.\d\d]?)/; > ## Split price into array on periods and commas > my @parts = split(/[\,\.]/, $price); > ## If the last grouping of digits is more than 2 characters, assume > there is no decimal value and put it back. > my $decimal = pop( @parts ); > - if ( length( $decimal ) > 2 ) { > + if ((scalar @parts == 0) or ( length( $decimal ) > 2 )) { > > > > > M. Saby Bonjour Mathieu Your solution will not work on prices with such pattern as : 1 000 EUR : To get it work you must add a space in first alternative here ( $price ) = $price =~ m/([\d\,\. ]+[\,|\.\d\d]?)/; and there : my @parts = split(/[\,\. ]/, $price); 125 € : You must delete the first part of the routine until the regular expression (may be it was implied in what you showed ?). Otherwise, when the symbol had been entered in mysql symbol field as "active currency" and is typed after the digit(s), the variable $price in $price = $parts[1] will be undef. £ 10 : I don't know if it could be frequent but in case, the result will be ".10". To avoid this, I add a mandatory digit at the beginning of regexp and replace + by * after the first alternative. Olivier Crouzet
Hello > > Your solution will not work on prices with such pattern as : > > 1 000 EUR : > To get it work you must add a space in first alternative here > ( $price ) = $price =~ m/([\d\,\. ]+[\,|\.\d\d]?)/; > and there : > my @parts = split(/[\,\. ]/, $price); You must be right! I only tested on a few records (10 EUR ; 10,50 EUR ; 10.50 EUR) > > 125 € : > You must delete the first part of the routine until the regular expression > (may be it was implied in what you showed ?). > Otherwise, when the symbol had been entered in mysql symbol field as "active > currency" and is typed after the digit(s), the variable $price in $price = > $parts[1] will be undef. > > £ 10 : > I don't know if it could be frequent but in case, the result will be ".10". > To avoid this, I add a mandatory digit at the beginning of regexp and > replace + by * after the first alternative. > Yes, I did not think of symbols... I think Biblibre is going to take part in this discussion. Mathieu
Hello, I don't understand the status "in discussion" : if this patch correct the problem, why don't you sign it off ? It can be applied in the next version of koha and biblibre can apply this patch in your university ? I'm not sure that the status in discussion is made for waiting to another solution ? Sonia (you can Recognize my perfect english..) I swith the status to "needs signoff"
Hello Sonia Biblibre told me they were working on a patch, and that your solution was maybe not the best. But they don't seem to take part in the discussion... I am going to ask them again. If they dont react, I will try to sign off tomorrow. Mathieu
Désolé... Failed QA In your marc file : Le tournant artiste de la littérature française / écrire avec la peinture au XIXe siècle by Bernard Vouilloux price = 54,80 EUR In my basket, after applying your patch : price = 54.00 EUR I also join an other marc file for testing other price schemes, like 1 125.00 EUR. M. Saby
Created attachment 15569 [details] some records to test
Created attachment 15587 [details] [review] Bug 9593 Prices not imported correctly from a staged file Oups, got it ! I did'nt see the comma's case. That's corrected.
Tested with a marc file ('some records to test') containting some records with different paterns for prices => everything is OK in created basket 010 $a 978-2-251-32888-1 $b br. sous jaquette $d 75 EUR 200 1 $a L'affirmation de la puissance romaine en Judee (63 a. C.-136 p. C.) basket : 75.00 010 $a 972-8462-28-X $b br. $d 25.12 EUR 200 1 $a Les bandits basket : 25.12 010 $a 978-2-227-48271-5 $b br. $d 21,35 EUR 200 1 $a Chocolat, clown negre basket : 21.35 010 $a 978-2-7010-1601-6 $b br. $d 49.00 EUR 200 1 $a Ephemera catholiques basket : 49.00 010 $a 2-7082-2961-3 $b br. $d 20,00 EUR 200 1 $a L'Histoire de France autrement basket : 20.00 010 $a 978-2-262-02799-5 $b br. $d 1125,52 EUR 200 1 $a Histoire de la Resistance, 1940-1945 basket : 1125.52 010 $a 978-2-07-044382-6 $b br. $d 1125 EUR 200 1 $a Ni droite ni gauche basket : 1125.00 010 $a 978-2-330-00227-5 $b br. $d 1 125 EUR 200 1 $a Ni valise, ni cercueil basket : 1125.00 010 $a 978-2-84952-166-3 $b br. $d 1 125.50 EUR 200 1 $a Nivelle basket : 1125.50 I sign off M. Saby
Created attachment 15615 [details] [review] Bug 9593 Prices not imported correctly from a staged file When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression and the variable is not initialized. Signed-off-by: Mathieu Saby <mathieu.saby@univ-rennes2.fr>
QA Comment: Like the idea of simplification! But I read in the deleted code: > This allows us to prefer this native currency price over other currency > prices, if possible. I understand that it so handles a thing like: $4, €3. Your solution makes that no longer possible. It just picks the first. Probably we still need the routine, and could you improve the code there?
Hello, I'm not sure to understand. This new patch actually handles prices like $2 and $€ : if you have a '$2' input price, it returns '2'. Is it not what is expected ? I add that the previous function MungeMarcPrice, from what I tested, did not handle prices like $2 !?! Olivier Crouzet
This is a VERY annoying bug for us. So does something be done to pass QA, or not ? M. Saby
I wanted to make some tests to see how your patch is dealing exactly with $ and €. But I got a nasty error in master when I wanted to add orders : 'Can't use string ("") as a HASH ref while "strict refs" in use at /home/msaby/kohamaster/src/C4/Items.pm line 2712.' I will create a new bug for that... I think that the function you suppressed assumed the currency symbol is always before the price, while this is not the case in every country, and especially in France. In France, we write : 15 €, 12 $. So I wonder if it was working well before... See http://en.wikipedia.org/wiki/Currency_sign : "Many currencies, especially in Latin America and the English-speaking world, place it before the amount (e.g., R$50,00); many others place it after the amount (e.g., 50.00 S₣)" Moreover, in France, according to cataloguing rules, we normally don't use those symbols, but 3 letters codes put after the price : 15 EUR, 12 USD, 10 GBP etc. Those 3 letter symbols are defined in standard ISO 4217. In the record we receive from our main providers (Electre, BNF). For Sudoc network (ABES), it is a recommandation, but symbols like $ are tolerated. Mathieu
Hello Lyon 3 I made the following test to check the way Koha was handling multi-currency prices. I defined these currencies : EUR € 1.00 (active) USD $ 0.50 Before applying your patch, I created a basket with staged records, selecting EUR currency in Accounting details (in addorderiso2709.pl). I applied the patch, and created an other basket with the same file of stage records. Here are the results. It is not good before your patch, not good after, but in a different way :( So managing multi-currency prices is a complex issue. I suggest you to rewrite your patch only to fix the initial bug (, instead of . in prices), so that we could push your fix as soon as possible. And create an other ticket for improving/fixing multi-currency prices support... What do you think of that? Detailed test : Record 1 : 010$d : 75.00 €, 30.00 $ Without your patch => in basket : 30 euros With your patch => in basket : 75 euros Record 2 : 010$d : 25.00€, 32.00$ Without your patch => in basket : 32 euros With your patch => in basket : 25 euros Record 3 : 010$d : €22.00, $56.00 Without your patch => in basket : 22 euros With your patch => in basket : 22 euros Record 4 : 010$d : $42.00, £12.00 Without your patch => in basket : 0 (uncertain price) With your patch => in basket : 0 (uncertain price) Record 5 : 010$d : 20.40 EUR, 12.50 USD Without your patch => in basket : 20.40 euros With your patch => in basket : 20.40 euros Record 6 : 010$d : 112.10 €, 110.15 $, 90.15 £ Without your patch => in basket : 110.15 euros With your patch => in basket : 112.10 euros Record 7 : 010$d : 1125.00 EUR Without your patch => in basket : 1125 euros With your patch => in basket : 1125 euros Record 8 : 010$d : €125.00 Without your patch => in basket : 125 euros With your patch => in basket : 125 euros Record 9 : 010$d : 1125.50 USD Without your patch => in basket : 1125.50 euros With your patch => in basket : 1125.50 euros Record 10 : 010$d : $1125.50 Without your patch => in basket : 0 (uncertain price) With your patch => in basket : 0 (uncertain price) Record 11 : 010$d : $10.00, €12.00 Without your patch => in basket : 0 (uncertain price) With your patch => in basket : 0 (uncertain price) Record 12 : 010$d : 10.00 $, 12.00 € Without your patch => in basket : 10 euros With your patch => in basket : 10 euros Record 13 : 010$d : 10.00 USD, 12.00 EUR Without your patch => in basket : 10 euros With your patch => in basket : 10 euros Mathieu
Created attachment 17923 [details] [review] Bug 9593 Prices not imported correctly from a staged file Hello I redid test with prices like $2 and €24, and it worked well for me. As far as regex is concerned, I see no reason that it would work for a price like '€ 22' and not for '$42'. I suspect an error during importation process, may be an encoding issue ? So I 've kept this part of the patch as it was, but, following your different observations, I restablished the possibility of choosing an 'active currency' when existing. I also add the ability to choose it from the isocode instead of symbol currency. However, I fear that it couldn 't treat every possible form of not normalized multicurrency price strings that may appear in price zone. So it's a little better than it was under this aspect but not completly satisfying. For example, it will work for string like this $2, £5 2$, 6£ $2 USD, $4 LRD 2 EUR, 5 GBP 13.5 EUR, 15 GBP but it will not for : $800 LRD, $10 US $15, 18 EUR Olivier Crouzet
Comment on attachment 17923 [details] [review] Bug 9593 Prices not imported correctly from a staged file Review of attachment 17923 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/updatedatabase.pl @@ +6785,5 @@ > } > > +$DBversion = "3.11.00.XXX"; > +if ( CheckVersion($DBversion) ) { > + $dbh->do("ALTER TABLE currncy ADD isocode VARCHAR(5) default NULL AFTER symbol;"); Typo!
Created attachment 17948 [details] [review] Bug 9593 Prices not imported correctly from a staged file typo corrected
Created attachment 17949 [details] [review] Bug 9593 Prices not imported correctly from a staged file Oups, sorry I corrected another stupidity : if ( substr($price,0,1) =~/\s*[0-9]/
Olivier, could you be more explicit in your commit message about what exactly your patch is supposed to do, and add a test plan ? Mathieu
Created attachment 18070 [details] [review] Prices not imported correctly from a staged file Here is a new version of my patch that seeks to treat correctly every form of price sequence whatever the position of the symbol and even if the iso code is used. It 's still possible to pick another price than the default one ( default=first of the price zone string) but unlike the previous mechanism of MungeMarcPrice which worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string in which it's included ('$US') or a plain latin character string ('Br'). Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates) So you can also choose to pickup a currency from its iso code. Isocode is checked first if provided and symbol then. If no active currency is defined, the first price will be systematically picked up; Beware that currency will be activated in currency table only if necessary, this to avoid unusefull checkings. Whenever active currency is enabled and neather the symbol nor the isocode is found in the price string, the resulting price is set to 0 rather than picking the first one. This, to avoid merging prices in different currencies. I furnish a little test file ("sampl_multi_currencies") where the price zones look like this : A hauteur d'homme : 75.40 EUR, 93.5 USD, 20 250 EGP A la conquête du Graal : $44, 23 €, 64 Br, £30 Bernanos et les âges de la vie : 25,5 EUR, $US32, $LD35 Chateaubriand et la gravité du comique : 38 Ksh, ¥300, $62 Ecrire la sculpture : 32 EUR, 45$ CAN, 46$ USD I customized it with MarcTools : it's important to note that in MarcTools the dollar as currency sign has to be wrote down {dollar} not to be taken as a separator and truncate the price zone. Test Plan for multi currencies price zone : 1) Go to Pro interface Administration/Currencies and exchange rates and enter the data for the currency you would like to be privileged when different currencies are present. Mean that you make it active of course. 2) Get on your pc, an isofile with multi currencies price zone. 3) Go to Tools/Stage MARC records for import and stage it in koha. 4) Go to Acquisition module, and an open basket of your choice and click on 'From a staged file' 5) Choose your test file and add it to the basket 6) Click on an individual 'add order' link where you know the 'active currency' is not on first price and is following the digit part : the price might beeing set to 00.00 in the 'Vendor price' field of Accounting details block. 7) now Apply the patch As a modification had to be made in currency sql table, the database update should be made eather through the Pro interface or if not proposed automatically, through the command line : cd src/installer/data/mysql perl updatadatabase.pl 9) Go to Pro interface Administration/Currencies and exchange rates and add the isocode for your active currency you would like to be privileged when several currencies are present you can make different test with both symbol AND isocode defined or only symbol. 10) Go back to Acquisition module and your staged test file 11) Redo several tests on the same link changing currency/isocode sign in currency table between every test, you can a) go back with go back function of your browser b) if you have 'lost' your link because it ends up to be really imported, go to Tools/Staged MARC record management and clean your file test. Then redo stages 3,4 and 5 12) The price you asked for might now be present (I hope so) 13) Repeat the tests on another link 14) At the end, you can disabled your active currency in Currencies and exchange rates (setting to 0) and redo same tests. You will see that the first currency of price zone is picked up as expected 15) Sign off
Created attachment 18075 [details] sample_multicurrencies
My 1st test, just to check the new behavior of currency module => seems ok My test plan: 1. $perl installer/data/mysql/updatedatabase.pl Upgrade to 3.11.00.XXX done (Added isocode to the currency table) 2. going to admin/currency.pl page : there is a new column for iso code => ok 3. editing my two currencies (euro and dollar) to add iso codes (EUR and USD) => ok 4. adding a new currency (sterling pound) with symbol £ and iso code GBP => ok 5. suppressing a currency => ok 6. changing the active currency => ok I am now testing the import of marc record Mathieu
Following of my test : add currencies matching codes and symbols in your records CANADA 1.00000 $ CAN DOLLAR 1.00000 $ USD EGYPT 1.00000 £E EGP EUR 1.00000 € EUR KENYAN SHI 1.00000 KSh KES LIVRE 1.00000 £ GBP YEN 1.00000 ¥ JPY (CAN should be CAD I suppose, but this is cosmetic) active currency : EUR Checking 010$d fields of your records in with yaz-marcdump. Staging records Filling a basket with staged records, selecting currency : EUR. Checking content of the basket : A hauteur d'homme / Rousseau et l'écriture de soi by Lionel Bourg <subfield code="d">75.40 EUR, 93.5 USD, 20 250 EGP</subfield> RRP tax exc : 75.40 A la conquête du Graal by Alicia Bekhouche <subfield code="d">$44, 23 €, 64 Br,£30</subfield> RRP tax exc : 23.00 Bernanos et les {copy}ages de la vie / actes du colloque d'Aix-en-Provence, 23, 24, 25 octobre 2008 by sous la direction de André Not <subfield code="d">25,5 EUR, $US32, $LD35</subfield> RRP tax exc : 25.50 Chateaubriand et la gravité du comique by Fabio Vasarri <subfield code="d">38 Ksh, ¥300, $62</subfield> RRP tax exc : 0 (uncertain) Ecrire la sculpture, XIXe-XXe siècles / actes du colloque organisé à l'Ecole normale supérieure et à la Maison de la recherche du 16 au 18 juin 2011 by sous la direction d'Ivanne Rialland <subfield code="d">32 EUR, 45$ CAN, 46$ USD</subfield> RRP tax exc : 32.00 => problem with "Chateaubriand et la gravité du comique". But I suppose we would never find such thing as "38 Ksh, ¥300, $62" in 010$d (more likely "Ksh38, ¥300, $62", or "38 Ksh, 300¥, 62$"). I will make other tests later Mathieu
Hey Mathieu, My god, you are everywhere ! fallen in a magic potion barrel when a baby ? As for Chateaubriand case, it's a normal result with euro as active currency because there's no euro currency in the string price '38 Ksh, ¥300, $62' As I 'd mentionned : Whenever active currency is enabled and neather the symbol nor the isocode of this currency is found in the price string, the resulting price is set to 0 (rather than picking the first one by default). I did this knowingly to make clear that no price has been picked. If first price would be picked, in your test context,38 would be the resulting price and we could think it's in euro. Olivier
I did not read all your description, I must confess ;-) Not sure to agree with this behavior if it is a new one. I think it should be : - in Koha, 3 currencies = euros €, dollar $, pound £ ; and active currency = euro - in 010 : 40 €, 50 $ => 40 € - in 010 : 50 $ => convert 50 $ in € using the exchange rate defined in Koha - in 010 : 50 $, 60 ¥ => convert 50 $ in € using the exchange rate defined in Koha (¥ should be ignored, as ¥ is not defined in Koha list of currencies) - in 010 : 60 ¥ => 0 (undefined) (¥ should be ignored, as ¥ is not defined in Koha list of currencies) - in 010 : 50 $, 30 £ => 2 solutions : - either 0 (undefined) - or maybe convert the first price of the list in euros, using exchange rate What's your opinion? Mathieu
Created attachment 18265 [details] [review] Bug 9593 Prices not imported correctly (plus conversion) My opinion is that seems a good idea but that would be really interesting to have the opinion of libraries likely to use the active currency mechanism. In Lyon3, we don't need it, so I have no view from experience about that. Anyhow, you'll find here an other version of patch (that do not necessarily cancel the previous one). It includes your idea to pick the first other currency found in the list (when active is not present in the price string) and convert it to active currency. Olivier
I will take a look tomorrow. we don't use it either in Rennes2, and I think no french library is using it. Maybe you can ask the question on koha list. Mathieu
Hello. I tested your 2d patch. It needs to be rebased (conflict in updatedatabse, and need to remplace 3.11.XXX by 3.13.XXX) The result is the same, except for Chateaubriand et la gravité du comique by Fabio Vasarri => 300.00 This is not good, but as I put before, this kind of data is not supposed to be found... <subfield code="d">38 Ksh, ¥300, $62</subfield> I will edit your sample batch of record, so it would be easier to test. Mathieu
Olivier, With your 2d patch, I made 4 tests, with a batch of 19 records (I join them) tva: 0 discount : 5% test A : Euro as active currency, all currency rates = 1 ; selecting "Euro" currency in addorderiso2709.pl (in Accounting details) test B : Euro as active currency ; rates : 1 € = 10 $ = 4 £ ; selecting "Euro" currency in addorderiso2709.pl (in Accounting details) test C : GB Pound as active currency ; rates : 1 £ = 10 $ = 5 € ; selecting "Pound" currency in addorderiso2709.pl (in Accounting details) test D : GB Pound as active currency ; rates : 1 £ = 10 $ = 5 € ; selecting "Euro" currency in addorderiso2709.pl (in Accounting details) A, B, C are OK D seems not OK, but I am not sure to understand how the currency you can choose when importing the record is supposed to interact with the currency stored of the record. What Koha is supposed to do if there is a price with £ symbol in the record, but if we select "euro" while importing? Maybe the currency choosen when importing is made to be used if the prices have no currency symbol in the record?? I did not test this point without your patch. Perhaps it is broken... If it is the case, I would agree to sign it off, as as the problem it fixes has been noticed in january. And we can fix it in an other patch. Could you give me your analysis? If you are as lost as myself, you can ask the koha-devel list ;-) Titre01 75.40 EUR, 93.5 USD, 20 250 GBP test A and test B : OK - should be : 75.40 - result : 75.40 test C : OK - should be : RRP = 20250.00 ; Total tax inc = 19237.50 - result : RRP : 20250.00 ; Total tax inc. (POUND) : 19237.50 test D : KO - should be : don't know - result : RRP : 4050.00 ; Total tax inc. (POUND) : 19237.50 => it means Koha knows £ is the active currency, takes the 20250 pounds price, BUT after that, the system considers it is 20250 euros, so it divides it by 5 (according to currency rate), to establish a RRP price of 4050£ AND rather strangely, the Total tax inc. price is calculated according to the raw value in the record (20250) and not to the wrongly calculated price of 4050£. Titre02 75.40 EUR, 93.5 USD, 20,250 GBP test A and test B : OK - should be : 75.40 - result : 75.40 test C : OK - should be : RRP = 20250.00 ; Total tax inc = 19237.50 - result : RRP : 20250.00 ; Total tax inc. (POUND) : 19237.50 test D : KO - should be : don't know - result : RRP : 4050.00 ; Total tax inc. (POUND) : 19237.50 Titre03 75.40 EUR, 93.5 USD, 20,250.10 GBP test A and test B : OK - should be : 75.40 - result : 75.40 test C : OK - should be : RRP = 20250.10 ; Total tax inc = 19237.60 - result : RRP : 20250.10 ; Total tax inc. (POUND) : 19237.60 test D : KO - should be : don't know - result : RRP : 4050.02 ; Total tax inc. (POUND) : 19237.60 Titre04 €75.40, $93.5, £20,250.10 test A and test B : OK - should be : 75.40 - result : 75.40 test C : OK - should be : RRP = 20250.10 ; Total tax inc = 19237.60 - result : RRP : 20250.10 ; Total tax inc. (POUND) : 19237.60 test D : KO - should be : don't know - result : RRP : 4050.02 ; Total tax inc. (POUND) : 19237.60 Titre10 32,10 EUR, 45,12 USD test A and test B : OK - should be : 32.10 - result : 32.10 test C : OK - should be : RRP = 6.42 (= 32.10/5) ; Total tax inc = 6.10 - result : = 6.42 (= 32.10/5) ; Total tax inc = 6.10 test D : KO - should be : don't know - result : RRP : 1.28 ; Total tax inc. (POUND) : 6.10 => not good : Koha took the first price (32.10), and considered it is euro. I don't know how, but for RRP price, it divided it by 5, and then again by 5 (32.10/5/5 = 1.28). For Total tax inc, it only took 32.10 /5 (minus the discount), that is correct. Titre11 32,10 €, 45,12 $ test A and test B : OK - should be : 32.10 - result : 32.10 test C : OK - should be : RRP = 6.42 (= 32.10/5) ; Total tax inc = 6.10 - result : = 6.42 (= 32.10/5) ; Total tax inc = 6.10 test D : KO - should be : don't know - result : RRP : 1.28 ; Total tax inc. (POUND) : 6.10 Titre12 32.10, USD 45.12 test A and test B : OK - should be : 32.10 - result : 32.10 test C : OK - should be : RRP = 6.42 (= 32.10/5) ; Total tax inc = 6.10 - result : = 6.42 (= 32.10/5) ; Total tax inc = 6.10 test D : KO - should be : don't know - result : RRP : 1.28 ; Total tax inc. (POUND) : 6.10 Titre13 €32.10, $45.12 test A and test B : OK - should be : 32.10 - result : 32.10 test C : OK - should be : RRP = 6.42 (= 32.10/5) ; Total tax inc = 6.10 - result : = 6.42 (= 32.10/5) ; Total tax inc = 6.10 test D : KO - should be : don't know - result : RRP : 1.28 ; Total tax inc. (POUND) : 6.10 Titre20 300 ¥, 62.20 $ test A : OK - should be : 62.20 - result : 62.20 test B : OK - should be : 6.22 - result : 6.22 test C : OK - should be : RRP = 6.22 (= 62.20/10) ; Total tax inc = 5.91 - result : RRP = 6.22 ; Total tax inc = 5.91 test D : KO - should be : don't know - result : RRP : 1.24 ; Total tax inc. (POUND) : 5.91 => same as before : RRP = raw price / 25 ; total tax inc. = raw price / 5 minus discount Titre22 ¥300, $62.20 test A : - should be : 62.20 - result : 62.20 test B : - should be : 6.22 (as 10 $ = 1 €) - result : 6.22 test C : OK - should be : RRP = 6.22 (= 62.20/10) ; Total tax inc = 5.91 - result : RRP = 6.22 ; Total tax inc = 5.91 test D : KO - should be : don't know - result : RRP : 1.24 ; Total tax inc. (POUND) : 5.91 Titre23 300 YEN, 62.20 USD test A : - should be : 62.20 - result : 62.20 => OK test B : OK - should be : 6.22 - result : 6.22 test C : OK - should be : RRP = 6.22 (= 62.20/10) ; Total tax inc = 5.91 - result : RRP = 6.22 ; Total tax inc = 5.91 test D : KO - should be : don't know - result : RRP : 1.24 ; Total tax inc. (POUND) : 5.91 Titre24 YEN 300, USD 62.20 test A : - should be : 62.20 - result : 62.20 test B : OK - should be : 6.22 - result : 6.22 test D : OK - should be : RRP = 6.22 (= 62.20/10) ; Total tax inc = 5.91 - result : RRP = 6.22 ; Total tax inc = 5.91 test D : KO - should be : don't know - result : RRP : 1.24 ; Total tax inc. (POUND) : 5.91 Titre30 75 EUR test A and B : OK - should be : 75.00 - result : 75.00 test C : OK - should be : RRP = 15£ (=75€/5) ; Total tax inc. : 14.25£ - result : RRP : 15 ; Total tax inc. (POUND) : 14.25 test D : - should be : don't know - result : RRP : 3 ; Total tax inc. (POUND) : 14.25 => same as before : RRP = raw price / 25 ; total tax inc. = raw price / 5 minus discount Titre31 €75 test A and B : OK - should be : 75.00 - result : 75.00 test C : OK - should be : RRP = 15£ (=75€/5) ; Total tax inc. : 14.25£ - result : RRP : 15 ; Total tax inc. (POUND) : 14.25 test D : - should be : don't know - result : RRP : 3 ; Total tax inc. (POUND) : 14.25 Titre32 EUR 75 test A and B : OK - should be : 75.00 - result : 75.00 test C : OK - should be : RRP = 15£ (=75€/5) ; Total tax inc. : 14.25£ - result : RRP : 15 ; Total tax inc. (POUND) : 14.25 test D : KO - should be : don't know - result : RRP : 3 ; Total tax inc. (POUND) : 14.25 Titre33 75 € test A and B : OK - should be : 75.00 - result : 75.00 => OK test C : OK - should be : RRP = 15£ (=75€/5) ; Total tax inc. : 14.25£ - result : RRP : 15 ; Total tax inc. (POUND) : 14.25 test D : KO - should be : don't know - result : RRP : 3 ; Total tax inc. (POUND) : 14.25 Titre34 ¥300 all tests : OK - should be : 0 - result : 0 => OK Titre35 300 YEN all tests : OK - should be : 0 - result : 0 => OK Titre36 300 ¥ all tests : OK - should be : 0 - result : 0 => OK Mathieu
Created attachment 18405 [details] 19records-testSaby-20130425 the records I used for testing
I made again test D, but WITHOUT your patch. GB Pound as active currency ; rates : 1 £ = 10 $ = 5 € ; selecting "Euro" currency in addorderiso2709.pl (in Accounting details) I call it test E Titre01, Titre02, Titre03, Titre30, Titre31, Titre32, Titre33 are broken, but that's logical. Titre04 has the same values as test D €75.40, $93.5, £20,250.10 test D & E - result : RRP : 4050.02 ; Total tax inc. (POUND) : 19237.60 Titre10, Titre11, Titre12, Titre13 as values different from test D 32,10 EUR, 45,12 USD Test D - result : RRP : 1.28 ; Total tax inc. (POUND) : 6.10 Test E - result : RRP : 6.42 ; Total tax inc. (POUND) : 30.50 Titre20, Titre21, Titre22, Titre24 as values different from test D Test D - result : RRP : 1.24 ; Total tax inc. (POUND) : 5.91 Test E - result : RRP : 12.44 ; Total tax inc. (POUND) : 59.09 So your patch seems to change Koha behavior on that point. Could you check it why? Mathieu
Created attachment 18640 [details] [review] Bug 9593 Prices not imported correctly from a staged file Hello Mathieu The last patch I proposed was not coherent with the possibility to choose in currency choice list of Accounting detail block a different currency than the active one. It could produce a double conversion of the price. So, at the end, I propose again the previous patch(with marginal modifications). You wrote in comment 28 "Not sure to agree with this behavior if it is a new one" but it's not. Even if it modifies thoroughly the MungeMarcPrice routine, it keeps the main logic ( picking the active currency if possible) with the advantage of finding it whatever the position of symbol or isocode regarding the digits part. Olivier
Applying: Bug 9593 Prices not imported correctly from a staged file Using index info to reconstruct a base tree... M C4/Biblio.pm M installer/data/mysql/kohastructure.sql M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/kohastructure.sql Auto-merging C4/Biblio.pm Patch failed at 0001 Bug 9593 Prices not imported correctly from a staged file The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 21274 [details] [review] Bug 9593 Prices not imported correctly from a staged file Patch rebased
Patch applied cleanly, go forth and signoff
Hello Koha Team Lyon 3, I have made tests with filetest.elc, some records to test, and sample_multicurrencies. First I have tested with currency in Euro => OK After I have tested in Dollar the case : 25,5 EUR, $US32, $LD35 is "Uncertain" for the others it's ok. For me you have done a great job, with your patch many cases are taken into account. Even if $US32 is not recognised, I think $US 32 will be, I am ok to sign of, because it's a good improvement and without the patch it's awful. Tell me if you want to add something, or I'll sign of tomorrow. Thx for the patch
As I said, it's ok for me. Signed off
Created attachment 21477 [details] [review] patch signed off
Hey Christophe I posted this one the same minute as yours and there was a collision !. I saw you just signed off but here is my comment nevertheless : I tested the string you told of ('25.5 EUR, $US32, $LD35') with symbol='$US' in a separated perl script, and I get 32 as expected, same for $LD. But I know that testing this import into koha may not be so easy. May be some cache issues caused by testing in a loop or test file with encoding errors ? For me, the important thing is that if the string is not found the returned price must be 0. ( better not price at all than a false one) Anyway, I could not swear it will cover whatever kind of price string but if it can deal with main part of not too screwy ones, you are welcome to sign it off. Oliver C.
QA comment: nice patch, but one question: why did you put + ( $subfield_value ) = $subfield_value =~ m/(\d[\d\,\. ]*\d{0,2})/; + # remove comma,dot or space when used as separators from hundreds + $subfield_value =~s/[\,\. ](\d{3})/$1/g; + # convert comma to dot to ensure correct display of decimals if existing + $subfield_value =~s/,/./; outside from MungeMarcPrice, it should be here as well, because it's related to discovering & retrieving the price from the MARC field So I think it should go here Otherwise : * great regexp description ! * patch does not apply anymore due to updatedatabase changes Failing QA, answer my question 1, provide an updated patch if my point is relevant and switch back to signed off
Created attachment 22151 [details] [review] Bug 9593 Prices not imported correctly from a staged file I did as you suggested and rebased. (Indeed,I could not remember any reason why I put it outside if ever so !). Olivier Crouzet.
Created attachment 22541 [details] [review] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code.
The patch was signed off. I'm fixing the status. Mathieu
The patch does not have a sign-off line. Could whoever meant to sign it please do so. As the original author of MungeMarcPrice I fully endorse these changes. However, I'd really like to see some unit tests to test all these various price string situations. This sub should be one of the absolute easiest ones to write unit tests for since all it does is take in a price string and return a price string.
Created attachment 22820 [details] [review] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code.
It was signed off the 26th september by C.Croullebois, in comment 41, (attachment 21477 [details] [review]), but a new patch was added the 21st october 'attachment.cgi?id=22151) without signoff line (see comment 44). I think Lyon 3 deleted by mistake the signoff line. Lyon 3, could you confirm that ? Mathieu
I know very few about unit test. Would something like that do the trick ? #!/usr/bin/perl use strict; use warnings; use C4::Biblio; use Test::More tests=>1; # start transaction my $dbh = C4::Context->dbh; $dbh->{AutoCommit} = 0; $dbh->{RaiseError} = 1; # set a test price string my $PRICE_STRING='25.5 EUR, $32, $LD35'; my $expectedout='32'; # set active currency test data my $CURRENCY = 'TEST'; my $SYMBOL = '$'; my $ISOCODE = 'USD'; my $RATE= 1; # disables existing active currency if necessary. my $active_currency = C4::Budgets->GetCurrency(); my $curr; if ($active_currency) { $curr = $active_currency->{'currency'}; $dbh->do("UPDATE currency set active = 0 where currency = '$curr'"); } $dbh->do("INSERT INTO currency ( currency,symbol,isocode,rate,active ) VALUES ('$CURRENCY','$SYMBOL','$ISOCODE','$RATE',1)"); my $mungemarcprice=MungeMarcPrice($PRICE_STRING); ok ($mungemarcprice eq $expectedout, "MungeMarcPrice returned $expectedout as expected"); # Cleanup $dbh->rollback; Olivier Crouzet
Hello Olivier I think it's the idea... But the idea of UT is also to test different configuraton. So, you should not only test my $PRICE_STRING='25.5 EUR, $32, $LD35'; my $expectedout='32'; but also made 3 or 4 other cases like '25,00 EUR' or '25 EUR' or '25 €' for example Also, could you confirm that the original patch was signed off by C. Croullebois, and restore its sign-off line? So that it will be clear that we are at "QA" stage, and not at "Need sign off" ? Mathieu
Created attachment 23580 [details] [review] Bug 9593 Prices not imported correctly from a staged file I added a unit test file (MungeMarcFile.t) and made slight modifications in MungeMarcPrice function in order that any currency signs could'nt be captured (even ones with slash and dots).
oups ! I meant "in order that any currency sign COULD BE captured"
*** Bug 7504 has been marked as a duplicate of this bug. ***
Created attachment 24086 [details] [review] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code.
rebased patch.
Created attachment 25984 [details] [review] Bug 9593 Prices not imported correctly from a staged file patch updated and rebased
Created attachment 26125 [details] [review] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code. Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>
Created attachment 26127 [details] File used to sign-off patch Patch tested with file liste.elec Without patch, all prices without decimals are replaced by 0 in order. With the patch, all prices are correctly set in order
Patch still applies, but there is a complaint from the QA script: FAIL t/db_dependent/MungeMarcPrice.t OK pod OK forbidden patterns OK valid FAIL critic I/O layer ":utf8" used at line 11, column 1. Use ":encoding(UTF-8)" to get strict validation. I/O layer ":utf8" used at line 12, column 1. Use ":encoding(UTF-8)" to get strict validation. Could you take a look at switch back to 'signed off' please?
Created attachment 27006 [details] [review] Bug 9593 Prices not imported correctly from a staged file I just changed ":utf8" for ":encoding(UTF-8)" in test unit as preconized.
Created attachment 27009 [details] [review] Bug 9593 - Additional Unit Tests
I'm afraid it appears that this patch introduces some regressions. If you try the examples from the original bug report that added MungeMarcPrice, you'll see that some variations fail that previously would not. I've attached a followup that with tests that pass with the original version of the subroutine but fail with the new version.
Created attachment 27123 [details] [review] Bug 9593 Prices not imported correctly from a staged file Well, prices string like '$9.99 USD' are now treated correctly. When neither currency symbol nor isocode is found like '18.95 (U.S.)', the first price is picked as done in previous routine. I included new test strings in MungeMarcPrice.t.
Created attachment 27124 [details] [review] Bug 9593 Prices not imported correctly from a staged file slight correction in comments
Just 2 notes about this, leaving final QA to Kyle: - I think the note on the currency page is misplaced. It would be better using a diferent styling (similar to other hints) below the table. - I am a bit torn about adding a new column for ISO code. The sample Koha currencies already have the ISO code as code and the overdue_notices.pl that prints the fine into the notices also assumes code to be the ISO code. Maybe we should just enforce the code to be the ISO code a bit more?
> - I think the note on the currency page is misplaced. It would be better > using a diferent styling (similar to other hints) below the table. I agree with this. In addition, I think it should be worded more like: When importing MARC files via the staging tools, the tool will attempt to find and use the price of the currently active currency. > - I am a bit torn about adding a new column for ISO code. The sample Koha > currencies already have the ISO code as code and the overdue_notices.pl that > prints the fine into the notices also assumes code to be the ISO code. Maybe > we should just enforce the code to be the ISO code a bit more? I agree, the default data convention is for the "Currency" field to be the ISO code. However, that doesn't mean that everyone is currently following this convention. I *don't* think this is a blocker. I could easily see the Currency being "Brazilian real" while the ISO code would be "BRL". Since not everyone memorizes ISO codes for currency, I think this may be an improvement anyway.
I wanted to note it - adding the column is ok with me.
Created attachment 27391 [details] [review] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 27392 [details] [review] Bug 9593 [QA Followup]
Created attachment 27657 [details] [review] [PASSED QA] Bug 9593 Prices not imported correctly from a staged file Initial bug : When there's a round price with no decimals after it, or when the symbol is after the digits, the price is not captured by regular expression in MungeMarcPrice routine and the variable is not initialized. Enhancement : The MungeMarcPrice routine had been widely modified. It 's still possible to priority pick the active currency but unlike the previous mechanism that worked only for prices preceded by the currency sign, it's now valid wherever the symbol is situated. As symbol you may enter a pure currency sign as well as a string including it like '$US'. Moreover, an 'isocode' column had been added in currency table (editable in the pro interface from Administration/Currencies and exchange rates). So the active currency can be picked either through its symbol or through its iso code. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Passes all tests, especially t/db_dependent/MungeMarcPrice.t Checked currencies can be added, edited and deleted. Notes: new ISO code field is mandatory. Sample sql files need to be updated (bug 12146)
Created attachment 27658 [details] [review] [PASSED QA] Bug 9593 [QA Followup] Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
I am still a bit torn about this patch and the new column. I think we need a follow up to clean up the sample files at least. Also the new field being mandatory means that you are forced to fill it in on editing now, but I see no way to get around that. Does it really need to be mandatory?
(In reply to Katrin Fischer from comment #74) > I am still a bit torn about this patch and the new column. I think we need a > follow up to clean up the sample files at least. > Also the new field being mandatory means that you are forced to fill it in > on editing now, but I see no way to get around that. Does it really need to > be mandatory? I don't think it needs to be mandatory, and I also think that if isocode is null, that it would be reasonable to fall back to checking for the main currency code/name (currency.currency) when parsing the prices. I'm willing to deal with that as an RM follow-up, but I would appreciate confirmation from the patch submitter that there's no issue with my proposed change.
(In reply to Kyle M Hall from comment #68) > I could easily see the > Currency being "Brazilian real" while the ISO code would be "BRL". Note that currency.currency is only a varchar(10).
Pushed to master, along with a follow-up implementing my suggestion to not make the ISO code required and to fall back to the currency code/name if the ISO code is not present. Thanks, Olivier!
Patch too complex and big to backport to 3.14.x
Sorry to be very late, I missed your question in comment 75. The isocode had to furnished in currencies table in order the whole thing to work whenever the prices string includes the isocode form. If you are sure that all your prices will only use the symbol form, it's ok but I doubt it could be always true. So I had made it required when entering data in table currencies. It could be left optionnal, but at least 2 things would have to be added : 1) Resetting an isocode column in the currencies table : now, you have to click the edit button to be aware of the existence of an isocode parameter, I think this column must appear anyway. 2) Indicating clearly in the legend that the isocode data is required for prices string that include it. Olivier Crouzet
I think it would be better to discuss any changes on a new bug report - as this is already in released versions.