Review: Disapprove
hello,
1) why did you change something in sale/sale_demo.xml? is there a reason? is it
intended? if yes: explain. if no, revert these changes: we want to keep the
diff as small as possible for the sake of clarity and easiness of reviewers.
2) new field pay_type should be renamed into advance_payment_method (+label),
which is more meaningfull
3) please add the context in the signature of the methods
+ def onchange_advance_product(self, cr, uid, ids, prod_id):
4) please pass the context while performing browse, we need it to get the
fields in the right language.
5) don't overwrite the default_get to set the product_id by default, use the
regular way with _default
5bis) btw, this default value won't work for multicompany cases because the
Advance product is dedicated to main company and the records rules prevent to
see products from other companies. You must either empty the company_id of
advance product or process this case in your method.
6) you removed the docstring of create_invoices(). Yes it was pretty useless
but it would have been better to /replace/ it by a real and usefull docstring.
7) change the error message to 'There is no income account defined as global
property'
8) there is a msising else statement jsut after this check, in line 155 of the
diff.
9) in the case the product isn't given, please use this as name instead of what
you've done
if type == fixed: 'Avance of %s %s' % (final_amount, currency_symbol)
if type == percentage: 'Advance of %s %% %(final_amount)
10) line 221 of your diff: context should be a kwarg
11) line 288 of your diff:
+ <field name="pay_type" nolabel="1" attrs="{'readonly':
[('product_id','=',51)]}" on_change="onchange_pay_type(pay_type,product_id)"/>
hahaha good one! you must be kidding, arent' you? no? really?
Dude, you can't hardcode the product_id of the advance product and expect it
will be the same in other database! There is nothing that ensure it and it for
sure break as soon as we'll add/remove a demo product for example.
12) Finally, i think we should allow to choose to enter the product or not for
both advance payment method. The fact it's a fixed amount or a percentage of
the sale order, should only affect the final amount, not the usability of
fields, and the chioce of a product can be optional, to choose the income
account (but the name should always be as i defined in 9)
contact me if one point isn't clear.
Thanks,
Quentin
--
https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-advance_invoice-ssu/+merge/102087
Your team OpenERP R&D Team is subscribed to branch
lp:~openerp-dev/openobject-addons/trunk-advance_invoice-ssu.
_______________________________________________
Mailing list: https://launchpad.net/~openerp-dev-gtk
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openerp-dev-gtk
More help : https://help.launchpad.net/ListHelp
