-
-
Notifications
You must be signed in to change notification settings - Fork 697
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] Fix build #1521
[16.0] Fix build #1521
Conversation
Hi @legalsylvain, |
Thanks ! |
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
@legalsylvain your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1521-by-legalsylvain-bump-patch. After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red. |
There's an unrelated error on |
The error is in the module @rousseldenis, @lmignon @MiquelRForgeFlow, @lmarion-source could you take a look ? |
I'm on it |
Now the issue is on |
To me the domain used to look for the customer info is wrong. This means that the selected customer info should be the one w/ price = 100.0 because is the one assigned to the specific variant. However, the selection gives you back the generic price:
When I look at the domain it seems wrong
By logic, I assume we should have an AND w/ partner and product and fallback w/ the OR. However I don't know this module at all... @pedrobaeza @sergio-teruel @MiquelRForgeFlow @AaronHForgeFlow anyone can provide some feedback? |
In a first sight, I don't see the domain incorrect. It's saying that:
The only weird thing is that the partner ID and the product ID is the same, but reading the code, it seems legit. |
IMO there's something wrong because even if I have 2 records matching the partner and only one of them matches the variant I get the generic one (matching only the tmpl) as a result. Hence, I assume the domain is wrong. |
But for that, there's this sort: https://github.com/OCA/product-attribute/blob/16.0/product_supplierinfo_for_customer/models/product_product.py#L138 |
IMO the solution for |
I think the matching of the variant is more important than the template. Perhaps the sorting is incorrect. Created PR with this: camptocamp#4 |
@victoralmau see my previous comments. Anyway, you can exercise the solution in one PR if you see it's the proper one. |
Fixed in #1528, although the solution doesn't convince me too much I have to say, as this reveals a flaw in the design. |
Tested in my local with this: camptocamp#4 and test were not failing |
But that's not very optimal in performance. We should try to avoid several filtered. |
@pedrobaeza after @lmignon suggestion it works the same without the use of filtered: https://github.com/camptocamp/product-attribute/pull/4/files |
Can you gather all and rebase for getting the proper solution? |
Hi @pedrobaeza is this comment in regards my PR? I am afraid I did not get what you mean. |
Yes, I mean to put here in this PR (by @simahawk or someone else) the needed things to get such proper code. |
I'll cherry-pick camptocamp#4 |
Monetary fields do not have any digits attribute since their precision comes from the currency. There's also no need to specify a currency field when currency_id is available on the model.
The test on unicity constraint is passing but the error log from the ORM must be muted to not get a false negative.
b414318
to
03d244c
Compare
Are we all fine w/ these fixes? |
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at c302a0e. Thanks a lot for contributing to OCA. ❤️ |
Tackling some issues from #1513