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: Add non-existent Item check and cleanup in validate_for_items #30509

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

marination
Copy link
Collaborator

Issue:

Traceback (most recent call last):
  File "apps/frappe/frappe/app.py", line 68, in application
    response = frappe.api.handle()
  File "apps/frappe/frappe/api.py", line 55, in handle
    return frappe.handler.handle()
  File "apps/frappe/frappe/handler.py", line 31, in handle
    data = execute_cmd(cmd)
  File "apps/frappe/frappe/handler.py", line 68, in execute_cmd
    return frappe.call(method, **frappe.form_dict)
  File "apps/frappe/frappe/__init__.py", line 1213, in call
    return fn(*args, **newargs)
  File "apps/frappe/frappe/desk/form/save.py", line 21, in savedocs
    doc.save()
  File "apps/frappe/frappe/model/document.py", line 287, in save
    return self._save(*args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 309, in _save
    return self.insert()
  File "apps/frappe/frappe/model/document.py", line 240, in insert
    self.run_before_save_methods()
  File "apps/frappe/frappe/model/document.py", line 971, in run_before_save_methods
    self.run_method("validate")
  File "apps/frappe/frappe/model/document.py", line 869, in run_method
    out = Document.hook(fn)(self, *args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 1161, in composer
    return composed(self, method, *args, **kwargs)
  File "apps/frappe/frappe/model/document.py", line 1144, in runner
    add_to_return_value(self, fn(self, *args, **kwargs))
  File "apps/frappe/frappe/model/document.py", line 866, in fn
    return method_object(*args, **kwargs)
  File "apps/erpnext/erpnext/buying/doctype/purchase_order/purchase_order.py", line 62, in validate
    validate_for_items(self)
  File "apps/erpnext/erpnext/buying/utils.py", line 65, in validate_for_items
    d.item_code, as_dict=1)[0]
IndexError: list index out of range

Fix:

  • Added a validation if invalid item code is passed via data import/API, etc.
  • Used DB APIs instead of raw sql
  • Separated checks into separate functions
  • Added return types to functions
  • Better variable naming and removed redundant utils import in function

- Added a validation if invalid item code ia passed via data import/API, etc.
- Used DB APIs instead of raw sql
- Separated checks into separate functions
- Added return types to functions
- Better variable naming and removed redundant utils import in function
@github-actions github-actions bot added buying needs-tests This PR needs automated unit-tests. labels Mar 31, 2022
@marination
Copy link
Collaborator Author

skipping tests because this is a defensive fix. Could not replicate this

@marination marination removed the needs-tests This PR needs automated unit-tests. label Mar 31, 2022
@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #30509 (93f6346) into develop (b981fae) will increase coverage by 8.57%.
The diff coverage is 92.00%.

@@             Coverage Diff             @@
##           develop   #30509      +/-   ##
===========================================
+ Coverage    52.28%   60.86%   +8.57%     
===========================================
  Files         1082     1082              
  Lines        69067    69076       +9     
===========================================
+ Hits         36114    42043    +5929     
+ Misses       32953    27033    -5920     
Impacted Files Coverage Δ
erpnext/buying/utils.py 75.80% <92.00%> (+4.10%) ⬆️
...rial_no_valuation/incorrect_serial_no_valuation.py 85.96% <0.00%> (-10.53%) ⬇️
...rpnext/accounts/doctype/shareholder/shareholder.py 80.00% <0.00%> (-10.00%) ⬇️
...urity_shortfall/process_loan_security_shortfall.py 93.75% <0.00%> (-6.25%) ⬇️
...eorder_level/itemwise_recommended_reorder_level.py 88.88% <0.00%> (-3.71%) ⬇️
erpnext/hr/doctype/job_applicant/job_applicant.py 63.93% <0.00%> (-3.28%) ⬇️
...e_sales_analytics/supplier_wise_sales_analytics.py 86.88% <0.00%> (-3.28%) ⬇️
erpnext/stock/doctype/warehouse/warehouse.py 82.78% <0.00%> (-1.64%) ⬇️
.../report/stock_projected_qty/stock_projected_qty.py 86.84% <0.00%> (-1.32%) ⬇️
.../support/report/issue_analytics/issue_analytics.py 88.98% <0.00%> (-0.85%) ⬇️
... and 177 more

@marination marination merged commit 901d8ea into frappe:develop Mar 31, 2022
marination added a commit that referenced this pull request Mar 31, 2022
…-30509

fix: Add non-existent Item check and cleanup in `validate_for_items` (backport #30509)
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant