[IMP] util.ensure_xmlid_match_record: allow setting the noupdate flag

[luisg123v]

The external IDs were always created as noupdate, now the flag can be specified:

• If it doesn't exist, it's created with the given flag, True by default (as before)
• If it already exists, the flag is only updated if it's explicitly set

[robodoo]

[luisg123v]

Hi @aj-fuentes,

What do you think?

Regards,

CC @desdelinux @moylop260

[luisg123v]

FYI @isaako34

[aj-fuentes]

Hi @luisg123v,

What's the use case for this? Why is this better¹ than forcing the flag after calling this util?

util.ensure_xmlid_match_record(cr, xmlid, values):
util.force_noudate(cr, xmlid, noupdate=no_update_value)

We prefer to have a clear use case before altering the API of the utility functions.

Footnotes

1. Not just to save one line of code. ↩

[luisg123v]

@aj-fuentes, If the external ID already exists, your way would be clearer and preferred. But if a new one is being created, It would be created as noupdate, to then being immediately updated. In other words, it would need to be fixed just after creation.

But that's the only advantage, I agree nothing is broken, it's just an improvement.

If you prefer, I could just add a note in the docstring mentioning the external ID is created as noupdate.

BTW, I didn't fully get the [1] reference.

Regards,

[aj-fuentes]

Hi @luisg123v

But if a new one is being created, It would be created as noupdate, to then being immediately updated.

Why are they necessarily immediately udpated? In most of our internal usage of this utility we never force the flag after. The point is that this tool ensures an xmlid points to a "valid" record. But if the xmlid didn't exist it is risky, without a case by case analysis, to let it be updated by the ORM. If you anyway had to do a case by case analysis, adding an extra line after for the force_create is not a big deal. OTOH if you are 100% positive that you can always set it noupdate=False, an extra line won't be a big deal either.

Here is the main use case of this utility: in many parts in Odoo certain record is expected to always exists and without a duplicate, otherwise something completely fails. Thus, by ensuring the xmlid exists and points to a "valid enough"¹ record we can avoid that failure. Updating the record via its xmlid later is out of scope here.

BTW, I didn't fully get the [1] reference.

I meant that if the purpose is only to avoid the extra call to force_create after the ensure_xmlid_match_record line, then this doesn't look like a strong enough reason for a patch that alters the API of the function. Note that I'm not saying this patch is totally useless or wrong. My point is that we prefer to understand the reasons for changes that alter the parameters of the functions ;)

If you prefer, I could just add a note in the docstring mentioning the external ID is created as noupdate.

Indeed, that's missing. Thanks!

Regards,

Footnotes

1. We don't use this utility lightly, in many cases we do let the error block the upgrade and do a case by case analysis. Only when the record is deemed "safe" to match we proceed. An example that comes to mind: utm sources ;) ↩