Merge lp:~therp-nl/banking-addons/6.1_lp1176783 into lp:banking-addons/6.1

[Holger Brunn (Therp)]

[Stefan Rijnhart (Opener) (stefan-opener)]

I must apologize for the elaborate 'type' discovery in _get_move_info() but what it attempts is to set match type to 'invoice' only if all candidate move lines are linked to an invoice. Your fix does not honour this admittedly implicit logic.

Of course, if match_type comes out as 'invoice' while not all move lines actually have an invoice then the code is broken and should be fixed. The loop where it checks for invoice on the move lines and a couple of other lines after it can probably be replaced by something like

if move_lines:
    if all(line.invoice for line in move_lines):
        retval['match_type'] = 'invoice'
        retval['type'] = type_map[move_lines[0].invoice.type]
    else:
        retval['match_type'] = 'move'

The method's signature allows for an apriori match_type argument but it does not seem to be used anywhere and could probably be refactored out.

Does this in fact fix your case or am I missing something?

review: Needs Fixing

[Holger Brunn (Therp) (hbrunn)]

This would indeed fix the problem at hand.

But if match_type's semantics is 'I have exclusively matches of type $match_type', then all the code in this function is broken. Adhering to the maxim of only fixing the bug in bug fixes, I pushed the necessary fix to lp:~therp-nl/banking-addons/6.1_fix__get_move_info_semantics - this will fix my problem en passant.

So if you confirm that's the right semantics, we can ditch this MP and I'll propose the branch mentioned above.

[Stefan Rijnhart (Opener) (stefan-opener)]

Yes, that is what I meant. The method should assign:
- match_type = 'invoice' if there are move lines and all have an invoice, or
- match_type = 'move_lines' if there are move lines,
- match_type = False otherwise

However, the code in your alternative branch does the same for partner and account. I do not think that that should be the case. The reason for this is that the code considers invoices a special case of move lines. If all move lines have invoices we prefer to ask the user to select the correct invoice instead of move lines.

There is no such specialization of partners and accounts. Whether the code should suggest the partner and account of the first match of multiple matches is just an interface issue. These are overwritten anyway when the user disambiguates the match.

[Holger Brunn (Therp) (hbrunn)]

rejected because it's fixed in lp:~therp-nl/banking-addons/6.1_fix__get_move_info_semantics

Unmerged revisions

=== modified file 'account_banking/banking_import_transaction.py'
--- account_banking/banking_import_transaction.py	2013-04-26 08:27:33 +0000
+++ account_banking/banking_import_transaction.py	2013-05-06 07:43:28 +0000
@@ -1048,6 +1048,7 @@
                         break
                 else:
                     retval['match_type'] = 'invoice'
+                retval['type'] = type_map[move_line.invoice.type]
             else:
                 if retval['match_type']: 
                     retval['match_type'] = False
@@ -1057,8 +1058,8 @@
         if move_lines and len(move_lines) == 1:
             retval['reference'] = move_lines[0].ref
         if retval['match_type'] == 'invoice':
-            retval['invoice_ids'] = list(set([x.invoice.id for x in move_lines]))
-            retval['type'] = type_map[move_lines[0].invoice.type]
+            retval['invoice_ids'] = list(set(
+                [x.invoice.id for x in move_lines if x.invoice]))
         return retval
     
     def match(self, cr, uid, ids, results=None, context=None):