Code review comment for lp:~openerp-dev/openobject-server/trunk-extended-view-inheritance-xmo

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

Note: moved to ~openerp-dev namespace for runbot testing, you probably need to sync with trunk to have the necessary *Bundle stuff for latest trunk compatibility

Looks good to me, some minor remarks:

- could we add a small @help for the new `mode` field to make its meaning crystal clear (you know, with the view_type/view_mode conspiracy ;-))
- shouldn't the import_xml.rng schema be modified to explicitly mention @primary for <template>?
- wouldn't _check_mode() be simpler to implement as a pure SQL constraint (_sql_constraints)?
- not sure if I missed it when reading the TestViewCombined testcase, but we theoretically have a valid case when we fork a second primary view within the same model (e.g. a4 could be forked off a1 with primary=true), leading to a case similar to test_cross_model_simple() except not cross-model ;-) Should we test it too?
- we do have existing views in addons that are using the old/implicit version of this hack (such as the stock.picking.{in,out} views based on stock.picking's views, shouldn't we turn this to explicit with primary=True, and get rid of the explicit view_id references where they're not needed anymore (as primary views will be selected by as default views).
- minor typos:
  + l.223 "mode=extendion"
  + l.268 "def testInheritPirmaryToExtension"

Thanks for the _invalids cleanup too!

« Back to merge proposal