-
-
Notifications
You must be signed in to change notification settings - Fork 699
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
[16.0][OU-ADD] hr #3911
Conversation
8220148
to
3134044
Compare
/ocabot migration hr |
17ae30a
to
aa4527c
Compare
openupgrade_scripts/scripts/hr/16.0.1.1/upgrade_analysis_work.txt
Outdated
Show resolved
Hide resolved
openupgrade_scripts/scripts/hr/16.0.1.1/upgrade_analysis_work.txt
Outdated
Show resolved
Hide resolved
ec7cbef
to
8544fd1
Compare
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)]) |
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 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
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.
Hmmm, if I comment the line it says the table doesn't exist, investigating
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 suspect this is an issue local to your computer.
Heads-up that the referenced code is being moved to OCA/openupgradelib#343
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.
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)
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.
@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()
.
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.
Hi @remytms did you progressed on this fix?
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.
Rémy is out of office for 2 weeks :-)
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.
@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.
any update? |
I'm working on it this week. |
this is wrong rebased... |
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, | ||
) |
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.
@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
?
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.
@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)
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.
Indeed, I will address this.
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.
@remi-filament Done.
1e910f9
to
7b0d480
Compare
@mostafabarmshory Your review is welcome, on this new proposal for the migration script of |
Dear @remytms 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. |
@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. |
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.
tx to your work
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.
tx to your work. Sorry, My last review is marked as a comment. It is my fault. I add a new review.
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.
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( |
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.
This one is not needed anymore: check https://github.com/OCA/openupgradelib/blob/29113956ab16eac1af01b4a4fd4db74257a45763/openupgradelib/openupgrade.py#L3042
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.
Would you explain why? The model is moved and the code is looking good.
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.
This is already done by the orm
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.
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 |
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.
It's always good to say why this is nothing to do:
hr / hr.department / parent_path (char) : NEW | |
hr / hr.department / parent_path (char) : NEW | |
# NOTHING TO DO: Computed by ORM in the module update. | |
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.
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 |
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.
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.
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 catching this. It works with demo data because there is no nested department in demo data.
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.
("mobile", "=", employee.mobile_phone), | ||
] | ||
) | ||
if len(matching_partner) == 1: |
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 link to the first one if len > 1, as at the end, it's the same result.
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.
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. :)
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 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.
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, I will go it your way. If there is a warning then information can be found anyway.
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.
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 |
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.
For preserving the previous behavior, pre-create the column for leaving it blank and don't let the default to act.
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, 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 ?
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.
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.
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.
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 |
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.
The same comment as in hr.plan
.
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.
# 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 |
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.
You can reorder this line to put it joined with the one2many field.
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.
|
||
hr / res.users / create_employee (boolean) : NEW hasdefault: default | ||
hr / res.users / create_employee_id (many2one) : NEW relation: hr.employee | ||
# NOTHING TO DO |
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.
Comment why they are nothing to do for easing later reviews.
# NOTHING TO DO | |
# NOTHING TO DO: store=False fields |
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.
Indeed, fixed.
# NOTHING TO DO | ||
|
||
hr / resource.resource / employee_id (one2many) : NEW relation: hr.employee | ||
# NOTHING TO DO |
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.
# NOTHING TO DO | |
# NOTHING TO DO: inverse field already existing in previous version. |
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.
Dear @remytms, |
@mostafabarmshory I will do the required changes. Normally this week. |
@remytms do you want me to do them in a superseded PR (respecting your attribution)? |
Please squash all commits into one. No need to split them, as GitHub already shows the differences between each push. |
c7d64c6
to
902246c
Compare
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
@pedrobaeza If I have not forget something, all comments were fixed. And commits were squashed. |
🎉 |
Move elements from
hr_contract
tohr
.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.