Merge lp:~openerp-community/openerp-mgmtsystem/nc-extend into lp:openerp-mgmtsystem/6.1

[Daniel Reis]

Changes done.

Maxime, note that many2many relations have their columns inverted. m2m parameters are: other_obj, rel_table, this_obj_id_column, other_obj_id_column). For example, in mgmtsystem_audit you find:
    fields.many2many('mgmtsystem.nonconformity', 'mgmtsystem_audit_nonconformity_rel', 'mgmtsystem_action_id', 'mgmtsystem_audit_id', 'Nonconformities'),

It's not quite right, but maybe we should leave it as it is.

[Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903)]

Can you solve the conflicts and resubmit?

review: Needs Fixing

[Daniel Reis (dreis-pt)]

Rebased the branch (to r16) and fixed the conflicts.
Please review.

review: Needs Resubmitting

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Quick eyeball review, I did not reviewed the business aspect.

dubious indentation at l. 647

837 + 'audit_ids': fields.many2many('mgmtsystem.audit','mgmtsystem_audit_nonconformity_rel','mgmtsystem_audit_id','mgmtsystem_action_id','Related Audits'),
Just a remark, the relation name and column names are no longer necessary, so you can just write:
'audit_ids': fields.many2many('mgmtsystem.audit', 'Related Audits'),
No need to change that though.

1646+ 'description': fields.text('Description', translation=True),
2801+ 'description': fields.text('Description', translation=True),
typo: s/translation/translate/

The context propagation is often missing.

1795+ vals = {'evaluation_date': time.strftime('%Y-%m-%d %H:%M'), 'evaluation_date_user_id': uid }
It would be better if the date time formatting use the constant openerp.tools.DEFAULT_SERVER_DATETIME_FORMAT

1655 +_STATES = [
1656 + ('d', _('Draft')),
1657 + ('a', _('Analysis')),
1658 + ('p', _('Pending Approval')),
1659 + ('o', _('In Progress')),
1660 + ('c', _('Closed')),
1661 + ('x', _('Cancelled')),
1662 + ]
The state codes are meaningless.
How do a developer knows what will be filtered in this domain? [('state','=','d')]
This lack of meaning seems to be common on the fields.selection of different classes across the merge proposal.

On the usage of namespaces, classes and instanciation of models:
2591 +from osv import fields, osv
2592 +
2593 +class mgmtsystem_nonconformity(osv.osv):
2594 + _inherit = "mgmtsystem.nonconformity"
2595 + _columns = {
2596 + 'analytic_account_id': fields.many2one('account.analytic.account', 'Contract'),
2597 + }
2598 +mgmtsystem_nonconformity()
You better have to use the complete namespace (by the way the import of osv is now useless if we just need Model) :
  from openerp.osv import fields, orm
and inherit of the new classes:
  class mgmtsystem_nonconformity(orm.Model):
The instanciation (l.2598) after the class declaration is no longer required.

Unrelated to the code, the review type "Resubmit" is to be used by a reviewer to ask you to resubmit, not the other way. You can see in the reviewer list on top of the page that you are now part of the reviewers for your proposal, and you ask yourself a resubmit ;-)

review: Needs Fixing

[Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903)]

Analysis for the migration of mgmtsystem_nonconformity v0.1 to v0.2:
* effectiveness_* needs to be concatenated into evaluation_comments
* corrective_action_id and preventive_action_id needs to be merged in action_ids

[Daniel Reis (dreis-pt)]

review: Abstain

[Daniel Reis (dreis-pt)]

Maxime, what do you think on changing the "state" codes from letters to words?
Do you prefer this to be done now, and you'll include that on the migration script, or should we postpone for the v7 migration?

[Daniel Reis (dreis-pt)]

Guewen: Thanks for the review; fixes are in progress.

I'd like to know a little more about your remark «On the usage of namespaces, classes and instanciation of models».
I did some browsing on the official addons, and found some cases where `from openerp.osv import fields, osv` instead of `from osv import fields, osv` (auth_openid, for instance).

But I couldn't find examples for the other recommendations (import orm instead of osv; class on orm.Model instead of osv.osv; deprecate class instanciation after declaration).

Do you have some links on resources for me to learn more on this?
Thanks.

[Daniel Reis (dreis-pt)]

Maxime: should licenses change from GPL3 to AGPL?

[Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903)]

> Maxime, what do you think on changing the "state" codes from letters to words?
> Do you prefer this to be done now, and you'll include that on the migration
> script, or should we postpone for the v7 migration?

Let's do it now.

> Maxime: should licenses change from GPL3 to AGPL?

Yes!

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

On 14/12/2012 11:24, Daniel Reis (SECURITAS SA) wrote:
> Guewen: Thanks for the review; fixes are in progress.
>
> I'd like to know a little more about your remark «On the usage of namespaces, classes and instanciation of models».
> I did some browsing on the official addons, and found some cases where `from openerp.osv import fields, osv` instead of `from osv import fields, osv` (auth_openid, for instance).
>
> But I couldn't find examples for the other recommendations (import orm instead of osv; class on orm.Model instead of osv.osv; deprecate class instanciation after declaration).
>
> Do you have some links on resources for me to learn more on this?
> Thanks.

Hello Daniel,

The source code of osv/osv.py says that the osv class is deprecated.
Obviously it's not going to disapear in 7, but Model is clearer IMO that
osv (and not that much longer to type).

The model class instanciation is not deprecated. It's juste that it is
no longer required since OpenERP 6. You can keep it, but it is just
noise and visual clutter in the source code, hence the advice to drop it.

There are some things on the move in the official addons in trunk. I
submitted a merge proposal to clean up modules but it has not been
proposed so far (see
https://bugs.launchpad.net/openobject-server/+bug/1052392).

--
Alexandre Fayolle
Chef de Projet
Tel : + 33 (0)4 79 26 57 94

Camptocamp France SAS
Savoie Technolac, BP 352
73377 Le Bourget du Lac Cedex
http://www.camptocamp.com

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

> There are some things on the move in the official addons in trunk. I
> submitted a merge proposal to clean up modules but it has not been
> proposed so far

s/proposed/processed/

[lp:~openerp-community/openerp-mgmtsystem/nc-extend]

53. By Maxime Chambreuil (http://www.savoirfairelinux.com) on 2012-12-18

[FIX] NameError: global name 'context' is not defined when resetting a NC

54. By Maxime Chambreuil (http://www.savoirfairelinux.com) on 2012-12-18

[ADD] Skeleton for migration script

55. By Maxime Chambreuil (http://www.savoirfairelinux.com) on 2012-12-19

[ADD] openupgrade analysis

56. By Maxime Chambreuil (http://www.savoirfairelinux.com) on 2012-12-19

[FIX] Fix analysis after fixing the environments

[Maxime Chambreuil (http://www.savoirfairelinux.com) (max3903)]

Needs migration scripts

review: Needs Fixing

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Hi,

Thanks for your changes.

You introduced a bug in your revision 53, you can't do that:

 3122 + def case_reset(self, cr, uid, ids, context=None, *args):

The keyword arguments should always be after the positional arguments.
Your method will be parsed correctly, but if you try to call it using the keyword and some extra args:

    case_reset(cr, uid, ids, context={}, arg1, arg2)

You'll have a syntax error:

    SyntaxError: non-keyword arg after keyword arg

I think that the only way to have a keyword argument with *args is:

    def case_reset(self, cr, uid, ids, *args, **kwargs):
        context = kwargs.get('context')

review: Needs Fixing (no test, review)

[lp:~openerp-community/openerp-mgmtsystem/nc-extend]

57. By Virgil Dupras on 2013-01-25

[ADD] Added migration scripts for 0.1 --> 1.0 in the modules that needed it.

mgmtsystem_nonconformity:

* o --> open c --> done state change.
* Migrating action relations to a many2many table.
* Concatening action comments in a single field

mgmtsystem_audit and mgmtsystem_review:

* o --> open c --> done state change.

58. By Virgil Dupras on 2013-01-25

[FIX] I had accidentally broken the nonconformity/action migration shortly before commit.

59. By Virgil Dupras on 2013-01-28

[ADD] Added the System field in Action form and filter views.

60. By Virgil Dupras on 2013-01-28

Merge

61. By Virgil Dupras on 2013-01-29

[MRG]

62. By Maxime Chambreuil (http://www.savoirfairelinux.com) on 2013-01-29

[REN] migration folders 6.1.1.0 -> 1.0

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

guewen: for the record, it is legal to have *args after a kw argument in a function definition in Python