Merge lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-import-statement-hook into lp:banking-addons/bank-statement-reconcile-70

[Laurent Mignon (Acsone)]

Hi,

In some cases, the imported file may contains information about the account.bank.statement.
The proposal add a new method 'get_st_vals' on the 'BankStatementImportParser' that can be used to provide statement info from the parsed file and a new method 'prepare_statement_vals' on 'AccountStatementProfil' that fills the statement values with the profile_id and the result to the call to the new method on the parser.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Laurent, very interesting and needed changes!

I was to say that you have made the MP to the wrong branch, but you corrected at the moment!

I have checked the code, and the solution seems perfect for Spain, where I'm adopting national standard statement format in 7.0 to these addons, and there is information about start and end date of the statement in the file that must be dumped.

Regards.

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Laurent,

Checking pending MPs, I have found that there is already an MP that issues this problem in a similar way:

https://code.launchpad.net/~akretion-team/banking-addons/bank-statement-reconcile-70-improve-import/+merge/197759

So I'm going to put this as rejected and let the other MP continues. Please review it to contribute with your comments.

Regards.

review: Disapprove

[Laurent Mignon (Acsone) (lmi)]

> Hi, Laurent,
>
> Checking pending MPs, I have found that there is already an MP that issues
> this problem in a similar way:
>
> https://code.launchpad.net/~akretion-team/banking-addons/bank-statement-
> reconcile-70-improve-import/+merge/197759
>
> So I'm going to put this as rejected and let the other MP continues. Please
> review it to contribute with your comments.
>
> Regards.

Hi,

Thanks for the review and the link to the other proposal. I'm a little bit disappointed by your decision. I've the feeling that the proposed approach is more generic that the one proposed by akretion-team.

In the other proposal, it's not the responsibility of the parser to provide additional values to the statement creation. So if I want to provide an other value from the parser to statement (ex the date), I need to override the statement profile. My point of view is that's the responsibility of the parser to provide values for statement and statement line creation. The profile is just there to make the glue between the parser and the logic. What do you think?

Regards

lmi

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

In a way other MP was created and Approved before this one. So disapproving this one seems fair.

However, your changes and improvements are welcome.

Sorry that it wasn't noticed by anybody before. BTW good catch Pedro

[Pedro Manuel Baeza (pedro.baeza)]

Sorry Laurent if I bother you. Please contribute now with a new MP over the merge changes with your improvements, that indeed are good ones! I want to make a new MP tonight, but I think it's better that you make it and get credited.

Regards.

[Pedro Manuel Baeza (pedro.baeza)]

Now I see redundant to add two methods for the same purpose: get_st_vals and prepare_statement_vals. Do you agree to merge both on one? If you do, I will make inmediately an exceptional merge to fix the mess I did merging the other one without consensus.

Regards.

review: Needs Information

[Laurent Mignon (Acsone) (lmi)]

Hi Pedro and once again thanks for the review.
> Now I see redundant to add two methods for the same purpose: get_st_vals and
> prepare_statement_vals. Do you agree to merge both on one?
The change mirrors the approach for statement lines with the same approach for the statement.
* on the parser we have 'get_st_line_vals' vs 'get_st_vals'
* on the account.statement.profile we have 'prepare_statetement_lines_vals' vs 'prepare_statement_vals'
The motivation behind the 'prepare_statement_vals' is to make a place where it's possible to complete/validate data from the parser in the context of Openerp without overriding the 'statement_import' method. Take a look at the profile developed to import statement files specific to Belgium http://bazaar.launchpad.net/~acsone-openerp/+junk/coda-completion/view/head:/account_statement_coda_import/statement.py#L84 . I think it's a good illustration of the motivation behind the 'prepare_statement_val'. What do you think?

Regards,

Laurent Mignon

[Pedro Manuel Baeza (pedro.baeza)]

Hi, I proceed then with the merge. Sorry for the past inconveniences.

Regards.

review: Approve

=== modified file 'account_statement_base_completion/statement.py'
--- account_statement_base_completion/statement.py	2014-01-23 08:44:24 +0000
+++ account_statement_base_completion/statement.py	2014-01-24 17:16:24 +0000
@@ -184,7 +184,7 @@
         inv = self._find_invoice(cr, uid, line, inv_type, context=context)
         if inv:
             # FIXME use only commercial_partner_id of invoice in 7.1
-            # this is for backward compatibility in 7.0 before 
+            # this is for backward compatibility in 7.0 before
             # the refactoring of res.partner
             if hasattr(inv, 'commercial_partner_id'):
                 partner_id = inv.commercial_partner_id.id
@@ -418,7 +418,7 @@
         """
         statement_line_obj = self.pool['account.bank.statement.line']
         model_cols = statement_line_obj._columns
-        sparse_fields = dict([(k , col) for k, col in model_cols.iteritems() if isinstance(col, fields.sparse) and col._type == 'char'])
+        sparse_fields = dict([(k, col) for k, col in model_cols.iteritems() if isinstance(col, fields.sparse) and col._type == 'char'])
         values = []
         for statement in statement_store:
             to_json_k = set()
@@ -429,10 +429,9 @@
                     serialized = st_copy.setdefault(col.serialization_field, {})
                     serialized[k] = st_copy[k]
             for k in to_json_k:
-                st_copy[k] =  simplejson.dumps(st_copy[k])
+                st_copy[k] = simplejson.dumps(st_copy[k])
             values.append(st_copy)
         return values
-        
 
     def _insert_lines(self, cr, uid, statement_store, context=None):
         """ Do raw insert into database because ORM is awfully slow
@@ -456,7 +455,7 @@
             when cheking security.
         TODO / WARM: sparse fields are skipped by the method. IOW, if your
         completion rule update an sparse field, the updated value will never
-        be stored in the database. It would be safer to call the update method 
+        be stored in the database. It would be safer to call the update method
         from the ORM for records updating this kind of fields.
         """
         cols = self._get_available_columns([vals])

=== modified file 'account_statement_base_import/parser/parser.py'
--- account_statement_base_import/parser/parser.py	2013-12-11 19:11:44 +0000
+++ account_statement_base_import/parser/parser.py	2014-01-24 17:16:24 +0000
@@ -20,7 +20,6 @@
 ##############################################################################
 import base64
 import csv
-from datetime import datetime
 
 
 def UnicodeDictReader(utf8_data, **kwargs):
@@ -115,6 +114,19 @@
         """
         return NotImplementedError
 
+    def get_st_vals(self):
+        """
+        This method return a dict of vals that ca be passed to
+        create method of statement.
+        :return: dict of vals that represent additional infos for the statement
+        """
+        return {
+                'name': self.statement_name,
+                'balance_start': self.balance_start,
+                'balance_end_real': self.balance_end,
+                'date': self.statement_date
+        }
+
     def get_st_line_vals(self, line, *args, **kwargs):
         """
         Implement a method in your parser that must return a dict of vals that can be
@@ -155,37 +167,6 @@
         self._post(*args, **kwargs)
         return self.result_row_list
 
-    def get_start_balance(self, *args, **kwargs):
-        """
-        This is called by the importation method to set the balance start
-        amount in the bank statement.
-            return: float of the balance start (self.balance_start)
-        """
-        return self.balance_start
-
-    def get_end_balance(self, *args, **kwargs):
-        """
-        This is called by the importation method to set the balance end
-        amount in the bank statement.
-            return: float of the balance end (self.balance_end)
-        """
-        return self.balance_end
-
-    def get_statement_name(self, *args, **kwargs):
-        """
-        This is called by the importation method to set the statement
-        name in the bank statement.
-            return: string of the statement name (self.statement_name)
-        """
-        return self.statement_name or '/'
-
-    def get_statement_date(self, *args, **kwargs):
-        """
-        This is called by the importation method to set the statement
-        date in the bank statement.
-            return: datetime of the statement date (self.statement_date)
-        """
-        return self.statement_date or datetime.now()
 
 def itersubclasses(cls, _seen=None):
     """

=== modified file 'account_statement_base_import/statement.py'
--- account_statement_base_import/statement.py	2013-12-04 17:39:04 +0000
+++ account_statement_base_import/statement.py	2014-01-24 17:16:24 +0000
@@ -90,7 +90,7 @@
             statement_id, context):
         """
         Hook to build the values of a line from the parser returned values. At
-        least it fullfill the statement_id and account_id. Overide it to add your
+        least it fullfill the statement_id and account_id. Override it to add your
         own completion if needed.
 
         :param dict of vals from parser for account.bank.statement.line (called by
@@ -126,14 +126,14 @@
         values['type'] = 'general'
         return values
 
-
-    def _prepare_statement_vals(self, cr, uid, prof, parser, context=None):
-        return {
-            'profile_id': prof.id,
-            'name': parser.get_statement_name(),
-            'balance_start': parser.get_start_balance(),
-            'balance_end_real': parser.get_end_balance(),
-        }
+    def prepare_statement_vals(self, cr, uid, profile_id, result_row_list, parser, context):
+        """
+        Hook to build the values of the statement from the parser and
+        the profile.
+        """
+        vals = {'profile_id': profile_id}
+        vals.update(parser.get_st_vals())
+        return vals
 
     def statement_import(self, cr, uid, ids, profile_id, file_stream, ftype="csv", context=None):
         """
@@ -168,8 +168,11 @@
                                      _("Column %s you try to import is not "
                                        "present in the bank statement line!") % col)
 
-        st_vals = self._prepare_statement_vals(cr, uid, prof, parser, context=context) 
-        statement_id = statement_obj.create(cr, uid, st_vals, context=context)
+        statement_vals = self.prepare_statement_vals(cr, uid, prof.id, result_row_list, parser, context)
+        statement_id = statement_obj.create(cr, uid,
+                                            statement_vals,
+                                            context=context)
+
         if prof.receivable_account_id:
             account_receivable = account_payable = prof.receivable_account_id.id
         else:

=== modified file 'account_statement_commission/commission.py'
--- account_statement_commission/commission.py	2013-12-04 14:14:56 +0000
+++ account_statement_commission/commission.py	2014-01-24 17:16:24 +0000
@@ -2,9 +2,11 @@
 import datetime
 from openerp.osv import orm, fields
 
+
 def float_or_zero(val):
     return float(val) if val else 0.0
 
+
 class AccountStatementProfil(orm.Model):
     _inherit = "account.statement.profile"
 
@@ -22,7 +24,7 @@
         commission_analytic_id = profile.commission_analytic_id and profile.commission_analytic_id.id or False
         comm_values = {
             'name': 'IN ' + _('Commission line'),
-            'date': parser.get_statement_date(),
+            'date': parser.get_st_vals().get('date') or datetime.datetime.now(),
             'amount': global_commission_amount,
             'partner_id': partner_id,
             'type': 'general',
@@ -36,6 +38,7 @@
         statement_line_obj = self.pool.get('account.bank.statement.line')
         statement_line_obj.create(cr, uid, comm_values, context=context)
 
+
 class AccountStatementLineWithCommission(orm.Model):
     _inherit = "account.bank.statement.line"
     _columns = {
@@ -45,6 +48,7 @@
             serialization_field='additionnal_bank_fields'),
     }
 
+
 class CreditPartnerStatementImporter(orm.TransientModel):
     _inherit = "credit.statement.import"