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

fix: incorrect bin qty on backdated reconciliation #28588

Merged
merged 2 commits into from
Dec 20, 2021

Conversation

ankush
Copy link
Member

@ankush ankush commented Nov 27, 2021

Bin quantities are updated by adding actual_qty from current SLE to
Bin's qty. In stock reconciliation qty is "reset" hence
qty_after_transaction is used to set qty in bin.

However, in the case of backdated stock reconciliation, there are future
transactions that will affect the final sum of quantity available in bin.

Note: This gets auto-corrected after reposting finishes for backdated
transactions, but Bin shows wrong quantity till then. Bin is used in item
dashboard hence it's confusing. Also, reposting is only supposed to be
for valuation, qty should be correct in real-time.

Solution: Instead of overriding Bin's quantity, replace it with last sle qty in case of backdated transactions.

To test:

  1. Pick one item - warehouse combination.
  2. Create some stock transactions.
  3. Open Bin and verify that current quantity is correct.
  4. Create a backdated stock reconciliation.
  5. Confirm that Bin's quantity matches with stock ledger and stock
    balance. (earlier this used to get reset to whatever stock reco
    quantity was)
  6. Cancel the stock reconciliation and verify stock ledger/balance
    again.

Example transactions ordered by creation; more test cases on #26158

Transaction Posting date Qty qty in Bin
Create A 0 +1 1
Create B -1 2 (reset via stock reco) 3 (this used to be 2 before)
Cancel B -1 0 1 (this used to be 0 before)

@codecov
Copy link

codecov bot commented Nov 27, 2021

Codecov Report

Merging #28588 (58cc6d3) into develop (66a10c0) will increase coverage by 0.04%.
The diff coverage is 85.71%.

❗ Current head 58cc6d3 differs from pull request most recent head f58728a. Consider uploading reports for the commit f58728a to get more accurate results

@@             Coverage Diff             @@
##           develop   #28588      +/-   ##
===========================================
+ Coverage    55.59%   55.64%   +0.04%     
===========================================
  Files         1128     1128              
  Lines        67285    67286       +1     
===========================================
+ Hits         37404    37438      +34     
+ Misses       29881    29848      -33     
Impacted Files Coverage Δ
erpnext/stock/doctype/bin/bin.py 87.50% <83.33%> (+0.22%) ⬆️
erpnext/stock/stock_ledger.py 85.39% <100.00%> (+0.36%) ⬆️
...payroll/doctype/income_tax_slab/income_tax_slab.py 83.33% <0.00%> (-16.67%) ⬇️
.../hr/doctype/upload_attendance/upload_attendance.py 44.69% <0.00%> (-1.52%) ⬇️
.../report/stock_projected_qty/stock_projected_qty.py 88.15% <0.00%> (-1.32%) ⬇️
erpnext/support/doctype/issue/issue.py 64.03% <0.00%> (-0.99%) ⬇️
erpnext/stock/report/stock_ledger/stock_ledger.py 77.23% <0.00%> (-0.82%) ⬇️
erpnext/selling/doctype/sales_order/sales_order.py 77.58% <0.00%> (-0.39%) ⬇️
erpnext/stock/get_item_details.py 79.29% <0.00%> (-0.16%) ⬇️
...stock/doctype/purchase_receipt/purchase_receipt.py 90.64% <0.00%> (+0.26%) ⬆️
... and 10 more

@marination marination added the needs-tests This PR needs automated unit-tests. label Nov 29, 2021
@ankush
Copy link
Member Author

ankush commented Nov 29, 2021

this (and all other cases of bin update) are broken in the case of intermediate stock reconciliation. refer: #26158

@ankush ankush modified the milestones: v13.16, v13.17 Nov 29, 2021
@ankush ankush marked this pull request as draft November 30, 2021 14:04
@ankush ankush removed the dont-merge label Dec 3, 2021
@ankush ankush modified the milestones: v13.17, v13.18 Dec 15, 2021
@ankush ankush marked this pull request as ready for review December 20, 2021 09:42
@ankush ankush added dont-merge CI-failing Unit tests or patch tests are failing. and removed needs-tests This PR needs automated unit-tests. dont-merge labels Dec 20, 2021
When making a backdated transactions current balance qty depends on
evaluation of whole ledger inbetween, instead of doing that just fetch
the last sle's qty_after_transaction when future transactions are
detected against SLE

fix: don't update bin's actual_qty

1. it's already updated by repost_current_voucher
2. update if future sle exists
@ankush ankush removed the CI-failing Unit tests or patch tests are failing. label Dec 20, 2021
@ankush
Copy link
Member Author

ankush commented Dec 20, 2021

uncovered lines intentionally don't run in the tests. coverage is 100% actually 😄

@ankush ankush merged commit 2bb7bca into frappe:develop Dec 20, 2021
@ankush ankush deleted the stock_reco_bin_qty branch December 20, 2021 14:35
ankush added a commit that referenced this pull request Dec 20, 2021
…-28588

fix: incorrect bin qty on backdated reconciliation (backport #28588)
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.

None yet

2 participants