-
-
Notifications
You must be signed in to change notification settings - Fork 664
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] [11.0] Migrate hr_employee_firstname to 11.0 #406
[MIG] [11.0] Migrate hr_employee_firstname to 11.0 #406
Conversation
…partner firstname lastname
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.
Thanks for the migration.
Tested on runbot -- Just a comment.
return ' ' if self.env.context.get('module') else False | ||
|
||
firstname = fields.Char( | ||
"Firstname", default=_firstname_default) |
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.
I would add the required=True
attribute here, without it I can create a user with firstname and then edit it to delete the first name. (while the displayed name still have that deleted first name)
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.
@lreficent required=True will brake the init_hook on already existing datas...it was required in history (see 196e0db#diff-0d3e28c86aee91593e80482604cd462d)
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.
Ok, it's not a big deal.
Also, Could you squash the transifex commits together? |
@lreficent I would like to keep all the commits as history, if ok, we can squash all when we merge... |
@feketemihai I think that the idea is to keep the previos versions history of the modules on migration. But, Transifex commits are not very important and they introduce too much noise in some cases, for instnace in this PR you can see two series of 5 and 3 transifex commits that could be harmless squashed. This way we respect the history and keep it the smallest possible. |
… value not the key of dict (OCA#215) * Update split names at module install to get the value not the key of the dict. * Add test for checking right values in firstname, lastname after install
cafd621
to
f0f142b
Compare
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.
LGTM 👍
def _firstname_default(self): | ||
return ' ' if self.env.context.get('module') else False | ||
|
||
firstname = fields.Char( |
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.
Could you change the order of fields and functions following the conventions?
I think it is weird to see the fields so below.
https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md#models
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.
done
@lreficent @etobella @OCA/human-resources-maintainers Can i have a last review... |
LGTM 👍 |
|
||
@api.model | ||
def split_name(self, name): | ||
clean_name = u" ".join(name.split(None)) if name else name |
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.
u"" is not needed for py3.5
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.
👍
…tname [MIG] [11.0] Migrate hr_employee_firstname to 11.0 Add invalidate cache to test.
[BSSFL-510] Proforma Invoice
No description provided.