Merge lp:~openerp-dev/openobject-server/6.0-opw-381612-ado into lp:openobject-server/6.0

[Amit Dodiya (OpenERP)]

Hello,

[FIX] Dependencies loop executes when you install a new module

When you install any new module, the dependencies loop is executes and check all the dependencies of all the dependant modules.

So when we have big dependencies tree, it will take lot of time to finish the installation.

Thanks,
Amit

[lp:~openerp-dev/openobject-server/6.0-opw-381612-ado]

3570. By Launchpad Translations on behalf of openerp on 2012-01-18

Launchpad automatic translations update.

3571. By Launchpad Translations on behalf of openerp on 2012-01-21

Launchpad automatic translations update.

[Naresh(OpenERP) (nch-openerp)]

review: Approve

[lp:~openerp-dev/openobject-server/6.0-opw-381612-ado]

3572. By Olivier Laurent (Open ERP) on 2012-01-27

[MERGE] opw 51026

3573. By Olivier Laurent (Open ERP) on 2012-01-27

[MERGE] opw 51093

3574. By Olivier Laurent (Open ERP) on 2012-01-27

[MERGE] opw 50831

3575. By Olivier Laurent (Open ERP) on 2012-01-27

[MERGE] opw 5801

3576. By Olivier Laurent (Open ERP) on 2012-01-27

reverted patch for 'opw 51026'

3577. By Launchpad Translations on behalf of openerp on 2012-01-28

Launchpad automatic translations update.

3578. By Launchpad Translations on behalf of openerp on 2012-02-04

Launchpad automatic translations update.

3579. By Launchpad Translations on behalf of openerp on 2012-02-08

Launchpad automatic translations update.

3580. By Launchpad Translations on behalf of openerp on 2012-02-09

Launchpad automatic translations update.

3581. By Olivier Laurent (Open ERP) on 2012-02-09

[FIX] ir_values: revert r3568: caused a crash when creating a new purchase.order

3582. By Christophe Simonis (OpenERP) on 2012-02-09

[IMP] backport of rev <email address hidden> of trunk: tools.translate: be more lenient and accept standart gettext source annotations

Original commit information:
------------------------------------------------------------
revno: 4002
revision-id: <email address hidden>
parent: <email address hidden>
committer: Olivier Dony <email address hidden>
branch nick: trunk
timestamp: Thu 2012-02-02 15:32:10 +0100
message:
  [IMP] tools.translate: be more lenient and accept standart gettext source annotations

   The GetText spec says that PO auto-comments indicating
    the source location have this form:
     #: /path/to/file:lineno
    However OpenERP's POT export system defaults to a modified
    version of this format with an extra 'type' field:
     #: type:/path/to/file:lineno
    The babel extractors we use for openerp-web's translations
    have the GetText format hardcoded, so it's a good idea
    to be more lenient and allow the standards annotations too.
    We can use a default 'code' type for such cases.

    For openerp-web translations the type does not matter,
    as the translations will be directly loaded by the
    web engine from the PO files and served in Javascript
   to the browser, with no typing needed.

    This patch simply avoids failing to parse updated PO files
    that will contain standard GetText annotations for the
    openerp-web related terms.

3583. By tfr (Openerp) on 2012-02-10

[MERGE] fix opw issue 56037 : res.log context need to allow more the 250 char

3584. By Olivier Laurent (Open ERP) on 2012-02-14

[FIX] resubmit patch for opw 51206 (cfr r3552, r3553, r3576). Also check addons revision 5056

3585. By Launchpad Translations on behalf of openerp on 2012-02-17

Launchpad automatic translations update.

3586. By Launchpad Translations on behalf of openerp on 2012-02-18

Launchpad automatic translations update.

3587. By Launchpad Translations on behalf of openerp on 2012-02-21

Launchpad automatic translations update.

3588. By Launchpad Translations on behalf of openerp on 2012-02-22

Launchpad automatic translations update.

[Xavier ALT (dex-phx)]

Hello,

Better change default parameter for res to None not "[]", and add a check like for "context" on top of method. That way you could remove changes inside "button_install" and also gracefully handle other call to state_update() (ex: in bin/addons/base/res/res_config.py)

Cheers,
Xavier

review: Needs Fixing

[lp:~openerp-dev/openobject-server/6.0-opw-381612-ado]

3589. By Amit Dodiya (OpenERP) on 2012-02-24

[FIX] Dependancies loop executes when you install a new module

[Amit Dodiya (OpenERP) (ado-openerp)]

Hello Xavier,

Thanks for the suggestion.

The code is changed as per the comment.

Regards,
Amit Dodiya

review: Needs Resubmitting

[Olivier Dony (Odoo) (odo-openerp)]

Sorry to interrupt, but I have a few remarks. (These remarks are quite generic and could apply for most developers and most commits, so do not take them personally :-)) :

1. "res" is a wrong and obfuscated name for a parameter, please change it to e.g. processed_modules.

Why do you always call everything "res"? It is a very bad idea and forces everyone to read the code completely to know what "res" is supposed to be! Code is only written once but is read countless times, so it's worth spending 2 seconds to find a meaningful name.

This is one of our most basic guidelines (2.2) in http://doc.openerp.com/v6.0/contribute/15_guidelines/coding_guidelines.html

2. @Naresh and Xavier: Is this even a bug? I'm not sure because the comments and explanations are far from clear, but it looks like an optimization of calculation speed when pressing the module install button... something that is suited for trunk but not for a stable branch. You had to patch the server for this and you changed the API (signature of the `state_update` method changed). These are 2 things that should be considered very carefully, especially if it is not even for a real bug, and only an "improvement"!
"stable" does not mean "merge anything that customers ask for"!
If there was really a bug then I take this point back, but it's really not clear when reading the patch and comment.

3. Please try to make your commit comments useful and self-descriptive:
- do not simply say "fix OPW xxx" because it makes reviewing the commit/merge history impossible (I've seen this many times!)
- please explain what you fixed and how, e.g. "Dependancies loop executes when you install a new module" does not say what is the problem and it does not say what you fixed! A "loop" is not a bug, every "for" statement is a loop. Something like
  "[IMP] Speed up dependency calculation by only processing each dependency once"
would have described in one sentence what was the problem and how you fixed it.

The above may seem like small details but in the long run it makes the difference between a bad developer who does not care about his work and a good one who can always be proud of his work and how his code and commits will be read by others. Do not take this personally :-)

Thanks for paying attention to this!

review: Disapprove

=== modified file 'bin/addons/base/module/module.py'
--- bin/addons/base/module/module.py	2011-01-13 09:41:00 +0000
+++ bin/addons/base/module/module.py	2012-02-24 12:50:22 +0000
@@ -261,21 +261,25 @@
                 msg = _('Unable to process module "%s" because an external dependency is not met: %s')
             raise orm.except_orm(_('Error'), msg % (module_name, e.args[0]))
 
-    def state_update(self, cr, uid, ids, newstate, states_to_update, context=None, level=100):
+    def state_update(self, cr, uid, ids, newstate, states_to_update, context=None, level=100, res=None):
+        if res is None:
+            res = []
         if level<1:
             raise orm.except_orm(_('Error'), _('Recursion error in modules dependencies !'))
         demo = False
         for module in self.browse(cr, uid, ids):
             mdemo = False
             for dep in module.dependencies_id:
-                if dep.state == 'unknown':
-                    raise orm.except_orm(_('Error'), _("You try to install module '%s' that depends on module '%s'.\nBut the latter module is not available in your system.") % (module.name, dep.name,))
-                ids2 = self.search(cr, uid, [('name','=',dep.name)])
-                if dep.state != newstate:
-                    mdemo = self.state_update(cr, uid, ids2, newstate, states_to_update, context, level-1,) or mdemo
-                else:
-                    od = self.browse(cr, uid, ids2)[0]
-                    mdemo = od.demo or mdemo
+                if dep.name not in res:
+                    res.append(dep.name)
+                    if dep.state == 'unknown':
+                        raise orm.except_orm(_('Error'), _("You try to install module '%s' that depends on module '%s'.\nBut the latter module is not available in your system.") % (module.name, dep.name,))
+                    ids2 = self.search(cr, uid, [('name','=',dep.name)])
+                    if dep.state != newstate:
+                        mdemo = self.state_update(cr, uid, ids2, newstate, states_to_update, context, level-1, res=res) or mdemo
+                    else:
+                        od = self.browse(cr, uid, ids2)[0]
+                        mdemo = od.demo or mdemo
 
             self.check_external_dependencies(module.name, newstate)
             if not module.dependencies_id: