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

[16.0][IMP] product_supplierinfo_import_by_barcode: To product_supplierinfo_import #1905

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

juancarlosonate-tecnativa
Copy link

@juancarlosonate-tecnativa juancarlosonate-tecnativa commented Feb 28, 2025

  • Add supplier in templates.
  • Allow matching at the template level by product.template char fields
  • Add the option to update only existing supplierinfos without creating new ones.
  • Add option at the template level, which groups records in the view by updated and non-updated after the import process.

@Tecnativa TT55210
@pedrobaeza @chienandalu

@juancarlosonate-tecnativa juancarlosonate-tecnativa changed the title [17.0][IMP] product_supplierinfo_import_by_barcode: To product_supplierinfo_import [16.0][IMP] product_supplierinfo_import_by_barcode: To product_supplierinfo_import Feb 28, 2025
@juancarlosonate-tecnativa juancarlosonate-tecnativa force-pushed the 16.0-imp-product_supplierinfo_import_by_barcode branch 2 times, most recently from 91cc896 to b816f8c Compare March 3, 2025 09:49
@juancarlosonate-tecnativa
Copy link
Author

ping @pedrobaeza @chienandalu can you take a look? thx!

@pedrobaeza pedrobaeza added this to the 16.0 milestone Mar 3, 2025
@pedrobaeza
Copy link
Member

You shouldn't rewrite the whole commit history for renaming it. That should be done when migrating to upper versions. Now, you just need to add the new feature on top in one commit, respecting the name AFAIK (not sure what you agreed with @chienandalu though, but for sure renaming this way not).

Please perform that change ASAP.

@juancarlosonate-tecnativa
Copy link
Author

You shouldn't rewrite the whole commit history for renaming it. That should be done when migrating to upper versions. Now, you just need to add the new feature on top in one commit, respecting the name AFAIK (not sure what you agreed with @chienandalu though, but for sure renaming this way not).

Please perform that change ASAP.

Alright, i thought this was the way to do it, anyway, i will wait for @chienandalu comments before proceeding, thanks!

@chienandalu
Copy link
Member

You shouldn't rewrite the whole commit history for renaming it. That should be done when migrating to upper versions. Now, you just need to add the new feature on top in one commit, respecting the name AFAIK (not sure what you agreed with @chienandalu though, but for sure renaming this way not).
Please perform that change ASAP.

Alright, i thought this was the way to do it, anyway, i will wait for @chienandalu comments before proceeding, thanks!

I think we agreed to avoid renaming the module in this version and do so in the future migration as the less problematic way to proceed.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Overall look in the good path :D

Some changes needed though:

@juancarlosonate-tecnativa juancarlosonate-tecnativa force-pushed the 16.0-imp-product_supplierinfo_import_by_barcode branch 3 times, most recently from f417327 to a823646 Compare March 6, 2025 15:05
@juancarlosonate-tecnativa
Copy link
Author

ping @chienandalu

@juancarlosonate-tecnativa juancarlosonate-tecnativa force-pushed the 16.0-imp-product_supplierinfo_import_by_barcode branch 2 times, most recently from cdcc620 to 6a2b358 Compare March 10, 2025 07:52
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

I'm testing in runboat with the complex spreadsheet. It is detected buy when imported, the following error ocurrs:

File "/mnt/data/odoo-addons-dir/product_supplierinfo_import_by_barcode/wizards/product_supplierinfo_import.py", line 247, in action_import_file
) = self._update_create_supplierinfo_data(parsed_data)
File "/mnt/data/odoo-addons-dir/product_supplierinfo_import_by_barcode/wizards/product_supplierinfo_import.py", line 175, in _update_create_supplierinfo_data
supplier_infos += self._create_new_supplierinfo(
File "/mnt/data/odoo-addons-dir/product_supplierinfo_import_by_barcode/wizards/product_supplierinfo_import.py", line 236, in _create_new_supplierinfo
product_supplierinfo.write(self._prepare_supplierinfo_values(row_data))
File "/mnt/data/odoo-addons-dir/product_supplierinfo_import_by_barcode/wizards/product_supplierinfo_import.py", line 121, in _prepare_supplierinfo_values
{
File "/mnt/data/odoo-addons-dir/product_supplierinfo_import_by_barcode/wizards/product_supplierinfo_import.py", line 122, in
tl.field_id.name: row_data[tl.header_name]
KeyError: 'Final\n'

@juancarlosonate-tecnativa
Copy link
Author

@chienandalu it seems to be due to these blank lines
image

@chienandalu
Copy link
Member

@chienandalu it seems to be due to these blank lines

Yes, it's deliberated. As when the user copy/pastes from the excel headers the could be pasting those line feeds as well it it was like that in the original.

That case was working before, if a don't remember wrong (and indeed the template is detected), do you know why it isn't now?

@juancarlosonate-tecnativa juancarlosonate-tecnativa force-pushed the 16.0-imp-product_supplierinfo_import_by_barcode branch from 6a2b358 to 3c74857 Compare March 10, 2025 15:40
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Let's try to avoid importing this rows. Add a method that checks if the columns corresponding to importing fields have any content and ignore the ones that doesn't.

image

).header_name
vendor_product_name = row_data.get(
vendor_product_name_header,
_("%(code)s ((product imported))", code=search_value),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_("%(code)s ((product imported))", code=search_value),
_("%(code)s (product imported)", code=search_value),

Choose a reason for hiding this comment

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

take a look at this and let me know what you think, don't like it much, but i think it works well

@juancarlosonate-tecnativa juancarlosonate-tecnativa force-pushed the 16.0-imp-product_supplierinfo_import_by_barcode branch from 3c74857 to 55972ff Compare March 12, 2025 15:45
Comment on lines +129 to +134
cleaned_values = {
key: value for key, value in values.items() if value not in (None, "")
}
if set(cleaned_values.keys()) <= {"delay", "date_start"}:
return None
return cleaned_values
Copy link
Member

Choose a reason for hiding this comment

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

A simpler implementation

    def _prepare_supplierinfo_values(self, row_data):
        """Overridable hook method so we can inject general wizard values"""
        values = {
            tl.field_id.name: row_data.get(
                self._parse_header([tl.header_name])[0], ""
            )
            for tl in self.template_id.template_line_ids
            if tl.field_id
        }
        # Avoid rows where there's no information given
        all_values_with_content = all([x not in {None, ""} for x in values.values()])
        if not all_values_with_content:
            return
        values.update({
            "delay": self.delay,
            "date_start": self.date_start,
        })
        return values

f"The product {vendor_product_name}"
f" could not be created due to a {search_field} collision."
)
return None
Copy link
Member

Choose a reason for hiding this comment

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

simply return is the same as return None. Anyway, prefer to return always the same datatype. For example, return self.env["product.product"]. It makes it potentially easier to deal with it the method later in different scenarios.

"""Creates a new supplier price list record, updating the previous one if needed."""
values = self._prepare_supplierinfo_values(row_data)
if not values:
return None
Copy link
Member

Choose a reason for hiding this comment

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

Same here return self.env["product.supplierinfo"]

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.

3 participants