c2c_currency_rate_update outdated XML lib and a crash

[Alexis de Lattre]

I am currently testing the module "c2c_currency_rate_update" from extra-addons, stable branch. I am using the current stable branch for server and addons.

The great news is that I managed to update several currencies with the service "Yahoo Finance" !

But it seems I found 2 issues with the module. I am a beginner in Python, so I may be wrong or say stupid things, please take it into account when reading this.

1) First issue : deprecated pyxml lib makes ECB and Admin.ch service unusable ?

In the file currency_rate_update.py, for the service ECB (European Central Bank) and Admin.ch, there is an import of a library :

from xml import xpath

(line 365 for Admin.ch and 417 for ECB)

For what I understand, this lib is part of the pyXML lib, available from this project : http://sourceforge.net/projects/pyxml/
Under Ubuntu, the lib was provided by the package python-xml. Since Karmic, this lib is no longer provided, see http://packages.ubuntu.com/search?keywords=python-xml&searchon=names&suite=all&section=all
Probably, the reason for that is written on http://sourceforge.net/projects/pyxml/ :

PyXML is no longer maintained.

So, shouldn't we use another XML lib ? If yes, which one do you suggest ?

2) if I start the server with --log-level=debug, when I do anything in the "Currency autoupdate configuration" (under Administration > Users > Companies) and save it, or when I click on Refresh currencies, or when the scheduled action to refresh currencies is executed, I get the following crash in the logs :

[2010-09-22 16:12:38,161][erp_des4] DEBUG:sql:bad query: select company_id from res_currency
[2010-09-22 16:12:38,162][erp_des4] DEBUG:sql:None
[2010-09-22 16:12:38,162][erp_des4] DEBUG:sql:[01]: ERREUR: la colonne « company_id » n'existe pas
[2010-09-22 16:12:38,162][erp_des4] DEBUG:sql:[02]: LIGNE 1 : select company_id from res_currency
[2010-09-22 16:12:38,163][erp_des4] DEBUG:sql:[03]: ^
[2010-09-22 16:12:38,164][erp_des4] DEBUG:sql:[01]: File "/usr/lib/python2.6/threading.py", line 504, in __bootstrap
[2010-09-22 16:12:38,164][erp_des4] DEBUG:sql:[02]: self.__bootstrap_inner()
[2010-09-22 16:12:38,164][erp_des4] DEBUG:sql:[03]: File "/usr/lib/python2.6/threading.py", line 532, in __bootstrap_inner

I confirm there is no company_id field on the res_currency object !

Do you confirm these two issues ? If yes, I am willing to spend time to try to fix them, but :
- for point n°1, I need some advice on which python XML lib we should use now
- for point n°2, I will probably need some help, because I don't see what triggers this bad SQL query...

[Alexis de Lattre (alexis-via)]

Here is a discussion about the removal of the package python-xml from Ubuntu and Debian :

https://bugs.launchpad.net/ubuntu/+source/python-xml/+bug/343242

The same issue happened in openerp-server and addons, as detailed in this bug report :

https://bugs.launchpad.net/openobject-server/+bug/429519

They say that they replaced pyXML with "lxml + etree standard Python XML libs". We should probably do the same, no ?

[Raphaël Valyi - http://www.akretion.com (rvalyi)]

Hello,

the right lib that is being used now in OpenERP is lxml2 via the lxml library here: http://codespeak.net/lxml/
That library is already used be the recent OpenERP V5 and v6 versions.

All you need for xpath support is an initial import such as:
from lxml import etree

Then if you want examples, of xpath manipulation, look in server/bin/osv/orm.py or in V6 at least server/bin/report/interface.py

Hope this helps

[Alexis de Lattre (alexis-via)]

I have started the work on migrating from the old XML lib to the new one, as suggested by Raphaël. I started with the ECB (European Central Bank) service. You will find enclosed a "work in progress" version of the patch, which works for the ECB service with the new lib.

Warning : this is a work in progress version of the patch, with debug "print", etc... It's not meant for inclusion ! It's meant for discussion and comments.

If some of you disagree with the new code, please say it NOW, before I start working on the other services (Admin.ch also needs to be converted to the new lib).

[Alexis de Lattre (alexis-via)]

In fact, I will rewrite my current code to use the "objectify" API of lxml, which is much easier to use and to read.

[Alexis de Lattre (alexis-via)]

Here is a new version of my patch which uses the "objectify" API of the lxml lib. The work has been made for both ECB and Admin.ch currency rate services.

It is still a development version of the patch, with my debug prints. All comments are welcomed.

[Grzegorz Grzelak (OpenGLOBE.pl) (grzegorz-og.pl)]

Hello, I have checked your patch of ECB getter for 4 currencies (USD, CHF, GBP and PLN). Looks that works fine but when I run it manually first time (using button "Refresh currencies") I've got error message with "MODE_NAME" see the log trace:

!!! 2010-10-07 11:20:36.468989 'ECB_getter' object has no attribute 'MOD_NAME' !!!
 currency updated at 2010-10-07 11:21:34.180412
 currency updated at 2010-10-07 11:22:31.414384
 currency updated at 2010-10-07 11:23:45.910160
 !!! 2010-10-07 11:35:16.288617 'ECB_getter' object has no attribute 'MOD_NAME' !!!
 currency updated at 2010-10-07 11:35:44.715450
 currency updated at 2010-10-07 12:10:21.541906
 currency updated at 2010-10-07 12:10:37.775501
 currency updated at 2010-10-07 12:11:44.438196

It was just when I enter first time into the "Currency auto update configuration" tab. Every next run was ok as you can see. And this error message didn't appear any more after server restarting. It is probably not related to your changes any way. Will you add your changes to extra-addons branch? If so I will make similar changes to polish NBP getter (as I see you have no plan to do it).

I have tried to fix this problem using 4suite package for python. And I was successful but this package is not standard so I didn't published my changes. Your solution looks better anyway. For any case I enclose my file if it can help with something. Changes are applied only to polish getter.

[Alexis de Lattre (alexis-via)]

Dear Grzegorz,

Thanks for your participation on this bug report !

I didn't see that the "NBP" service was using the old XML lib... I just missed it ! I will do the work for the NBP also, don't worry.

Yes, I plan to add my changes to the extra-addons branch, but I don't have commit rights myself, so I will depend on someone with commit rights accepting my patch. I've seen that you have the commit rights and you have already contributed to this module, so you will be able to commit my patch proposal, when it will become final.

Here is what left on my roadmap before I consider my patch as final :
1) convert NBP service
2) in company.py, in the function "def _multi_curr_enable", I plan to change the following code :
    try :
            cr.execute('select company_id from res_currency')
            cr.fetchall()
     except:
            cr.rollback()
            enable = 0
I want to avoid going through an exception just to test if res_currency has a company_id field, which should solve the issue n°2 I mentionned in my bug report.
3) Have a look at the issue you mentionned with 'ECB_getter' object has no attribute 'MOD_NAME'. I guess it is caused by the code line 316/318, I am not sure self.MOD_NAME exists, because MOD_NAME is just defined as a string in currency_rate_update class... I'll ask some guys with more python experience than me.
4) Do more tests with the new code, then remove the debug print.

I hope to have a final version of the patch next week.

[Alexis de Lattre (alexis-via)]

I have found a third bug in the c2c_currency_rate_update module :
- if you try to update 'CHF' with Admin.ch webservice
- OR if you try to update 'EUR' with the ECB webservice
- OR if you try to update 'PLN' with the PL NBP webservice
it doesn't work and you get an error message "cannot import name xpath !!!".

I have fixed this third issue too. So I don't have any more bugs in my TODO list for the c2c_currency_rate_update module...

By the way, althouth the objectify API of lib lxml was cool, I came back to the traditionnal xpath approach and used the "firebug" firefox extension to help me with xpath generation.

I now need to figure out how to do a merge proposal to submit my code...

[Alexis de Lattre (alexis-via)]

Oups, sorry, my previous comment was wrong ; the error message "cannot import name xpath !!!" is caused by the absence of the old pyXML lib, not by the issue that I describe ! Sorry for this mistake.

But the third bug is still valid. In fact, the exact issue depend on the Webservice :
- Admin.ch : if your main_currency != CHF, then you can't update the rate of the 'CHF' currency (but it will work fine for other currencies)
- ECB : the computation is wrong if main_currency != EUR
- PL NBP : the computation is wrong if main_currency != PLN

I confirm that my work will fix this issue too.

[Alexis de Lattre (alexis-via)]

OK, I have just submitted my merge proposal from lp:~akretion-team/openobject-addons/fixes_for_c2c_currency_rate_update to lp:openobject-addons/extra-5.0

It is now in the review process.

[Alexis de Lattre (alexis-via)]

Any update about the review of my merge proposal ?

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

It is running but I am totally under flooded.
I have not tested all configurations and services. I have tested the swiss and the Yahoo providers yet.

But I have to test the others.

I think I'm gonna do a less deep testing and commit. Tickets will fly on there own.

Regards

Nicolas

[Alexis de Lattre (alexis-via)]

Nicolas,

I have not modified the code of the Yahoo webservice, I have only modified the code of the XML-based webservice i.e. admin.ch, ECB and PL NBP.

[Amit Parik (amit-parik)]

Hello Alexis,

"c2c_currency_rate_update" module is in extra addons branch. Our R&D Teams are focused on the latest OpenERP version, and this issue does not affect it. So i am closing this bug.

Thanks.

[Alexis de Lattre (alexis-via)]

Why the hell do you change the assignee ?
Why the hell do you change the status ?

I understand that Tiny's R&D Team is focusing on other tasks, which is absolutely great, so please let me and other members of the community work on this bug, so please don't put the mess in our work !

[Vinay Rana (OpenERP) (vra-openerp)]

I am reassigning to related module author of this bug.

Thanks.

[Alexis de Lattre (alexis-via)]

By the way, I just added an important new feature in the branch lp:~akretion-team/openobject-addons/fixes_for_c2c_currency_rate_update : there is now a mecanism to check if rates given by the webservice are "fresh" enough to be written in OpenERP. I added a 'max_delta_days' parameter for each currency update service, and if the delta between the current date and the rate date is bigger than 'max_delta_days', then the rate is not updated in OpenERP. I have implemented the check for all webservices except Yahoo.

[Truong Vinh Trinh (truongvinhtrinh2011)]

Hello Alexis,

Nice post!!
- VND is not supported with webservice from ECB, when choose to get rate exchange for VND the program will throw exception and the other currency types will not has their exchange rate values.
To ignore the unsupported currencies when using ECB's webservice, I added:

if curr not in self.supported_currency_array :
                continue

AFTER:
if main_currency != 'EUR':
            main_curr_data = self.rate_retrieve(dom, ecb_ns, main_currency)
        for curr in currency_array:

Trinh V Truong.

[Truong Vinh Trinh (truongvinhtrinh2011)]

Hello,

Please ignore my previous comment!!! the right code to make it work is here:

1. In currency_rate_update.py -- function: run_currency_update, I added 1 more condition to ignore unsupported currencies:

....
rate_name = time.strftime('%Y-%m-%d')
                    for curr in service.currency_to_update :
                        do_create = True
                        if curr.name in res : # Trinh added to ignore the unsupported currencies from the list. *********
                            for rate in curr.rate_ids :
......

2. I removed unsupported currencies function get_updated_currency of class ECB_getter in file currency_rate_update.py, as here:

# Trinh added to remove unsupported currencies out of getting rate exchanges loop *********
        supported_currency_array = currency_array
        for curr in currency_array :
            if curr not in self.supported_currency_array :
                supported_currency_array.remove(curr)

        for curr in supported_currency_array: # Trinh changed to use supported_currency_array from currency_array ********
            self.validate_cur(curr)
            if curr == 'EUR':

Regards,
Trinh V Truong.