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: erase UOMs when changing stock UOM #29062

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

anupamvs
Copy link
Contributor

@anupamvs anupamvs commented Dec 28, 2021

Issue- When item's default stock UOM is changed, the old conversion factors become unusable, erase UOM conversion list upon updating stock UOM.

Screenshot 2022-01-12 at 7 25 45 PM

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #29062 (50f9a4d) into develop (5cda4ea) will decrease coverage by 0.30%.
The diff coverage is 100.00%.

❗ Current head 50f9a4d differs from pull request most recent head d3806db. Consider uploading reports for the commit d3806db to get more accurate results

@@             Coverage Diff             @@
##           develop   #29062      +/-   ##
===========================================
- Coverage    58.06%   57.75%   -0.31%     
===========================================
  Files         1110     1107       -3     
  Lines        68007    67912      -95     
===========================================
- Hits         39489    39225     -264     
- Misses       28518    28687     +169     
Impacted Files Coverage Δ
erpnext/stock/doctype/item/item.py 66.88% <100.00%> (+0.08%) ⬆️
...work_order_stock_report/work_order_stock_report.py 50.00% <0.00%> (-50.00%) ⬇️
...unts/report/purchase_register/purchase_register.py 34.24% <0.00%> (-45.90%) ⬇️
...ext/accounts/report/balance_sheet/balance_sheet.py 36.36% <0.00%> (-21.82%) ⬇️
...e_purchase_register/item_wise_purchase_register.py 58.00% <0.00%> (-20.00%) ⬇️
...em_wise_sales_register/item_wise_sales_register.py 51.36% <0.00%> (-12.28%) ⬇️
...t/accounts/report/sales_register/sales_register.py 73.33% <0.00%> (-7.28%) ⬇️
...next/accounts/doctype/pricing_rule/pricing_rule.py 65.59% <0.00%> (-5.74%) ⬇️
.../manufacturing/report/bom_explorer/bom_explorer.py 94.44% <0.00%> (-5.56%) ⬇️
erpnext/setup/utils.py 84.26% <0.00%> (-5.43%) ⬇️
... and 40 more

@anupamvs
Copy link
Contributor Author

anupamvs commented Dec 30, 2021

@Mergifyio backport version-13-hotfix

self.remove(d)
else:
d.idx = idx
idx += 1
Copy link
Member

Choose a reason for hiding this comment

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

  1. mutating list while iterating over it will lead to runtime errors.
  2. it's simpler to just reset the index instead of this complicated logic. Once all modifications are done...
for idx, row in enumerate(doc.child_table, start=1):
    row.idx = idx

@anupamvs anupamvs requested a review from ankush January 7, 2022 11:57
Copy link
Member

@ankush ankush left a comment

Choose a reason for hiding this comment

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

just realized something very basic.

When stock UOM change, the old conversions don't make sense at all. They all need to be removed 😄

e.g.

  • Nos is default stock UOM
  • 20 Nos = 1 Box
  • You change default stock UOM to Kg and now conversion is 20 Nos = 1KG now.

IMO lets just erase the whole table and show toast that UOM conversions are cleared.

@ankush ankush changed the title fix: item uoms idx issue fix: erase UOMs when changing stock UOM Jan 12, 2022
@ankush ankush added the squash Meant to tell reviewers that this PR should be squashed into a single commit while merging. label Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport version-13-hotfix squash Meant to tell reviewers that this PR should be squashed into a single commit while merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants