-
-
Notifications
You must be signed in to change notification settings - Fork 234
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
[14.0][MIG] stock_inventory_valuation_report #292
[14.0][MIG] stock_inventory_valuation_report #292
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional ok!
This migration was done in order to make use of module report_async to generate report in case system takes a long time to provide inventory valuation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
self.ensure_one() | ||
action = ( | ||
report_type == "xlsx" | ||
and self.env.ref( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add raise_if_not_found=False
for ref function.
products = ( | ||
self.env["product.product"] | ||
.with_context( | ||
dict(to_date=self.date, company_owned=True, create=False, edit=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do without dict
.
with_context(to_date=self.date, company_owned=True, create=False, edit=False)
734f7a2
to
26b2d15
Compare
@OCA/logistics-maintainers good for merge? |
@rafaelbn looks good to you? :) |
/ocabot migration stock_inventory_valuation_report |
@ps-tubtim could you please review o ping a colleage? this is a Ecosoft original module 😄 Thank you! |
@ps-tubtim gentle reminder :) |
@simahawk could we go? :) |
@sebalix could you take a look at this? thank you! |
@OCA/logistics-maintainers gentle reminder |
@@ -1 +1,3 @@ | |||
* Pimolnat Suntian <[email protected]> | |||
* Ooops404 <https://ooops404.com> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put contributors' names. You can add a company as title.
"cost_method": product.cost_method, | ||
} | ||
if product.qty_at_date != 0: | ||
if product.qty_available != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@solo4games Shouldn't it be 'quantity_svl' field instead ? See stock_account/models/product.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about that, because, when I did it, one of the tests were failed because it did not pass that if condition, so I can not said that I am right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition should be dropped as it is already part of the search on products
This PR has the |
26b2d15
to
22c87ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bugs are not related to migration but sill the report seems wrong
) | ||
ReportLine = self.env["stock.inventory.valuation.view"] | ||
for product in products: | ||
standard_price = product.standard_price |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks wrong. This is not the price at date. It should be value_svl
"name": product.name, | ||
"reference": product.default_code, | ||
"barcode": product.barcode, | ||
"qty_at_date": product.qty_available, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should also likely be quantity_svl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback
You are right
I fix it, please check again
"cost_method": product.cost_method, | ||
} | ||
if product.qty_at_date != 0: | ||
if product.qty_available != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition should be dropped as it is already part of the search on products
a617a3b
to
baafd54
Compare
@jbaudoux good for you? |
d3c3736
to
2203fb2
Compare
a01bcb7
to
acc4e99
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial code review, Still some issues IMO
access_report_stock_inventory_valuation_report,report.stock.inventory.valuation.report,model_report_stock_inventory_valuation_report,base.group_user,1,1,1,1 | ||
access_stock_inventory_valuation_view,stock_inventory_valuation_view,model_stock_inventory_valuation_view,base.group_user,1,1,1,1 | ||
access_report_s_i_v_r_report_stock_inventory_valuation_report_xlsx,report.s_i_v_r.report_stock_inventory_valuation_report_xlsx,model_report_s_i_v_r_report_stock_inventory_valuation_report_xlsx,base.group_user,1,1,1,1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: IMO base.group_user
is too broad. At the very least, we should restrict access to stock.group_stock_user
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I'll fix it
compute_at_date = fields.Selection( | ||
[("0", "Current Inventory"), ("1", "At a Specific Date")], | ||
string="Compute", | ||
help="Choose to analyze the current inventory or from a specific date in the past.", | ||
default="0", | ||
) | ||
inventory_datetime = fields.Datetime( | ||
"Inventory at Date", | ||
help="Choose a date to get the inventory at that date", | ||
default=fields.Datetime.now, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: I still don't understand why there's a need to re-declare these fields instead of using the fields that already exist.
Cf. the module on v12 that we are porting to v14: https://github.com/OCA/stock-logistics-reporting/blob/12.0/stock_inventory_valuation_report/wizard/stock_quantity_history.py
default=fields.Datetime.now, | ||
) | ||
|
||
def open_table(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: same as above - why is this function being declared again (from Odoo v12) instead of using open_at_date
which is available in Odoo v14?
Please look at the module on v12 https://github.com/OCA/stock-logistics-reporting/blob/12.0/stock_inventory_valuation_report/wizard/stock_quantity_history.py
and the base wizard in stock
and stock_account
https://github.com/OCA/OCB/blob/14.0/addons/stock/wizard/stock_quantity_history.py#L8
https://github.com/OCA/OCB/blob/14.0/addons/stock_account/wizard/stock_quantity_history.py
acc4e99
to
6b19bc7
Compare
@francesco-ooops , please do a functional review) |
@dessanhemrayev PDF report don't seem to work on runboat |
fd55d02
to
ec7f098
Compare
@francesco-ooops, I fixed it |
@aleuffre, please check) |
@dessanhemrayev if that's ok for you we can take over this task, thanks for the work done so far! |
@rousseldenis this can be closed |
No description provided.