Skip to content
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

[PORT] hr_employee_transfer #216

Closed
wants to merge 10 commits into from

Conversation

saltonmassally
Copy link

@andhit-r this is one of the dependencies of hr_infraction

@andhit-r
Copy link
Member

@tarzan0820 i've create a PR to your branch to fix few minor things. I've found several exception also. Will push it again after testing.

@andhit-r
Copy link
Member

@tarzan0820 i think it's better if:

  1. contract from, job from, and department form is readonly. This due transfer should be made from existing contract, job, and department. If those field is not readonly then user can choose job, contract and department different then existing data.
  2. if employee doesn't have a valid contract then job from and department from filled by job and department on employee data not from contract

What do you think?

@andhit-r
Copy link
Member

i've push another commit to your branch that fix:

  1. employee onchange exception when that employee doesn't have contract
  2. change dependency from hr into hr_contract

@saltonmassally
Copy link
Author

@andhit-r I agree with making those fields readonly can you please do in a PR?

Fix PEP8 & Pylint. Use simple header. Use OCA icon
default = {
'job_id': job_id,
'date_start': effective_date,
'name': False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tarzan0820 this code will produce error unless we depend this module to hr_contract_reference. What do you think we should do:

  1. assign default value for name (e.g. /)
  2. depend to hr_contract_reference so that module will automatically create sequence for new contract

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a tough one; but I always think the less the dependencies the better:

Also this block hr_contract_reference will eventually assign a ref to the contract if we got with option 1

if vals.get('number', '/') == '/': vals['name'] = self.env['ir.sequence'].next_by_code('contract.ref')

I say let's go with option 1

@andhit-r
Copy link
Member

Hi @tarzan0820 , i'm sorry i think i can not continue to help on this PR. This is because i believe that we are going to need a generic models to handle employee career transition (e.g: mutation, promotion, employement status change, retirement, etc). With this PR, eventually we are going to need a models for each needs. I already saw hr_employe_termination on PR list. This approach, IMHO, will give use problems when we want to make career transition history.

I will submit my PR regarding this matter in the near future.

@saltonmassally
Copy link
Author

@andhit-r understandable and thanks for the help; tag me in when you submit the PR in the new future and I will help with it

@andhit-r
Copy link
Member

@tarzan0820 thank you for your understanding. I tag you on several PR. I will be grateful if you can help review it. Thanks in advance.

@feketemihai
Copy link
Member

Please reopen the PR if it's important to you.

Mraimou pushed a commit to camptocamp/hr that referenced this pull request Nov 25, 2019
Update pending merges to add .travis.yml build except
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants