-
-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NEW] 360-degree-feedback module #19
Conversation
068f14c
to
8a2d715
Compare
8a2d715
to
f1732cb
Compare
f1732cb
to
6b46fe2
Compare
1 similar comment
6b46fe2
to
c60401c
Compare
1 similar comment
c60401c
to
363ae93
Compare
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mileo, please increase coverage, complete the README and remove empty keys in the manifest file.
'hr_evaluation' | ||
], | ||
'data': [ | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove empty keys
363ae93
to
28accd8
Compare
28accd8
to
f999589
Compare
Hi, @tarteo No tests add yet, but can be tag as need review. |
@mileo I'm not authorized to do that. |
The linter found a small issue: https://travis-ci.org/OCA/survey/jobs/179964796#L314 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution!
I made a few questions and suggestions.
|
||
To use this module, you need to: | ||
|
||
#. Go to ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go ahead and write something about the Configuration steps to perform, and guide a new user to the steps to use it.
------------ | ||
|
||
* Firstname Lastname <email.address@example.org> | ||
* Second Person <second.person@example.org> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put your name here 😄
|
||
Source: https://en.wikipedia.org/wiki/360-degree_feedback | ||
|
||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's best to move this text into the README
for phase in evaluation.plan_id.phase_ids: | ||
if phase.action != '360': | ||
return super(HrEvaluationEvaluation, | ||
self).button_plan_in_progress() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want to perform super
on evaluation
instead of self
.
And on the 360
case, do we really need to completely override/ignore the parent code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because core implementation set state as waiting withouy any validation
https://github.com/odoo/odoo/blob/8.0/addons/hr_evaluation/hr_evaluation.py#L230
[('department_id', '=', | ||
evaluation.employee_id.department_id.id)] | ||
) | ||
children |= evaluation.employee_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to isolate this children logic into it's own preparation method.
That would make it easier to override/customize this logic.
For example, it could be extended to add Customer you worked to, or to implement some randomization on the final list.
of chieldren methods
f999589
to
3be59dd
Compare
3be59dd
to
74f300f
Compare
1 similar comment
@mileo Thinking a bit longer on this, I think this module would be much better hosted in the OCA/hr project. |
1 similar comment
@dreispt We are running our first evaluation internally this week. If we do not detect any bug i will make a new PR in hr. But I'm not intending to add tests for now. |
'author': 'KMEE,Odoo Community Association (OCA)', | ||
'website': 'www.kmee.com.br', | ||
'depends': [ | ||
'hr_evaluation' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This cries out "put in the the hr project!" 😄
item.phase_id.action == '360-anonymous'): | ||
item.user_id = False | ||
item.request_id.partner_id = False | ||
item.request_id.email = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a more efficient single write
call instead of the three implicit write operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading it again: why is a constraint validation function performing writes on the data?
) | ||
|
||
@api.multi | ||
def button_plan_in_progress(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this overriding an hr_evaluation
method? We should perform a super()
call somewhere in order not to break the inheritance chain.
Also, the logic added is non-trivial enough to deserve a docstring explanation .
|
||
from openerp import api, fields, models | ||
|
||
evaluation_360_list = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use uppercase for globals?
|
||
children = self.env['hr.employee'] | ||
for item in evaluation_360_list: | ||
children |= item(self, evaluation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a 360 evaluation not every evaluator is an Employee.
AFAIK it is common to ask evaluations from other Partners, such as Customers and Suppliers.
parser.parse( | ||
datetime.now().strftime('%Y-%m-%d') | ||
) + relativedelta(months=+1)).strftime( | ||
'%Y-%m-%d'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a hardcoded deadline computation, it would be much nicer (and easier) to have this date configured in the Evaluation Plan.
Note that we are in the OCA Code Sprint and if runbot, travis are green and test coverage is above 80% you can get this merge today! Thanks! |
Hi, @rafaelbn I can´ t continue Today. But we are using in our Odoo in production. Fell free to review Daniel comments. Thanks in advance! |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
https://en.wikipedia.org/wiki/360-degree_feedback