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

[MIG] Migrate hr_employee_firstname to V8 #99

Closed
wants to merge 12 commits into from

Conversation

feketemihai
Copy link
Member

No description provided.

@api.cr_context
def _auto_init(self, cr, context=None):
res = super(hr_employee, self)._auto_init(cr, context=context)
cr.execute("""
Copy link

Choose a reason for hiding this comment

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

Please explain what this query does in docstring and explain why it cannot be done in a "create" method. Also, add a couple unit tests that check exactly what it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a test...the ideea is that if you already have employee's in database, and install after the module, it will split the name_related fields to firstname and lastname fields for the ones that are recorded...i've put the calculation about "-2" is to check if by mistake the employee's name doesn't start with a " ", and if it start's to pass it.

@max3903 max3903 mentioned this pull request Apr 24, 2015
58 tasks
@max3903 max3903 added this to the 8.0 milestone Apr 24, 2015
@coveralls
Copy link

Coverage Status

Coverage increased (+5.92%) to 49.51% when pulling 82339c1 on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+5.92%) to 49.51% when pulling 82339c1 on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+13.42%) to 57.01% when pulling 2b41671 on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+13.42%) to 57.01% when pulling 69399dc on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

@feketemihai
Copy link
Member Author

@dufresnedavid Can you help me here...i modify the auto_init but on runbot with the demo data, it raises error. How can i init the class and update the fields firstname and lastname to be ok?
Thanks.

@ghost
Copy link

ghost commented Apr 29, 2015

@feketemihai let me a couple of days and i'll get back to you with a solution.

dufresnedavid and others added 2 commits May 3, 2015 07:31
@feketemihai
Copy link
Member Author

Thanks David for your work...i just merged it to see if it's ok on runbot.

@coveralls
Copy link

Coverage Status

Coverage increased (+14.17%) to 57.76% when pulling 215bb68 on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+14.17%) to 57.76% when pulling 215bb68 on feketemihai:mig_hr_employee_firstname into a96858e on OCA:8.0.

@api.multi
def write(self, vals):
if vals.get('firstname') or vals.get('lastname'):
self.ensure_one()
Copy link
Member

Choose a reason for hiding this comment

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

Write should work on multiple records, this prevents it.

Copy link

Choose a reason for hiding this comment

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

@dreispt You should never write the same firstname and lastname for multiple employees at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

@dufresnedavid I find this constraint a bit artificial

Copy link

Choose a reason for hiding this comment

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

@StefanRijnhart What would you suggest instead ?

Copy link
Member

Choose a reason for hiding this comment

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

Don't check if first or last name are written to more than one record. It's a responsibility of the user. They might even come up with a valid use case for that.

Copy link

Choose a reason for hiding this comment

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

@StefanRijnhart I agree.

Copy link
Member

Choose a reason for hiding this comment

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

and what the code actually does is to forbid to write either first or last name to multiple records. This two lines definitely should go away

_inherit = 'hr.employee'

def split_name(self, name):
new_name = [w for w in name.split(' ') if w]
Copy link
Member

Choose a reason for hiding this comment

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

you can tell python how many parts you want: https://docs.python.org/2/library/stdtypes.html#str.split
so name.split(' ', 1) gives you at most two tokens

@yvaucher
Copy link
Member

Superseeded by #127

@yvaucher yvaucher closed this Aug 13, 2015
@feketemihai feketemihai deleted the mig_hr_employee_firstname branch November 11, 2015 05:12
sambarros pushed a commit to sambarros/hr that referenced this pull request Jul 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants