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: Use get for conditionally available fields while setting missing values #29426

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

marination
Copy link
Collaborator

@marination marination commented Jan 24, 2022

To Replicate Issue:

  • Add custom field supplier in Material Request
  • Create a new MR > Save > watch the world burn

Issue:

  • Due to custom field "supplier" and missing field "supplier_address", dot operator breaks in buying_controller > set_missing_values > get_party_details
     File "apps/erpnext/erpnext/controllers/buying_controller.py", line 74, in set_missing_values
       doctype=self.doctype, company=self.company, party_address=self.supplier_address, 
       shipping_address=self.get('shipping_address'),
     AttributeError: 'MaterialRequest' object has no attribute 'supplier_address'
  • This scenario is brought to you by customisation

Fix:

  • Make sure to use get instead of just dot operator if field is in some doctypes and not all

Ambiguity:

  • Adding such custom fields with core field names, triggers JS functions also that technically aren't supposed to be triggered. The user won't have a clue. Not sure how to go about that

… values

- Due to custom field "supplier" and missing field "supplier_address", dot operator breaks
- Make sure to use "get" instead of just dot operator if field is in some doctypes, not all
@marination marination added the Skip Manual Testing The changes in this PR does not require manual testing label Jan 24, 2022
@github-actions github-actions bot added buying needs-tests This PR needs automated unit-tests. labels Jan 24, 2022
@marination marination removed the needs-tests This PR needs automated unit-tests. label Jan 24, 2022
@marination
Copy link
Collaborator Author

marination commented Jan 24, 2022

Skipping tests here, its a defensive fix

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #29426 (78b6b29) into develop (108d104) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #29426      +/-   ##
===========================================
- Coverage    57.99%   57.91%   -0.08%     
===========================================
  Files         1091     1091              
  Lines        67868    67868              
===========================================
- Hits         39357    39303      -54     
- Misses       28511    28565      +54     
Impacted Files Coverage Δ
erpnext/controllers/buying_controller.py 87.02% <100.00%> (ø)
erpnext/utilities/product.py 14.70% <0.00%> (-35.30%) ⬇️
erpnext/shopping_cart/product_info.py 30.30% <0.00%> (-21.22%) ⬇️
.../manufacturing/report/bom_explorer/bom_explorer.py 94.44% <0.00%> (-5.56%) ⬇️
...ctype/woocommerce_settings/woocommerce_settings.py 80.76% <0.00%> (-3.85%) ⬇️
...eorder_level/itemwise_recommended_reorder_level.py 90.56% <0.00%> (-3.78%) ⬇️
erpnext/stock/report/stock_ledger/stock_ledger.py 74.79% <0.00%> (-3.26%) ⬇️
...e/shopping_cart_settings/shopping_cart_settings.py 66.03% <0.00%> (-1.89%) ⬇️
erpnext/stock/report/stock_ageing/stock_ageing.py 91.32% <0.00%> (-1.74%) ⬇️
erpnext/stock/reorder_item.py 76.06% <0.00%> (-1.71%) ⬇️
... and 20 more

@marination marination added the WIP label Jan 24, 2022
@marination
Copy link
Collaborator Author

Gave it some thought. One way to go about controlling what is triggered where is to have a clear map of fields and documents.
But that is ultra tedious. We use where field is present in a lot of places.
Not worth the effort, ending it at this fix

@marination marination merged commit a707d37 into frappe:develop Jan 25, 2022
@marination marination removed the WIP label Jan 25, 2022
@marination
Copy link
Collaborator Author

@Mergifyio backport version-13-hotfix

@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

backport version-13-hotfix

✅ Backports have been created

nextchamp-saqib added a commit that referenced this pull request Feb 2, 2022
…-29426

fix: Use get for conditionally available fields while setting missing values (backport #29426)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buying Skip Manual Testing The changes in this PR does not require manual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant