Review: Needs Fixing Hello,
please find hereunder few comment on your proposal. Line numbers are the lines inside the diff. * lines 77 to 140: why do you remove that part? it was usefull to test few things (but unfortunately it was not implemented). So can you please put those lines back and improved them in order to: --test that the second invoice created has a total amount equals to (SO amount - Advance invoice amount). --to test the percentage of invoice on the SO * line 341: your test is not checking anything except that manual_invoice() returns some value. But if you check if the returned value is a valid action, then it can stay. Otherwise it's useless. * lines 664 to 677: why do you define again vals? can't you just use res['value'] and the defaults? * lines 1093, 1110,.. : 'invoiceS' (check the plural of the label displayed as you're working on 2 invoices and not on 1 anymore) * lines 1142 to 1153: this is not a good test to check if an error is raised because if no exception is raised, the system will never execute the code into the except. You should do try: <do the part that should raise an error> assert False, "An error is not raised" except Exception, e: <continue or just print something> And finally, please make sure that all the test (yours and the existing ones) are working with your modifications. I had this error (not linked to your modif but still, it's important when you propose something to show that you've checked that everything was (still) working. ;-) Thanks, in the meanwhile i'm setting this merge prop in 'work in progress' to clean the TODO-list, Quentin -- https://code.launchpad.net/~openerp-dev/openobject-addons/trunk-sale_coverage-mtr/+merge/73351 Your team OpenERP R&D Team is subscribed to branch lp:~openerp-dev/openobject-addons/trunk-sale_coverage-mtr. _______________________________________________ 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
