Department Management

Merge lp://qastaging/~camptocamp/department-mgmt/add-account-department-fix-default-anyltic-jge into lp://qastaging/~department-core-editors/department-mgmt/7.0

add-account-department-fix-default-anyltic-jge
Merge into 7.0

Proposed by Joël Grand-Guillaume @ camptocamp on 2014-05-08

Status:	Merged
Merged at revision:	19
Proposed branch:	lp://qastaging/~camptocamp/department-mgmt/add-account-department-fix-default-anyltic-jge
Merge into:	lp://qastaging/~department-core-editors/department-mgmt/7.0
Diff against target:	118 lines (+66/-3) 2 files modified analytic_department/analytic.py (+51/-3) analytic_department/analytic_view.xml (+15/-0)
To merge this branch:	bzr merge lp://qastaging/~camptocamp/department-mgmt/add-account-department-fix-default-anyltic-jge
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Daniel Reis		2014-05-13	Approve on 2014-05-13
Guewen Baconnier @ Camptocamp	code review	2014-05-08	Approve on 2014-05-12
JB (eficent.com)		2014-05-13	Pending
Review via email: mp+218807@code.qastaging.launchpad.net

Description of the change

Hi,

Add a default value for department_uid and add a new field to manage account's related department.

As suggested in the MP related to task's department (https://code.launchpad.net/~camptocamp/department-mgmt/add-dep-on-project-task-jge/+merge/217068) I think we have 2 important informations in analytic line,.

* The first one is the user's employee related department
* The second is the analytic account's related department

Both information are valuable, depending on what you want to know. In my use case, the department_id of the line is fulfill by the employee's department through timesheet lines mostely.

I suggest here a way of dealing with this, basically:

* department_id is the user's related department, given by a default value calling _get_department
* A new field called account_department_id is a field.related on the analytic account department_id
* Adapt all search, group and tree view

This is another suggestion/proposal against this one:

https://code.launchpad.net/~jb.eficent/department-mgmt/department-mgmt-bugfixes_analytic/+merge/212299

Regards,

Joël

Revision history for this message

Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote on 2014-05-09:

l.21: context propagation is missing
l.25-25: indentation could be aligned
l.37: if the department of the account changes, the analytic lines won't be changed. That's maybe what is wanted though. I propose either to add triggers, either to add a comment why the departement on the analytic lines should not be changed

review: Needs Fixing

lp://qastaging/~camptocamp/department-mgmt/add-account-department-fix-default-anyltic-jge updated on 2014-05-12

21. By Joël Grand-Guillaume @ camptocamp on 2014-05-12: [IMP] Add triggers on needed field (as suggested by review)
[FIX] Context propagation
[IMP] Few PEP8

Revision history for this message

Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c) wrote on 2014-05-12:

Hi,

Thanks for the review I missed the trigger :( I corrected all the points.

Regards,

Joël

Revision history for this message

Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote on 2014-05-12:

Thanks!
LGTM

review: Approve (code review)

Revision history for this message

Daniel Reis (dreis-pt) wrote on 2014-05-13:

I believe this covers the distinct user cases better than Jordi's MP.
So I would prefer for this one to be merged.
Anyway, I invited him for this review.

On the syle side, it's just a personal preference, but for:

    employee_ids = employee_obj.search(cr, uid,
                                       [('user_id','=', uid)],
                                       context=context)

I usually prefer:

employee_ids = employee_obj.search(
cr, uid,[('user_id','=', uid)], context=context)

review: Approve

Revision history for this message

Pedro Manuel Baeza (pedro.baeza) wrote on 2014-05-13:

FYI, first form doesn't comply with PEP8 indentation rule that if you put arguments of first line, you have to put vertical alignment, but the second one lacks some spaces after commas ;)

That it's marked as E128 error on flake8.

Regards.