Merge lp:~camptocamp/banking-addons/bank-statement-reconcile-70-improve-import-usability into lp:banking-addons/bank-statement-reconcile-70
- bank-statement-reconcile-70-improve-import-usability
- Merge into bank-statement-reconcile-70
Proposed by
Nicolas Bessi - Camptocamp
Status: | Superseded |
---|---|
Proposed branch: | lp:~camptocamp/banking-addons/bank-statement-reconcile-70-improve-import-usability |
Merge into: | lp:banking-addons/bank-statement-reconcile-70 |
Diff against target: |
216 lines (+77/-33) 3 files modified
account_statement_base_import/parser/file_parser.py (+65/-27) account_statement_base_import/parser/generic_file_parser.py (+9/-4) account_statement_transactionid_import/parser/transactionid_file_parser.py (+3/-2) |
To merge this branch: | bzr merge lp:~camptocamp/banking-addons/bank-statement-reconcile-70-improve-import-usability |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Nicolas Bessi - Camptocamp (community) | Needs Resubmitting | ||
Alexandre Fayolle - camptocamp | code review, no test | Needs Fixing | |
Review via email: mp+148665@code.launchpad.net |
This proposal has been superseded by a proposal from 2013-03-01.
Commit message
Description of the change
Improve statement import global usability by retuning usable error message while parsing files.
Allows empty value for float in parsed CSV.
Minor code improvements.
To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote : | # |
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote : | # |
l19: do not use a mutable value as a default value. Please replace with an empty tuple.
review:
Needs Fixing
(code review, no test)
Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote : | # |
Fixes applied.
convertion/
Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) : | # |
review:
Needs Resubmitting
Unmerged revisions
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'account_statement_base_import/parser/file_parser.py' | |||
2 | --- account_statement_base_import/parser/file_parser.py 2012-12-20 13:37:01 +0000 | |||
3 | +++ account_statement_base_import/parser/file_parser.py 2013-02-26 08:28:21 +0000 | |||
4 | @@ -17,8 +17,8 @@ | |||
5 | 17 | # along with this program. If not, see <http://www.gnu.org/licenses/>. | 17 | # along with this program. If not, see <http://www.gnu.org/licenses/>. |
6 | 18 | # | 18 | # |
7 | 19 | ############################################################################## | 19 | ############################################################################## |
8 | 20 | |||
9 | 21 | from openerp.tools.translate import _ | 20 | from openerp.tools.translate import _ |
10 | 21 | from openerp.osv.osv import except_osv | ||
11 | 22 | import tempfile | 22 | import tempfile |
12 | 23 | import datetime | 23 | import datetime |
13 | 24 | from parser import BankStatementImportParser | 24 | from parser import BankStatementImportParser |
14 | @@ -34,12 +34,13 @@ | |||
15 | 34 | Generic abstract class for defining parser for .csv or .xls file format. | 34 | Generic abstract class for defining parser for .csv or .xls file format. |
16 | 35 | """ | 35 | """ |
17 | 36 | 36 | ||
19 | 37 | def __init__(self, parse_name, keys_to_validate=[], ftype='csv', convertion_dict=None, header=None, *args, **kwargs): | 37 | def __init__(self, parse_name, keys_to_validate=None, ftype='csv', conversion_dict=None, |
20 | 38 | header=None, *args, **kwargs): | ||
21 | 38 | """ | 39 | """ |
22 | 39 | :param char: parse_name : The name of the parser | 40 | :param char: parse_name : The name of the parser |
23 | 40 | :param list: keys_to_validate : contain the key that need to be present in the file | 41 | :param list: keys_to_validate : contain the key that need to be present in the file |
24 | 41 | :param char ftype: extension of the file (could be csv or xls) | 42 | :param char ftype: extension of the file (could be csv or xls) |
26 | 42 | :param: convertion_dict : keys and type to convert of every column in the file like | 43 | :param: conversion_dict : keys and type to convert of every column in the file like |
27 | 43 | { | 44 | { |
28 | 44 | 'ref': unicode, | 45 | 'ref': unicode, |
29 | 45 | 'label': unicode, | 46 | 'label': unicode, |
30 | @@ -54,9 +55,10 @@ | |||
31 | 54 | if ftype in ('csv', 'xls'): | 55 | if ftype in ('csv', 'xls'): |
32 | 55 | self.ftype = ftype | 56 | self.ftype = ftype |
33 | 56 | else: | 57 | else: |
37 | 57 | raise Exception(_('Invalide file type %s. please use csv or xls') % (ftype)) | 58 | raise except_osv(_('User Error'), |
38 | 58 | self.keys_to_validate = keys_to_validate | 59 | _('Invalid file type %s. Please use csv or xls') % ftype) |
39 | 59 | self.convertion_dict = convertion_dict | 60 | self.keys_to_validate = keys_to_validate if keys_to_validate is not None else [] |
40 | 61 | self.conversion_dict = conversion_dict | ||
41 | 60 | self.fieldnames = header | 62 | self.fieldnames = header |
42 | 61 | self._datemode = 0 # used only for xls documents, | 63 | self._datemode = 0 # used only for xls documents, |
43 | 62 | # 0 means Windows mode (1900 based dates). | 64 | # 0 means Windows mode (1900 based dates). |
44 | @@ -99,7 +101,8 @@ | |||
45 | 99 | parsed_cols = self.result_row_list[0].keys() | 101 | parsed_cols = self.result_row_list[0].keys() |
46 | 100 | for col in self.keys_to_validate: | 102 | for col in self.keys_to_validate: |
47 | 101 | if col not in parsed_cols: | 103 | if col not in parsed_cols: |
49 | 102 | raise Exception(_('Column %s not present in file') % (col)) | 104 | raise except_osv(_('Invalid data'), |
50 | 105 | _('Column %s not present in file') % col) | ||
51 | 103 | return True | 106 | return True |
52 | 104 | 107 | ||
53 | 105 | def _post(self, *args, **kwargs): | 108 | def _post(self, *args, **kwargs): |
54 | @@ -128,17 +131,13 @@ | |||
55 | 128 | wb_file.write(self.filebuffer) | 131 | wb_file.write(self.filebuffer) |
56 | 129 | # We ensure that cursor is at beginig of file | 132 | # We ensure that cursor is at beginig of file |
57 | 130 | wb_file.seek(0) | 133 | wb_file.seek(0) |
69 | 131 | wb = xlrd.open_workbook(wb_file.name) | 134 | with xlrd.open_workbook(wb_file.name) as wb: |
70 | 132 | self._datemode = wb.datemode | 135 | self._datemode = wb.datemode |
71 | 133 | sheet = wb.sheet_by_index(0) | 136 | sheet = wb.sheet_by_index(0) |
72 | 134 | header = sheet.row_values(0) | 137 | header = sheet.row_values(0) |
73 | 135 | res = [] | 138 | res = [] |
74 | 136 | for rownum in range(1, sheet.nrows): | 139 | for rownum in range(1, sheet.nrows): |
75 | 137 | res.append(dict(zip(header, sheet.row_values(rownum)))) | 140 | res.append(dict(zip(header, sheet.row_values(rownum)))) |
65 | 138 | try: | ||
66 | 139 | wb_file.close() | ||
67 | 140 | except Exception, e: | ||
68 | 141 | pass # file is already closed | ||
76 | 142 | return res | 141 | return res |
77 | 143 | 142 | ||
78 | 144 | def _from_csv(self, result_set, conversion_rules): | 143 | def _from_csv(self, result_set, conversion_rules): |
79 | @@ -149,11 +148,30 @@ | |||
80 | 149 | for line in result_set: | 148 | for line in result_set: |
81 | 150 | for rule in conversion_rules: | 149 | for rule in conversion_rules: |
82 | 151 | if conversion_rules[rule] == datetime.datetime: | 150 | if conversion_rules[rule] == datetime.datetime: |
86 | 152 | date_string = line[rule].split(' ')[0] | 151 | try: |
87 | 153 | line[rule] = datetime.datetime.strptime(date_string, | 152 | date_string = line[rule].split(' ')[0] |
88 | 154 | '%Y-%m-%d') | 153 | line[rule] = datetime.datetime.strptime(date_string, |
89 | 154 | '%Y-%m-%d') | ||
90 | 155 | except ValueError as err: | ||
91 | 156 | raise except_osv(_("Date format is not valid."), | ||
92 | 157 | _(" It should be YYYY-MM-DD for column: %s" | ||
93 | 158 | " value: %s \n \n" | ||
94 | 159 | " \n Please check the line with ref: %s" | ||
95 | 160 | " \n \n Detail: %s") % (rule, | ||
96 | 161 | line.get(rule, _('Missing')), | ||
97 | 162 | line.get('ref', line), | ||
98 | 163 | repr(err))) | ||
99 | 155 | else: | 164 | else: |
101 | 156 | line[rule] = conversion_rules[rule](line[rule]) | 165 | try: |
102 | 166 | line[rule] = conversion_rules[rule](line[rule]) | ||
103 | 167 | except Exception as err: | ||
104 | 168 | raise except_osv(_('Invalid data'), | ||
105 | 169 | _("Value %s of column %s is not valid." | ||
106 | 170 | "\n Please check the line with ref %s:" | ||
107 | 171 | "\n \n Detail: %s") % (line.get(rule, _('Missing')), | ||
108 | 172 | rule, | ||
109 | 173 | line.get('ref', line), | ||
110 | 174 | repr(err))) | ||
111 | 157 | return result_set | 175 | return result_set |
112 | 158 | 176 | ||
113 | 159 | def _from_xls(self, result_set, conversion_rules): | 177 | def _from_xls(self, result_set, conversion_rules): |
114 | @@ -164,17 +182,37 @@ | |||
115 | 164 | for line in result_set: | 182 | for line in result_set: |
116 | 165 | for rule in conversion_rules: | 183 | for rule in conversion_rules: |
117 | 166 | if conversion_rules[rule] == datetime.datetime: | 184 | if conversion_rules[rule] == datetime.datetime: |
120 | 167 | t_tuple = xlrd.xldate_as_tuple(line[rule], self._datemode) | 185 | try: |
121 | 168 | line[rule] = datetime.datetime(*t_tuple) | 186 | t_tuple = xlrd.xldate_as_tuple(line[rule], self._datemode) |
122 | 187 | line[rule] = datetime.datetime(*t_tuple) | ||
123 | 188 | except Exception as err: | ||
124 | 189 | raise except_osv(_("Date format is not valid"), | ||
125 | 190 | _("Please modify the cell formatting to date format" | ||
126 | 191 | " for column: %s" | ||
127 | 192 | " value: %s" | ||
128 | 193 | "\n Please check the line with ref: %s" | ||
129 | 194 | "\n \n Detail: %s") % (rule, | ||
130 | 195 | line.get(rule, _('Missing')), | ||
131 | 196 | line.get('ref', line), | ||
132 | 197 | repr(err))) | ||
133 | 169 | else: | 198 | else: |
135 | 170 | line[rule] = conversion_rules[rule](line[rule]) | 199 | try: |
136 | 200 | line[rule] = conversion_rules[rule](line[rule]) | ||
137 | 201 | except Exception as err: | ||
138 | 202 | raise except_osv(_('Invalid data'), | ||
139 | 203 | _("Value %s of column %s is not valid." | ||
140 | 204 | "\n Please check the line with ref %s:" | ||
141 | 205 | "\n \n Detail: %s") % (line.get(rule, _('Missing')), | ||
142 | 206 | rule, | ||
143 | 207 | line.get('ref', line), | ||
144 | 208 | repr(err))) | ||
145 | 171 | return result_set | 209 | return result_set |
146 | 172 | 210 | ||
147 | 173 | def _cast_rows(self, *args, **kwargs): | 211 | def _cast_rows(self, *args, **kwargs): |
148 | 174 | """ | 212 | """ |
150 | 175 | Convert the self.result_row_list using the self.convertion_dict providen. | 213 | Convert the self.result_row_list using the self.conversion_dict providen. |
151 | 176 | We call here _from_xls or _from_csv depending on the self.ftype variable. | 214 | We call here _from_xls or _from_csv depending on the self.ftype variable. |
152 | 177 | """ | 215 | """ |
153 | 178 | func = getattr(self, '_from_%s' % self.ftype) | 216 | func = getattr(self, '_from_%s' % self.ftype) |
155 | 179 | res = func(self.result_row_list, self.convertion_dict) | 217 | res = func(self.result_row_list, self.conversion_dict) |
156 | 180 | return res | 218 | return res |
157 | 181 | 219 | ||
158 | === modified file 'account_statement_base_import/parser/generic_file_parser.py' | |||
159 | --- account_statement_base_import/parser/generic_file_parser.py 2012-12-20 13:37:01 +0000 | |||
160 | +++ account_statement_base_import/parser/generic_file_parser.py 2013-02-26 08:28:21 +0000 | |||
161 | @@ -29,6 +29,11 @@ | |||
162 | 29 | except: | 29 | except: |
163 | 30 | raise Exception(_('Please install python lib xlrd')) | 30 | raise Exception(_('Please install python lib xlrd')) |
164 | 31 | 31 | ||
165 | 32 | def float_or_zero(val): | ||
166 | 33 | """ Conversion function used to manage | ||
167 | 34 | empty string into float usecase""" | ||
168 | 35 | return float(val) if val else 0.0 | ||
169 | 36 | |||
170 | 32 | 37 | ||
171 | 33 | class GenericFileParser(FileParser): | 38 | class GenericFileParser(FileParser): |
172 | 34 | """ | 39 | """ |
173 | @@ -38,16 +43,16 @@ | |||
174 | 38 | """ | 43 | """ |
175 | 39 | 44 | ||
176 | 40 | def __init__(self, parse_name, ftype='csv'): | 45 | def __init__(self, parse_name, ftype='csv'): |
178 | 41 | convertion_dict = { | 46 | conversion_dict = { |
179 | 42 | 'ref': unicode, | 47 | 'ref': unicode, |
180 | 43 | 'label': unicode, | 48 | 'label': unicode, |
181 | 44 | 'date': datetime.datetime, | 49 | 'date': datetime.datetime, |
184 | 45 | 'amount': float, | 50 | 'amount': float_or_zero, |
185 | 46 | 'commission_amount': float | 51 | 'commission_amount': float_or_zero |
186 | 47 | } | 52 | } |
187 | 48 | # Order of cols does not matter but first row of the file has to be header | 53 | # Order of cols does not matter but first row of the file has to be header |
188 | 49 | keys_to_validate = ['ref', 'label', 'date', 'amount', 'commission_amount'] | 54 | keys_to_validate = ['ref', 'label', 'date', 'amount', 'commission_amount'] |
190 | 50 | super(GenericFileParser, self).__init__(parse_name, keys_to_validate=keys_to_validate, ftype=ftype, convertion_dict=convertion_dict) | 55 | super(GenericFileParser, self).__init__(parse_name, keys_to_validate=keys_to_validate, ftype=ftype, conversion_dict=conversion_dict) |
191 | 51 | 56 | ||
192 | 52 | @classmethod | 57 | @classmethod |
193 | 53 | def parser_for(cls, parser_name): | 58 | def parser_for(cls, parser_name): |
194 | 54 | 59 | ||
195 | === modified file 'account_statement_transactionid_import/parser/transactionid_file_parser.py' | |||
196 | --- account_statement_transactionid_import/parser/transactionid_file_parser.py 2012-12-20 13:37:01 +0000 | |||
197 | +++ account_statement_transactionid_import/parser/transactionid_file_parser.py 2013-02-26 08:28:21 +0000 | |||
198 | @@ -30,7 +30,7 @@ | |||
199 | 30 | """ | 30 | """ |
200 | 31 | 31 | ||
201 | 32 | def __init__(self, parse_name, ftype='csv'): | 32 | def __init__(self, parse_name, ftype='csv'): |
203 | 33 | convertion_dict = { | 33 | conversion_dict = { |
204 | 34 | 'transaction_id': unicode, | 34 | 'transaction_id': unicode, |
205 | 35 | 'label': unicode, | 35 | 'label': unicode, |
206 | 36 | 'date': datetime.datetime, | 36 | 'date': datetime.datetime, |
207 | @@ -39,7 +39,8 @@ | |||
208 | 39 | } | 39 | } |
209 | 40 | # Order of cols does not matter but first row of the file has to be header | 40 | # Order of cols does not matter but first row of the file has to be header |
210 | 41 | keys_to_validate = ['transaction_id', 'label', 'date', 'amount', 'commission_amount'] | 41 | keys_to_validate = ['transaction_id', 'label', 'date', 'amount', 'commission_amount'] |
212 | 42 | super(TransactionIDFileParser, self).__init__(parse_name, keys_to_validate=keys_to_validate, ftype=ftype, convertion_dict=convertion_dict) | 42 | super(TransactionIDFileParser, self).__init__(parse_name, keys_to_validate=keys_to_validate, |
213 | 43 | ftype=ftype, conversion_dict=conversion_dict) | ||
214 | 43 | 44 | ||
215 | 44 | @classmethod | 45 | @classmethod |
216 | 45 | def parser_for(cls, parser_name): | 46 | def parser_for(cls, parser_name): |
Nitpickings
l.30
s/Invalide/invalid/
s/please/Please/
remove extraneous parenthesis around `ftype`
l.40
remove extraneous parenthesis around `col`
l.80,93,113,128
I would recommend the new syntax python3-like for the exceptions as it is more readable
except Exception as err:
BTW, I don't have the context here, but don't we have a more precise exception to catch?
l.147 conversion/
s/convertion/
l.149
return float(val) if val else 0.0