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

fix: Dont set idx while adding WO items to Stock Entry #30377

Merged
merged 6 commits into from
Mar 30, 2022
Merged

fix: Dont set idx while adding WO items to Stock Entry #30377

merged 6 commits into from
Mar 30, 2022

Conversation

marination
Copy link
Collaborator

@marination marination commented Mar 23, 2022 •
edited
Loading

Issue:

  • Create a BOM of the foll.g structure:

    Orange Juice : [
     Double Dozen Egg, 
     Parent BOM Item: [Child Item 1]
 ]
    Screenshot 2022-03-29 at 1 08 22 PM

  • Now create a WO, with Skip Material Transfer = 1

  • Finish the WO, the Manufacture Stock Entry will have wrong idx
    2022-03-29 13 10 58

Fix:

  • In add_stock_entry_detail(), remove idx mapping
  • idx must be auto-computed by base document's self.append() function, so do not set it
  • Remove redundant idx querying and useless idx unsetting
  • idx is fetched in the query for server side sorting
    2022-03-30 10 23 11

ToDo:

  • Manually Test impact areas
  • Check if selecting idx is required in query and remove
  • Add test for idx in SE

@marination
fix: Dont set idx while adding WO items to Stock Entry
a787ebb 
- `idx` must be computed by base document's `self.append()` function, so do not set it
@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Mar 23, 2022
@codecov
Copy link

codecov bot commented Mar 23, 2022 •
edited
Loading

Codecov Report

Merging #30377 (b5ad626) into develop (760e68b) will increase coverage by 24.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##           develop   #30377       +/-   ##
============================================
+ Coverage    36.80%   60.83%   +24.02%     
============================================
  Files         1081     1082        +1     
  Lines        68920    69059      +139     
============================================
+ Hits         25367    42010    +16643     
+ Misses       43553    27049    -16504
Impacted Files Coverage Δ
erpnext/manufacturing/doctype/bom/bom.py 87.61% <ø> (+17.97%) ⬆️
erpnext/stock/doctype/stock_entry/stock_entry.py 81.02% <ø> (+17.12%) ⬆️
...item_wise_sales_history/item_wise_sales_history.py 32.35% <100.00%> (+32.35%) ⬆️
...payroll/doctype/income_tax_slab/income_tax_slab.py 83.33% <0.00%> (-16.67%) ⬇️
erpnext/e_commerce/product_data_engine/query.py 75.36% <0.00%> (ø)
erpnext/hr/doctype/expense_claim/expense_claim.py 82.91% <0.00%> (+0.50%) ⬆️
erpnext/accounts/report/tax_detail/tax_detail.py 89.89% <0.00%> (+0.50%) ⬆️
...next/accounts/doctype/subscription/subscription.py 82.69% <0.00%> (+0.54%) ⬆️
.../hr/doctype/leave_application/leave_application.py 82.14% <0.00%> (+0.59%) ⬆️
..._scorecard_variable/supplier_scorecard_variable.py 25.54% <0.00%> (+0.72%) ⬆️
... and 445 more

ankush and others added 2 commits March 28, 2022 20:21
@ankush
Merge branch 'develop' into stock-entry-items-idx
40d7418
@marination
chore: Remove redundant idx query and value setting
639d380 
- idx can be removed from `select_columns` as it is already in the main query
- setting idx to '' is not required as it is not used further
@marination marination force-pushed the stock-entry-items-idx branch 3 times, most recently from af152cc to 639d380 Compare March 29, 2022 07:32
@marination marination marked this pull request as ready for review March 29, 2022 08:24
@marination marination added backport version-13-hotfix and removed needs-tests This PR needs automated unit-tests. labels Mar 29, 2022
@marination marination merged commit 7e719a1 into frappe:develop Mar 30, 2022
@mergify mergify bot mentioned this pull request Mar 30, 2022
Merged
ankush added a commit that referenced this pull request Mar 30, 2022
@mergify @marination @ankush
fix: Dont set idx while adding WO items to Stock Entry (backport #3…
46e6d16 
…0377) (#30485)

* fix: Dont set `idx` while adding WO items to Stock Entry

- `idx` must be computed by base document's `self.append()` function, so do not set it

(cherry picked from commit a787ebb)

# Conflicts:
#	erpnext/stock/doctype/stock_entry/stock_entry.py

* chore: Remove redundant idx query and value setting

- idx can be removed from `select_columns` as it is already in the main query
- setting idx to '' is not required as it is not used further

(cherry picked from commit 639d380)

# Conflicts:
#	erpnext/stock/doctype/stock_entry/stock_entry.py

* test: idx mapping correctness

(cherry picked from commit fa3b953)

* fix: Linter

(cherry picked from commit b5ad626)

* fix: resolve conflicts

Co-authored-by: marination <maricadsouza221197@gmail.com>
Co-authored-by: Ankush Menat <ankush@frappe.io>
@deepeshgarg007 deepeshgarg007 mentioned this pull request Apr 6, 2022
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rohitwaghchaure rohitwaghchaure Awaiting requested review from rohitwaghchaure rohitwaghchaure is a code owner

@ankush ankush Awaiting requested review from ankush

Assignees
No one assigned
Labels
backport version-13-hotfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marination @ankush