fix: update cancel_depreciation_entries logic and add test cases for asset depreciation

[ljain112]

closes: #4640
Issue only if the user has upgraded from v14.
The latest version of erpnext already has validation for a mandatory finance book for more than one finance book.

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 35 complexity

Metric	Results
Complexity	35

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[greptile-apps]

Blank-row regression remains untested

This scrap fixture contains only named REGULAR and INCOME-TAX finance-book rows, so it never exercises the blank finance_book branch that the cancellation change fixes. The former early return behavior still passes this test because it reaches the Income Tax row normally. Add a blank finance-book row before the Income Tax row and retain the cancellation assertion; that fixture fails under the old behavior and verifies that processing continues after blank rows.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Artifacts

Executable cancellation-loop comparison script

• Authored Python harness that imports the repository cancellation function, restores only the old branch in a temporary file, and runs named-book and blank-row cases; it demonstrates the test distinction.

Old early-return cancellation behavior

• Captured command output for the temporary old implementation; the named-book case passes while a blank row before the income-tax row prevents cancellation, confirming the existing test would pass before the fix.

Current skip-blank-row cancellation behavior

• Captured command output for the current repository implementation; both named-book and blank-row cases cancel the income-tax journal entry, confirming the changed branch fixes the reproduced failure.

_{Ran code and verified through T-Rex}

[greptile-apps]

Confidence Score: 3/5

Not safe to merge until T-Rex findings are addressed.

The cancellation behavior handles the reproduced blank-row case, but the committed test would also pass with the previous behavior that left the later Income Tax Act depreciation entry active.

T-Rex reproduced 2 failing behaviors at runtime in india_compliance/income_tax_india/overrides/test_asset_depreciation_schedule.py; the change needs fixes before it is safe to merge.

Files Needing Attention: india_compliance/income_tax_india/overrides/test_asset_depreciation_schedule.py

T-Rex Logs

What T-Rex did

• T-Rex produced a P1 finding proof and included an executable cancellation-loop comparison script plus logs showing old versus current cancellation behavior.
• A second P1 finding-comment proof was produced.
• A general-contract-validation-proof documents the before/after behavior changes and explains that full Frappe integration could not run; the harness loads and calls the updated repository function directly.

_{Ran code and verified through T-Rex}

Comments Outside Diff (1)

1. General comment

   Scrap cancellation test does not exercise the blank finance-book branch

   • Bug
     • test_only_income_tax_finance_book_is_cancelled_on_scrap supplies only REGULAR and INCOME-TAX finance-book rows. Its assertions pass when the prior if not row.finance_book: return implementation is restored, so it does not prove that a blank row is skipped rather than aborting cancellation.
   • Cause
     • The fixture has no blank finance_book row before the income-tax row, which is the only path that distinguishes the changed continue from the old return.
   • Fix
     • Add a blank finance-book row before the income-tax row in the scrap test (and retain the assertion that the income-tax depreciation journal entry is cancelled).

   _{Ran code and verified through T-Rex}

_{Reviews (1): Last reviewed commit: "test: update asset depreciation tests fo..." | Re-trigger Greptile}

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds integration coverage for Income Tax Act asset depreciation, including finance-book selection, 180-day rate rules, annual and monthly schedules, first-year treatment, leap-year proration, and disposal-year cancellation. Adds asset creation and company depreciation-setting test utilities. Updates cancellation logic to skip finance-book rows without a finance book and continue processing subsequent rows.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No author description was provided, so relevance to the changeset can't be assessed.	Add a brief description of the depreciation logic change and the new test coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the logic fix and added asset depreciation tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

---

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.}

[coderabbitai]

🧹 Nitpick comments (2)

india_compliance/tests/erpnext_test_utils.py (1)

366-387: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

value() can't express an explicit falsy per-row override.

row.get(fieldname) or args.get(fieldname) or default treats an explicitly-set falsy row value (e.g. daily_prorata_based=0, rate_of_depreciation=0) the same as "not set", silently falling through to the top-level args value instead. This is dormant today (no current test sets a falsy row override against a truthy top-level default) but will silently produce the wrong finance-book row for the next test author who does.

♻️ Suggested fix using a sentinel to distinguish "missing" from "falsy"

-        def value(row, fieldname, default=None):
-            return row.get(fieldname) or args.get(fieldname) or default
+        _unset = object()
+
+        def value(row, fieldname, default=None):
+            for source in (row, args):
+                v = source.get(fieldname, _unset)
+                if v is not _unset and v is not None:
+                    return v
+            return default

india_compliance/income_tax_india/overrides/test_asset_depreciation_schedule.py (1)

371-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a finance-book row without a finance_book to actually cover the fix.

Both rows here (fb_regular, fb_income_tax) set finance_book explicitly, so this test only exercises the pre-existing fb_for_income_tax_map skip in cancel_depreciation_entries (india_compliance/income_tax_india/overrides/asset_depreciation_schedule.py Lines 131-132), not the return→continue fix on Line 129. That fix specifically matters when a row has no finance_book (e.g. the default company book) before an Income Tax Act row — previously that would abort the whole loop and leave the Income Tax book's disposal-year entries uncancelled.

🧪 Suggested addition to pin the actual fix

     def test_only_income_tax_finance_book_is_cancelled_on_scrap(self):
         asset = self._create_asset(
             "2024-04-01",
             finance_books=[
+                # a blank finance_book row (e.g. the default company book) must not
+                # abort processing of the Income Tax Act row that follows it
+                {"depreciation_start_date": "2025-03-31"},
                 {"finance_book": self.fb_regular.name, "depreciation_start_date": "2025-03-31"},
                 {"finance_book": self.fb_income_tax.name, "depreciation_start_date": "2025-03-31"},
             ],
         )

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1d2c46c5-a85a-4c7c-ba80-4e5d4ebdbd51

📥 Commits

Reviewing files that changed from the base of the PR and between 8a04601 and 2d89e7c.

📒 Files selected for processing (3)

• india_compliance/income_tax_india/overrides/asset_depreciation_schedule.py
• india_compliance/income_tax_india/overrides/test_asset_depreciation_schedule.py
• india_compliance/tests/erpnext_test_utils.py