[16.0][ADD] shopinvader_search_engine_product_multi_price #1498

qgroulard · 2024-01-26T15:37:35Z

See #1441 and #1467 for PRs on related matter.

Remove price from the schema in shopinvader_product as there are a lot of different ways to expose product prices and they should be handled in dedicated modules.
'_get_price()' method has already been extracted to OCA/product-attribute module 'product_get_price_helper'.
Add a module to export prices for a list of pricelists set on the index or backend.

simahawk · 2024-02-26T08:18:13Z

@qgroulard the 1st commit can be dropped now, no?

simahawk · 2024-02-26T08:20:26Z

shopinvader_search_engine_product_multi_price/models/product_product.py

+        index_id = self.env.context.get("index_id", False)
+        index = self.env["se.index"].browse(index_id)
+        for product in self:
+            prices = {"default": product._get_price()}


wouldn't make more consistent to have the ID of the default pricelist here?

I went for the simplest solution possible. Here we have no such thing as "default pricelist", the default price is the price without pricelist.

Maybe it would make even more sense to remove this "default" key (I can still add it at my project level).

For now we have no reliable solution for prices on the main branch, I think it would be great to have at lease one simple option available. Then we can build up on that or add other modules for other ways of computing and exposing prices.

I'm glad to finally see someone care about prices 😁

I'm testing this now and if the pricelist is already on the backend or the index we'll have a duplicated price info

As for the default: either we add a specific field to configure which pricelist is the default one or we drop this "default" key and we assume that the client must know which pricelist to use.

Another option: add a key _default pointing to the default id but it will still index duplicated info across all records

So you are testing with both shopinvader_search_engine_product_multi_price and shopinvader_search_engine_product_price (#1441) installed ?

I actually think we don't need a "default" key in prices of shopinvader_search_engine_product_multi_price, if people want a default price they can install shopinvader_search_engine_product_price, choose the (default) pricelist on the backend, and consider the key price as the default price.

We can also rename the key prices into prices_by_pricelist or pricelist_prices to clarify things.

No, I'm not testing w/ both modules. The default key comes because of this module. You are not replacing the dict you are updating it. price_by_pricelist is probably more explicit.

Sorry, I've been busy for the past two weeks.
I have removed the key "default" and I have renamed the key prices to price_by_pricelist.

qgroulard · 2024-02-26T09:42:20Z

@qgroulard the 1st commit can be dropped now, no?

In theory yes, the keys price and prices can coexist.

But I think price should be removed anyway as it is a broken first draft. Prices were still WIP when schemas have been merged, keeping it on the main branch seems wrong to me.

simahawk · 2024-02-26T09:58:04Z

@qgroulard the 1st commit can be dropped now, no?

In theory yes, the keys price and prices can coexist.

But I think price should be removed anyway as it is a broken first draft. Prices were still WIP when schemas have been merged, keeping it on the main branch seems wrong to me.

I meant that you already removed it when creating the helper module, no?

simahawk · 2024-02-26T10:59:14Z

@qgroulard the 1st commit can be dropped now, no?

In theory yes, the keys price and prices can coexist.
But I think price should be removed anyway as it is a broken first draft. Prices were still WIP when schemas have been merged, keeping it on the main branch seems wrong to me.

I meant that you already removed it when creating the helper module, no?

It seems not, in fact 😅

simahawk · 2024-02-26T11:03:43Z

@qgroulard can we fast track #1507 while fixing this one?

mmequignon · 2024-02-28T14:50:53Z

shopinvader_search_engine_product_multi_price/models/product_product.py

+        for product in self:
+            prices = {"default": product._get_price()}
+            if index_id:
+                for pricelist in index._get_pricelists():


isn't this returning the default pricelist as well?
If so, I would've put {"default": default_list.id}.
Otherwise you compute the price twice.

Resolved with the last change, the "default" key has been removed.

mmequignon · 2024-02-28T14:51:44Z

shopinvader_search_engine_product_multi_price/models/product_product.py

+            if index_id:
+                for pricelist in index._get_pricelists():
+                    price = product._get_price(pricelist=pricelist)
+                    prices.update({f"{pricelist.id}": price})


why not {str(pricelist_id): price}?
I don't think we need to format strings here.

simahawk · 2024-04-08T07:43:20Z

@qgroulard ping :)

lmignon · 2024-06-04T08:22:52Z

shopinvader_search_engine_product_multi_price/schemas/product.py

+
+
+class ProductProduct(BaseProduct, extends=True):
+    price_by_pricelist: dict[str, ProductPriceInfo] = {}


Suggested change

price_by_pricelist: dict[str, ProductPriceInfo] = {}

price_by_pricelist: dict[int, ProductPriceInfo] = {}

shopinvader_search_engine_product_multi_price/tests/test_product_binding.py

shopinvader_search_engine_product_multi_price/models/product_product.py

shopinvader_search_engine_product_multi_price/tests/test_product_binding.py

lmignon

@qgroulard The key should be an integer.... It's confusing to have to convert an identifier from the user's profile for example to a string to get the right price. The type of a value should not vary depending where it's used...

sebastienbeau

LGTM (code review)

lmignon · 2024-06-04T16:20:24Z

/ocabot merge nobump

shopinvader-git-bot · 2024-06-04T16:20:31Z

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1498-by-lmignon-bump-nobump, awaiting test results.

shopinvader-git-bot · 2024-06-04T16:34:24Z

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

qgroulard force-pushed the 16.0-shopinvader_product_multi_price-qgr branch from b3e26de to 12a4226 Compare January 26, 2024 15:42

simahawk reviewed Feb 26, 2024

View reviewed changes

simahawk mentioned this pull request Feb 26, 2024

[16.0] Add shopinvader_api_customer_price #1506

Open

simahawk mentioned this pull request Feb 26, 2024

[REF] shopinvader_product: Remove price #1507

Merged

mmequignon reviewed Feb 28, 2024

View reviewed changes

qgroulard force-pushed the 16.0-shopinvader_product_multi_price-qgr branch 4 times, most recently from c1cb7f7 to 45eb5e3 Compare March 13, 2024 12:59

qgroulard force-pushed the 16.0-shopinvader_product_multi_price-qgr branch from 45eb5e3 to 2c1c43f Compare June 3, 2024 14:26

sebastienbeau added this to the 16.0 milestone Jun 3, 2024

lmignon reviewed Jun 4, 2024

View reviewed changes

shopinvader_search_engine_product_multi_price/tests/test_product_binding.py Show resolved Hide resolved

lmignon reviewed Jun 4, 2024

View reviewed changes

shopinvader_search_engine_product_multi_price/models/product_product.py Outdated Show resolved Hide resolved

lmignon reviewed Jun 4, 2024

View reviewed changes

shopinvader_search_engine_product_multi_price/tests/test_product_binding.py Show resolved Hide resolved

lmignon requested changes Jun 4, 2024

View reviewed changes

qgroulard force-pushed the 16.0-shopinvader_product_multi_price-qgr branch from 2c1c43f to 18ef55b Compare June 4, 2024 09:23

[ADD] shopinvader_search_engine_product_multi_price

005e5ff

qgroulard force-pushed the 16.0-shopinvader_product_multi_price-qgr branch from 18ef55b to 005e5ff Compare June 4, 2024 12:27

sebastienbeau approved these changes Jun 4, 2024

View reviewed changes

lmignon approved these changes Jun 4, 2024

View reviewed changes

shopinvader-git-bot added the approved label Jun 4, 2024

shopinvader-git-bot added the bot is merging ⏳ label Jun 4, 2024

shopinvader-git-bot merged commit 033d62d into shopinvader:16.0 Jun 4, 2024
3 checks passed

shopinvader-git-bot added merged 🎉 and removed bot is merging ⏳ labels Jun 4, 2024

lmignon deleted the 16.0-shopinvader_product_multi_price-qgr branch June 4, 2024 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][ADD] shopinvader_search_engine_product_multi_price #1498

[16.0][ADD] shopinvader_search_engine_product_multi_price #1498

qgroulard commented Jan 26, 2024 •

edited

Loading

simahawk commented Feb 26, 2024

simahawk Feb 26, 2024

qgroulard Feb 26, 2024

simahawk Feb 26, 2024

simahawk Feb 26, 2024

simahawk Feb 26, 2024 •

edited

Loading

qgroulard Feb 26, 2024

simahawk Feb 27, 2024

qgroulard Mar 13, 2024

qgroulard Apr 8, 2024

qgroulard commented Feb 26, 2024

simahawk commented Feb 26, 2024

simahawk commented Feb 26, 2024

simahawk commented Feb 26, 2024

mmequignon Feb 28, 2024

qgroulard Mar 13, 2024

mmequignon Feb 28, 2024

qgroulard Mar 13, 2024

simahawk commented Apr 8, 2024

lmignon Jun 4, 2024

qgroulard Jun 4, 2024

lmignon left a comment

sebastienbeau left a comment

lmignon commented Jun 4, 2024

shopinvader-git-bot commented Jun 4, 2024

shopinvader-git-bot commented Jun 4, 2024



		class ProductProduct(BaseProduct, extends=True):
		price_by_pricelist: dict[str, ProductPriceInfo] = {}

	price_by_pricelist: dict[str, ProductPriceInfo] = {}
	price_by_pricelist: dict[int, ProductPriceInfo] = {}

[16.0][ADD] shopinvader_search_engine_product_multi_price #1498

[16.0][ADD] shopinvader_search_engine_product_multi_price #1498

Conversation

qgroulard commented Jan 26, 2024 • edited Loading

simahawk commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simahawk Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qgroulard commented Feb 26, 2024

simahawk commented Feb 26, 2024

simahawk commented Feb 26, 2024

simahawk commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simahawk commented Apr 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmignon left a comment

Choose a reason for hiding this comment

sebastienbeau left a comment

Choose a reason for hiding this comment

lmignon commented Jun 4, 2024

shopinvader-git-bot commented Jun 4, 2024

shopinvader-git-bot commented Jun 4, 2024

qgroulard commented Jan 26, 2024 •

edited

Loading

simahawk Feb 26, 2024 •

edited

Loading