Odoo Addons (MOVED TO GITHUB)

Refactor some code that is currently to hard to extend cleanly

Registered by Raphaël Valyi - http://www.akretion.com

Some code at the core of the modules is very hard to extend. This is bad because:
- you'll be missing some nice extensions because people can't code them
- you'll have a growing number of modules that try to extend anyway, but
    - are hard to read and share because they do more things than required, are too fat
    - are hard to maintain
    - are hard to reuse themselves

The causes of those bad designs are often:
1) methods are too fat. Probably they grew over the time or they were quickly done or third party additions. Methods larger than say 60 lines are too large for sure.
2) methods create/write records in the database without giving a chance to override what data should be created in the database. So currently we are too often forced to undo what the super method did (or override the world).
3) methods create/write records in the database without passing back the right ids that were created/writen. That's a basic things methods creating things in the database should do: return the ids of created records. OpenERP often do that, but not always unfortunately. The result is that customization modules have no clue about what is created, so it's hard for them to alter writen data in the database afterwards.

Basically you should
- split those methods into smaller bits
- when a method will create records at the end of it, make sure you call some method that could be overriden before and that could properly alter what will be created.
- when a method create some records, it should almost always pass back the ids of the created records.

Blueprint information

Status:
Complete
Approver:
None
Priority:
High
Drafter:
None
Direction:
Needs approval
Assignee:
Raphaël Valyi - http://www.akretion.com
Definition:
Discussion
Series goal:
None
Implementation:
Implemented
Milestone target:
milestone icon 6.0
Started by
Raphaël Valyi - http://www.akretion.com
Completed by
Raphaël Valyi - http://www.akretion.com

Sprints

Whiteboard

Fabien Pinckaers March 05:
Just a note to say we are working on this blueprint within the next two weeks.

Also, old-style wizards are a problem too: they can not be extended at all and have to be copied/pasted if their behavior has to change in an addon. They are also harder to write unit tests against.

Raphaël Valyi March 05:
Fabien, this is a great news. Now, I worked on this last week, and I should say, I should correct a few of things in the above list as a few of the issues here are wrong. But most of them hold true. The good news however, is that I think I fixed half of the real one in those two merge proposals I sent:
https://code.launchpad.net/~akretion-team/openobject-addons/extensible-inventory/+merge/20293
https://code.launchpad.net/~rvalyi/openobject-addons/extensible-mrp/+merge/20294
and that one that has been merged already: https://code.launchpad.net/~akretion-team/openobject-addons/5.2-refactor-methods-splits/+merge/19842

As for the old wizards: I made that blueprint: https://blueprints.launchpad.net/openobject-addons/+spec/refactor-old-wizards-on-osv-memory I agree that it's harder to test them indeed. Still, I recall that OERPScenario is perfect to test them. Now I see OpenERP SA already did a large part of that refactoring which is cool (except we double worked on a part of it because that was not communicated in advance). Now let me make a point: I think there is no need to refactor all of them in the middle term. Indeed, I think you never need to override 80% of those wizards in real projects. And I think there is nothing wrong either if you are the only one earth that copy/paste/change 20 lines of old wizard style code. Now, there are a few wizards you often need to customize indeed. I have listed our top list on the wizard blueprint page (that list can be completed). So here is the point: I think this is risky (and also useless for now) to try to refactor them all and merge them BEFORE having a very large test suite. We are yet to report it, but the only wizard we refactored too, when we looked at the code OpenERP S.A refactored in parallel, we think we just found a bug where the invoice on picking wizard would not work when invoicing several invoices. I mean this is tested nowhere and would be a hugly regression, if this were to happen in many wizards, I suggest we rather refactor only the main ones: those larges, with screens and often used, and rather spend time testing that's the refactoring is correct rather than refactoring all the other ones. hen we would finish the job in a next OpenERP version, once we have an extensive test suite.
I would appreciate you take a stance upon that.

All in all, here is a list of code that deserves refactoring:

======================================
1) ACCOUNT MODULE

in account/invoice.py

account_invoice.action_move_create
-> VERY IMPORTANT!

what is bad:
- too fat: 168 lines!!!
- create way to much things (like account lines) without giving a chance to alter them
- don't return the ids of created records

account_invoice._refund_cleanup_lines
-> VERY IMPORTANT!

what is bad:
- only works with the existing hardcoded fields. Any extension adding fields to an invoice should override this method to avoid refund to break.
You could may be make it work with introspection rather than having to hardcode the fields
Or you could at least remove the underscore at the beginning of the method to explicit modules should override it

account_invoice.poduct_id_change
-> in 5.2, it's now 128 lines -> too fat!

in account/account.py
wizard_multi_charts_accounts.action_create
-> NOT REALY IMPORTANT FOR ME

what is bad:
- too fat: 268 lines
- create to much thing without giving ids or giving control

======================================
2) SALE MODULE

in sale/sale.py

sale_order.action_ship_create
-> VERY IMPORTANT

what is bad:
- too large: 101 lines
- create procurements without linking them back to the sale order line with a key
- create records without giving back the id in general

sale_order_line.product_id_change
-> IMPORTANCE: MEDIUM

what is bad:
method is too large: 123 lines

======================================
3) MRP MODULE

in mrp/mrp.py

mrp_procurement.action_produce_assign_product
-> VERY IMPORTANT

what is bad:
We iterate over the procurements and create mrp.production objects.
But those mrp.production objects aren't linked backed safely by foreign key to their respective mrp.procurements.
Moreover, at the end of the method, we return the id of the last mrp.production object.

This is a problem because potentially this method can create SEVERAL mrp.production objects, so that single return id won't be enough to safely retrieve all the created mrp.production objects if you override action_produce_assign_product.

Instead, a minimum change would be to return the table of the ids of the created mrp.production objects.
Having a a foreign ley in mrp.production pointing back on the mrp.procurement could also help but may be that last change isn't acceptable for you. Having the table of ids returned would already help a lot and avoid copying the whole method when overriding the default behavior of action_produce_assign_product.

OK, I consider this to be done by now on v6. Great!

(?)

Work Items