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

refactor: move FIFO valuation logic to separate class #28947

Merged
merged 10 commits into from
Dec 20, 2021

Conversation

ankush
Copy link
Member

@ankush ankush commented Dec 18, 2021

Summary: refactor the way valuation methods are applied (not how they work)

Currently "update_entries_after" is a mighty class that carries the whole stock module's most critical business logic.

Changes:

  • valuation.py containing classes for valuation method.
  • Abstract away all the nonsensical internal implementation of FIFO queue to a separate class
  • Make FIFO queue testable without making stock transactions. New FIFO implementation is pure python object with no DB interactions, so it's independently testable. This means it can also be used outside of update_entries_after in other cases where incoming rate is to be determined :)
  • Improve readability of queue implementation. (still sorta WIP)
  • Add unit tests for FIFO queue.
  • returned popped qty and values for better assertions
  • Hypothesis tests! https://hypothesis.works/
  • WIP: further Simplifcation and re-evaluation of existing code.

This is how simple the new update stock looks like (ignoring 1 stupid edge case)

fifo_queue = FifoValuation(self.wh_data.stock_queue)
if actual_qty > 0:
    fifo_queue.add_stock(qty=actual_qty, rate=incoming_rate)
else:
    fifo_queue.remove_stock(qty=abs(actual_qty), rate=outgoing_rate)

stock_qty, stock_value = fifo_queue.get_total_stock_and_value()
self.wh_data.stock_queue = fifo_queue.get_state()

@ankush ankush added refactor tech-debt technical debt, long pending issues labels Dec 18, 2021
@ankush ankush added this to the v14-beta milestone Dec 18, 2021
@codecov
Copy link

codecov bot commented Dec 18, 2021

Codecov Report

Merging #28947 (2f5be0e) into develop (0ca467a) will increase coverage by 0.09%.
The diff coverage is 97.75%.

@@             Coverage Diff             @@
##           develop   #28947      +/-   ##
===========================================
+ Coverage    55.54%   55.64%   +0.09%     
===========================================
  Files         1127     1128       +1     
  Lines        67241    67281      +40     
===========================================
+ Hits         37351    37437      +86     
+ Misses       29890    29844      -46     
Impacted Files Coverage Δ
erpnext/stock/stock_ledger.py 85.21% <93.33%> (-0.88%) ⬇️
erpnext/stock/valuation.py 98.64% <98.64%> (ø)
...e/asset_value_adjustment/asset_value_adjustment.py 86.04% <0.00%> (-3.49%) ⬇️
.../hr/doctype/upload_attendance/upload_attendance.py 44.69% <0.00%> (-1.52%) ⬇️
...ice_creation_tool/opening_invoice_creation_tool.py 60.95% <0.00%> (-0.69%) ⬇️
erpnext/stock/get_item_details.py 79.29% <0.00%> (-0.16%) ⬇️
...xt/accounts/doctype/sales_invoice/sales_invoice.py 78.11% <0.00%> (+0.08%) ⬆️
erpnext/payroll/doctype/salary_slip/salary_slip.py 83.99% <0.00%> (+0.26%) ⬆️
erpnext/projects/doctype/project/project.py 54.09% <0.00%> (+0.35%) ⬆️
erpnext/portal/product_configurator/utils.py 33.46% <0.00%> (+0.40%) ⬆️
... and 13 more

@ankush ankush changed the title refactor: stock valuation methods refactor: move FIFO valuation logic to separate class Dec 18, 2021
@ankush ankush marked this pull request as ready for review December 19, 2021 13:34
@ankush ankush enabled auto-merge (rebase) December 20, 2021 07:49
@ankush ankush disabled auto-merge December 20, 2021 07:49
@ankush ankush merged commit 107b404 into frappe:develop Dec 20, 2021
@ankush ankush deleted the refactor/valuation branch December 20, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor tech-debt technical debt, long pending issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant