Merge lp:~akretion-team/banking-addons/bank-statement-reconcile-70-multi-statements into lp:banking-addons/bank-statement-reconcile-70

[Florian da Costa]

Hi,
In some case like the CFONB norme (French norme) many bank statement can be extracted from the same file.
So I need to change the signature in order to always work with a list of id, than in my can I am able to inherit everything and process all of my bank statement (http://bazaar.launchpad.net/~akretion-team/+junk/account_statement_cfonb/view/head:/account_statement_cfonb/statement.py)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

Thanks for the contribs !

My opinion on that one is that I prefer letting this method "def statement_import" as it is. His purpose is to import one single statement.

Why not simply adding a new method called "def multi_statement_import" that call the first one for each instance ?

This way, you reduce the risk of breaking others work by returning a dict instead of a int/long.

For this reason, I mark it as need fixing.

Regards,

Joël

review: Needs Fixing (code review, no tests)

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

Ok it's ok for me. We will propose a new solution using def multi_statement_import

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

LGTM. It's ok for me. We do not change anymore the original API.

review: Approve (no test, code review)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

Thanks for the changes, that looks good now !

Regards,

Joël

review: Approve (code review, no tests)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Why do you pass the context in the parser?

    result_row_list = parser.parse(file_stream, context=context)

review: Needs Information

[Florian da Costa (florian-dacosta)]

The context can be useful for custom parsers.
It is for example used for the cfonb parser to handle the fact that a file can contain more than one bank statement.

http://bazaar.launchpad.net/~akretion-team/+junk/account_statement_cfonb/view/head:/account_statement_cfonb/parser/cfonb_parser.py

http://bazaar.launchpad.net/~akretion-team/+junk/account_statement_cfonb/view/head:/account_statement_cfonb/statement.py

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

There are conflicts please resubmit

review: Needs Resubmitting

[Florian da Costa (florian-dacosta)]

Here, I re-submited it. Hope it resolves the conflicts.

[Florian da Costa (florian-dacosta)]

If there are no more conflicts, Is it possible to merge this mp?
Thanks

[Stéphane Bidoul (Acsone) (sbi)]

Hello,

First of all, apologies for commenting so late in the review process.

We are also looking at solutions to support multiple statements in the same file, in order to support belgian CODA files.

I have looked at your merge proposal and the cfonb parser and I'm actually a bit uneasy with the proposed mechanism which requires storing parser state in the context between calls to parse().

Would you consider a variant where the parser is created (new_bank_statement_parser) in multi_statement_import, and then having statement_import called repeatedly with the same parser instance until parser.parse returns no data?

Best regards,

review: Needs Information (code review)

[Florian da Costa (florian-dacosta)]

Hello,

I agree it is not ideal to do it this way, passing the parser state through the context, but the goal was not to change the statement_import function. This way we do not break anything.

Maybe we could merge it as it is for the v7 and plan to refactore the statement_import function for the v8.

Regards

[Laurent Mignon (Acsone) (lmi)]

Hi,

I would like to propose an alternative based on the principles enunciated by Stéphane. I'm also uneasy with the proposed mechanism that requires to subclass the AccountStatementProfile class and stores the parser state in the context.

By looking in more detail in the code, I can not find overrides or other usages of the import_statement method than the one in the CreditPartnerStatementImporter wizard. Therefore, IMO, it's not a problem to change the 'statement_import' function to make the code more natural.

You can find a first draft of the proposed alternative at http://bazaar.launchpad.net/~acsone-openerp/banking-addons/bank-statement-reconcile-70-multi-statements/revision/147

Best regards,

lmi

review: Needs Information (code review)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

I'm in favor of Laurent's suggestion. If you would please consider his remakrs here it would be great ! Thanks a lot for your contribution here !

review: Needs Fixing (code review, no tests)

[Laurent Mignon (Acsone) (lmi)]

Hi Florian,

I've implemented the multi statement support in an addon to import the bank statements from a standard CODA file (belgium standard).

The implementation use the draft proposal unchanged. ( http://bazaar.launchpad.net/~acsone-openerp/banking-addons/bank-statement-reconcile-70-multi-statements/revision/147)

As expected, the changes to the parser were minor and seem to take advantage of existing methods.

http://bazaar.launchpad.net/~acsone-openerp/+junk/coda-completion/revision/14
http://bazaar.launchpad.net/~acsone-openerp/+junk/coda-completion/view/head:/account_statement_coda_import/parser/coda_file_parser.py

What do you think of this approach?

Regards,

lmi

review: Needs Information

[Florian da Costa (florian-dacosta)]

Hello Laurent,

I took a look on your mp and it seems great.
You approach is ok for me. I guess we can close this mp and merge yours!

Thanks a lot

[Laurent Mignon (Acsone) (lmi)]

Hello Florian,

Thanks for the review. I'll propose my branch for merging.

Regards,

lmi

=== modified file 'account_statement_base_import/statement.py'
--- account_statement_base_import/statement.py	2014-03-06 09:40:57 +0000
+++ account_statement_base_import/statement.py	2014-03-07 08:38:37 +0000
@@ -144,6 +144,14 @@
         vals.update(parser.get_st_vals())
         return vals
 
+
+    def multi_statement_import(self, cr, uid, ids, profile_id, file_stream,
+                               ftype="csv", context=None):
+        statement_id = self.statement_import(cr, uid, ids, profile_id, file_stream,
+                                             ftype=ftype, context=context)
+        return [statement_id]
+
+
     def statement_import(self, cr, uid, ids, profile_id, file_stream, ftype="csv", context=None):
         """
         Create a bank statement with the given profile and parser. It will fullfill the bank statement
@@ -165,7 +173,7 @@
         prof = prof_obj.browse(cr, uid, profile_id, context=context)
 
         parser = new_bank_statement_parser(prof.import_type, ftype=ftype)
-        result_row_list = parser.parse(file_stream)
+        result_row_list = parser.parse(file_stream, context=context)
         # Check all key are present in account.bank.statement.line!!
         if not result_row_list:
             raise osv.except_osv(_("Nothing to import"),

=== modified file 'account_statement_base_import/wizard/import_statement.py'
--- account_statement_base_import/wizard/import_statement.py	2013-12-04 14:10:49 +0000
+++ account_statement_base_import/wizard/import_statement.py	2014-03-07 08:38:37 +0000
@@ -99,7 +99,7 @@
         ftype = self._check_extension(importer.file_name)
         context['file_name'] = importer.file_name
         sid = self.pool.get(
-                'account.statement.profile').statement_import(
+                'account.statement.profile').multi_statement_import(
                                             cr,
                                             uid,
                                             False,
@@ -112,5 +112,5 @@
         action_obj = self.pool.get('ir.actions.act_window')
         action_id = model_obj.get_object_reference(cr, uid, 'account', 'action_bank_statement_tree')[1]
         res = action_obj.read(cr, uid, action_id)
-        res['domain'] = res['domain'][:-1] + ",('id', '=', %d)]" % sid
+        res['domain'] = res['domain'][:-1] + ",('id', 'in', %s)]" % sid
         return res