[FIX][14.0] connector_importer and connector_importer_product: proposition of 'fixes' for additional flexibility

[Ricardoalso]

No description provided.

[OCA-git-bot]

Hi @mmequignon, @simahawk, @sebalix,
some modules you are maintaining are being modified, check this out!

[mmequignon]

This looks like a breaking change to me.

req = {"a": "1"}
option_req = {"b": "2"}

before, it would've returned {"a": "1", "b": "2"}
Not it returns {"b": "2"}
What's the purpose of this ?

[Ricardoalso]

The purpose is to take full control of what are the required_keys if we explicitly use the parameter.
For my use case, I want to update some measurements fields over product.products where the required_keys and odoo_unique_key are barcode

It was seen during a live training session with @simahawk, but it can indeed be a subject of debate

[simahawk]

It's not breaking because this option is kind of new. I'm favor of this change as it makes it more clear that you have full control from the conf.

Yet, we might improve this a little bit. The original issue for this fix was that when updating records you can skip some fields in the source file. When that happens you can define a specific import.type that rewrites the required keys (eg: product name is required only on create, not on write).
Today, if you don't provide all the fields that the specific mapper defines, the record will be skipped.

We have some choices:

1. remove the required key from the product mapper and rely on the fact that the system will still fail if you don't provide a name but it will happen only at creation -> issue on write solved

If we do so, then we should improve how required fields are displayed on the docs of the recordset: there we use only the mapper configuration but we could in fact retrieve required fields from the model itself by checking fields_get (we do this already in the dynamic mapper but only for technical reasons).

The drawback of this change is that for existing installations it will give a different result: the import will fail instead of report a missing key. I think we can live w/ this. Then, in the case of the product, we can document in the specific module that if you don't specify the key in the import.type it won't be skipped.
But we have opt 3, see below.

2. improve this configuration by specifying if the required keys are for create or write or both.

Eg:

# for both
required_keys:
    foo: __foo
    # for create
    for_create:
         bar: bar
    for_write:
         baz: baz

then we can propagate the current action via ctx key (note that on the base importer we already have the create= key but is not used yet).

3. by default make al required keys mandatory (as mentioned above by checking fields_get) unless you specified otherwise. We could have a specific key on the mapper, eg:

mapper:
     required_keys_ignore_model_required: true

or

mapper:
    required_keys:
        _ignore_model_required: true

And to get back to the original remark by MMQ, we can have a specific flag to decide to override or not, eg:

mapper:
    required_keys:
        _override_mapper: true

For the reason explained above IMO this key should be set to true by default to true because the record will fail anyway.

[Ricardoalso]

I modified my commit in order to add the property required_keys_ignore_model_required into "importer.base.mapper"

[mmequignon]

Can you please rename your commits ?
First line is meant to be the title of the commit's description and shouldn't describe what's done
May I suggest:

connector_importer: Avoid unnecessary code execution
connector_importer_product: Avoid dropping template during updates
<not sure about the 3rd one>

However, you can add as many lines after and describe with more technical details about what's in the commit.

[Ricardoalso]

Thanks for you review @mmequignon I added some additional infos answering your first comment and updated the commits

[simahawk]

Suggested change

	and "product_tmpl_id" in odoo_record._fields.keys()
	and "product_tmpl_id" in odoo_record._fields

and please report here as a comment the explanation of the commit.

[simahawk]

Suggested change

	"required_keys_ignore_model_required", False
	"required_keys_ignore_mapper" False