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

[14.0][IMP]product_supplierinfo_for_customer_sale: Search SO by customerinfo #3484

Conversation

toita86
Copy link

@toita86 toita86 commented Dec 12, 2024

Add a new search field that retrieves all the SOs that contains the product with the given product_name or product_code.

@toita86 toita86 force-pushed the 14.0-IMP-product_supplierinfo_for_customer_sale-toita86 branch from 9e85c94 to b27d22f Compare December 16, 2024 12:35
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

LGTM

Use case: if product "Acoustic bloc screens" is sold to customer with customer name "Megablock", customer can ask you to retrieve an order containing the product "Megablock" => user needs to be able to perform search using customer name and/or customer code

With this PR we add a search field with the same properties as "Product" field in SO list view, searching for customer name and code

Copy link

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Code review, LGTM

@francesco-ooops
Copy link
Contributor

@pedrobaeza good to go? :)

@pedrobaeza pedrobaeza added this to the 14.0 milestone Dec 17, 2024
@@ -11,21 +11,30 @@ class SaleOrderLine(models.Model):

product_customer_code = fields.Char(
compute="_compute_product_customer_code",
store=True,
Copy link
Member

Choose a reason for hiding this comment

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

This is a no-go. You can't convert a field to stored on a table that can be populated with a lot of records.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the review, while I get your point I will like to ask how do you would you implement such a feature.

My approach derives by a similar implementation found here https://github.com/OCA/stock-logistics-workflow/blob/14.0/product_supplierinfo_for_customer_picking/models/stock_move.py .
The situation is very similar so I thought that was a possible solution.
For backward compatibility I can also make a migration script for the store attribute of the field.

But I will like to have you opinion first.

Choose a reason for hiding this comment

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

One possibility is to not store the field, and implement the search attribute

Copy link
Author

Choose a reason for hiding this comment

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

@aleuffre Thank you for your suggestion. I have implemented the changes based on your feedback.

@toita86 toita86 force-pushed the 14.0-IMP-product_supplierinfo_for_customer_sale-toita86 branch from b27d22f to 45f622c Compare December 23, 2024 10:23
Copy link
Contributor

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional still ok!

@pedrobaeza code is good now?

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-3484-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4ced8fa into OCA:14.0 Dec 24, 2024
10 of 11 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e2defab. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants