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

[16.0][OU-ADD] hr #3911

Merged
merged 1 commit into from
Feb 14, 2024
Merged

[16.0][OU-ADD] hr #3911

merged 1 commit into from
Feb 14, 2024

Conversation

remytms
Copy link
Contributor

@remytms remytms commented Jun 7, 2023

Move elements from hr_contract to hr.
Replace M2M relation between hr.plan and hr.plan.activity.type to O2M and warn about data loss.
Import noupdate_changes.

Tested with some data in hr.plan and hr.plan.activity.type.

@remytms remytms force-pushed the 16-add-hr branch 3 times, most recently from 8220148 to 3134044 Compare June 7, 2023 15:34
@pedrobaeza
Copy link
Member

/ocabot migration hr

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Jun 7, 2023
@remytms remytms force-pushed the 16-add-hr branch 5 times, most recently from 17ae30a to aa4527c Compare June 12, 2023 18:56
@remytms remytms marked this pull request as ready for review June 13, 2023 09:58
openupgrade.rename_xmlids(env.cr, _xmlid_renames)
# Backup Many2many relation between hr.plan and hr.plan.activity.type
openupgrade.remove_tables_fks(env.cr, ["hr_plan_hr_plan_activity_type_rel"])
openupgrade.rename_tables(env.cr, [("hr_plan_hr_plan_activity_type_rel", None)])

Choose a reason for hiding this comment

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

I have tested this using a git-aggregator file and it fails here.

2023-08-31 10:43:42,391 1 DEBUG mig OpenUpgrade: -1 rows affected after 0:00:00.000516 running ALTER TABLE "hr_plan_hr_plan_activity_type_rel" DROP CONSTRAINT "hr_plan_hr_plan_activity_type_rel_hr_plan_activity_type_id_fkey" 
2023-08-31 10:43:42,391 1 DEBUG mig OpenUpgrade: -1 rows affected after 0:00:00.000317 running ALTER TABLE "hr_plan_hr_plan_activity_type_rel" DROP CONSTRAINT "hr_plan_hr_plan_activity_type_rel_hr_plan_id_fkey" 
2023-08-31 10:43:42,391 1 INFO mig OpenUpgrade: table hr_plan_hr_plan_activity_type_rel: renaming to openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel 
2023-08-31 10:43:42,392 1 ERROR mig odoo.sql_db: bad query: ALTER INDEX "hr_plan_hr_plan_activity_type_rel_hr_plan_id_idx" RENAME TO "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_plan_id_idx"
ERROR: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists
 
2023-08-31 10:43:42,393 1 ERROR mig OpenUpgrade: hr: error in migration script /odoo/src/openupgrade/openupgrade_scripts/scripts/hr/16.0.1.1/pre-migration.py: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists
 
2023-08-31 10:43:42,393 1 ERROR mig OpenUpgrade: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists
 
Traceback (most recent call last):
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 2271, in wrapped_function
    func(
  File "/odoo/src/openupgrade/openupgrade_scripts/scripts/hr/16.0.1.1/pre-migration.py", line 32, in migrate
    openupgrade.rename_tables(env.cr, [("hr_plan_hr_plan_activity_type_rel", None)])
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 838, in rename_tables
    cr.execute(
  File "/odoo/src/odoo/odoo/sql_db.py", line 321, in execute
    res = self._obj.execute(query, params)
psycopg2.errors.DuplicateTable: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists

2023-08-31 10:43:42,419 1 WARNING mig odoo.modules.loading: Transient module states were reset 
2023-08-31 10:43:42,440 1 ERROR mig odoo.modules.registry: Failed to load registry 
Traceback (most recent call last):
  File "/odoo/src/odoo/odoo/modules/registry.py", line 90, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/odoo/src/odoo/odoo/modules/loading.py", line 484, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/odoo/src/odoo/odoo/modules/loading.py", line 372, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/odoo/src/odoo/odoo/modules/loading.py", line 183, in load_module_graph
    migrations.migrate_module(package, 'pre')
  File "/odoo/src/openupgrade/openupgrade_framework/odoo_patch/odoo/modules/migration.py", line 18, in migrate_module
    MigrationManager.migrate_module._original_method(self, pkg, stage)
  File "/odoo/src/odoo/odoo/modules/migration.py", line 189, in migrate_module
    migrate(self.cr, installed_version)
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 2271, in wrapped_function
    func(
  File "/odoo/src/openupgrade/openupgrade_scripts/scripts/hr/16.0.1.1/pre-migration.py", line 32, in migrate
    openupgrade.rename_tables(env.cr, [("hr_plan_hr_plan_activity_type_rel", None)])
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 838, in rename_tables
    cr.execute(
  File "/odoo/src/odoo/odoo/sql_db.py", line 321, in execute
    res = self._obj.execute(query, params)
psycopg2.errors.DuplicateTable: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists

2023-08-31 10:43:42,440 1 CRITICAL mig odoo.service.server: Failed to initialize database `mig`. 
Traceback (most recent call last):
  File "/odoo/src/odoo/odoo/service/server.py", line 1299, in preload_registries
    registry = Registry.new(dbname, update_module=update_module)
  File "<decorator-gen-14>", line 2, in new
  File "/odoo/src/odoo/odoo/tools/func.py", line 87, in locked
    return func(inst, *args, **kwargs)
  File "/odoo/src/odoo/odoo/modules/registry.py", line 90, in new
    odoo.modules.load_modules(registry, force_demo, status, update_module)
  File "/odoo/src/odoo/odoo/modules/loading.py", line 484, in load_modules
    processed_modules += load_marked_modules(cr, graph,
  File "/odoo/src/odoo/odoo/modules/loading.py", line 372, in load_marked_modules
    loaded, processed = load_module_graph(
  File "/odoo/src/odoo/odoo/modules/loading.py", line 183, in load_module_graph
    migrations.migrate_module(package, 'pre')
  File "/odoo/src/openupgrade/openupgrade_framework/odoo_patch/odoo/modules/migration.py", line 18, in migrate_module
    MigrationManager.migrate_module._original_method(self, pkg, stage)
  File "/odoo/src/odoo/odoo/modules/migration.py", line 189, in migrate_module
    migrate(self.cr, installed_version)
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 2271, in wrapped_function
    func(
  File "/odoo/src/openupgrade/openupgrade_scripts/scripts/hr/16.0.1.1/pre-migration.py", line 32, in migrate
    openupgrade.rename_tables(env.cr, [("hr_plan_hr_plan_activity_type_rel", None)])
  File "/odoo/.local/lib/python3.10/site-packages/openupgradelib/openupgrade.py", line 838, in rename_tables
    cr.execute(
  File "/odoo/src/odoo/odoo/sql_db.py", line 321, in execute
    res = self._obj.execute(query, params)
psycopg2.errors.DuplicateTable: relation "openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl" already exists

Choose a reason for hiding this comment

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

Hmmm, if I comment the line it says the table doesn't exist, investigating

Copy link
Member

Choose a reason for hiding this comment

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

I suspect this is an issue local to your computer.

Heads-up that the referenced code is being moved to OCA/openupgradelib#343

Copy link
Contributor

@remi-filament remi-filament Oct 12, 2023

Choose a reason for hiding this comment

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

Hi @remytms @carmenbianca thanks for your work !
I face the same issue here...
Found out that replacing the None by a dummy name on line 32 :
openupgrade.rename_tables(env.cr, [("hr_plan_hr_plan_activity_type_rel", 'test_hr_plan_rel')])

(and then replacing the openupgrade.get_legacy_name occurences in post_migration.py with my dummy name)

It works properly.
I am wondering if there is not a limit with the number of characters a database table name can support ? (I am using Postgres 13 here)

Copy link
Contributor Author

@remytms remytms Nov 10, 2023

Choose a reason for hiding this comment

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

@remi-filament is right. It has to be with the maximum number of character for the name of a field in PostgreSQL.

I had to search the PostgreSQL manuel, but it's here. 63 ASCII character long for all the names of the table, fields, etc.

If name specified is to long, PostgreSQL cut it at 63 characters. When creating default name for indices, constrains, etc it use a dedicated algorithm, which consist of cropping the longest element of the name until the whole name fits into 63 characters.

In this case, we use get_legacy_name() to retrieve a new name for the table. get_legacy_name() add something like openupgrade_legacy_16_0_.... Also the rename_table() function also tries to rename the indices, constrains, etc.

The original indices name for this table are:

Indexes:
    "hr_plan_hr_plan_activity_type_rel_pkey" PRIMARY KEY, btree (hr_plan_id, hr_plan_activity_type_id)
    "hr_plan_hr_plan_activity_type_hr_plan_activity_type_id_hr_p_idx" btree (hr_plan_activity_type_id, hr_plan_id)
Foreign-key constraints:
    "hr_plan_hr_plan_activity_type_rel_hr_plan_activity_type_id_fkey" FOREIGN KEY (hr_plan_activity_type_id) REFERENCES hr_plan_activity_type(id) ON DELETE CASCADE
    "hr_plan_hr_plan_activity_type_rel_hr_plan_id_fkey" FOREIGN KEY (hr_plan_id) REFERENCES hr_plan(id) ON DELETE CASCADE

Which gives after the rename with get_legacy_name():

  • openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel (57 char)
  • openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_pkey (62 char)
  • openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_hr_plan_activity_type_id_hr_p_idx (87 char) -> openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_hr_plan_a (63 char).
  • openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_plan_activity_type_id_fkey (87 char) -> openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl (63 char)
  • openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_plan_id_fkey (73 char) -> openupgrade_legacy_16_0_hr_plan_hr_plan_activity_type_rel_hr_pl (63 char)

We see that two constrains are renamed the same way, which is a problem.

I think that this case should be fixed directly on openupgradelib. I will suggest something.

In this script I will set the new name of the table instead of using get_legacy_name().

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @remytms did you progressed on this fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rémy is out of office for 2 weeks :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marielejeune I've not proposed fixes on openupgradelib yet, but in this migration script I use a custom name for the table. So that it should not be an issue anymore.

@mostafabarmshory
Copy link

mostafabarmshory commented Dec 29, 2023

any update?

@remytms
Copy link
Contributor Author

remytms commented Jan 10, 2024

I'm working on it this week.

@MiquelRForgeFlow
Copy link
Contributor

this is wrong rebased...

Comment on lines 105 to 116
if employee.user_id and employee.user_id.partner_id:
# FIXME: not checking that the partner has the same email
# and mobile than the employee may lead to a loss of data.
# work_email and mobile_phone will be replaced by the one
# from the partner.
employee.work_contact_id = employee.user_id.partner_id
_logger.info(
"Set work_contact_id of hr.employee(%s) to "
"res.partner(%s) (the res.user partner).",
employee.id,
employee.user_id.partner_id.id,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@remi-filament I implemented your idea to link the partner linked to the user_id if there is one.

But I'm not sure if such a link with work_contact_id should be done if the mobile and email of the employee and the partner are not the same. Because this may lead to a loss of data.

And I think that the goal of an openupgrade script is to avoid losing data.

Should I check that the partner found on the user_id has the same mobile and email as the employee before assigning it to work_contact_id ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@remytms thanks for that !
Yes it would make sense to check this out (and maybe have some specific action in case fields are empty on partner or employee side)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I will address this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@remytms remytms force-pushed the 16-add-hr branch 2 times, most recently from 1e910f9 to 7b0d480 Compare January 12, 2024 12:52
@remytms
Copy link
Contributor Author

remytms commented Jan 12, 2024

@mostafabarmshory Your review is welcome, on this new proposal for the migration script of hr module. :)

@mostafabarmshory
Copy link

@mostafabarmshory Your review is welcome, on this new proposal for the migration script of hr module. :)

Dear @remytms
Tx for your mentions. That's your kindness.

I followed your changes and compared them with the head revision. I do not know how to write a review, honestly. Could you show me how to do that, please?

Finally tx to your work.

@remytms
Copy link
Contributor Author

remytms commented Jan 15, 2024

@mostafabarmshory If it's your first time doing a pull request review, I invite you to read this documentation on how to make comments and approve a pull request

If you are used to making pull request review but don't see the button, then you may need to be part of OCA organisation on github I think. Here is documentation.

Copy link

@mostafabarmshory mostafabarmshory left a comment

Choose a reason for hiding this comment

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

tx to your work

Copy link

@mostafabarmshory mostafabarmshory left a comment

Choose a reason for hiding this comment

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

tx to your work. Sorry, My last review is marked as a comment. It is my fault. I add a new review.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Wow, hard work with some of the changes here...

Please also comment in the noupdate_changes files the records with empty plan_activity_type_ids, as they are not adding value, and maybe they will empty their list.

openupgrade.update_module_moved_models(
env.cr, "hr.contract.type", "hr_contract", "hr"
)
openupgrade.update_module_moved_fields(
Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

@pedrobaeza

Would you explain why? The model is moved and the code is looking good.

Copy link
Member

Choose a reason for hiding this comment

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

This is already done by the orm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

# NOTHING TO DO

hr / hr.department / master_department_id (many2one): NEW relation: hr.department, isfunction: function, stored
hr / hr.department / parent_path (char) : NEW
Copy link
Member

Choose a reason for hiding this comment

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

It's always good to say why this is nothing to do:

Suggested change
hr / hr.department / parent_path (char) : NEW
hr / hr.department / parent_path (char) : NEW
# NOTHING TO DO: Computed by ORM in the module update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, changed.

hr / hr.contract.type / sequence (integer) : NEW
# NOTHING TO DO

hr / hr.department / master_department_id (many2one): NEW relation: hr.department, isfunction: function, stored
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the logs, it seems this one is computed before computing the parent_path:

2024-01-12 15:59:27,310 22110 INFO openupgrade odoo.modules.registry: module hr: creating or updating database tables 
2024-01-12 15:59:27,336 22110 INFO openupgrade odoo.models: Prepare computation of hr.department.master_department_id 
2024-01-12 15:59:27,336 22110 INFO openupgrade odoo.models: Computing parent_path for table hr_department... 

so the values won't be correct. Have you checked them in a test DB?

If not correct, you can:

  • Pre-create the column for avoiding the normal computation.
  • Call manually the compute on post-migration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. It works with demo data because there is no nested department in demo data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

("mobile", "=", employee.mobile_phone),
]
)
if len(matching_partner) == 1:
Copy link
Member

Choose a reason for hiding this comment

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

I would link to the first one if len > 1, as at the end, it's the same result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not exactly the same result. If there is only one match, we can be pretty sure that the partner found is related to the employee.

But if there is several matches, then we cannot be sure which one to choose. So in such a case, I chose to create a new partner to link to the employee and show a warning in the logs (I will improve the log to differentiate the case where there is no matches and the case where there is several matches).

At the end, for a case with several matches, there will be one matches more. I think that a merge between all the contact should be done by a user after checking the reason why there is so many matches.

Also cleaning the database by removing the duplicate can be done before migrating. And with the warning, the duplicate partner can be found easily.

I will ping you when my new log warning proposal will be ready. :)

Copy link
Member

Choose a reason for hiding this comment

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

I still consider that linking to the first one found is a reasonable trade-off, better than finishing with still one more contact created. Any needed merge can be done later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will go it your way. If there is a warning then information can be found anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

hr / hr.job / state (selection) : DEL required, selection_keys: ['open', 'recruit']
# NOTHING TO DO: hr.job does not work with state anymore.

hr / hr.plan / company_id (many2one) : NEW relation: res.company, hasdefault: default
Copy link
Member

Choose a reason for hiding this comment

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

For preserving the previous behavior, pre-create the column for leaving it blank and don't let the default to act.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so that, with a blank field for company_id, the hr.plan will not belong to a specific company. If we do nothing it will be linked to the default company set in env which is not the same. Do I understand correctly ?

Copy link
Member

Choose a reason for hiding this comment

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

Correct, changing the behavior from previous version, where all the plans were available for all the companies. Users now will have the option to restrict them in this version, but the provided default should be backwards compatible.

Copy link
Contributor Author

@remytms remytms Feb 14, 2024

Choose a reason for hiding this comment

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

Fixed with tests.

# DONE: pre-migration and post-migration: move data from many2many table to plan_id colomn in hr.plan.activity.type

hr / hr.plan.activity.type / company_id (many2one) : NEW relation: res.company, hasdefault: default
# NOTHING TO DO: not a required field so no default to set
Copy link
Member

Choose a reason for hiding this comment

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

The same comment as in hr.plan.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

# NOTHING TO DO: not a required field so no default to set

hr / hr.plan.activity.type / plan_id (many2one) : NEW relation: hr.plan
# DONE: see plan_activity_type_ids from hr.plan
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 reorder this line to put it joined with the one2many field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


hr / res.users / create_employee (boolean) : NEW hasdefault: default
hr / res.users / create_employee_id (many2one) : NEW relation: hr.employee
# NOTHING TO DO
Copy link
Member

Choose a reason for hiding this comment

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

Comment why they are nothing to do for easing later reviews.

Suggested change
# NOTHING TO DO
# NOTHING TO DO: store=False fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, fixed.

# NOTHING TO DO

hr / resource.resource / employee_id (one2many) : NEW relation: hr.employee
# NOTHING TO DO
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# NOTHING TO DO
# NOTHING TO DO: inverse field already existing in previous version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@mostafabarmshory
Copy link

Dear @remytms,
@pedrobaeza has a request to change the migration code. what next?

@remytms
Copy link
Contributor Author

remytms commented Feb 13, 2024

@mostafabarmshory I will do the required changes. Normally this week.

@pedrobaeza
Copy link
Member

@remytms do you want me to do them in a superseded PR (respecting your attribution)?

@pedrobaeza
Copy link
Member

Please squash all commits into one. No need to split them, as GitHub already shows the differences between each push.

@remytms remytms force-pushed the 16-add-hr branch 2 times, most recently from c7d64c6 to 902246c Compare February 14, 2024 15:51
Move elements from `hr_contract` to `hr`.
Replace M2M relation between hr.plan and hr.plan.activity.type to O2M
and warn about data loss.
Import noupdate_changes
@remytms
Copy link
Contributor Author

remytms commented Feb 14, 2024

@pedrobaeza If I have not forget something, all comments were fixed. And commits were squashed.

@pedrobaeza pedrobaeza merged commit 4e5ded9 into OCA:16.0 Feb 14, 2024
2 checks passed
@robinkeunen
Copy link
Contributor

🎉

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.