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

Proposed by Holger Brunn (Therp)
Status: Rejected
Rejected by: Holger Brunn (Therp)
Proposed branch: lp:~therp-nl/banking-addons/6.1_lp1176783
Merge into: lp:banking-addons/6.1
Diff against target: 22 lines (+3/-2)
1 file modified
account_banking/ (+3/-2)
To merge this branch: bzr merge lp:~therp-nl/banking-addons/6.1_lp1176783
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) Needs Fixing
Review via email:
To post a comment you must log in.
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

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]
        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
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

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.

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

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.

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

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_banking/'
2--- account_banking/ 2013-04-26 08:27:33 +0000
3+++ account_banking/ 2013-05-06 07:43:28 +0000
4@@ -1048,6 +1048,7 @@
5 break
6 else:
7 retval['match_type'] = 'invoice'
8+ retval['type'] = type_map[move_line.invoice.type]
9 else:
10 if retval['match_type']:
11 retval['match_type'] = False
12@@ -1057,8 +1058,8 @@
13 if move_lines and len(move_lines) == 1:
14 retval['reference'] = move_lines[0].ref
15 if retval['match_type'] == 'invoice':
16- retval['invoice_ids'] = list(set([ for x in move_lines]))
17- retval['type'] = type_map[move_lines[0].invoice.type]
18+ retval['invoice_ids'] = list(set(
19+ [ for x in move_lines if x.invoice]))
20 return retval
22 def match(self, cr, uid, ids, results=None, context=None):


People subscribed via source and target branches