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: item-warehouse based reposting #28124

Merged
merged 10 commits into from
Nov 25, 2021
Merged

Conversation

ankush
Copy link
Member

@ankush ankush commented Oct 28, 2021

Motivations:

  1. When you're doing a series of backdated transactions, each transaction causes reposting and some of it is duplicate work.

Example backdated transactions:

Transaction Posting Time Submission Time
A 0 5
B 1 6
C 2 7
D 3 8
E 4 9

Assuming all operating on the same set of items (or some shared items)

A causes reposting of A, B, C, D, E.
B causes reposting of B, C, D, E
C causes reposting of C, D and E, and so on... Ideally, the system should just figure out that reposting A is all that's required.

  1. During the last few months me, @rohitwaghchaure and @marination have experimented many times and found that item-warehouse based reposting on big sites is orders of magnitude faster. There are a few gotchas related to perf but those can be addressed separately.

Changes:

  • Allow creating item-warehouse based reposting records instead of voucher-based.
  • Make this configurable via repost settings (will be dropped in future, this is internal API, hence not a breaking change.)
  • Enforce uniqueness of item-wh-queued state. New status "Skipped" added on Repost Item Valuation for reposts that are skipped because of this.
  • Multifield index on item-warehouse for speeding up deduplication query.
  • Tests
    • creation of item-wh reposts
    • deduplication

WIP: Deferred to a separate PR

  • batch GL reposting for a voucher.
  • Handle dependent SLE concept (FG, repack)
Should I enable this?

No, if you don't understand how Stock Reposting works in ERPNext then avoid using any of this. These changes will become the default in the coming months anyway.

'tsa zoke

How to test:

  1. go to stock reposting settings and enable "item based reposting"
  2. Pick 2-3 items, create stock transactions for each of them (in/out etc)
  3. Create 2-3 backdated transaction for each item-warehoue combination.
  4. Check "Repost Item Valuation". There should be a repost item valuation record for each combination of Item and warehouse in backdated transactions.
Testing deduplication:
  1. Create a repost item valuation manually for an item-warehouse.
  2. Create another repost item valuation with same values but with future posting time.
  3. go to "scheduled job" and trigger "repost_entries" scheduled job manually.
  4. check repost item valuations again, the one with newer posting date should be "Skipped".

@ankush ankush changed the title refactor: item-warehouse based refactoring refactor: item-warehouse based reposting Oct 28, 2021
@codecov
Copy link

codecov bot commented Oct 28, 2021

Codecov Report

Merging #28124 (3e777bc) into develop (ab2c1f6) will increase coverage by 0.01%.
The diff coverage is 82.97%.

@@             Coverage Diff             @@
##           develop   #28124      +/-   ##
===========================================
+ Coverage    55.29%   55.31%   +0.01%     
===========================================
  Files         1119     1119              
  Lines        66636    66672      +36     
===========================================
+ Hits         36848    36879      +31     
- Misses       29788    29793       +5     
Impacted Files Coverage Δ
...ype/repost_item_valuation/repost_item_valuation.py 66.08% <68.42%> (-0.58%) ⬇️
erpnext/controllers/stock_controller.py 90.88% <92.85%> (+0.09%) ⬆️
...tch_item_expiry_status/batch_item_expiry_status.py 69.23% <0.00%> (-25.00%) ⬇️
...wise_balance_history/batch_wise_balance_history.py 68.96% <0.00%> (-24.14%) ⬇️
...eport/item_variant_details/item_variant_details.py 84.33% <0.00%> (-3.62%) ⬇️
...xt/stock/report/stock_analytics/stock_analytics.py 91.08% <0.00%> (-2.98%) ⬇️
erpnext/stock/report/stock_ageing/stock_ageing.py 89.05% <0.00%> (-2.19%) ⬇️
.../hr/doctype/upload_attendance/upload_attendance.py 44.69% <0.00%> (-1.52%) ⬇️
.../report/stock_projected_qty/stock_projected_qty.py 86.84% <0.00%> (-1.32%) ⬇️
erpnext/accounts/deferred_revenue.py 75.11% <0.00%> (-0.89%) ⬇️
... and 21 more

@ankush ankush force-pushed the item_wh_reposting branch 3 times, most recently from d1eef5e to 0f36460 Compare November 2, 2021 10:43
@ankush ankush added the needs-tests This PR needs automated unit-tests. label Nov 2, 2021
@ankush ankush marked this pull request as ready for review November 2, 2021 10:44
@ankush ankush force-pushed the item_wh_reposting branch 2 times, most recently from 3a99511 to cfdd895 Compare November 2, 2021 12:50
@ankush ankush added backport version-13-hotfix and removed needs-tests This PR needs automated unit-tests. labels Nov 2, 2021
@ankush ankush added this to In progress in ERPNext Roadmap via automation Nov 3, 2021
@ankush ankush added the dont squash Don't squash and merge label Nov 5, 2021
@ankush
Copy link
Member Author

ankush commented Nov 7, 2021

Deduplication here can be moved to the background job and done while processing the queue. This has two benefits:

  1. Since the queue is sorted by posting time, there is no need to compare the dates. Any future duplicate repost can be skipped. This is simpler to implement.
  2. 2 fewer queries while submitting documents.

Edit: this change is done now.

@ankush ankush modified the milestones: v13.15, v13.16 Nov 15, 2021
In current implementation selecting Item-Warehouse based reposting is
better for few users, who don't use depenent SLEs but have frequent
transactions involving same items.

This change lets them switch to item-warehouse based reposting if
required.

Only use this if you understand technicalities of stock reposting. This
is experimental but will become mainstream in coming days.
Item-WH based reposting requires querying existing similar repost.
Assuming there is only 1 max extra entry with same params just indexing
item-WH is sufficient to speed up the query.
Using basic idea that repost with older posting date will also take care
of subsequent posting dates...

When Item-WH reposts are queued:

1. If another repost with same item-wh but older posting date exists
    then skip current one.
2. If another repost with same item-wh but newer posting date exists
    then skip another one.
@marination marination self-assigned this Nov 25, 2021
@ankush ankush enabled auto-merge (rebase) November 25, 2021 11:07
@ankush ankush disabled auto-merge November 25, 2021 11:07
@ankush ankush merged commit 0a2964d into frappe:develop Nov 25, 2021
ERPNext Roadmap automation moved this from In progress to Done Nov 25, 2021
@ankush ankush deleted the item_wh_reposting branch November 25, 2021 11:07
ankush added a commit that referenced this pull request Nov 25, 2021
…-28124

refactor: item-warehouse based reposting (backport #28124)
@ankush ankush mentioned this pull request Aug 18, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants