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

[11.0][MIG] stock_putaway_product #380

Merged
merged 6 commits into from
Mar 13, 2018
Merged

[11.0][MIG] stock_putaway_product #380

merged 6 commits into from
Mar 13, 2018

Conversation

asaunier
Copy link

@asaunier asaunier commented Feb 16, 2018

Migration of module stock_putaway_product to Odoo 11.

Requires the new stock_putaway_method module introduced by #379 to restore the method selector removed from the stock module of Odoo 11. As for now I have cherry-picked it into this branch.

Still investigating problems with product_tmpl_id...

Depends on:

@pedrobaeza
Copy link
Member

pedrobaeza commented Feb 16, 2018

You should remove the extra module here, and don't make to depend this one on it. If you want the extra feature, you install the other module. If not (as most people), then you don't.

@asaunier
Copy link
Author

@pedrobaeza Actually the extra module is required here because it adds the method field back that stock_putaway_product uses to add a new putaway strategy option. It cannot work without the extra module stock_putaway_method.

@pedrobaeza
Copy link
Member

Then you will need to create another glue module between then, but you can't force people that want the putaway strategy at product level but don't want other strategies except fixed location to also install the other module.

@yvaucher
Copy link
Member

@asaunier what @pedrobaeza means here is that you shouldn't include the commits of the PR for product_putaway_method module. It must be one PR per module. I understand thought that you did it that way to have a proper travis build. You can do this by adding an entry in oca_dependencies.txt I think.

@pedrobaeza the other module is simply a dependency. Fixed location is the name of the standard odoo behavior. The same name was kept from v10.

@pedrobaeza
Copy link
Member

Well, I insist that the module shouldn't be a dependency. To have the putaway strategy at product level you don't need the field to say what strategy to use. Do you understand me?

@yvaucher
Copy link
Member

@pedrobaeza The module stock_putaway_method reintroduce only a field to select a method. It does nothing else on its own. For me the stock_putaway_method module is just there to allow other methods to be implemented in addition to default and per_product as it way in previous odoo version. It does no harm. By default it will have no effect.

The module in the current PR stock_putaway_product requires the restored field of stock_putaway_method.
The field also helps on the view to make sure per product config is only displayed if required.
We could do without it defining this in the current module, but then it would lock the possibility to have other implemented methods.

So let me insist on my argument against your insistence but stock_putaway_product must depend on stock_putaway_method, and it ensure that you can choose between standard method or per_product method. There are dependency on field and on view.

If you mean that the module stock_putaway_method is useless, please explain why, express your fears and tell us more. I'm missing some information and context from your last messages to understand your concern.

@jgrandguillaume
Copy link
Member

Hi there,

I'm also find more convenient to have a module listing the putaway methods for a given product. We'll need to develop more methods in a near future, so having a generic module to gather a same manner to define them sounds good to me. @pedrobaeza what is bad with this ? Do you think it is better that each module providing putawy method define its own ? I don't understand may be ?

@lreficent @jbeficent any opinion ? This is on the dependency chain of DDMRP wip.

@asaunier asaunier changed the title [WIP][11.0][MIG] stock_putaway_product [11.0][MIG] stock_putaway_product Feb 27, 2018
@asaunier
Copy link
Author

The PR is now ready.

The last commit 6811080 (Putaway strategy: add module to restore the method field) is only a cherry-pick of PR #379 and will be removed when the latter PR is merged.

The actual changes are done by commit d695c17

What it does:

  • make the module depend on module stock_putaway_method (added by PR [11.0] Putaway strategy: add module to restore the method field #379) that restores the former putaway method selector (as in Odoo 10).
  • use product templates as basis case for view product.xml and adapt it for product variants by overriding function fields_view_get output in the product.product model.
  • rewrite tests a bit to use SavepointCase for tests and move demo data to the tests' setUpClass function, making sure data changes are rolled back after each test (we had conflicts with module stock_request_purchase tests).

@jgrandguillaume I have checked in the Odoo UI the changes were consistent but having an expert user test would make sense as well, thanks :)

Copy link

@mpanarin mpanarin left a comment

Choose a reason for hiding this comment

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

apart from some minor things, a fall back variant should be present for strategies, imo


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/153/10.0

Choose a reason for hiding this comment

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

this should point to 11.0 ;)

* Jos De Graeve - Apertoso N.V. <Jos.DeGraeve@apertoso.be>
* Carlos Dauden - Tecnativa <carlos.dauden@tecnativa.com>
* Denis Roussel - ACSONE SA/NV <denis.roussel@acsone.eu>
* Thomas Fossoul - WINK SA/NV <tfossoul@wink.be>

Choose a reason for hiding this comment

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

consider adding yourself as a contributor

@@ -0,0 +1,5 @@
# © 2016 Jos De Graeve - Apertoso N.V. <Jos.DeGraeve@apertoso.be>

Choose a reason for hiding this comment

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

© -> Copyright
adding
# Copyright 2018 Camptocamp SA
would be a good move ;)
same for other files as well

product_putaway_ids = fields.One2many(
comodel_name='stock.product.putaway.strategy',
inverse_name='product_tmpl_id',
string="Product stock locations")

Choose a reason for hiding this comment

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

not blocking but I would be consistent with formatting and change the last row to

string="Product stock locations",
)

""" Custom redefinition of fields_view_get to adapt the context
to product variants.
"""
res = super(ProductProduct, self).fields_view_get(view_id=view_id,

Choose a reason for hiding this comment

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

super in python3 no longer requires explicit class and instance definition, also I'd change the formatting


@api.model
def _get_putaway_options(self):
ret = super(ProductPutaway, self)._get_putaway_options()

Choose a reason for hiding this comment

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

same as above

strategy = product.product_putaway_ids.filtered(
lambda strategy: (strategy.putaway_id == self))
if not strategy:
strategy = product.product_tmpl_id.product_putaway_ids.filtered(

Choose a reason for hiding this comment

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

I think that a fallback variant is still required. As if, for whatever reason, Not product, nor it's template has a strategy -> this will fail. And this is pretty much possible as we saw during fixing of the tests.

Copy link
Author

Choose a reason for hiding this comment

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

It indeed makes sense. Here is what I suggest with 83d1bfa:

  • if method == 'per_product' and if a strategy has been found:
    return strategy.fixed_location_id.id (as formerly)
  • else fall back to the parent class putaway_apply function

See https://github.com/asaunier/stock-logistics-warehouse/blob/83d1bfa131da574cc0c06b3dbc4d3de0ef82ba3c/stock_putaway_product/models/product_putaway.py#L38-L44

# right location to update stock.
@api.model
def default_get(self, fields):
res = super(StockChangeProductQty, self).default_get(fields)

Choose a reason for hiding this comment

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

update super ;)

carlosdauden and others added 6 commits March 1, 2018 12:07
… and Warehouse/Manager can access to the product form. After this PR, all users can access to the product form, but only users that belong to the group "Warehouse/User" can access to the section for Putaway strategies in the Inventory tab.
@pedrobaeza
Copy link
Member

pedrobaeza commented Mar 1, 2018

What I was trying to say is to remove the need of the strategy type and only relies in the one2many table. You don't need to say "My strategy is 'per product'", but only to add products with their locations in the strategy, and when getting the putaway location, look at that table. If there's a match, then put it. If not, call super.

Do you understand me?

@LoisRForgeFlow
Copy link
Contributor

My opinion here is more in line with @jgrandguillaume I think is better to specifically select the strategy you want to apply (it will more user friendly I think).

It also does not add that much complexity in the development side, in fact it ease the migration as the functionality keep exatly the same and it allows to add more strategies in the future without any extra complexity in the inheritance chain to make the current methods and the new ones coexist (in this sense you just select the method you want to apply among the ones installed).

@pedrobaeza
Copy link
Member

OK

@@ -13,7 +12,7 @@ class StockChangeProductQty(models.TransientModel):
# right location to update stock.
@api.model
def default_get(self, fields):
res = super(StockChangeProductQty, self).default_get(fields)
res = super().default_get(fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

What means empty super?

Copy link
Member

Choose a reason for hiding this comment

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

New in Python 3. It saves you to put class and instance

@asaunier
Copy link
Author

@yvaucher Perhaps you could remove the "work in progress" label since the PR is ready for reviewing. Thanks :)

@yvaucher yvaucher merged commit 08c353b into OCA:11.0 Mar 13, 2018
@asaunier asaunier deleted the 11.0-mig-stock_putaway_product branch March 13, 2018 13:09
@pedrobaeza pedrobaeza mentioned this pull request Mar 13, 2018
46 tasks
@bealdav
Copy link
Member

bealdav commented Mar 13, 2018

Sorry, I have seen the PR before but it miss this code from v8.

f0c744a

Only if it's interesting for someone.

@pedrobaeza
Copy link
Member

FYI, Odoo has integrated in master (v12) the possibility of defining putaway strategy per product:

odoo/odoo@2c1611d

@bealdav
Copy link
Member

bealdav commented Apr 4, 2018

thanks a lot @pedrobaeza

@rousseldenis
Copy link
Sponsor Contributor

@pedrobaeza Nice! At least they did it.

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.

None yet