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.

Sign up for GitHub

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

Jump to bottom

[16.0][ADD] stock_valuation_specific_identification #2139

Open
wants to merge 14 commits into
base: 16.0
Choose a base branch
from
Open

[16.0][ADD] stock_valuation_specific_identification #2139

wants to merge 14 commits into from

Conversation

matt454357
Copy link

New module to add Specific Identification Inventory Valuation Method.

@matt454357 matt454357 force-pushed the 16.0-add-stock_valuation_specific_identification branch from 7fcb654 to 7349ed2 Compare August 14, 2024 20:51
@yostashiro
Copy link
Member

I think this module shares a similar objective with OCA/stock-logistics-workflow#1527.

One major difference, I understand, is that this module creates an SVL record per serial and the other one (stock_valuation_fifo_lot) does not (multiple lots/serials can be linked to an SVL record). Do you see a conceptual issue with the approach of the other module?

@matt454357 matt454357 force-pushed the 16.0-add-stock_valuation_specific_identification branch from 7349ed2 to 84b204c Compare August 15, 2024 15:03
@matt454357
Copy link
Author

Hi @yostashiro,

Thanks for pointing out stock_valuation_fifo_lot. I had not noticed it.

I like the approach taken in stock_valuation_fifo_lot, but it doesn't correctly handle revaluations.

I believe stock_valuation_specific_identification better describes the module. There are similar 3rd party modules with similar descriptions (https://apps.odoo.com/apps/modules/browse?search=specific+identification). However, if stock_valuation_fifo_lot is the preferred name, I can move the changes to that module.

AungKoKoLin1997
AungKoKoLin1997 reviewed Aug 19, 2024
View reviewed changes
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 left a comment

Choose a reason for hiding this comment

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

Partial code review.

stock_valuation_specific_identification/models/stock_lot.py Outdated
_inherit = "stock.lot"

specific_ident_cost = fields.Boolean(
related="product_id.categ_id.property_specific_ident_cost",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
related="product_id.categ_id.property_specific_ident_cost",
related="product_id.property_specific_ident_cost",

stock_valuation_specific_identification/models/product.py Outdated
class ProductProduct(models.Model):
_inherit = "product.product"

specific_ident_cost = fields.Boolean(
Copy link
Contributor

Choose a reason for hiding this comment

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

I failed to understand to add this field in product.product model with readonly instead of just using from product.template

stock_valuation_specific_identification/models/stock_lot.py Outdated
"""Prepare the values for a stock valuation layer created by a delivery.
This should only be called for specific identification valuation method.

:param quantity: the quantity to value, expressed in `product_id..uom_id`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:param quantity: the quantity to value, expressed in `product_id..uom_id`
:param quantity: the quantity to value, expressed in `product_id.uom_id`

stock_valuation_specific_identification/models/stock_lot.py Outdated
Comment on lines 108 to 109
quantity = -1 * quantity # negative for out valuation layers
sid_vals = self._run_out_spec_ident(abs(quantity), company)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
quantity = -1 * quantity # negative for out valuation layers
sid_vals = self._run_out_spec_ident(abs(quantity), company)
sid_vals = self._run_out_spec_ident(quantity, company)

stock_valuation_specific_identification/models/stock_lot.py Outdated
"product_id": self.product_id.id,
"value": sid_vals.get("value"),
"unit_cost": sid_vals.get("unit_cost"),
"quantity": quantity,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"quantity": quantity,
"quantity": -1 * quantity,

stock_valuation_specific_identification/models/product.py Outdated
Comment on lines 95 to 102
for (
candidate_line
) in candidate.stock_move_id.move_line_ids.filtered(
lambda x: x.lot_id == lot and x.remaining_qty > 0.0
):
qty_taken_on_candidate = min(
qty_to_take_on_candidates, candidate_line.remaining_qty
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this part. candidate_line is an instance of stock move line, isn't? I don't think stock move line record has remaining_qty field. Am I missing something?

stock_valuation_specific_identification/models/stock_lot.py

# Find back incoming stock valuation layers (called candidates here) to
# value `quantity`.
qty_to_take_on_candidates = quantity
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for the purpose of code readable?

@matt454357 matt454357 force-pushed the 16.0-add-stock_valuation_specific_identification branch from 84b204c to b1d111d Compare August 19, 2024 17:21
mymage and others added 13 commits August 19, 2024 15:39
@mymage @matt454357
Translated using Weblate (Italian)
e93a8e9 
Currently translated at 100.0% (185 of 185 strings)

Translation: stock-logistics-warehouse-16.0/stock-logistics-warehouse-16.0-stock_reserve
Translate-URL: https://translation.odoo-community.org/projects/stock-logistics-warehouse-16-0/stock-logistics-warehouse-16-0-stock_reserve/it/
@rousseldenis @matt454357
[12.0][ADD] stock_production_lot_quantity_tree
7800f9b
@oca-travis @matt454357
[UPD] Update stock_production_lot_quantity_tree.pot
94fc268
@manuelcalerosolis @matt454357
[MIG] stock_production_lot_quantity_tree: Migration to 13.0
7d91652
@oca-travis @matt454357
[UPD] Update stock_production_lot_quantity_tree.pot
f764474
@OCA-git-bot @matt454357
[UPD] README.rst
1b1f9c5
@OCA-git-bot @matt454357
[UPD] README.rst
259e7a3
@AdrianaSaiz @matt454357
[IMP] stock_production_lot_quantity_tree: pre-commit stuff
4d4dcc4
@AdrianaSaiz @matt454357
[MIG] stock_production_lot_quantity_tree: Migration to 16.0
4e0e212
@matt454357
[UPD] Update stock_production_lot_quantity_tree.pot
0c171ec
@OCA-git-bot @matt454357
[BOT] post-merge updates
21c5d4b
@chienandalu @matt454357
[FIX] stock_vlm_mgmt: location general inventory
5658d59 
We were lacking the default location id so we could select the tray. It
was also needed to avoid compting a pos when no tray is selected.
@OCA-git-bot @matt454357
[BOT] post-merge updates
cc3addc
@matt454357 matt454357 force-pushed the 16.0-add-stock_valuation_specific_identification branch 2 times, most recently from 912701c to 3f70122 Compare August 19, 2024 22:03
@AungKoKoLin1997
Copy link
Contributor

@matt454357 There are some changes and commits that are not related to the PR. It seems like something went wrong during your rebase.

@AungKoKoLin1997
Copy link
Contributor

@matt454357 I decided to go with stock_valuation_fifo_lot for two reasons:

  • The community is more likely to accept an existing module than a new one.
  • In my opinion, this module's approach is simpler and involves fewer complete overrides of Odoo's standard functions.

I will create a new PR with some adjustments since the author of the existing PR has not followed up on it. Would you be interested in contributing your ideas (e.g., the revaluation process) after I create the PR?

@rousseldenis rousseldenis added this to the 16.0 milestone Aug 20, 2024
@matt454357
Copy link
Author

Hi @AungKoKoLin1997,

It has been a long time since I last did a PR. I shouldn't have merged the upstream changes made after starting my PR. I thought it might fix a test that was failing in stock_valuation_layer_accounting_date. It seems stock_valuation_layer_accounting_date has a poorly written test that sometimes fails, depending on the timezone and time of day.

Anyway, I am happy to contribute to the stock_valuation_fifo_lot module. I would like to add more than just the revaluation stuff. For example, drop the stock_no_negative requirement, and enforce no-negative for specific identification moves only.

Revaluation is accessed from the valuation tree view, after grouping by lot/serial. Therefore, we may want to change stock_valuation_fifo_lot to a single lot/serial per SVL record, rather than multiple per SVL record.

@matt454357
[16.0][ADD] stock_valuation_specific_identification
bd806b3 
New module to add Specific Identification Inventory Valuation Method.
@matt454357 matt454357 force-pushed the 16.0-add-stock_valuation_specific_identification branch from 3f70122 to bd806b3 Compare August 20, 2024 16:27
@AungKoKoLin1997
Copy link
Contributor

Anyway, I am happy to contribute to the stock_valuation_fifo_lot module. I would like to add more than just the revaluation stuff. For example, drop the stock_no_negative requirement, and enforce no-negative for specific identification moves only.

@matt454357 Thank you for your interest. I have created draft state PR OCA/stock-logistics-workflow#1676 and I have to add test cases and consider fixing issue that is proposed OCA/stock-logistics-workflow#1350 (comment). Let me ping you when it is ready.

@AungKoKoLin1997 AungKoKoLin1997 mentioned this pull request Aug 21, 2024
Open
@AungKoKoLin1997 AungKoKoLin1997 mentioned this pull request Sep 20, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AungKoKoLin1997 AungKoKoLin1997 AungKoKoLin1997 left review comments

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
10 participants
@matt454357 @yostashiro @AungKoKoLin1997 @rousseldenis @mymage @oca-travis @manuelcalerosolis @OCA-git-bot @AdrianaSaiz @chienandalu