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

Proposed by Holger Brunn (Therp)
Status: Merged
Merged at revision: 168
Proposed branch: lp:~therp-nl/banking-addons/6.1_fix__get_move_info_semantics
Merge into: lp:banking-addons/6.1
Diff against target: 79 lines (+26/-41)
1 file modified
account_banking/ (+26/-41)
To merge this branch: bzr merge lp:~therp-nl/banking-addons/6.1_fix__get_move_info_semantics
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) Approve
Holger Brunn (Therp) Needs Resubmitting
Review via email:

Description of the change

Quote from the discussion on

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.

To post a comment you must log in.
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

Now I ask myself: Why do we loop through all move lines, setting partner_id and account_id, but resetting it if they differ? If we don't care, we can take the first *_id found and then break. If we do care, we should do something in the lines of this MP

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

Beg your pardon and ignore my last comment. This branch is spot on.

One question: is the fact that identification between the browse records works dependent on an implementation detail of browse_record or is it truely robust?

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

it's because browse_record takes care for that in __eq__:

But how is that not truely robust?

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

Yes, I'd call that truely robust. It would be bad if it would only work with a specific implementation of record caching, but the code you referred to does check the record id.

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

Found a typo while testing: missing closing square bracket in the three calls to all()

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

Sorry, just the first one actually.

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

Still one more in l.65

170. By Holger Brunn (Therp)

[FIX] add closing brackets where they belong

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

Thanks for the pointers, that's fixed now

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


review: Approve

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-20 08:09:27 +0000
4@@ -1016,49 +1016,34 @@
5 'account_id': False,
6 }
7 move_lines = self.pool.get('account.move.line').browse(cr, uid, move_line_ids)
8- for move_line in move_lines:
9- if move_line.partner_id:
10- if retval['partner_id']:
11- if retval['partner_id'] !=
12- retval['partner_id'] = False
13- break
14- else:
15- retval['partner_id'] =
16- else:
17- if retval['partner_id']:
18- retval['partner_id'] = False
19- break
20- for move_line in move_lines:
21- if move_line.account_id:
22- if retval['account_id']:
23- if retval['account_id'] !=
24- retval['account_id'] = False
25- break
26- else:
27- retval['account_id'] =
28- else:
29- if retval['account_id']:
30- retval['account_id'] = False
31- break
32- for move_line in move_lines:
33- if move_line.invoice:
34- if retval['match_type']:
35- if retval['match_type'] != 'invoice':
36- retval['match_type'] = False
37- break
38- else:
39- retval['match_type'] = 'invoice'
40- else:
41- if retval['match_type']:
42- retval['match_type'] = False
43- break
44- if move_lines and not retval['match_type']:
46+ if not move_lines:
47+ return retval
49+ if move_lines[0].partner_id and all(
50+ [move_line.partner_id == move_lines[0].partner_id
51+ for move_line in move_lines]):
52+ retval['partner_id'] = move_lines[0]
54+ if move_lines[0].account_id and all(
55+ [move_line.account_id == move_lines[0].account_id
56+ for move_line in move_lines]):
57+ retval['account_id'] = move_lines[0]
59+ if move_lines[0].invoice and all(
60+ [move_line.invoice == move_lines[0].invoice
61+ for move_line in move_lines]):
62+ retval['match_type'] = 'invoice'
63+ retval['type'] = type_map[move_lines[0].invoice.type]
64+ retval['invoice_ids'] = list(
65+ set([ for x in move_lines]))
67+ if not retval['match_type']:
68 retval['match_type'] = 'move'
69- if move_lines and len(move_lines) == 1:
71+ if len(move_lines) == 1:
72 retval['reference'] = move_lines[0].ref
73- if retval['match_type'] == 'invoice':
74- retval['invoice_ids'] = list(set([ for x in move_lines]))
75- retval['type'] = type_map[move_lines[0].invoice.type]
77 return retval
79 def match(self, cr, uid, ids, results=None, context=None):


People subscribed via source and target branches