feat(asset): make asset depreciation failure notification role configurable

[SowmyaArunachalam]

Issue:
When asset depreciation fails, it sends a notification to the Account Manager by default.

fixes: #48878

Solution:
Added a field in Accounts Settings to configure the role

no-docs

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a new Accounts Settings field role_used_for_depreciation_failure (Link → Role) and updates field ordering and metadata in the doctype JSON. Adds a corresponding type hint in the Python doctype file. Changes the asset depreciation notification logic to read the configured role from Accounts Settings and select recipients accordingly, falling back to "Accounts Manager" and then to "System Manager" if no recipients are found. Timestamp in the JSON metadata is updated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review notify_depr_entry_posting_error for correct fallback behavior and that recipient lookup handles empty or missing role gracefully.
• Verify the JSON field role_used_for_depreciation_failure is declared correctly and placed in field_order.
• Confirm the Python type hint in accounts_settings.py matches the JSON field and is under TYPE_CHECKING only.
• Check for needed translations, migrations, or UI refresh implications.
• Run a quick integration test: configured role, unset role, and role with no users to verify notification recipients.

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 (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly summarizes the main change: making asset depreciation failure notification role configurable.
Description check	✅ Passed	The PR description is related to the changeset, explaining the issue, solution, and linking to the relevant issue #48878.
Linked Issues check	✅ Passed	The PR fully addresses issue #48878 by adding a configurable role field in Accounts Settings and modifying the depreciation failure notification logic to use it.
Out of Scope Changes check	✅ Passed	All changes (new field in Accounts Settings, type hints, and notification logic update) are directly scoped to making depreciation failure notifications configurable as required by issue #48878.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent 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 e830cca and d52d986.

📒 Files selected for processing (1)

• erpnext/accounts/doctype/accounts_settings/accounts_settings.json (3 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). (6)

• GitHub Check: Patch Test
• GitHub Check: Python Unit Tests (2)
• GitHub Check: Python Unit Tests (3)
• GitHub Check: Python Unit Tests (1)
• GitHub Check: Python Unit Tests (4)
• GitHub Check: Summary

🔇 Additional comments (2)

erpnext/accounts/doctype/accounts_settings/accounts_settings.json (2)

76-77: Field structure and placement look correct.

The new role_used_for_depreciation_failure field is properly positioned in the Asset Settings section, follows established patterns for role-link fields (matching credit_controller, frozen_accounts_modifier, etc.), and includes a clear description. The absence of a default value is appropriate for enabling fallback logic at runtime.

Also applies to: 663-669

---

663-669: All related changes are properly implemented and verified.

The type hint exists in accounts_settings.py, the notification logic correctly reads role_used_for_depreciation_failure from Accounts Settings (line 310), and the fallback chain is properly implemented: configured role → "Accounts Manager" → "System Manager" (lines 311-314). Missing or unconfigured values are handled correctly.

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

erpnext/assets/doctype/asset/depreciation.py (1)

309-324: Good implementation with sensible fallback logic.

The change successfully makes the depreciation failure notification role configurable while maintaining backward compatibility with "Accounts Manager" as the default fallback.

Fallback chain clarification:
The current logic is: configured_role → Accounts Manager (if not configured) → System Manager (if no recipients).

However, if a custom role is configured but has no users assigned, the code skips "Accounts Manager" and goes directly to "System Manager". Consider whether this is the desired behavior, or if you want a more defensive approach:

user_role = frappe.db.get_single_value("Accounts Settings", "role_used_for_depreciation_failure")
recipients = get_users_with_role(user_role or "Accounts Manager")

if not recipients and user_role:
    # Custom role configured but has no users, try Accounts Manager
    recipients = get_users_with_role("Accounts Manager")

if not recipients:
    # Last resort fallback
    recipients = get_users_with_role("System Manager")

This ensures admins always get notified via Accounts Manager or System Manager, even if they misconfigure an empty role.

erpnext/accounts/doctype/accounts_settings/accounts_settings.json (1)

662-669: Field configuration looks good.

The new field is properly configured with appropriate metadata, clear description, and logical placement in the Assets settings section.

Optional enhancement: Consider adding validation in the AccountsSettings.validate() method to ensure the configured role exists if set. This would provide immediate feedback to administrators if they select a role that gets deleted later:

def validate(self):
    # ... existing validation code ...
    self.validate_depreciation_failure_role()

def validate_depreciation_failure_role(self):
    if self.role_used_for_depreciation_failure:
        if not frappe.db.exists("Role", self.role_used_for_depreciation_failure):
            frappe.throw(_("Role {0} does not exist").format(self.role_used_for_depreciation_failure))

📜 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 a8d3e9b and e830cca.

📒 Files selected for processing (3)

• erpnext/accounts/doctype/accounts_settings/accounts_settings.json (3 hunks)
• erpnext/accounts/doctype/accounts_settings/accounts_settings.py (1 hunks)
• erpnext/assets/doctype/asset/depreciation.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). (6)

• GitHub Check: Python Unit Tests (1)
• GitHub Check: Python Unit Tests (2)
• GitHub Check: Python Unit Tests (4)
• GitHub Check: Python Unit Tests (3)
• GitHub Check: Patch Test
• GitHub Check: Summary

🔇 Additional comments (2)

erpnext/accounts/doctype/accounts_settings/accounts_settings.py (1)

68-68: LGTM! Type hint correctly added.

The type hint for the new configuration field is correctly defined and follows the same pattern as other role fields in this class.

erpnext/accounts/doctype/accounts_settings/accounts_settings.json (1)

76-76: Field ordering is appropriate.

The field is logically placed after book_asset_depreciation_entry_automatically, keeping related asset depreciation settings together.