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_available: Migration to 11.0 #405

Merged
merged 22 commits into from
Apr 23, 2018
Merged

[11.0][MIG] stock_available: Migration to 11.0 #405

merged 22 commits into from
Apr 23, 2018

Conversation

asaunier
Copy link

@asaunier asaunier commented Apr 9, 2018

Based upon PR #353 by @kikopeiro
I have only squashed the "OCA Transbot updated translations from Transifex" commits as suggested by @yvaucher

I have also removed the 2 [MIG] Make modules uninstallable commits.

I have not removed the commented out section in res_config_settings_views.xml as pointed by @gurneyalex Shoud I? Why is this section commented out?

@asaunier
Copy link
Author

@gurneyalex Since you have approved the original PR #353, do you approve this one wich is the same with a few squashed commits?

Anyone else for a review? Thanks :)

@asaunier
Copy link
Author

Hi @pedrobaeza !
Could you add the "needs review" label please?
Thanks :)

Copy link
Contributor

@rousseldenis rousseldenis 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. Minor changes.

Could you please also consider commits in 10.0 branch history after the 9th of May 2017 ?

for p in product.product_variant_ids:
qty = variant_available[p.id]["immediately_usable_qty"]
immediately_usable_qty += qty
if p.potential_qty > potential_qty:
Copy link
Contributor

Choose a reason for hiding this comment

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

@asaunier Please use float_compare

</div>
</div>

<!--<div>-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unneeded comments


By default, this module computes the stock available to promise as the virtual
stock.
To take advantage of the additional features, you must which information you
Copy link

@jcdrubay jcdrubay Apr 20, 2018

Choose a reason for hiding this comment

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

you must define on which

Copy link

@jcdrubay jcdrubay left a comment

Choose a reason for hiding this comment

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

Very nice module, some comments for very minor requests.

# Copyright 2016 Sodexis
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from odoo import models, fields, api

Choose a reason for hiding this comment

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

Should be alphebetical order also within the line:

from odoo import api, models, fields

help="Stock for this Product that can be safely proposed "
"for sale to Customers.\n"
"The definition of this value can be configured to suit "
"your needs")

Choose a reason for hiding this comment

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

add a final dot

"your needs."

Copy link
Member

@congdpt congdpt left a comment

Choose a reason for hiding this comment

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

👍 Thanks, but I think you need some points to fix

# Copyright 2016 Sodexis
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from . import product_template
Copy link
Member

Choose a reason for hiding this comment

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

You should sort the import lines base on Alphabet

# Copyright 2016 Sodexis
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from odoo import models, fields, api
Copy link
Member

Choose a reason for hiding this comment

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

It should be from odoo import api, fields, models


class ProductProduct(models.Model):

"""Add a field for the stock available to promise.
Copy link
Member

Choose a reason for hiding this comment

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

I think you need split """ and the text to 2 lines, it will be look well

@api.multi
@api.depends('virtual_available')
def _compute_immediately_usable_qty(self):
"""No-op implementation of the stock available to promise.
Copy link
Member

Choose a reason for hiding this comment

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

I think you need split """ and the text to 2 lines, it will be look well

@api.multi
@api.depends()
def _compute_potential_qty(self):
"""Set potential qty to 0.0 to define the field defintion used by
Copy link
Member

Choose a reason for hiding this comment

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

I think you need split """ and the text to 2 lines, it will be look well


if isinstance(product.id, models.NewId):
continue
immediately_usable_qty = 0.0
Copy link
Member

Choose a reason for hiding this comment

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

I think we need one line before this line and no need a line after this line

@api.multi
@api.depends('product_variant_ids.immediately_usable_qty')
def _compute_immediately_usable_qty(self):
"""No-op implementation of the stock available to promise.
Copy link
Member

Choose a reason for hiding this comment

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

I think you need split """ and the text to 2 lines, it will be look well

variants.
"""
res = self._product_available()
for tmpl in self.filtered(lambda x: not isinstance(x.id,
Copy link
Member

Choose a reason for hiding this comment

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

If will better like that:

for tmpl in self.filtered(
    lambda x: not isinstance(x.id, models.NewId)):

@api.multi
@api.depends('product_variant_ids.potential_qty')
def _compute_potential_qty(self):
"""Compute the potential as the max of all the variants's potential.
Copy link
Member

Choose a reason for hiding this comment

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

I think you need split """ and the text to 2 lines, it will be look well

# Copyright 2016 Sodexis
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).

from odoo import api, models, fields
Copy link
Member

Choose a reason for hiding this comment

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

It should be from odoo import api, fields, models

Copy link

@tschanzt tschanzt left a comment

Choose a reason for hiding this comment

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

Only minor nitpicking otherwise 👍

License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). -->

<odoo>
<record model="ir.ui.view" id="view_stock_available_form">

Choose a reason for hiding this comment

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

indentation is wrong.

License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). -->

<odoo>
<record model="ir.ui.view" id="product_normal_form_view">

Choose a reason for hiding this comment

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

indentation is wrong.

<field name="model">res.config.settings</field>
<field name="inherit_id" ref="stock.res_config_settings_view_form" />
<field name="arch" type="xml">
<data>

Choose a reason for hiding this comment

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

you can drop the data tag.

def _compute_available_quantities(self):
res = self._compute_available_quantities_dict()
for product in self:
for key, value in res[product.id].iteritems():

Choose a reason for hiding this comment

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

iteritems doesn't exist anymore in python3. use items.

@asaunier
Copy link
Author

@rousseldenis @jcdrubay @congdpt @tschanzt Thanks for reviewing!

I have taken your feedbacks into account and added the commits done in 10.0 since the initial PR #353 was created, 5 months ago.

Copy link
Contributor

@rousseldenis rousseldenis 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

@pedrobaeza
Copy link
Member

I think commit history can be squashed a bit, specially at the beginning where a lot of commits from the same person and corresponding to the initial logic are there.

Lionel Sausin (Numérigraphe) and others added 14 commits April 23, 2018 14:54
Generic module to compute the stock quantity available to promise using several implementations.
stock_available_immediatly is changed to become the first optional implementation.
Cherry pick of commit 0b060f6 from the v7 branch

[IMP] stock_available* uses new API

[ADD] stock_available_mrp

Module to take immediate manufaturing capability into account in the stock quantity available to promise.

Conflicts:
	stock_available/res_config.py

[DEL] move stock_available_mrp to __unported__

[ADD] stock_available_sale

Take sale quotations into account in the stock quantity available to promise
Cherry-picked from 497068f

Conflicts:
	stock_available/res_config.py

[DEL] move stock_available_sale to __unported__

[IMP] READMEs and TODOs

Cherry-picked from v7 at 8add4be

Conflicts:
	__unported__/stock_available_mrp/__openerp__.py
	stock_available/__openerp__.py
	stock_available_immediately/__openerp__.py

[IMP] respect product decimal precision

odoo/odoo#5512 and odoo-dev/odoo@b3e5a94 makes it clear the standard intends to support decimal precision on the product form.
…uct and

product.template, now takes in account variants and correctly displays value.
[FLAKE8]

Removing duplicate modules and moving README.rst into __unported__

[ADD} location calculations

[FIX] typo
not using internal qty_available that seems not to
take in consideration reserved quants.

[ADD] Tests
Commit 6c16913 changed the way we compute the immediately_usable_qty: instead of using the virtual stock, we used the sum of quants without reservations. But a quant may actually be reserved and still be available (for example it may be reserved for an internal move).
Fixes #79

Remove loop and use correct decorator

Restore the features of stock_available_immediately

The previous fix restored stock_available but then there was no way to exclude the incomming moves from the count. This belongs in stock_available_immediately, restoring it cleanly.
This commit also takes care to respect the distinction between templates and variants, so it should fix #73 too.

Restore the qty avail. to promise on variant treeview

PEP8
* fix the dependencies for the computed field

* use api.multi instead of api.one to avoid calling
  super()._immediately_usable_qty in a loop (this improves perfs on a tree view
  display)
There are cases where we dot NOT want to simply sum the quantities of all the
variants. For example when dealing with manufacturing capacities, we may have
to chose between variants because we can't make ALL of them with the same
components.

So instead of a simple non-modular implementation, we'll let each module define
his own implementation of how to compute the product template's quantity
available for sale.

Conflicts:
	stock_available/__openerp__.py
	stock_available_immediately/__openerp__.py
@asaunier
Copy link
Author

asaunier commented Apr 23, 2018

@pedrobaeza I have just rebased the PR squashing consecutive commits from the same contributors together. The PR now has 22 commits instead of 39.

@pedrobaeza
Copy link
Member

Great, thanks!

@pedrobaeza pedrobaeza merged commit fa3791e into OCA:11.0 Apr 23, 2018
@asaunier asaunier deleted the 11.0-mig-stock_available branch April 23, 2018 15:26
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.