Skip to content

refactor: Use framework functions instead of separate implementions#54555

Open
nabinhait wants to merge 1 commit intofrappe:developfrom
nabinhait:so-refactoring-1
Open

refactor: Use framework functions instead of separate implementions#54555
nabinhait wants to merge 1 commit intofrappe:developfrom
nabinhait:so-refactoring-1

Conversation

@nabinhait
Copy link
Copy Markdown
Member

No description provided.

@github-actions github-actions Bot added skip-release-notes This PR should not be mentioned in the release notes needs-tests This PR needs automated unit-tests. labels Apr 27, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 145d2934-b231-4c7c-8b92-74b63e936238

📥 Commits

Reviewing files that changed from the base of the PR and between 837cdc9 and 290580d.

📒 Files selected for processing (3)
  • erpnext/buying/doctype/purchase_order/purchase_order.py
  • erpnext/selling/doctype/sales_order/sales_order.py
  • erpnext/stock/doctype/material_request/material_request.py

📝 Walkthrough

Walkthrough

Three document types (purchase order, sales order, material request) are refactored to replace their internal check_modified_date() validation methods with calls to check_if_latest(). The removed methods compared database-stored modified timestamps against the document's modified field to detect stale edits. The new flow delegates this recency check to check_if_latest() before proceeding with status updates. The pattern is consistent across all three files, with the msgprint import removed where it is no longer needed.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning No pull request description was provided by the author, which is problematic for understanding intent and changes. Add a description explaining the refactoring goal, why check_if_latest() is preferred over check_modified_date(), and any testing performed.
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main refactoring objective: replacing separate implementations with framework functions across multiple doctype files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-tests This PR needs automated unit-tests. skip-release-notes This PR should not be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant