Merge lp:~jb.eficent/department-mgmt/department-mgmt-bugfix-1296055 into lp:~department-core-editors/department-mgmt/7.0

[JB (eficent.com)]

When the user_id is changed in crm.lead the application determines the section (sales team) and based on that, the department that the sales team is assigned to. If the sales team does not have an associated department the department of the employee of the salesperson is used.
See: http://www.eficent.com/openerp_en/department-management-in-openerp/

Thanks for your work!

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Jordi,

Thanks for starting to contribute to community modules! Let me make some suggestions on the proposed code. Some of them are requirements of the coding conventions of the community. Others are little improvements that you can apply to your day to day code. I know that current modules don't comply with these recommendations, but we have inherited a lot of code that we are adapting slowly, so we have to be more open-mind with it, but for new contributions, we encourage to make them good from the start.

- Remove line 17: print "on_change_user", because it only has a debug purpose.

- You don't need these lines if you are not going to use in the method context variable (you don't need them either if you pass context to another methods as argument):

if context is None:
    context = {}

- Please respect PEP8 convention of 79 cols maximum width. It makes code more readable and favor things like diff comparison. At first, it seems ugly, but when you get used to, it's better, I assure you. Here you have full PEP8 specification:

http://legacy.python.org/dev/peps/pep-0008/

- When you change only one dictionary value, it's faster and more readable to assign it directly:

res.update({'section_id': section_id})

replaced by

res['section_id'] = section_id

- From 6.1, when you need the object reference for a model, you can call self.pool['object.name'] instead of self.pool.get('object.name'). It's shorter and if you misspell one model name, you get the error traceback in the correct line, not a few lines (or a lot) after, when you use the variable.

- It's also convenient to reserve variables for object references to get shorter lines (to respect PEP8) when calling search, browse and so on, and you only make the reference getting once:

section_ids = self.pool.get('crm.case.section').search(...

become

section_obj = self.pool['crm.case.section']
section_ids = section_obj.search(...

- It's faster to include in the 'for' sentence directly the browse method, than call each time with only one ID:

employee_ids = self.pool.get('hr.employee').search(cr, uid, [('user_id','=',user_id)], context=context)
for employee_id in employee_ids:
    employee = self.pool.get('hr.employee').browse(cr, uid, employee_id, context=context)
    ...

become

employee_obj = self.pool['hr.employee']
employee_ids = employee_obj.search(cr, uid, [('user_id','=',user_id)],
                                   context=context) # PEP8
for employee in employee_obj.browse(cr, uid, employee_ids, context=context):
    ...

- I'm not sure about the flow you propose, because there is already an onchange method for section_id that assigns section department to the lead. Maybe a better flow is to only assign section_id from the user, and let onchange_section_id to do the rest, or directly get department from hr.employee only (because we are working at employee level, not section level). First approach requires to propose the patch on core, not here, so I bet for the second one.

Sorry if I am too picky, but I hope this doesn't discourage you from contributing.

Regards.

review: Needs Fixing (code review)

[lp:~jb.eficent/department-mgmt/department-mgmt-bugfix-1296055]

20. By JB (eficent.com) on 2014-03-26

Code cleanup

21. By JB (eficent.com) on 2014-03-26

More code style changes.

[JB (eficent.com) (jb.eficent)]

Now corrected as per your advice.

Thanks for your review!

Jordi.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Jordi, thanks for the changes. There are still self.pool.get() references that you can replace by self.pool[], but this is not a blocking issue. What about my last question about the conveniency of the current flow?

Regards.

[JB (eficent.com) (jb.eficent)]

Thanks Pedro,

The existing module as it is now drives the department from the section. Perhaps, as you suggest, it should derive it only from the employee, as it is done in other objects in this same project.

The bug found to the existing code refers to the fact that changing the employee in the lead will change the section, and will not refresh the department if the new section lead to a different one.

So, if you agree I will completely remove the department determination from the section, and use instead the determination from employee.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Jordi,

Yeah, I think it's the best way.

Thanks for the efforts.

Regards.

[lp:~jb.eficent/department-mgmt/department-mgmt-bugfix-1296055]

22. By JB (eficent.com) on 2014-05-03

Replace self.pool.get() references by self.pool[]
Removed department determination from sales team. Determination only occurs from employee.

[JB (eficent.com) (jb.eficent)]

Now corrected as per what we discussed. Please review & approve if you're ok.

Thanks!
Jordi.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Jordi,

Thank you very much for the changes and the patience.

Regards.

review: Approve (code review)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

LGTM, thanks !

review: Approve (code review, no tests)

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

Sorry I miss something :

Line 24 and 55, if employee has no department_id define, the code will break.

Can you please add a test on department_id.

Regards,

review: Needs Fixing (code review, no tests)

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

It should be fine. If employee has no department_id, employee. Employee.department_id will return browse_null

and browse_null.id will return False

review: Approve (code review, no test)

=== modified file 'crm_department/crm.py'
--- crm_department/crm.py	2013-05-13 08:34:13 +0000
+++ crm_department/crm.py	2014-05-03 11:07:27 +0000
@@ -3,6 +3,7 @@
 #
 #    Author: Joël Grand-guillaume (Camptocamp)
 #    Contributor: Yannick Vaucher (Camptocamp)
+#    Contributor: Eficent
 #    Copyright 2011 Camptocamp SA
 #
 #    This program is free software: you can redistribute it and/or modify
@@ -28,11 +29,13 @@
         }
 
     def _get_department(self, cr, uid, ids, context=None):
-        employee_obj = self.pool.get('hr.employee')
+        employee_obj = self.pool['hr.employee']
         department_id = False
         employee_ids = employee_obj.search(cr, uid, [('user_id','=', uid)])
         if employee_ids:
-            department_id = employee_obj.browse(cr, uid, employee_ids[0], context=context).department_id.id
+            department_id = employee_obj.browse(cr, uid, 
+                                                employee_ids[0], 
+                                                context=context).department_id.id
         return department_id
 
     _defaults = {
@@ -43,19 +46,22 @@
 class CrmLead(orm.Model):
     _inherit = "crm.lead"
 
-    def onchange_section_id(self, cr, uid, ids, section_id=False, context=None):
-        print "onchange_section_id"
-        """ Updates res dictionary with the department corresponding to the section """
-        if context is None:
-            context = {}
-        res = {}
-        if section_id:
-            section = self.pool.get('crm.case.section').browse(cr, uid, section_id, context=context)
-            if section.department_id.id:
-                res.update({'department_id': section.department_id.id})
-
+    def on_change_user(self, cr, uid, ids, user_id, context=None):
+        """ Updates res dictionary with the department 
+        corresponding to the section """
+        employee_obj = self.pool['hr.employee']
+        res = {}        
+        res['department_id'] = False
+        if user_id:
+            employee_ids = employee_obj.search(cr, uid, 
+                                               [('user_id','=',user_id)],
+                                                context=context)
+            for employee in employee_obj.browse(cr, uid, 
+                                               employee_ids, context=context):
+                if employee.department_id.id:
+                    res['department_id'] = employee.department_id.id
         return {'value': res}
-
+    
     _columns = {
         'department_id': fields.many2one('hr.department', 'Department'),
         }