Merge lp:~pablocm/account-payment/7.0-account-payment-extension-fix-invoice-field into lp:~account-payment-team/account-payment/7.0

Proposed by PabloCM
Status: Needs review
Proposed branch: lp:~pablocm/account-payment/7.0-account-payment-extension-fix-invoice-field
Merge into: lp:~account-payment-team/account-payment/7.0
Diff against target: 31 lines (+4/-3)
1 file modified
account_payment_extension/account_move_line.py (+4/-3)
To merge this branch: bzr merge lp:~pablocm/account-payment/7.0-account-payment-extension-fix-invoice-field
Reviewer Review Type Date Requested Status
Pedro Manuel Baeza Needs Fixing
Review via email: mp+203948@code.launchpad.net

Description of the change

[FIX] Fixed _invoice parameter names and redefined the invoice field so that the new _invoice and _invoice_search get called
[FIX] Removed unused variable

To post a comment you must log in.
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

Hi, Pablo, thank you very much for the fix, because it's very critical to get this thing fix, but there are some things you can fix:

- You don't need to redefine the field invoice. This field is already defined in account module and if you put the same method name, it will be got called by inheritance. If you overwrite the field, any change in the core in the future will not be reflected.

- You can make the renaming with another way: instead of staying with cursor and user variables and change signature, you can rename all the ocurrences to the more standarized cr and uid. This is no problem, because overriding methods, positional arguments names doesn't matter.

- Why are you removing 'invoice_ids = []' line?

- Doesn't occur the same problem with arguments with _invoice_search?

Regards.

review: Needs Fixing
Revision history for this message
PabloCM (pablocm) wrote :

> Hi, Pablo, thank you very much for the fix, because it's very critical to get
> this thing fix, but there are some things you can fix:

Hi, np, it's the least I can do.

> - You don't need to redefine the field invoice. This field is already defined
> in account module and if you put the same method name, it will be got called
> by inheritance. If you overwrite the field, any change in the core in the
> future will not be reflected.

I know it is really bad to overwrite the field, shadowing future changes, but
I had to. If I don't, the new methods won't get called. Maybe it's only happening
in the instance I've tested it (openerp-7.0-20130806-231058) but are you sure it
works as you say?

> - You can make the renaming with another way: instead of staying with cursor
> and user variables and change signature, you can rename all the ocurrences to
> the more standarized cr and uid. This is no problem, because overriding
> methods, positional arguments names doesn't matter.

The parameters in the original function it is redefining are named that way and
I thought it would be better to have both with the same signature.
I'll change them to follow the standard as you suggest.

> - Why are you removing 'invoice_ids = []' line?

It was unused, why keep it?

> - Doesn't occur the same problem with arguments with _invoice_search?

As you say, there is no problem when having different names for positional arguments.
For consistency shake I should have renamed them, but since we are going to stick to
the standarized names -cr and uid- it is fine as it is.

Regards

Revision history for this message
PabloCM (pablocm) wrote :

> I know it is really bad to overwrite the field, shadowing future changes, but
> I had to. If I don't, the new methods won't get called. Maybe it's only
> happening
> in the instance I've tested it (openerp-7.0-20130806-231058) but are you sure
> it
> works as you say?

By the way, this is the reason the problem with the variable names had
passed unnoticed: because since the field wasn't overwritten, the new _invoice
was never being called. It should have thrown an error as soon as you opened the
"Journal Items" menu.

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

This project is now hosted on https://github.com/OCA/account-payment. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

Unmerged revisions

107. By Pablo <pablo@pablo-pc>

[FIX] Fixed _invoice parameter names and redefined the invoice field so that the new _invoice and _invoice_search get called
[FIX] Removed unused variable

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_payment_extension/account_move_line.py'
2--- account_payment_extension/account_move_line.py 2013-11-10 17:00:26 +0000
3+++ account_payment_extension/account_move_line.py 2014-01-30 12:34:27 +0000
4@@ -31,7 +31,7 @@
5 class account_move_line(orm.Model):
6 _inherit = 'account.move.line'
7
8- def _invoice(self, cr, uid, ids, name, arg, context=None):
9+ def _invoice(self, cursor, user, ids, name, arg, context=None):
10 invoice_obj = self.pool.get('account.invoice')
11 res = {}
12 for line_id in ids:
13@@ -43,8 +43,7 @@
14 'left join account_move_line p on l.reconcile_partial_id=p.reconcile_partial_id and l.id<>p.id ' \
15 'where (i.move_id = l.move_id or i.move_id = r.move_id or i.move_id = p.move_id) ' \
16 'AND l.id IN %s',
17- (tuple(ids),))
18- invoice_ids = []
19+ (tuple(ids),))
20
21 for line_id, invoice_id in cursor.fetchall():
22 name = invoice_obj.name_get(cursor, user, [invoice_id], context=context)
23@@ -195,6 +194,8 @@
24 'payment_type': fields.function(_payment_type_get,
25 type="many2one", relation="payment.type", method=True,
26 string="Payment type", fnct_search=_payment_type_search),
27+ 'invoice': fields.function(_invoice, string='Invoice',
28+ type='many2one', relation='account.invoice', fnct_search=_invoice_search),
29 }
30
31 def write(self, cr, uid, ids, vals, context=None,

Subscribers

People subscribed via source and target branches