Skip to content

fix: remove state based logic from patch - #4721

Open
ljain112 wants to merge 1 commit into
resilient-tech:developfrom
ljain112:fix-ic-default-patch
Open

fix: remove state based logic from patch#4721
ljain112 wants to merge 1 commit into
resilient-tech:developfrom
ljain112:fix-ic-default-patch

Conversation

@ljain112

Copy link
Copy Markdown
Member

Issue: Users were using state-based categories, due to which these categories were ignored.

  • This still doesn't completely resolve the issue because an incorrect category can be set, as categories may match using state.
  • We are considering it as an exception.
  • Also, increased the patch counter as the previous patch was skipped because we forgot to create the custom field.

@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

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.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The patch entry now includes the #1 annotation. The Tax Category backfill now considers categories referenced by enabled sales or purchase tax templates. The lookup selects category names and GST state flags and orders results by creation time. Default selection now uses only inter-state and reverse-charge values.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: removing state-based logic from the compliance patch.
Description check ✅ Passed The description explains the state-based category issue and the patch counter update, which match the changeset.
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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 99d358c4-6599-444b-b8c6-4b53bc7fc672

📥 Commits

Reviewing files that changed from the base of the PR and between aa4c0f6 and c14b4f2.

📒 Files selected for processing (2)
  • india_compliance/patches.txt
  • india_compliance/patches/post_install/set_india_compliance_default_tax_category.py

Comment on lines +35 to +42
fields=["name", "is_inter_state", "is_reverse_charge"],
filters={"disabled": 0, "name": ["in", list(used_categories)]},
order_by="creation desc",
)

defaults = []
for is_inter_state, is_reverse_charge in DEFAULT_SCENARIOS:
category = get_default_tax_category(
tax_categories, used_categories, is_inter_state, is_reverse_charge
)
category = get_default_tax_category(tax_categories, is_inter_state, is_reverse_charge)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not use creation time to choose the global default category.

If enabled templates use different category names for the same (is_inter_state, is_reverse_charge) pair, Line 37 places the newest category first. Lines 55-57 then select that category and mark it as the global default.

The downstream lookup searches each company for a template using that single default category. A newer custom or company-specific category can therefore make another company or template type return no automatic tax template.

Select a stable canonical category instead of relying on creation time. Add an integration test with different enabled categories for the same scenario across companies or template doctypes.

Based on Lines 972-993 of india_compliance/gst_india/overrides/transaction.py, the default flag is a global selector for each scenario.

Also applies to: 55-57

@greptile-apps

greptile-apps Bot commented Aug 10, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

The reviewed migration behavior is safe to merge based on the exercised category-selection and rerun paths.

The actual patch module was executed against category and template fixtures covering all four selection scenarios, disabled and unreferenced categories, and repeat execution. No scoring findings remain.

Files Needing Attention: No files need author action. A full Frappe/ERPNext database run remains unavailable locally because the frappe package is not installed.

T-Rex T-Rex Logs

What T-Rex did

  • T-Rex loaded and invoked india_compliance.patches.post_install.set_india_compliance_default_tax_category with enabled sales and purchase template references across all four (is_inter_state, is_reverse_charge) scenarios, including old/new category candidates, a disabled referenced category, and a category referenced only by a disabled template.
  • T-Rex ran the default-tax-category-runtime-harness.py to capture the empty pre-migration state, then reran it with --after to confirm the post-migration behavior.
  • The after-run selected exactly the four newest eligible categories, excluded disabled and unreferenced categories, and produced no additional updates on a subsequent run.
  • An attempt to import frappe in the environment failed with ModuleNotFoundError, so a database-backed Frappe runtime could not be exercised here.
  • Artifacts and logs captured by the harness validate the migration scope and demonstrate that a real Frappe runtime was unavailable in this environment.

View all artifacts

T-Rex Ran code and verified through T-Rex

Reviews (1): Last reviewed commit: "fix: remove state based logic from patch" | Re-trigger Greptile

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant