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

[MIG][8.0] replenishment_cost + product_cost_incl_bom #30

Closed
wants to merge 2 commits into from

Conversation

SodexisTeam
Copy link
Member

No description provided.

@pedrobaeza pedrobaeza mentioned this pull request Apr 22, 2016
10 tasks
@atchuthan atchuthan force-pushed the 8.0_replenishment_cost_with_bom branch from 18dd17f to 0398e26 Compare April 26, 2016 11:56
@coveralls
Copy link

coveralls commented Apr 26, 2016

Coverage Status

Coverage decreased (-7.4%) to 88.618% when pulling 0398e26 on sodexis:8.0_replenishment_cost_with_bom into 617bc02 on OCA:8.0.

README.md Outdated
[product_replenishment_cost](product_replenishment_cost/) | 8.0.2.0.0 | Replenishment cost
[product_standard_margin](product_standard_margin/) | 8.0.2.0.0 | Product Margin and Margin Rate

Unported addons
---------------
addon | version | summary
--- | --- | ---
[product_cost_incl_bom](product_cost_incl_bom/) | 1.0 (unported) | Product Cost incl. BOM

Choose a reason for hiding this comment

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

This file is updated automatically by an OCA script.

cost = 0
if product.id in res.keys():
cost = res[product.id]
product.replenishment_cost = cost

Choose a reason for hiding this comment

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

Maybe product.replenishment_cost = res.get(product.id, 0) ?


from . import product_product
from . import product_product, product_template

Choose a reason for hiding this comment

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

Please split one import per line.


replenishment_cost = fields.Float(
compute=_get_replenishment_cost, store=True,

Choose a reason for hiding this comment

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

Why did you remove the store attribute ?

Copy link
Member

Choose a reason for hiding this comment

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

I had 2 reasons to remove store=True

  1. The replenishment cost field defined in product_replenishment_cost module with store=True, creates the field in database with the value of product's standard_price and the value is not recomputed at the time of product_cost_incl_bom module installation.
  2. Replenishment cost needs to be changed for each change in bom lines of product, when there is a change in their replenishment cost value and it did not work in that case as the method was not triggered for store=True. (when we change the BOM directly, it did not trigger the compute method and it does not allow multi-level BOM)

Copy link
Member

Choose a reason for hiding this comment

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

@sylvain-garancher please check the points I have listed out.

Choose a reason for hiding this comment

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

Ok, I see, and I don't find a trigger for that. With the explanation, it's fine.

If the issue is with the other module, maybe you can leave store=True here, and explicitely change to store=False in the other module ? (with a comment explaining why)

@api.multi
@api.depends('standard_price')
def _get_replenishment_cost(self):
replenishment_cost = 0.0

Choose a reason for hiding this comment

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

If a template has no variant, it will pick the value computed for the previous template in the loop.
You should initialize the variable in the for loop.

def _get_replenishment_cost(self):
replenishment_cost = 0.0
for template in self:
variants = template.mapped('product_variant_ids')

Choose a reason for hiding this comment

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

variants = template.product_variant_ids is enough here, mapped is useful when you work on multiple records, or through multiple fields.

@atchuthan atchuthan force-pushed the 8.0_replenishment_cost_with_bom branch 3 times, most recently from e29e4e0 to 0bf1e71 Compare November 4, 2016 11:34
@atchuthan
Copy link
Member

@sylvain-garancher corrections done

Copy link
Member

@dvdhinesh dvdhinesh 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 👍

@guewen
Copy link
Member

guewen commented Mar 31, 2017

Restarted Travis and I don't get why the builds are shown as failed whereas the builds exit with 0.

@atchuthan atchuthan force-pushed the 8.0_replenishment_cost_with_bom branch from 0bf1e71 to 2a11559 Compare July 27, 2017 07:50
@legalsylvain
Copy link
Contributor

closing.
replenishment_cost is available in V8 and in V10.

You can also review the PR for V12 here : #50

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.

9 participants