Merge lp:~pedro.baeza/account-payment/6.1-set_done_optimization into lp:~account-payment-team/account-payment/6.1

Proposed by Pedro Manuel Baeza on 2013-08-30
Status: Merged
Merged at revision: 111
Proposed branch: lp:~pedro.baeza/account-payment/6.1-set_done_optimization
Merge into: lp:~account-payment-team/account-payment/6.1
Diff against target: 201 lines (+65/-50)
1 file modified
account_payment_extension/payment.py (+65/-50)
To merge this branch: bzr merge lp:~pedro.baeza/account-payment/6.1-set_done_optimization
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) Approve on 2013-12-10
Joël Grand-Guillaume @ camptocamp code review, no tests Needs Fixing on 2013-10-17
Maxime Chambreuil (http://www.savoirfairelinux.com) code review 2013-08-30 Approve on 2013-09-30
Review via email: mp+183234@code.launchpad.net

Commit message

[IMP] account_payment_extension: set_done() method in payment.py execution optimized
[FIX] account_payment_extension: date used for some account move lines are not the same for the same account move, what is illegal.

Description of the change

This MP contains two things for account_payment_extension module:

- Method set_done() in payment.py execution has been optimized, gaining a lot of perfomance of big payment orders, because it skips the validation of the complete move each time a line is added.
- Date used for some account move lines are not the same for the same account move, what is illegal.

This improvement has been already merged in 7.0 branch:

https://code.launchpad.net/~pedro.baeza/account-payment/7.0-improvements/+merge/164400

To post a comment you must log in.
Pedro Manuel Baeza (pedro.baeza) wrote :

Can anyone review this MP, please?

review: Approve (code review)

Hi,

Thank you for the contribution ! I would have explicitly written context=context (or ctx) in lines : 14, 79, 108, 125, 144, 153.

Otherwise good !

Thanks,

review: Needs Fixing (code review, no tests)
107. By Pedro Manuel Baeza on 2013-10-17

[IMP] account_payment_extension: Some PEP8 and keyword arguments.

108. By Pedro Manuel Baeza on 2013-10-17

[FIX] account_payment_extension: corrected context variable that can't be passed as a keyword argument.

Pedro Manuel Baeza (pedro.baeza) wrote :

Thanks for the review, Joël. Indeed, when I made this MP, we haven't discussed yet about keyword argumentsap. We are refining us each day ;)

I have made also some PEP8 refactorisation, but there is one context variable that I can't put as keyword because super method (in account_payment module) has *args in the signature.

Please make the merge when you approve the MP, because it has been a lot of time since I made the proposal.

Regards.

Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Joël, can you please re-review this MP and proceed with the merge?

Regards.

l.199, is there a reason that you don't call account_move_line::post()?

LGTM otherwise.

review: Needs Information
Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Stefan, I'm using the same code for this task that was previously set, but I don't know if there is any reason to make that on first place. Making a quick scan over post method on account.move model, I see that two tasks are performed:

- An integrity check. This is maybe redundant or useless, and bad for performance when you do big moves (I have some environments with payment orders with 3k lines).
- A sequence number assignation. This makes me wonder how the number is assigned in this code.

Tomorrow I will check deeply.

Thanks for the tip.

Regards.

Hi Pedro, thank you for looking into this. Apart from your observations, using the post() method is API, and the use of that should be encouraged to prevent incompatibilities amongst various modules.

> Hi, Stefan, I'm using the same code for this task that was previously set, but
> I don't know if there is any reason to make that on first place. Making a
> quick scan over post method on account.move model, I see that two tasks are
> performed:
>
> - An integrity check. This is maybe redundant or useless, and bad for
> performance when you do big moves (I have some environments with payment
> orders with 3k lines).
> - A sequence number assignation. This makes me wonder how the number is
> assigned in this code.
>
> Tomorrow I will check deeply.
>
> Thanks for the tip.
>
> Regards.

Hi, could you check?

Pedro Manuel Baeza (pedro.baeza) wrote :

Sorry, I have been very busy. I will do it ASAP.

Regards.

109. By Pedro Manuel Baeza on 2013-11-25

[IMP] account_payment_extension: call to post method

Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Stefan, I have checked the impact putting a call to post method instead of making the change directly to the object, and it has not too much impact. I have tried with a 3k lines payment order and it only takes about 20 seconds more. What is to worry about is that the rest of the process takes 76 minutes! But this is another question...

I have changed the code accordingly

Regards.

Thanks!

review: Approve

I'm merging this because all the other comments have been honoured too.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_payment_extension/payment.py'
2--- account_payment_extension/payment.py 2013-06-25 20:17:09 +0000
3+++ account_payment_extension/payment.py 2013-11-25 09:42:05 +0000
4@@ -200,29 +200,42 @@
5 def set_done(self, cr, uid, ids, context=None):
6 result = super(payment_order, self).set_done(cr, uid, ids, context)
7
8- company_currency_id = self.pool.get('res.users').browse(cr, uid, uid, context).company_id.currency_id.id
9+ move_obj = self.pool.get('account.move')
10+ move_line_obj = self.pool.get('account.move.line')
11+ currency_obj = self.pool.get('res.currency')
12+ payment_line_obj = self.pool.get('payment.line')
13+ users_obj = self.pool.get('res.users')
14+
15+ user = users_obj.browse(cr, uid, uid, context=context)
16+ currency = user.company_id.currency_id
17+ company_currency_id = currency.id
18
19 for order in self.browse(cr, uid, ids, context):
20 if order.create_account_moves != 'direct-payment':
21 continue
22
23- # This process creates a simple account move with bank and line accounts and line's amount. At the end
24- # it will reconcile or partial reconcile both entries if that is possible.
25-
26- move_id = self.pool.get('account.move').create(cr, uid, {
27+ # This process creates a simple account move with bank and line
28+ # accounts and line's amount. At the end it will reconcile or
29+ # partial reconcile both entries if that is possible.
30+ move_id = move_obj.create(cr, uid, {
31 'name': '/',
32 'journal_id': order.mode.journal.id,
33 'period_id': order.period_id.id,
34 }, context)
35-
36+
37 for line in order.line_ids:
38 if not line.amount:
39 continue
40
41 if not line.account_id:
42- raise osv.except_osv(_('Error!'), _('Payment order should create account moves but line with amount %.2f for partner "%s" has no account assigned.') % (line.amount, line.partner_id.name ) )
43+ raise osv.except_osv(_('Error!'), _('Payment order should '
44+ 'create account moves but line with amount %.2f for '
45+ 'partner "%s" has no account assigned.') %(
46+ line.amount, line.partner_id.name))
47
48- currency_id = order.mode.journal.currency and order.mode.journal.currency.id or company_currency_id
49+ currency_id = (order.mode.journal.currency and
50+ order.mode.journal.currency.id or
51+ company_currency_id)
52
53 if line.type == 'payable':
54 line_amount = line.amount_currency or line.amount
55@@ -233,22 +246,20 @@
56 account_id = order.mode.journal.default_credit_account_id.id
57 else:
58 account_id = order.mode.journal.default_debit_account_id.id
59- acc_cur = ((line_amount<=0) and order.mode.journal.default_debit_account_id) or line.account_id
60+ acc_cur = ((line_amount<=0) and
61+ order.mode.journal.default_debit_account_id
62+ ) or line.account_id
63
64 ctx = context.copy()
65 ctx['res.currency.compute.account'] = acc_cur
66- amount = self.pool.get('res.currency').compute(cr, uid, currency_id, company_currency_id, line_amount, context=ctx)
67- move_date = order.date_done
68- if order.date_prefered == 'due':
69- move_date = line.move_line_id.date_maturity
70- if order.date_prefered == 'fixed':
71- move_date = order.date_scheduled
72-
73+ amount = currency_obj.compute(cr, uid, currency_id,
74+ company_currency_id, line_amount,
75+ context=ctx)
76+
77 val = {
78 'name': line.move_line_id and line.move_line_id.name or '/',
79 'move_id': move_id,
80- 'date': move_date,
81- 'date_created': order.date_done,
82+ 'date': order.date_done,
83 'ref': line.move_line_id and line.move_line_id.ref or False,
84 'partner_id': line.partner_id and line.partner_id.id or False,
85 'account_id': line.account_id.id,
86@@ -257,22 +268,31 @@
87 'journal_id': order.mode.journal.id,
88 'period_id': order.period_id.id,
89 'currency_id': currency_id,
90+ 'state': 'valid',
91 }
92
93- amount = self.pool.get('res.currency').compute(cr, uid, currency_id, company_currency_id, line_amount, context=ctx)
94 if currency_id <> company_currency_id:
95- amount_cur = self.pool.get('res.currency').compute(cr, uid, company_currency_id, currency_id, amount, context=ctx)
96+ amount_cur = currency_obj.compute(cr, uid,
97+ company_currency_id,
98+ currency_id, amount,
99+ context=ctx)
100 val['amount_currency'] = -amount_cur
101
102- if line.account_id and line.account_id.currency_id and line.account_id.currency_id.id <> company_currency_id:
103+ if line.account_id and line.account_id.currency_id and \
104+ line.account_id.currency_id.id <> company_currency_id:
105 val['currency_id'] = line.account_id.currency_id.id
106 if company_currency_id == line.account_id.currency_id.id:
107 amount_cur = line_amount
108 else:
109- amount_cur = self.pool.get('res.currency').compute(cr, uid, company_currency_id, line.account_id.currency_id.id, amount, context=ctx)
110+ amount_cur = currency_obj.compute(cr, uid,
111+ company_currency_id,
112+ line.account_id.currency_id.id, amount,
113+ context=ctx)
114 val['amount_currency'] = amount_cur
115
116- partner_line_id = self.pool.get('account.move.line').create(cr, uid, val, context, check=False)
117+ partner_line_id = move_line_obj.create(cr, uid, val,
118+ context=context,
119+ check=False)
120
121 # Fill the secondary amount/currency
122 # if currency is not the same than the company
123@@ -283,11 +303,10 @@
124 amount_currency = False
125 move_currency_id = False
126
127- self.pool.get('account.move.line').create(cr, uid, {
128+ move_line_obj.create(cr, uid, {
129 'name': line.move_line_id and line.move_line_id.name or '/',
130 'move_id': move_id,
131- 'date': move_date,
132- 'date_created': order.date_done,
133+ 'date': order.date_done,
134 'ref': line.move_line_id and line.move_line_id.ref or False,
135 'partner_id': line.partner_id and line.partner_id.id or False,
136 'account_id': account_id,
137@@ -297,45 +316,41 @@
138 'period_id': order.period_id.id,
139 'amount_currency': amount_currency,
140 'currency_id': move_currency_id,
141- }, context)
142-
143- aml_ids = [x.id for x in self.pool.get('account.move').browse(cr, uid, move_id, context).line_id]
144- for x in self.pool.get('account.move.line').browse(cr, uid, aml_ids, context):
145- if x.state <> 'valid':
146- raise osv.except_osv(_('Error !'), _('Account move line "%s" is not valid') % x.name)
147+ 'state': 'valid',
148+ }, context=context, check=False)
149
150 if line.move_line_id and not line.move_line_id.reconcile_id:
151 # If payment line has a related move line, we try to reconcile it with the move we just created.
152 lines_to_reconcile = [
153 partner_line_id,
154 ]
155-
156 # Check if payment line move is already partially reconciled and use those moves in that case.
157 if line.move_line_id.reconcile_partial_id:
158 for rline in line.move_line_id.reconcile_partial_id.line_partial_ids:
159- lines_to_reconcile.append( rline.id )
160+ lines_to_reconcile.append(rline.id)
161 else:
162- lines_to_reconcile.append( line.move_line_id.id )
163-
164+ lines_to_reconcile.append(line.move_line_id.id)
165 amount = 0.0
166- for rline in self.pool.get('account.move.line').browse(cr, uid, lines_to_reconcile, context):
167+ for rline in move_line_obj.browse(cr, uid,
168+ lines_to_reconcile,
169+ context=context):
170 amount += rline.debit - rline.credit
171-
172- currency = self.pool.get('res.users').browse(cr, uid, uid, context).company_id.currency_id
173-
174- if self.pool.get('res.currency').is_zero(cr, uid, currency, amount):
175- self.pool.get('account.move.line').reconcile(cr, uid, lines_to_reconcile, 'payment', context=context)
176+
177+ if currency_obj.is_zero(cr, uid, currency, amount):
178+ move_line_obj.reconcile(cr, uid, lines_to_reconcile,
179+ 'payment', context=context)
180 else:
181- self.pool.get('account.move.line').reconcile_partial(cr, uid, lines_to_reconcile, 'payment', context)
182-
183- if order.mode.journal.entry_posted:
184- self.pool.get('account.move').write(cr, uid, [move_id], {
185- 'state':'posted',
186- }, context)
187-
188- self.pool.get('payment.line').write(cr, uid, [line.id], {
189+ move_line_obj.reconcile_partial(cr, uid,
190+ lines_to_reconcile,
191+ 'payment',
192+ context=context)
193+ # Annotate the move id
194+ payment_line_obj.write(cr, uid, [line.id], {
195 'payment_move_id': move_id,
196 }, context)
197+ # Post the move
198+ if order.mode.journal.entry_posted:
199+ move_obj.post(cr, uid, [move_id], context=context)
200
201 return result
202

Subscribers

People subscribed via source and target branches