Skip to content

Conversation

@mihir-kandoi
Copy link
Contributor

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 31, 2026

📝 Walkthrough

Walkthrough

Selling price validation now runs per-item and per-row, deriving valuation_rate from each line's incoming_rate (when present) and comparing it to base_net_rate with per-item precision. Batch-based valuation_rate_map and batch/warehouse valuation lookups were removed. last_purchase_rate_in_sales_uom conversion uses per-item precision. Validation messages now reference a dynamic Selling Settings label and a generic link to Selling Settings. An integration test test_validate_selling_price was added to Delivery Note tests (appears duplicated).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ❌ 3
❌ Failed checks (1 warning, 2 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title partially relates to the changeset. It mentions 'validation considers moving average' but is cut off and doesn't fully explain the actual changes involving per-item valuation rate handling and selling price validation messaging updates. Complete the title and clarify the main change. Consider: 'fix: use per-item incoming rate for selling price validation instead of moving average' or similar.
Description check ❓ Inconclusive The description only provides a support ticket reference without explaining any aspect of the changeset or its objectives. Add details about what the fix addresses, such as the per-item valuation logic changes and validation behavior improvements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@erpnext/controllers/selling_controller.py`:
- Around line 334-340: The comparison currently uses item.base_net_rate <
item.incoming_rate without numeric coercion; update the check to use flt() on
both sides (e.g., flt(item.base_net_rate) < flt(item.incoming_rate)) so the
comparison is consistent with the earlier flt() usage (see other comparison
flt(item.base_net_rate) < flt(last_purchase_rate_in_sales_uom)) and keep the
throw_message(...) call unchanged; locate this in the same validation block
where item.base_net_rate, item.incoming_rate, and throw_message are used.

@codecov
Copy link

codecov bot commented Jan 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.25%. Comparing base (2d6b43f) to head (eac1c06).
⚠️ Report is 12 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #52246   +/-   ##
========================================
  Coverage    79.24%   79.25%           
========================================
  Files         1168     1168           
  Lines       122699   122695    -4     
========================================
+ Hits         97237    97239    +2     
+ Misses       25462    25456    -6     
Files with missing lines Coverage Δ
erpnext/controllers/selling_controller.py 84.15% <100.00%> (+0.32%) ⬆️
.../stock/doctype/delivery_note/test_delivery_note.py 99.91% <100.00%> (+<0.01%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@erpnext/stock/doctype/delivery_note/test_delivery_note.py`:
- Around line 2866-2881: The test test_validate_selling_price incorrectly sets
dn.items[0].stock_qty (which is recomputed) so it has no effect; change the
assignment to update dn.items[0].qty (or adjust the conversion factor) before
calling dn.save so the incoming rate recalculation is driven by the persisted
quantity; keep references to create_delivery_note, dn.save and
IntegrationTestCase.change_settings as-is.

@mihir-kandoi mihir-kandoi removed the needs-tests This PR needs automated unit-tests. label Feb 2, 2026
@mihir-kandoi mihir-kandoi disabled auto-merge February 2, 2026 04:24
@mihir-kandoi mihir-kandoi force-pushed the st58765 branch 2 times, most recently from 5f4cc5c to 69200a8 Compare February 2, 2026 04:45
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@erpnext/controllers/selling_controller.py`:
- Around line 541-543: The condition directly accesses self._action and can
raise AttributeError on flows where _action isn't set; update the guard to use a
defensive lookup (e.g., replace direct self._action usage in the condition that
currently reads "self._action in ['save', 'submit'] or not d.incoming_rate or
self.is_internal_transfer()" with a safe access like getattr(self, '_action',
None) (or self.get('_action') if this object implements .get) so the expression
becomes robust in non-save/submit flows while preserving the checks against
d.incoming_rate and self.is_internal_transfer().

@mihir-kandoi mihir-kandoi force-pushed the st58765 branch 4 times, most recently from fc98f08 to 30c0ec1 Compare February 2, 2026 07:16
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@erpnext/controllers/selling_controller.py`:
- Around line 307-308: The get_link_to_form call for the Single DocType is
missing the docname parameter, causing inconsistent navigation; update the
get_link_to_form invocation used with Selling Settings to include the docname by
changing get_link_to_form("Selling Settings") to get_link_to_form("Selling
Settings", "Selling Settings") so it matches other usages (e.g., the one at line
822) and ensures correct navigation for the Single DocType.

@mihir-kandoi mihir-kandoi merged commit 135a433 into frappe:develop Feb 2, 2026
12 checks passed
mergify bot pushed a commit that referenced this pull request Feb 2, 2026
(cherry picked from commit 135a433)

# Conflicts:
#	erpnext/stock/doctype/delivery_note/test_delivery_note.py
mergify bot pushed a commit that referenced this pull request Feb 2, 2026
mihir-kandoi added a commit that referenced this pull request Feb 3, 2026
…-52246

fix: validation considers moving average by default instead of set va… (backport #52246)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant