Merge lp:~ovnicraft/web-addons/6.1-export-view-float-field into lp:~webaddons-core-editors/web-addons/6.1

Proposed by Cristian Salamea
Status: Merged
Merged at revision: 21
Proposed branch: lp:~ovnicraft/web-addons/6.1-export-view-float-field
Merge into: lp:~webaddons-core-editors/web-addons/6.1
Diff against target: 71 lines (+37/-1)
2 files modified
web_export_view/AUTHORS.txt (+1/-0)
web_export_view/controllers.py (+36/-1)
To merge this branch: bzr merge lp:~ovnicraft/web-addons/6.1-export-view-float-field
Reviewer Review Type Date Requested Status
Holger Brunn (Therp) code review Approve
Leonardo Pistone Approve
Cristian Salamea (community) Needs Resubmitting
Web-Addons Core Editors Pending
Review via email: mp+169678@code.launchpad.net

Description of the change

XLS file now has correct float values and user can operate with it.

Use language configuration for thounsand and decimal separator.

To post a comment you must log in.
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

This one is very similar to https://code.launchpad.net/~eoc/web-addons/6.1-web-addons_fix_number_parsing_add_es_translation/+merge/167570. As only one of your two proposals can be merged, I think you two should cooperate.

review: Needs Fixing
Revision history for this message
Cristian Salamea (ovnicraft) wrote :

> This one is very similar to https://code.launchpad.net/~eoc/web-addons/6.1
> -web-addons_fix_number_parsing_add_es_translation/+merge/167570. As only one
> of your two proposals can be merged, I think you two should cooperate.

This is my only MP what you mention here is from another developer.
About my MP i would like to know is needs any fix.
Reading the MP mentioned by you i am not agree about test explicit about 'en' or 'es' in lang what about other languages ?

My code get lang from context and read params from res.lang model i consider the best way to identify separators.

Waiting for your comments.

Regards,

Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

I also disagree with hardcoding languages, and I like your approach getting those values from OpenERP's language configuration. The 'needs fixing' was more meant to look into cooperation here.

As for the review:
#28 you escape the decimal separator. This goes wrong if the decimal separator isn't a special character. Better just put it into square brackets. And you should compile the regex
#34 you should replace the thousands separator here, not ',', right?
#53 default that to English if this ever happens to be called without a language

Revision history for this message
Mariano Ruiz (marianoruiz) wrote :

I agree with this branch is a better solution than my code ;(

But, also I agree with some issues hitted by hbrunn, like a default English language, and in #34, you make a hardcodiing to, because ',' is a literal, and in Spanish, that is the decimal separator, not thousands.

The fix for #34 will be:

    cell_value = float(cell_value.replace(separators['thousands_sep'],'').replace(separators['decimal_point'],'.'))

The last literal '.' is a valid hardcoding, because is the unique decimal separator valid for float() function.

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

Thanks Holger for your feedback, i will fix it and update the merge and will take in care Mariano suggest.

Regards,

review: Needs Fixing
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> > This one is very similar to https://code.launchpad.net/~eoc/web-addons/6.1
> > -web-addons_fix_number_parsing_add_es_translation/+merge/167570. As only one
> > of your two proposals can be merged, I think you two should cooperate.
>
> This is my only MP what you mention here is from another developer.
> About my MP i would like to know is needs any fix.
> Reading the MP mentioned by you i am not agree about test explicit about 'en'
> or 'es' in lang what about other languages ?
>
> My code get lang from context and read params from res.lang model i consider
> the best way to identify separators.
>
> Waiting for your comments.
>
>
> Regards,

Can you discuss with him? Only both of you have the entire context and knowledge on these proposals.
Ideally you should propose a unique proposal which is suitable for you 2.

Thanks

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> > > This one is very similar to https://code.launchpad.net/~eoc/web-addons/6.1
> > > -web-addons_fix_number_parsing_add_es_translation/+merge/167570. As only
> one
> > > of your two proposals can be merged, I think you two should cooperate.
> >
> > This is my only MP what you mention here is from another developer.
> > About my MP i would like to know is needs any fix.
> > Reading the MP mentioned by you i am not agree about test explicit about
> 'en'
> > or 'es' in lang what about other languages ?
> >
> > My code get lang from context and read params from res.lang model i consider
> > the best way to identify separators.
> >
> > Waiting for your comments.
> >
> >
> > Regards,
>
> Can you discuss with him? Only both of you have the entire context and
> knowledge on these proposals.
> Ideally you should propose a unique proposal which is suitable for you 2.
>
> Thanks

Sorry, I missed the 3 last comments, so forget my answer.

I put your MP as 'Work in progress' and let you put it back to 'Needs review' when you are done with it.

Thanks

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

I update the MP with fixes, is not compiling the re the reason is i found this http://stackoverflow.com/questions/452104/is-it-worth-using-pythons-re-compile to not complex regexp.

Regards,

Revision history for this message
Mariano Ruiz (marianoruiz) wrote :

Cristian, the code is not compiling because have a missing dot between ")" and "replace" function in #34.

Revision history for this message
Mariano Ruiz (marianoruiz) wrote :

Fixing the dot bug in the branch, I test the code and work OK with English and Spanish context.

But.. the regular expression "^[\d%s]+(\%s\d+)?$" is not compatible with negative numbers, and are exported as strings with a "'" prefix.

I not know how fix the expression. Cristian, can you fix this?

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

Hi Mariano can you share the fix, i can applied.

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

> Fixing the dot bug in the branch, I test the code and work OK with English and
> Spanish context.
>
> But.. the regular expression "^[\d%s]+(\%s\d+)?$" is not compatible with
> negative numbers, and are exported as strings with a "'" prefix.

I will check it, dont see at first how to generate negative numbers in ERP
>
> I not know how fix the expression. Cristian, can you fix this?

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

Update code with en_US by default lang and decimal and thousand separator check it.

review: Needs Resubmitting
17. By Cristian Salamea

[FIX] type error replacing separators and credits for fixing :)

Revision history for this message
Leonardo Pistone (lepistone) wrote :

Thanks for your work!

I tested that and works fine for me. Libreoffice sees the column as float as it should be.
I did a little MP into your branch with some pep8 refactoring:
https://code.launchpad.net/~lpistone/web-addons/6.1-export-view-float-field-pep8/+merge/186509

Leo

review: Approve
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

With the changes from lp:~lpistone/web-addons/6.1-export-view-float-field-pep8, I would merge this one. Anyone who disagrees?

review: Approve (code review)
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Christian, can you merge Leonardo's proposal into your branch, so that we can proceed?

Revision history for this message
Cristian Salamea (ovnicraft) wrote :

On Tue, Oct 22, 2013 at 3:14 PM, Stefan Rijnhart (Therp) <email address hidden>wrote:

> Christian, can you merge Leonardo's proposal into your branch, so that we
> can proceed?
>
> --
>
> https://code.launchpad.net/~ovnicraft/web-addons/6.1-export-view-float-field/+merge/169678
> You are the owner of lp:~ovnicraft/web-addons/6.1-export-view-float-field.
>

Ok, i will do it tonight, then you can proceed.

Regards,

--
Cristian Salamea
@ovnicraft

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'web_export_view/AUTHORS.txt'
2--- web_export_view/AUTHORS.txt 2013-01-28 18:35:43 +0000
3+++ web_export_view/AUTHORS.txt 2013-08-17 20:11:54 +0000
4@@ -4,3 +4,4 @@
5 Simone Orsi <simone.orsi@domsense.com> [simahawk]
6 Lorenzo Battistini <lorenzo.battistini@agilebg.com>
7 Stefan Rijnhart <stefan@therp.nl>
8+Cristian Salamea <ovnicraft@gmail.com>
9
10=== modified file 'web_export_view/controllers.py'
11--- web_export_view/controllers.py 2013-01-20 15:16:52 +0000
12+++ web_export_view/controllers.py 2013-08-17 20:11:54 +0000
13@@ -18,6 +18,10 @@
14 # along with this program. If not, see <http://www.gnu.org/licenses/>.
15 #
16 ##############################################################################
17+import re
18+import xlwt
19+from cStringIO import StringIO
20+
21 try:
22 import json
23 except ImportError:
24@@ -31,6 +35,33 @@
25 class ExcelExportView(ExcelExport):
26 _cp_path = '/web/export/xls_view'
27
28+ def from_data(self, fields, rows, separators):
29+ workbook = xlwt.Workbook()
30+ worksheet = workbook.add_sheet('Sheet 1')
31+
32+ for i, fieldname in enumerate(fields):
33+ worksheet.write(0, i, fieldname)
34+ worksheet.col(i).width = 8000 # around 220 pixels
35+
36+ style = xlwt.easyxf('align: wrap yes')
37+ m = "^[\d%s]+(\%s\d+)?$" % (separators['thousands_sep'], separators['decimal_point'])
38+ for row_index, row in enumerate(rows):
39+ for cell_index, cell_value in enumerate(row):
40+ if isinstance(cell_value, basestring):
41+ cell_value = re.sub("\r", " ", cell_value)
42+ if re.match(m, cell_value):
43+ cell_value = float(cell_value.replace(separators['thousands_sep'],'').replace(separators['decimal_point'],'.'))
44+ style = xlwt.easyxf(num_format_str='#,##0.00')
45+ if cell_value is False: cell_value = None
46+ worksheet.write(row_index + 1, cell_index, cell_value, style)
47+
48+ fp = StringIO()
49+ workbook.save(fp)
50+ fp.seek(0)
51+ data = fp.read()
52+ fp.close()
53+ return data
54+
55 @openerpweb.httprequest
56 def index(self, req, data, token):
57 data = json.loads(data)
58@@ -39,8 +70,12 @@
59 rows = data.get('rows',[])
60
61 context = req.session.eval_context(req.context)
62+ lang = context.get('lang', 'en_US')
63+ Model = req.session.model('res.lang')
64+ ids = Model.search([['code','=',lang]])
65+ record = Model.read(ids, ['decimal_point','thousands_sep'])
66
67- return req.make_response(self.from_data(columns_headers, rows),
68+ return req.make_response(self.from_data(columns_headers, rows, record[0]),
69 headers=[('Content-Disposition', 'attachment; filename="%s"' % self.filename(model)),
70 ('Content-Type', self.content_type)],
71 cookies={'fileToken': int(token)})

Subscribers

People subscribed via source and target branches