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

[17.0][MIG] hr_employee_lastnames: Migration to 17.0 #1315

Merged
merged 75 commits into from
Dec 26, 2023

Conversation

andreagidaltig
Copy link
Contributor

@andreagidaltig andreagidaltig commented Nov 16, 2023

  • Rename module from hr_employee_lastnames → hr_employee_second_lastname
    in order to be consistent with partner_second_lastname module.
  • Fix some new lints.
  • Update incoming parameters of post_init_hook method since cr, registry
    were replaced by env in [1].
  • Introduce the use of new onchange method [2] (renamed in [3]) to
    return the fields specification from a view description.

[1] odoo/odoo@b4a7996
[2] odoo/odoo@f5e6494

@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch 2 times, most recently from 4b4c2d0 to 29a2ce1 Compare November 16, 2023 22:23
@andreagidaltig
Copy link
Contributor Author

Hi @luisg123v could you review this please?

cc. @luistorresm

@FernandoRomera
Copy link
Contributor

@andreagidaltig
Could we change the name of the module to synchronize it with partner_second_lastname?.

Then "hr_employee_lastnames" -> "hr_employee_second_lastname"

else:
lastname2 = self.lastname2
vals["name"] = self._get_name_lastnames(lastname, firstname, lastname2)
elif vals.get("name"):
Copy link
Member

Choose a reason for hiding this comment

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

Hi @andreagidaltig, i created a PR fixing some issues in the v14 module ( #1283) I could reproduce the bug on the runboat v17.
To reproduce you can create an employee with a first name equals to " Name 1 Name 2 name 3" and a lastname equals to "name4" if you export the data (including the external_id) and importing again the data, the name will be changed.

Could you fix it or cherry pick my commit? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

@FrancoMaxime since your PR is not merged yet, it shouldn't be included in the migration (because it's still subject to changes).

Copy link
Member

Choose a reason for hiding this comment

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

@luisg123v Thanks, I hadn’t thought of that!

@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch from 29a2ce1 to 63591ac Compare November 27, 2023 18:08
@luisg123v
Copy link
Contributor

@FernandoRomera

Could we change the name of the module to synchronize it with partner_second_lastname?.

Not sure if that's a good idea. That will "break" systems being migrated from previous versions, because the old module would result uninstallable and the new one not installed.

@@ -0,0 +1,2 @@
from . import models
from .hooks import post_init_hook
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename hooks file in the previous version, please?

@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch 2 times, most recently from 3f311c4 to 2cf094e Compare November 29, 2023 21:19
@FernandoRomera
Copy link
Contributor

FernandoRomera commented Dec 1, 2023

@FernandoRomera

Could we change the name of the module to synchronize it with partner_second_lastname?.

Not sure if that's a good idea. That will "break" systems being migrated from previous versions, because the old module would result uninstallable and the new one not installed.

@luisg123v
Don't worry, there is already a procedure for this in OpenUpgrade.

If you want we can propose the PR in OpenUpgrade.

apriory.py in OpenUpgrade

Renamed modules is a mapping from old module name to new module name
renamed_modules = {
...
# OCA/hr
"hr_employee_lastnames": "hr_employee_second_lastname",
...

This PR is a example: https://github.com/OCA/OpenUpgrade/pull/4246/files

@luisg123v
Copy link
Contributor

Got it. I think module renaming should be on a separate commit, though, before running pre-commit

@moylop260 what do you think about renaming the module?

@moylop260
Copy link

I'm not a fan to rename modules

But I don't have the enough context to do it

Consider that you will need to change all the xmlids and you will need to change all the depends from the manifest of modules using this module

@FernandoRomera
Copy link
Contributor

Don't worry, there is already a procedure for this in OpenUpgrade.

@moylop260
Don't worry, there is already a procedure for all changes in OpenUpgrade.

I'm not a fan to rename modules

But I don't have the enough context to do it

Consider that you will need to change all the xmlids and you will need to change all the depends from the manifest of modules using this module

@moylop260
Don't worry, there is already a procedure for all changes in OpenUpgrade.

@luisg123v
Copy link
Contributor

Don't worry, there is already a procedure for all changes in OpenUpgrade.

Not for this:

and you will need to change all the depends from the manifest of modules using this module

😄

@FernandoRomera
Copy link
Contributor

Don't worry, there is already a procedure for all changes in OpenUpgrade.

Not for this:

and you will need to change all the depends from the manifest of modules using this module

😄

You are right, but now is the time to do it and make the nomenclature of the modules consistent.

in the first line of the commit you indicate that you synchronize with the module partner_second_lastname

Therefore partner_contact is like this:
partner_firstname
partner_second_lastname

and in hr
hr_employee_firstname
hr_employee_second_lastname

This is my last comment for it, thanks for all the work done.

@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch 2 times, most recently from 917966f to 2e1db70 Compare December 4, 2023 02:34
@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch from 2e1db70 to 3c58fef Compare December 21, 2023 17:52
mymage and others added 6 commits December 21, 2023 18:03
Currently translated at 100.0% (8 of 8 strings)

Translation: hr-16.0/hr-16.0-hr_employee_lastnames
Translate-URL: https://translation.odoo-community.org/projects/hr-16-0/hr-16-0-hr_employee_lastnames/es/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: hr-16.0/hr-16.0-hr_employee_lastnames
Translate-URL: https://translation.odoo-community.org/projects/hr-16-0/hr-16-0-hr_employee_lastnames/
Rename file of hooks to follow guidelines.
@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch from 3c58fef to f466431 Compare December 21, 2023 18:03
@andreagidaltig
Copy link
Contributor Author

Hi @luisg123v could you review this, please? I just added the commit to rename the module.

@luisg123v
Copy link
Contributor

/ocabot migration hr_employee_lastnames

@OCA-git-bot
Copy link
Contributor

Sorry @luisg123v you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit message:

  • AFAIK tag for module renaming is [MOV]
  • Commit message doesn't mention where the module is being renamed from. It should include both from and to, e.g.:
    Module is renamed from hr_employee_lastnames → hr_employee_second_lastname inorder to be consistent with...

@@ -1,7 +1,7 @@
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
Copy link
Contributor

Choose a reason for hiding this comment

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

Commit message:

  • sicronize -> synchronize. Besides, this would only go in PR description because it's not done on this commit anymore
  • "Rename hooks file to follow guidelines.": not done on this commit anymore. This was done on v16, so not part of the migration anymore (hence should be removed also from PR's description).



class TestEmployeeLastnames(TransactionCase):
def setUp(self):
super(TestEmployeeLastnames, self).setUp()
super().setUp()
Copy link
Contributor

Choose a reason for hiding this comment

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

Document in commit description

# (firstname, lastname and lastname2).
#
# For example:
# After install hr_employee_fisrtname and before install hr_employee_lastnames:
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing renaming module here

# firstname = 'John'
# lastname = 'Peterson'
# lastname2 = 'Clinton'
env.cr.execute("UPDATE hr_employee SET firstname = NULL, lastname = NULL")
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing renaming module here

@@ -0,0 +1,3 @@
# Copyright (C) 2014 Savoir-faire Linux. All Rights Reserved.
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl).
from . import test_hr_employee_lastnames
Copy link
Contributor

Choose a reason for hiding this comment

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

And here.

Please fix all similar cases.

@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch 3 times, most recently from 2c79f9d to aacd8a6 Compare December 22, 2023 21:36
@andreagidaltig
Copy link
Contributor Author

Hi @luisg123v changes applied, could you review again, please?

from odoo.tools import submap


class TestEmployeeLastnames(TransactionCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to update lastnames -> second lastname here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just updated this, could you review again, please?

Module is renamed from hr_employee_lastnames → hr_employee_second_lastname
in order to be consistent with partner_second_lastname module.
- Fix some new lints.
- Update incoming parameters of post_init_hook method since `cr, registry`
  were replaced by `env` in [1].
- Deprecate the use of _onchange_spec method and use the method new method
  _get_fields_spec to return the fields specification from a view
  description since it was introduced in [2].

[1] odoo/odoo@b4a7996
[2] odoo/odoo@f5e6494
@andreagidaltig andreagidaltig force-pushed the 17.0-mig-hr-employee-lastnames branch from aacd8a6 to 4fb14e7 Compare December 26, 2023 16:07
Copy link
Contributor

@luisg123v luisg123v left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@moylop260 could you review/merge, please?

@moylop260
Copy link

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-1315-by-moylop260-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e1415b8 into OCA:17.0 Dec 26, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 7bfa760. Thanks a lot for contributing to OCA. ❤️

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.