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

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

Open
wants to merge 3 commits into
base: 14.0
Choose a base branch
from

Conversation

Ricardoalso
Copy link
Contributor

No description provided.

@Ricardoalso Ricardoalso marked this pull request as draft September 6, 2024 12:12
@OCA-git-bot
Copy link
Contributor

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

@Ricardoalso Ricardoalso changed the title [FIX][14.0] connector_importer: proposition of 'fixes' for additional flexibility [FIX][14.0] connector_importer and connector_importer_product: proposition of 'fixes' for additional flexibility Sep 6, 2024
req.update(self.work.options.mapper.get("required_keys", {}))
return req

return self.work.options.mapper.get("required_keys", {}) or req
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@simahawk simahawk Sep 11, 2024

Choose a reason for hiding this comment

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

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.

  1. 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).

  1. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@mmequignon
Copy link
Member

mmequignon commented Sep 6, 2024

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 Ricardoalso force-pushed the fixes_connector_importer_product branch from 66eeb6c to 6ed2b3b Compare September 6, 2024 15:46
@Ricardoalso
Copy link
Contributor Author

Ricardoalso commented Sep 6, 2024

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

@Ricardoalso Ricardoalso force-pushed the fixes_connector_importer_product branch 2 times, most recently from 0e5d562 to 0962ab2 Compare September 10, 2024 16:53
@Ricardoalso Ricardoalso marked this pull request as ready for review September 10, 2024 16:55
@Ricardoalso Ricardoalso force-pushed the fixes_connector_importer_product branch from 0962ab2 to 5409d70 Compare September 11, 2024 11:38
Early return if no product.attribute.value is to be imported
@Ricardoalso Ricardoalso marked this pull request as draft September 11, 2024 11:39
@Ricardoalso Ricardoalso force-pushed the fixes_connector_importer_product branch 4 times, most recently from 9811540 to d4c0377 Compare September 11, 2024 12:20
def odoo_pre_write(self, odoo_record, values, orig_values):
if (
"product_tmpl_id" not in orig_values
and "product_tmpl_id" in odoo_record._fields.keys()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@property
def required_keys_ignore_model_required(self):
return self.work.options.mapper.get(
"required_keys_ignore_model_required", False
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"required_keys_ignore_model_required", False
"required_keys_ignore_mapper" False

When updating product.product records, product_tmpl_id is set to False in values if not specified in the import type.
But we don't want to set it to False as the value already exists in the odoo_record
@Ricardoalso Ricardoalso force-pushed the fixes_connector_importer_product branch from d4c0377 to fcd2242 Compare September 12, 2024 11:38
@Ricardoalso Ricardoalso marked this pull request as ready for review September 12, 2024 11:39
Not forcing required default value if required_keys_ignore_mapper and required_keys are defined in options.mapper.
Adding the new property required_keys_ignore_mapper in order to override the result of required_keys function
@Ricardoalso Ricardoalso force-pushed the fixes_connector_importer_product branch from fcd2242 to b40334a Compare September 12, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants