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

[12.0] [ADD] product_pack #498

Closed
wants to merge 70 commits into from

Conversation

@ernestotejeda
Copy link
Member

commented Aug 22, 2019

@ernestotejeda ernestotejeda force-pushed the Tecnativa:12.0-add-product_pack branch from d4f158c to 0016676 Sep 16, 2019
@ernestotejeda

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

@zaoral , I have a question in this part of the code in the original module:
https://github.com/ingadhoc/product/blob/0e9d8979efeeec5aae2aa051499af2155b5ccc76/product_pack/models/product_product.py#L51

I would like you could help me. I'm extracting the redefinition of this method (_compute_quantities_dict) to a new module as commented here #498 (comment):
why here it is forced the 'virtual_available' key of the dictionary to a positive value or False?, why it cannot be negative?.
Wouldn't it be right to do something similar to what is done with the 'qty_available' key? I mean do this:

'virtual_available': (
                    pack_virtual_available and
                    min(pack_virtual_available) or False),
@Yajo

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I think that one of the most confusing parts of the module is that dropdown that modifies the pack behavior, possibly because it's a single field modifying many things, and we could make it more simple if we remove it and replace it with several fields:

  • Show components in sale order (bool pack_component_show): Indicates if the components should be displayed individually in the sale order.

  • Component prices (select pack_component_price): Indicates what to do with individual component prices. It should have these options:

    1. Detailed per component (detailed): It would include each component's price into its corresponding line. Constrained to pack_component_show=True.
    2. Merged in main product (merged): Pack price = main product price + sum of all component prices. If pack_component_show=True, individual component prices will be always 0.
    3. Ignored (ignored): Component prices are set to 0 and only the main product price is used.

What do you think about it?

Also another question: do any of these fields have any meaning without the sale module installed? 🤔 I don't think so, so all of them should go into sale_product_pack, right?

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

do any of these fields have any meaning without the sale module installed?

The idea is to reuse them on a future purchase_product_pack.

@legalsylvain

This comment has been minimized.

Copy link

commented Sep 18, 2019

Hi all.
first, thanks for this module I just discovered right now !
I'm very interested to install and use this V12.0 module.

  1. Also interested to develop a pos_product_pack to make the feature available in the PoS. Is this work planed by other people ?

  2. About the feature, one of my needs is not covered : In some cases, the content of the pack is changing during the year. (depending of date). In my shops, we are selling sometime packs of various products including fruits or vegetables. Of course, the list of fruits and vegetables depend of dates, but the pack is always the same.

to cover this extra feature, I propose to make a PR against your branch, and include the following changes :

  • add a date_start and a date_stop fields on the pack.line model. (field optional)
  • add a function in the pack model named _get_pack_lines(date=False) that return the content of the pack (product list), depending of the date. (if no date, today is used).
  • call the function in the different part of this module. (replacing self.mapped('pack_line_ids.product_id')))
  • add a group to make optional all that features. (something like "Pack Content Seasonality"). So, it is an extra feature, that will not make this module more complex, in most cases.

@ernestotejeda, all, what about this proposal ?

I thought to add an extra module that depend on this module, but the function should be in this module, to make all the other ones working.

I can work on that point very quickly, and propose a PR this week.

Thanks for your comments !

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Hi, @legalsylvain thanks for your interest and your feature is interesting, although maybe it's better to include it in a new module for having that isolated. It can be included here the function you need though, returning by default the mapped, and being overwritten in that module. But my position is not strong in this if you want to have it on main module, but I think it's going to be more difficult with all the group stuff, and this module won't need other glue modules for any of the extensions (sale, purchase, stock, pos...).

About pos_product_pack, it's not yet planned in our side, but the idea of the module split is to allow things like that.

Now I'm wondering if we should create a repository OCA/pack for this or spread them like now.

@legalsylvain

This comment has been minimized.

Copy link

commented Sep 18, 2019

Hi @pedrobaeza : Thanks for your quick reply !

It can be included here the function you need though, returning by default the mapped, and being overwritten in that module.

That's is the "must have" point for me, indeed.

But my position is not strong in this if you want to have it on main module, but I think it's going to be more difficult with all the group stuff, and this module won't need other glue modules for any of the extensions (sale, purchase, stock, pos...).

Having one module :

  • pro : Less work, in each migration. (one per year is a lot, I see it as a big OCA problem, for the time being).
  • cons : what you say.

Maybe we should make one module, if most of the people think it could be interesting to have this option, and make two module if it's very specific case ?
In food context, it's a must have feature, but I don't know for other context / verticalization. Let's see what other OCA people are saying ?
any way, not a blocking point for me.

Now I'm wondering if we should create a repository OCA/pack for this or spread them like now.

for the time being :

  • product_pack
  • product_pack_seasonality (maybe !)
  • sale_pack
  • sale_stock_pack ?
  • purchase_pack
  • pos_pack

Indeed, it maybe deserves a specific repository. 👍

Do you think it's relevant to make this change for 12.0, or to wait the super fast, super easy and super simple 13.0 coming soon version ?

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@ernestotejeda please add test and let's merge this.

@zaoral @jjscarafia do we have your bless? https://youtu.be/bk_02-9OBmM

We cannot invest hundred hours in this module and we will maintain it for sure 😄

@legalsylvain

This comment has been minimized.

Copy link

commented Sep 18, 2019

@ernestotejeda please add test and let's merge this.

please don't, before I added the function _get_pack_lines.

I'll do this stuff in a few moment.

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

Planned module names are:

  • product_pack
  • sale_product_pack
  • stock_product_pack
  • whatever_product_pack

Why? Because pack is a very generic word, used also on logistics and other places, so product_pack seems better. The repository must be OCA/product-pack in fact. I think it's better to do it now than later and have to move issues, modules, and so on.

@rafaelbn

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

We used product_bundle in the past: #180

It was beacause Odoo have already packages, packaging... 🤕

@legalsylvain

This comment has been minimized.

Copy link

commented Sep 18, 2019

@pedrobaeza I introduced the required function get_pack_products().
PR available here. Tecnativa#1

finally, the problem I see if we set the seasonability feature in an extra module, that will require a lot of glue modules. I mean :

  • product_pack_seasonability : add date_start, date_stop + overload get_pack_products() to use a context value pack_date)
  • sale_product_pack_seasonability : to pass the date of the sale order in the context
  • purchase_product_pack_seasonability : to pass the date of the purchase order in the context
  • etc...

It will generate a lot of glue modules, only for a python line.

Solution 1 : A lot of glue modules. (I'd like to avoid that).
Solution 2 : put seasonability features in product_pack module.
Solution 3 : in the sale_product_pack module (and other modules) call product_pack function with a with_context(pack_date=xxxx) that will be handled in the product_pack_seasonability. (Lazy dependency)

What do you think ?

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@legalsylvain I don't see the need of that glue modules: sale_product_pack is going to call the method get_pack_products with the order date in the base. What changes is that the standard product_pack ignores that argument and returns always the same (the mapped), but your dependent module will change the return value according it.

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I have created OCA/product-pack: https://github.com/OCA/product-pack. @ernestotejeda please move PRs to that repository.

…dable

[IMP] possibility to overload product_pack
@zaoral

This comment has been minimized.

Copy link

commented Sep 18, 2019

@ernestotejeda for me looks good.
Thank for the work.

About what you ask for the virtual_available, I have tested and discuss internally with the team, and for be honest we do not remember why we force to virtual_available to be positive, we were not able to found a user case, feel free to change it to

'virtual_available': (
                    pack_virtual_available and
                    min(pack_virtual_available) or False),

About the migration script, thank you for change it

Best Regards

Copy link
Member

left a comment

Rather interesting to see how this module will interact with the rest. Probably a lot of changes have to be done (reporting, stock valuation etc) to take (or not take) into consideration the products of type pack. (Perhaps adding a product type would be better than the pack_ok)

What is the use case for this?
I understand that the products that are marked as Packs are supposed to contain other products, they are not comprised by these products.
If this is a picking issue, maybe some sort of grouping of stock.moves would be better.

'parent product company'))

@api.multi
def write(self, vals):

This comment has been minimized.

Copy link
@daramousk

daramousk Sep 19, 2019

Member

What kind of error are you trying to avoid here? What if you set store=True to pack_line_ids or provide an inverse?

@@ -0,0 +1,3 @@
id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink

This comment has been minimized.

Copy link
@daramousk

daramousk Sep 19, 2019

Member

Perhaps the security settings should mimic the settings in product module?

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

I think more changes will be needed if you create a new product type, as all the domains, filters, etc works over existing ones. The bridge for stock operations is OCA/stock-logistics-warehouse#718.

All of this must be moved though to the new OCA/product-pack repo.

@rousseldenis

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Hi all, thanks for this.

Sorry to arrive late here but what is the difference with BOM's ?

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

@rousseldenis there are plenty of reasons for having this:

  • Don't depend on mrp.
  • Having several prices options (like computing price from components + discount).
  • Components display options (detailed/non detailed/assisted...).
  • Expand it to other parts (like purchase or PoS).
  • Seasonability like @legalsylvain proposes.
  • etc.
@zaoral

This comment has been minimized.

Copy link

commented Sep 26, 2019

Hi @ernestotejeda
This PR can be closed? making the references to the PR that supersedes this one?

Best Regards

@pedrobaeza

This comment has been minimized.

Copy link
Member

commented Sep 26, 2019

Yes, superseded by OCA/product-pack#1

@pedrobaeza pedrobaeza closed this Sep 26, 2019
@pedrobaeza pedrobaeza deleted the Tecnativa:12.0-add-product_pack branch Sep 26, 2019
@rafaelbn rafaelbn referenced this pull request Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.