Code review comment for lp:~openerp-dev/openobject-addons/trunk-addons-context

Revision history for this message
Olivier Dony (Odoo) (odo-openerp) wrote :

Hello,

This looks great, and will be a very nice improvement!

I see two little remaining things to fix to make it perfect :-)

1. I noticed a few _constraints in addons that still do not have a context in this merge prop:
- account.account.template check_cycle
- account.tax.code check_cycle
- account.tax.code.template check_cycle
(These 3 could be replaced by the generic osv.osv._check_recursion I think!)

- stock.move _check_product_lot
- stock.move _check_tracking

(Note: I've also fixed the _constraints remaining in the server, just pushed it)

2. We should only test the value of the context within a method that actually reads/writes the context. Everywhere else the context should just be passed around, e.g.:

   def method(self, cr, uid, ids, context=None):
      self.read(cr, uid, ids, context=context)

No need to test the context value, it's okay to forward a None value. If down in the stack another function needs to actually read/write the context, then it will do the check for None.

Thus there are many similar blocks that can be removed, to make the code smaller (which is always good). In the current state of the merge proposal I see many cases, for example:

  - account/wizard/account_subscription_generate.py
  - account_analytic_analysis/account_analytic_analysis.py (only needed in _read_flat)
  - etc.

Thanks a lot for the good work!

review: Needs Fixing ((more small improvements))

« Back to merge proposal