OCA / openupgradelib

A library with support functions to be called from Odoo migration scripts.
GNU Affero General Public License v3.0
85 stars 171 forks source link

[FIX] load_data: update to version 17.0 #369

Closed royle-vietnam closed 3 months ago

royle-vietnam commented 3 months ago

changed on 17.0 (https://github.com/odoo/odoo/commit/5bf1207c8cb7575fba4501aff4932c2cc952bf9f)

pedrobaeza commented 3 months ago

Thanks for this change. I'm wondering if we should instead convert the first argument to env_or_cr and delegate the responsibility of passing env or cr to the migration scripts, documenting it in the method that for 17+, the env is needed.

What do you think, @StefanRijnhart @legalsylvain @hbrunn ?

legalsylvain commented 3 months ago

I dont get it.

pedrobaeza commented 3 months ago

On v16-, you call the method load_data(cr, ...). On v17+, you call it load_data(env, ...).

legalsylvain commented 3 months ago

OK !

You propose to replace

def load_data(cr, module_name, filename, idref=None, mode="init"):

by

def load_data(env_or_cr, module_name, filename, idref=None, mode="init"):

Adding a comment to say that an cr is required until 16 and an env is required since 17, right ?

Personnaly, I prefer to keep the same signature def load_data(cr, module_name, filename, idref=None, mode="init"):

Then, add :

env_or_cr = api.Environment(cr, SUPERUSER_ID, {}) if version_info[0] >= 17 else cr

then simply replace tools.convert_xml_import(cr, module_name, fp, idref, mode=mode) by tools.convert_xml_import(env_or_cr, module_name, fp, idref, mode=mode)

But both works, so it's maybe a matter of taste. I think that it can maybe generate more errors to change the type of the argument, depending of the version of the module that call the function.

royle-vietnam commented 3 months ago

OK !

You propose to replace

def load_data(cr, module_name, filename, idref=None, mode="init"):

by

def load_data(env_or_cr, module_name, filename, idref=None, mode="init"):

Adding a comment to say that an cr is required until 16 and an env is required since 17, right ?

Personnaly, I prefer to keep the same signature def load_data(cr, module_name, filename, idref=None, mode="init"):

Then, add :

env_or_cr = api.Environment(cr, SUPERUSER_ID, {}) if version_info[0] >= 17 else cr

then simply replace tools.convert_xml_import(cr, module_name, fp, idref, mode=mode) by tools.convert_xml_import(env_or_cr, module_name, fp, idref, mode=mode)

But both works, so it's maybe a matter of taste. I think that it can maybe generate more errors to change the type of the argument, depending of the version of the module that call the function.

OK !

You propose to replace

def load_data(cr, module_name, filename, idref=None, mode="init"):

by

def load_data(env_or_cr, module_name, filename, idref=None, mode="init"):

Adding a comment to say that an cr is required until 16 and an env is required since 17, right ?

Personnaly, I prefer to keep the same signature def load_data(cr, module_name, filename, idref=None, mode="init"):

Then, add :

env_or_cr = api.Environment(cr, SUPERUSER_ID, {}) if version_info[0] >= 17 else cr

then simply replace tools.convert_xml_import(cr, module_name, fp, idref, mode=mode) by tools.convert_xml_import(env_or_cr, module_name, fp, idref, mode=mode)

But both works, so it's maybe a matter of taste. I think that it can maybe generate more errors to change the type of the argument, depending of the version of the module that call the function.

@legalsylvain Thank you for your solution. I find it very interesting and low risk. I have fixed it according to your solution.

pedrobaeza commented 3 months ago

Why I was talking to pass env is for the same reason as stated in the Odoo commit: to re-use the environment and avoid to regenerate it each time, and to share the context. Don't you think it's worth?

royle-vietnam commented 3 months ago

Why I was talking to pass env is for the same reason as stated in the Odoo commit: to re-use the environment and avoid to regenerate it each time, and to share the context. Don't you think it's worth?

@pedrobaeza I have updated it according to your suggestion, please review it