Skip to content

Conversation

@l0gesh29
Copy link
Contributor

Issue: Upon Invoice deletion, with cancelled repost entries getting an error for updation.

Ref: 53866

Steps to reproduce:

  • Create and submit a Sales Invoice.
  • Create a Repost Accounting Ledger entry for this invoice.
  • Cancel the repost entry.
  • Cancel the original invoice.
  • Attempt to delete the invoice → system throws a browser error, user didn't get the proper error message.

Before:

image

After:

image

Backport needed: Version-15

@github-actions github-actions bot added accounts needs-tests This PR needs automated unit-tests. labels Nov 27, 2025
@l0gesh29 l0gesh29 marked this pull request as ready for review November 27, 2025 10:23
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

📝 Walkthrough

Walkthrough

This change introduces a validation guard in the _remove_references_in_repost_doctypes method within the accounts controller. The modification queries for any cancelled repost entries (with docstatus == 2) matching the current document type and name. If cancelled entries are found, the method constructs clickable links to those entries and raises an error, blocking the operation from proceeding. The existing cleanup and repost flow logic remains intact but is now preceded by this cancellation check.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Single-file, localized change to one method with added pre-check guard logic
  • New query and conditional error-raising logic requires verification of query correctness and error message clarity
  • Ensure the guard properly handles edge cases (empty result sets, multiple cancelled entries)

Suggested labels

backport version-15-hotfix

Suggested reviewers

  • ruthra-kumar
  • rohitwaghchaure
  • mihir-kandoi

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding validation for cancelled reposting entries, which directly matches the core objective of preventing browser errors during invoice deletion.
Description check ✅ Passed The description is well-related to the changeset, providing issue context, reproduction steps, support ticket reference, and before/after screenshots that illustrate the fix for cancelled repost entries.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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: 0

🧹 Nitpick comments (1)
erpnext/controllers/accounts_controller.py (1)

367-383: Consider automatic cleanup of cancelled repost entries.

The current implementation blocks deletion and asks users to manually delete cancelled repost entries first. While this is safe and explicit, it creates additional manual work for users. Since cancelled entries (docstatus == 2) are already locked and have no functional impact, consider automatically deleting them during this cleanup process, similar to how the method handles references in active repost entries (lines 385-408).

If the manual approach is intentional for audit trail purposes, consider:

  1. Adding a more specific error message explaining why manual deletion is required
  2. Checking if the user has delete permissions and providing a direct "delete" action if possible
  3. Documenting this design decision

Alternatively, you could add a helper that automatically cleans up cancelled repost entries if the user has appropriate permissions, falling back to the current error message only if automatic cleanup fails.

Example enhancement:

 		if cancelled_entries:
-			entries = "<br>".join([get_link_to_form(d.parenttype, d.parent) for d in cancelled_entries])
-
-			frappe.throw(
-				_(
-					"The following cancelled repost entries exist for <b>{0}</b>:<br><br>{1}<br><br>"
-					"Kindly delete these entries before continuing."
-				).format(self.name, entries)
-			)
+			# Attempt automatic cleanup of cancelled entries
+			try:
+				for entry in cancelled_entries:
+					repost_doc = frappe.get_doc(entry.parenttype, entry.parent)
+					if repost_doc.docstatus == 2:  # Verify it's cancelled
+						repost_doc.delete()
+			except frappe.PermissionError:
+				# Fallback to manual deletion request
+				entries = "<br>".join([get_link_to_form(d.parenttype, d.parent) for d in cancelled_entries])
+				frappe.throw(
+					_(
+						"The following cancelled repost entries exist for <b>{0}</b>:<br><br>{1}<br><br>"
+						"These entries must be deleted before continuing. You may need delete permissions."
+					).format(self.name, entries)
+				)
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a5fd5f and d8fc369.

📒 Files selected for processing (1)
  • erpnext/controllers/accounts_controller.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary

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

Labels

accounts needs-tests This PR needs automated unit-tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant