feat : Summary of Inward Supplies Report #3354
Conversation
|
""" WalkthroughA new "Summary of Inward Supplies" report has been introduced within the GST India compliance module. This addition includes a Python backend for data aggregation and classification of purchase invoices and bills of entry, a JSON definition for report metadata, permissions, and configuration, and a JavaScript frontend for user interface filters, formatting, and total calculations. The report supports filtering by company, GSTIN, and date range, and presents data hierarchically with indentation and custom formatting. It is designed for Indian companies and accessible to roles such as Accounts User, Purchase User, Accounts Manager, and Auditor. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant JS_Report_UI
participant Python_Report_Backend
participant DB
User->>JS_Report_UI: Select filters (Company, GSTIN, Date Range)
JS_Report_UI->>Python_Report_Backend: Request report data with filters
Python_Report_Backend->>DB: Query Purchase Invoices and Bills of Entry
DB-->>Python_Report_Backend: Return raw data
Python_Report_Backend->>Python_Report_Backend: Aggregate, classify, and transform data
Python_Report_Backend-->>JS_Report_UI: Return formatted report data
JS_Report_UI->>User: Display report with hierarchy, formatting, and totals
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.json (1)
4-10: Emptycolumnsandfiltersarrays may confuse future maintainersThe actual run‑time filters/columns are injected through the JS definition, but that coupling is not obvious when someone inspects only the DocType record. A short
"description"key (or a comment in the JS) that points readers to the JS implementation would avoid the “why is this blank?” moment.india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.js (1)
49-57: Minor: jQuery dependency may be unnecessaryThe formatter creates a temporary jQuery object solely to set
font-weight. Vanilla DOM creation (document.createElement("span")) would remove the jQuery runtime cost and keep the bundle lighter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.js(1 hunks)india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.json(1 hunks)india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.js (2)
india_compliance/gst_india/report/gstin_status/gstin_status.js (1)
column(37-37)india_compliance/public/js/components/data_table_manager.js (1)
datatable_options(127-143)
🪛 GitHub Actions: Linters
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
[error] 139-139: frappe-semgrep-rules.rules.frappe-query-debug-statement: Did you mean to leave this debug statement in? Found 'return query.run(as_dict=True, debug=True)'
[error] 175-175: frappe-semgrep-rules.rules.frappe-query-debug-statement: Did you mean to leave this debug statement in? Found 'return query.run(as_dict=True, debug=True)'
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
| get_datatable_options(datatable_options) { | ||
| datatable_options.hooks = { | ||
| columnTotal: function (...args) { | ||
| return this.datamanager.data.reduce((acc, row) => { | ||
| const column_field = args[1].column.fieldname; | ||
| if (column_field === "details") return; | ||
|
|
||
| if (row.indent !== 1) { acc += row[column_field] || 0; } | ||
|
|
||
| return acc; | ||
| }, 0); | ||
| }, | ||
| }; | ||
|
|
||
| return datatable_options; |
There was a problem hiding this comment.
Two correctness issues inside the custom columnTotal hook
-
datatable_options.hooks = { … }overwrites any hooks that the caller might already have defined (e.g. provided bydata_table_manager.js). UseObject.assign()to merge instead of clobbering. -
Inside
reduce()the earlyreturn;for the details column yieldsundefined, turning the accumulator intoundefinedon the very first iteration and breaking the summation.
- datatable_options.hooks = {
- columnTotal: function (...args) {
+ datatable_options.hooks = Object.assign({}, datatable_options.hooks, {
+ columnTotal: function (...args) {
const column_field = args[1].column.fieldname;
- return this.datamanager.data.reduce((acc, row) => {
- if (column_field === "details") return;
+ return this.datamanager.data.reduce((acc, row) => {
+ if (column_field === "details") {
+ return acc; // keep accumulator unchanged
+ }
if (row.indent !== 1) {
acc += row[column_field] || 0;
}
return acc;
}, 0);
- },
- };
+ },
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get_datatable_options(datatable_options) { | |
| datatable_options.hooks = { | |
| columnTotal: function (...args) { | |
| return this.datamanager.data.reduce((acc, row) => { | |
| const column_field = args[1].column.fieldname; | |
| if (column_field === "details") return; | |
| if (row.indent !== 1) { acc += row[column_field] || 0; } | |
| return acc; | |
| }, 0); | |
| }, | |
| }; | |
| return datatable_options; | |
| get_datatable_options(datatable_options) { | |
| - datatable_options.hooks = { | |
| - columnTotal: function (...args) { | |
| + datatable_options.hooks = Object.assign({}, datatable_options.hooks, { | |
| + columnTotal: function (...args) { | |
| const column_field = args[1].column.fieldname; | |
| - return this.datamanager.data.reduce((acc, row) => { | |
| - if (column_field === "details") return; | |
| + return this.datamanager.data.reduce((acc, row) => { | |
| + if (column_field === "details") { | |
| + return acc; // keep accumulator unchanged | |
| + } | |
| if (row.indent !== 1) { | |
| acc += row[column_field] || 0; | |
| } | |
| return acc; | |
| }, 0); | |
| - }, | |
| - }; | |
| + }, | |
| + }); | |
| return datatable_options; | |
| } |
| def get_detail_type(row) -> int: | ||
| for detail_type, mapping in MAPPING_FIELD.items(): | ||
| if mapping["get_detail_type"](row): | ||
| return detail_type | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
get_detail_type returns None silently
If a row doesn’t match any mapping, subsequent logic fails. Either raise an explicit error or provide a fallback bucket to avoid silent data loss.
raise ValueError(f"Unable to classify row {row}")| def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: | ||
| return get_columns(), get_data(filters) | ||
|
|
||
|
|
||
| def get_columns() -> list[dict]: |
There was a problem hiding this comment.
filters may be None – add a safe default
execute() forwards filters directly to get_data() where attribute access is performed. Without a default, the report crashes when run without filters (common during unit tests).
-def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]:
- return get_columns(), get_data(filters)
+def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]:
+ filters = frappe._dict(filters or {}) # allows both key & attribute access
+ return get_columns(), get_data(filters)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: | |
| return get_columns(), get_data(filters) | |
| def get_columns() -> list[dict]: | |
| def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: | |
| filters = frappe._dict(filters or {}) # allows both key & attribute access | |
| return get_columns(), get_data(filters) |
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3354 +/- ##
===========================================
- Coverage 58.97% 58.94% -0.04%
===========================================
Files 136 137 +1
Lines 13638 14041 +403
===========================================
+ Hits 8043 8276 +233
- Misses 5595 5765 +170
... and 28 files with indirect coverage changes 🚀 New features to boost your workflow:
|
karm1000
force-pushed
the
summary_of_inward_supplies
branch
from
ad93ee8 to
9eab155
Compare
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (9)
45-46:⚠️ Potential issue
filtersmay beNone– add a safe default
execute()forwardsfiltersdirectly toget_data()where attribute access is performed. Without a default, the report crashes when run without filters (common during unit tests).-def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: - return get_columns(), get_data(filters) +def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: + filters = frappe._dict(filters or {}) # allows both key & attribute access + + # Extract date range once for all functions + if filters.get("date_range"): + filters.from_date, filters.to_date = filters.date_range + + return get_columns(), get_data(filters)
97-100:⚠️ Potential issue
row.igst_amountuses attribute style on adict– will raiseAttributeErrorDictionary access should be used instead of attribute access for consistency and to avoid AttributeError.
- summary[detail_type]["igst_amount"] += row.igst_amount - summary[detail_type]["cgst_amount"] += row.cgst_amount - summary[detail_type]["sgst_amount"] += row.sgst_amount - summary[detail_type]["cess_amount"] += row.cess_amount + summary[detail_type]["igst_amount"] += row["igst_amount"] + summary[detail_type]["cgst_amount"] += row["cgst_amount"] + summary[detail_type]["sgst_amount"] += row["sgst_amount"] + summary[detail_type]["cess_amount"] += row["cess_amount"]
109-109: 🛠️ Refactor suggestionExtract date range once in execute function
Date range is extracted multiple times in different functions. Extract it once in the execute function as suggested in past reviews.
130-137:⚠️ Potential issueAdd condition to exclude self-supplied goods and fix attribute access
There's a missing condition to ensure company_gstin != supplier_gstin. Also, attribute access on filters dict should be replaced with get() method.
- & (doc.company == filters.company) + & (doc.company == filters.get("company")) + # Don't include self-supplied goods + if filters.get("company_gstin"): + query = query.where( + (doc.company_gstin == filters.get("company_gstin")) + & (doc.company_gstin != doc.supplier_gstin) + ) + else: + query = query.where(doc.company_gstin != doc.supplier_gstin) - if filters.get("company_gstin"): - query = query.where(doc.company_gstin == filters.company_gstin)
155-157:⚠️ Potential issueMissing alias causes
itc_classificationkey to be absent in result rowsWithout
.as_("itc_classification"), subsequent logic that callsrow.get("itc_classification")evaluates toNone, mis-classifying imports.- .select( - Case("itc_classification") - .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") - .else_("Import Of Goods"), + .select( + Case() + .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") + .else_("Import Of Goods") + .as_("itc_classification"),
210-214:⚠️ Potential issue
get_detail_typereturnsNonesilentlyIf a row doesn't match any mapping, subsequent logic fails. Either raise an explicit error or provide a fallback bucket to avoid silent data loss.
def get_detail_type(row: dict) -> int: for detail_type, mapping in MAPPING_FIELD.items(): if mapping["is_part_of"](row): return detail_type + raise ValueError(f"Unable to classify row {row}")
226-229:⚠️ Potential issueDictionary access required instead of attribute access
Similar to the issue above, dictionary access should be used instead of attribute access for row elements.
- summary[key]["igst_amount"] += row.igst_amount - summary[key]["cgst_amount"] += row.cgst_amount - summary[key]["sgst_amount"] += row.sgst_amount - summary[key]["cess_amount"] += row.cess_amount + summary[key]["igst_amount"] += row["igst_amount"] + summary[key]["cgst_amount"] += row["cgst_amount"] + summary[key]["sgst_amount"] += row["sgst_amount"] + summary[key]["cess_amount"] += row["cess_amount"]
159-159: 🛠️ Refactor suggestionRemove unnecessary literal value for reverse charge
As mentioned in past reviews, literal value is not required for reverse charge, and comparisons should use
not row.get(reverse_charge)instead of comparison with 0.- LiteralValue(False).as_("is_reverse_charge"), + LiteralValue(0).as_("is_reverse_charge"),
158-158:⚠️ Potential issueFix quotes in LiteralValue
The single quotes inside the double quotes are unnecessary and may cause SQL syntax errors.
- LiteralValue("'Overseas'").as_("gst_category"), + LiteralValue("Overseas").as_("gst_category"),
🧹 Nitpick comments (3)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (3)
35-35: Remove unnecessary SEZ exclusion conditionThe condition
and row.get("gst_category") != "SEZ"is redundant here. As noted in past reviews, ifitc_classificationis "Import Of Service", thengst_categorywill never be "SEZ".- "is_part_of": lambda row: row.get("itc_classification") == "Import Of Service" - and row.get("gst_category") != "SEZ", + "is_part_of": lambda row: row.get("itc_classification") == "Import Of Service",
138-138: Remove debug flag from query executionDebug flags should not be present in production code as they can leak SQL queries and cause linting issues.
- return query.run(as_dict=True, debug=True) + return query.run(as_dict=True)
152-153: Optimize item retrieval for capital goodsBased on previous review comments, joining with the Item table can be avoided by using a different approach to identify capital goods.
- .inner_join(item) - .on(doc_item.item_code == item.name)Consider fetching capital goods items separately and passing them to the function, as suggested in the past review:
- Don't join with the item table
- Get a list of capital goods items before processing
- Check if item_code is in that list to set subcategory as capital goods
- Don't fetch is_fixed_asset column in Purchase Invoice
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
…ng; remove unnecessary alias;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (9)
19-21:⚠️ Potential issue
filtersmay beNone– add a safe defaultThe execute function doesn't handle the case where
filtersis None, which can cause errors when methods try to access attributes on None.def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: + filters = frappe._dict(filters or {}) # allows both key & attribute access report = InwardSuppliesGSTSummary(filters) return report.get_columns(), report.get_data()
104-108:⚠️ Potential issue
get_categoryreturnsNonesilentlyIf a row doesn't match any mapping, subsequent logic will fail since it depends on having a valid category.
def get_category(self, row: dict) -> str: for detail_type, mapping in self.mapping.items(): if (fn := mapping["category"]) and fn(row): return detail_type + raise ValueError(f"Unable to classify row {row}")
154-156:⚠️ Potential issueIncorrect attribute access on
filtersUsing attribute access on filters may fail if filters is a plain dict.
.where( (doc.docstatus == 1) - & (doc.posting_date[filters.from_date : filters.to_date]) - & (doc.company == filters.company) + & (doc.posting_date[filters.get("from_date") : filters.get("to_date")]) + & (doc.company == filters.get("company")) & (doc.company_gstin != doc.supplier_gstin) & (doc.is_opening == "No") )
161-162:⚠️ Potential issueIncorrect attribute access on
filters.company_gstinUsing attribute access on filters may fail if filters is a plain dict.
if filters.get("company_gstin"): - query = query.where(doc.company_gstin == filters.company_gstin) + query = query.where(doc.company_gstin == filters.get("company_gstin"))
178-181:⚠️ Potential issueMissing alias causes
itc_classificationkey to be absent in result rowsWithout
.as_("itc_classification"), subsequent logic that usesrow.get("itc_classification")will evaluate toNone, misclassifying imports..select( - Case("itc_classification") - .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") - .else_("Import Of Goods"), - LiteralValue("'Overseas'").as_("gst_category"), + Case() + .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") + .else_("Import Of Goods") + .as_("itc_classification"), + LiteralValue("Overseas").as_("gst_category"), item.is_fixed_asset,
193-194:⚠️ Potential issueMore incorrect attribute access on
filtersUsing attribute access on filters may fail if filters is a plain dict.
(doc.docstatus == 1) - & (doc.posting_date[filters.from_date : filters.to_date]) - & (doc.company == filters.company) + & (doc.posting_date[filters.get("from_date") : filters.get("to_date")]) + & (doc.company == filters.get("company"))
198-199:⚠️ Potential issueIncorrect attribute access on
filters.company_gstinUsing attribute access on filters may fail if filters is a plain dict.
if filters.get("company_gstin"): - query = query.where(doc.company_gstin == filters.company_gstin) + query = query.where(doc.company_gstin == filters.get("company_gstin"))
209-210:⚠️ Potential issueIncorrect attribute access on
filters.date_rangeUsing attribute access on filters may fail if filters is a plain dict.
- filters.from_date, filters.to_date = filters.date_range + filters.from_date, filters.to_date = filters.get("date_range") self.filters = filters
291-293:⚠️ Potential issue
row.tax_fielduses attribute style on adict– will raiseAttributeErrorUsing
getattr(row, tax_field)will fail if row is a plain dict. Use dictionary access instead.for tax_field in TAX_FIELDS: - summary_entry[tax_field] += getattr(row, tax_field) + summary_entry[tax_field] += row.get(tax_field, 0)
🧹 Nitpick comments (2)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (2)
1-1: Future copyright year needs correctionThe copyright year is set to 2025, which is in the future. Update this to the current year.
-# Copyright (c) 2025, Resilient Tech and contributors +# Copyright (c) 2024, Resilient Tech and contributors
204-206: Consider adding docstrings to explain class hierarchyThe inheritance structure is complex and unclear. Adding docstrings would help future maintainers understand the purpose and relationship between these classes.
class InwardSuppliesGSTSummary( InwardSuppliesGSTSummaryCategory, InwardSuppliesGSTSummaryData ): + """Main class that processes inward supplies data for the GST summary report. + + Inherits category classification logic from InwardSuppliesGSTSummaryCategory + and data fetching logic from InwardSuppliesGSTSummaryData. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Analyze (javascript)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (2)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (2)
240-272: LGTM! Good column definitions with appropriate labels and widthsThe column definitions are well structured with appropriate labels, field types, and widths. Using _() for labels enables translation.
294-317: Well-structured transformation with proper indentationThe transformation logic for the summary data is well-structured, creating a hierarchical view with proper indentation for subcategories.
karm1000
force-pushed
the
summary_of_inward_supplies
branch
from
74d153e to
2726df4
Compare
| "owner": "Administrator", | ||
| "prepared_report": 0, | ||
| "ref_doctype": "Purchase Invoice", | ||
| "report_name": "Summary of Inward Supplies", |
There was a problem hiding this comment.
Can you suggest some name?
| "A": self._is_inward_supplies_from_registered, | ||
| "B": self._is_inward_supplies_from_unregistered, | ||
| "C": self._is_inward_supplies_from_registered_reverse_charge, | ||
| "D": self._is_import_of_goods_sez, | ||
| "E": self._is_import_of_services, | ||
| "F": self._is_itc_received_from_isd, |
There was a problem hiding this comment.
Better constructed with a tree structure like this. First assign most very specific conditions like export etc. Balance can be unregistered
| ) | ||
|
|
||
| def _is_import_of_goods_sez(self, row: dict) -> bool: | ||
| return row.get("itc_classification") == "Import Of Goods" |
| self._get_purchase_invoice_data(filters), | ||
| ) | ||
|
|
||
| def _get_purchase_invoice_data(self, filters: dict) -> list[dict]: |
There was a problem hiding this comment.
can we use common query for purchase data?
| }, | ||
| "E": { | ||
| "title": "Import Of Services (excluding inward supplies from SEZ)", | ||
| "category": ..., |
There was a problem hiding this comment.
Can be set to none instead (codacy error)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (1)
20-23: 🛠️ Refactor suggestionEnsure
filtersis initialized safely
The signature allowsfilters=None, butInwardSuppliesGSTSummaryexpects attribute‐style access and a populateddate_range. Coercefiltersto afrappe._dictand validate required keys upfront.def execute(filters: dict | None = None) -> tuple[list[dict], list[dict]]: - report = InwardSuppliesGSTSummary(filters) + filters = frappe._dict(filters or {}) + report = InwardSuppliesGSTSummary(filters) return report.get_columns(), report.get_data()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (1)
Learnt from: vorasmit
PR: resilient-tech/india-compliance#3344
File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0
Timestamp: 2025-05-26T03:16:30.230Z
Learning: In GST account-wise summary reports, the filtering logic `doc.company_gstin != IfNull(doc[counterparty_gstin_field], "")` is intentionally designed to exclude internal transfers (where company GSTIN equals party GSTIN) while including transactions with unregistered parties (empty GSTIN). Internal transfers are excluded as they are only for internal reporting purposes like stock transfers between branches.
🪛 Pylint (3.3.7)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
[refactor] 47-47: Too few public methods (0/2)
(R0903)
[refactor] 84-104: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 79-79: Either all return statements in a function should return an expression, or none of them should.
(R1710)
[refactor] 110-119: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 122-122: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Python Unit Tests
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (13)
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py (13)
4-17: Imports and TAX_FIELDS look solid
The necessary modules and tax field constants are correctly imported and defined.
25-44: Category enum is well‐structured
The enumeration and properties correctly capture titles and subcategory presence.
47-50: Subcategory holder class is concise
Constants for subcategories are clear and used downstream.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 47-47: Too few public methods (0/2)
(R0903)
53-75: Category–subcategory mapping is correct
Mapping entries align with the intended structure;Nonecorrectly denotes no subcategories.
106-120:get_subcategorylogic is sound
Fixed‐asset check, HSN prefix, and default bucket are correctly ordered.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 110-119: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
122-127: Data source chaining is appropriate
Usingitertools.chainstreams both data sources without materializing lists twice.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 122-122: Too few public methods (0/2)
(R0903)
209-222: Initialization of summary buckets is correct
Zero‐tax defaults and dynamic subcategory buckets are built as intended.
224-257: Column definitions follow conventions
Fieldnames, labels, and widths align with other GST reports.
258-275: Aggregation loop is straightforward
Iterating rows, categorizing, and incrementing tax fields works and relies on the added fallback inget_category.
277-302: Transformed summary construction is clear
Lettered categories and indented subcategory rows are built in a readable, maintainable manner.
303-310: Summation helper is minimal and effective
No further improvements needed here.
312-317: Entry creator helper is concise
Merging details and tax data into a record dict is implemented cleanly.
164-200:⚠️ Potential issueFix the
Caseconstruction and missing alias in bill‐of‐entry query
Case("itc_classification")is misused and the result isn’t aliased—downstreamrow.get("itc_classification")will always beNone..select( - Case("itc_classification") - .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") - .else_("Import Of Goods"), + Case() + .when(doc_item.gst_hsn_code.like("99%"), "Import Of Service") + .else_("Import Of Goods") + .as_("itc_classification"), ConstantColumn("Overseas").as_("gst_category"), item.is_fixed_asset, ... )⛔ Skipped due to learnings
Learnt from: Ninad1306 PR: resilient-tech/india-compliance#3344 File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0 Timestamp: 2025-04-25T16:30:32.870Z Learning: In Frappe's query builder, the Case constructor can take a string argument that sets the column alias directly. For example, `Case("column_alias")` will create a Case expression with "column_alias" as the alias for the resulting column.Learnt from: Ninad1306 PR: resilient-tech/india-compliance#3344 File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0 Timestamp: 2025-04-25T16:30:32.870Z Learning: In Frappe's query builder, the Case constructor can take a string argument that sets the column alias directly. For example, `Case("column_alias")` will create a Case expression with "column_alias" as the alias for the resulting column.Learnt from: Ninad1306 PR: resilient-tech/india-compliance#3344 File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0 Timestamp: 2025-04-25T16:30:32.870Z Learning: In Frappe's query builder, the Case constructor can take a string argument that sets the column alias directly. For example, `Case("column_alias")` will create a Case expression with "column_alias" as the alias for the resulting column.Learnt from: Ninad1306 PR: resilient-tech/india-compliance#3344 File: india_compliance/gst_india/report/gst_account_wise_summary/gst_account_wise_summary.py:0-0 Timestamp: 2025-04-25T16:30:32.870Z Learning: In Frappe's query builder, the Case constructor can take a string argument that sets the column alias directly. For example, `Case("column_alias")` will create a CASE expression with "column_alias" as the alias for the resulting column.
| class InwardSuppliesGSTSummary( | ||
| InwardSuppliesGSTSummaryCategory, InwardSuppliesGSTSummaryData | ||
| ): | ||
| def __init__(self, filters: dict) -> None: | ||
| filters.from_date, filters.to_date = filters.get("date_range") | ||
| self.filters = filters | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden __init__: validate and normalize filters
Relying on attribute access without normalization will break when filters is a plain dict. Enforce frappe._dict and ensure date_range is present.
def __init__(self, filters: dict) -> None:
- filters.from_date, filters.to_date = filters.get("date_range")
- self.filters = filters
+ filters = frappe._dict(filters or {})
+ if not filters.get("date_range"):
+ raise ValueError("`date_range` filter is required for Summary of Inward Supplies report")
+ filters.from_date, filters.to_date = filters.get("date_range")
+ self.filters = filters📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class InwardSuppliesGSTSummary( | |
| InwardSuppliesGSTSummaryCategory, InwardSuppliesGSTSummaryData | |
| ): | |
| def __init__(self, filters: dict) -> None: | |
| filters.from_date, filters.to_date = filters.get("date_range") | |
| self.filters = filters | |
| class InwardSuppliesGSTSummary( | |
| InwardSuppliesGSTSummaryCategory, InwardSuppliesGSTSummaryData | |
| ): | |
| def __init__(self, filters: dict) -> None: | |
| filters = frappe._dict(filters or {}) | |
| if not filters.get("date_range"): | |
| raise ValueError("`date_range` filter is required for Summary of Inward Supplies report") | |
| filters.from_date, filters.to_date = filters.get("date_range") | |
| self.filters = filters |
🤖 Prompt for AI Agents
In
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
around lines 202 to 208, the __init__ method assumes filters supports attribute
access, which fails if filters is a plain dict. Fix this by converting filters
to frappe._dict at the start, then validate that "date_range" exists in filters
before unpacking it into from_date and to_date attributes to avoid runtime
errors.
| class InwardSuppliesGSTSummaryCategory: | ||
| def get_category(self, row: dict) -> None: | ||
| gst_category = row.get("gst_category") | ||
| itc_classification = row.get("itc_classification") | ||
| is_reverse_charge = row.get("is_reverse_charge") | ||
|
|
||
| if ( | ||
| gst_category != "Overseas" | ||
| and not is_reverse_charge | ||
| and itc_classification != "Input Service Distributor" | ||
| ): | ||
| return InwardSuppliesCategory.INWARD_DOMESTIC | ||
|
|
||
| elif gst_category == "Unregistered" and is_reverse_charge: | ||
| return InwardSuppliesCategory.UNREG_RCM | ||
|
|
||
| elif gst_category != "Unregistered" and is_reverse_charge: | ||
| return InwardSuppliesCategory.REG_RCM | ||
|
|
||
| elif itc_classification == "Import Of Goods": | ||
| return InwardSuppliesCategory.IMPORT_GOODS | ||
|
|
||
| elif itc_classification == "Import Of Service": | ||
| return InwardSuppliesCategory.IMPORT_SERVICES | ||
|
|
||
| elif itc_classification == "Input Service Distributor": | ||
| return InwardSuppliesCategory.ITC_FROM_ISD | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit fallback in get_category
If a row doesn’t match any branch, None is returned silently—leading to summary.get(None). Fail fast or provide a default to avoid silent data loss.
def get_category(self, row: dict) -> None:
...
- elif itc_classification == "Input Service Distributor":
- return InwardSuppliesCategory.ITC_FROM_ISD
+ elif itc_classification == "Input Service Distributor":
+ return InwardSuppliesCategory.ITC_FROM_ISD
+ # Fallback for unclassified rows
+ else:
+ raise ValueError(f"Unable to classify inward supply row: {row}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class InwardSuppliesGSTSummaryCategory: | |
| def get_category(self, row: dict) -> None: | |
| gst_category = row.get("gst_category") | |
| itc_classification = row.get("itc_classification") | |
| is_reverse_charge = row.get("is_reverse_charge") | |
| if ( | |
| gst_category != "Overseas" | |
| and not is_reverse_charge | |
| and itc_classification != "Input Service Distributor" | |
| ): | |
| return InwardSuppliesCategory.INWARD_DOMESTIC | |
| elif gst_category == "Unregistered" and is_reverse_charge: | |
| return InwardSuppliesCategory.UNREG_RCM | |
| elif gst_category != "Unregistered" and is_reverse_charge: | |
| return InwardSuppliesCategory.REG_RCM | |
| elif itc_classification == "Import Of Goods": | |
| return InwardSuppliesCategory.IMPORT_GOODS | |
| elif itc_classification == "Import Of Service": | |
| return InwardSuppliesCategory.IMPORT_SERVICES | |
| elif itc_classification == "Input Service Distributor": | |
| return InwardSuppliesCategory.ITC_FROM_ISD | |
| class InwardSuppliesGSTSummaryCategory: | |
| def get_category(self, row: dict) -> None: | |
| gst_category = row.get("gst_category") | |
| itc_classification = row.get("itc_classification") | |
| is_reverse_charge = row.get("is_reverse_charge") | |
| if ( | |
| gst_category != "Overseas" | |
| and not is_reverse_charge | |
| and itc_classification != "Input Service Distributor" | |
| ): | |
| return InwardSuppliesCategory.INWARD_DOMESTIC | |
| elif gst_category == "Unregistered" and is_reverse_charge: | |
| return InwardSuppliesCategory.UNREG_RCM | |
| elif gst_category != "Unregistered" and is_reverse_charge: | |
| return InwardSuppliesCategory.REG_RCM | |
| elif itc_classification == "Import Of Goods": | |
| return InwardSuppliesCategory.IMPORT_GOODS | |
| elif itc_classification == "Import Of Service": | |
| return InwardSuppliesCategory.IMPORT_SERVICES | |
| elif itc_classification == "Input Service Distributor": | |
| return InwardSuppliesCategory.ITC_FROM_ISD | |
| # Fallback for unclassified rows | |
| else: | |
| raise ValueError(f"Unable to classify inward supply row: {row}") |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 84-104: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 79-79: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🤖 Prompt for AI Agents
In
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
around lines 78 to 105, the get_category method lacks an explicit fallback
return value, causing it to return None silently if no conditions match. Add a
final else clause or a default return statement at the end of the method to
return a sensible default category or raise an error to fail fast, preventing
silent data loss from summary.get(None).
| def _get_purchase_invoice_data(self, filters: dict) -> list[dict]: | ||
| doc = frappe.qb.DocType("Purchase Invoice") | ||
| doc_item = frappe.qb.DocType("Purchase Invoice Item") | ||
|
|
||
| query = ( | ||
| frappe.qb.from_(doc) | ||
| .inner_join(doc_item) | ||
| .on(doc.name == doc_item.parent) | ||
| .select( | ||
| doc.gst_category, | ||
| doc.itc_classification, | ||
| doc_item.is_fixed_asset, | ||
| doc.is_reverse_charge, | ||
| doc_item.gst_hsn_code, | ||
| doc_item.cgst_amount, | ||
| doc_item.sgst_amount, | ||
| doc_item.igst_amount, | ||
| (doc_item.cess_amount + doc_item.cess_non_advol_amount).as_( | ||
| "cess_amount" | ||
| ), | ||
| ) | ||
| .where( | ||
| (doc.docstatus == 1) | ||
| & (doc.posting_date[filters.get("from_date") : filters.get("to_date")]) | ||
| & (doc.company == filters.get("company")) | ||
| & (doc.company_gstin != doc.supplier_gstin) | ||
| & (doc.is_opening == "No") | ||
| ) | ||
| ) | ||
|
|
||
| if filters.get("company_gstin"): | ||
| query = query.where(doc.company_gstin == filters.get("company_gstin")) | ||
|
|
||
| return query.run(as_dict=True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use IFNULL to exclude only true internal transfers
Comparing company_gstin != supplier_gstin will filter out rows where supplier_gstin is NULL. To include unregistered parties (empty or null), coalesce supplier_gstin.
from frappe.query_builder.custom import ConstantColumn
+from frappe.qb.functions import IfNull
.where(
(doc.docstatus == 1)
& (doc.posting_date[filters.get("from_date") : filters.get("to_date")])
& (doc.company == filters.get("company"))
- & (doc.company_gstin != doc.supplier_gstin)
+ & (doc.company_gstin != IfNull(doc.supplier_gstin, ""))
& (doc.is_opening == "No")
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _get_purchase_invoice_data(self, filters: dict) -> list[dict]: | |
| doc = frappe.qb.DocType("Purchase Invoice") | |
| doc_item = frappe.qb.DocType("Purchase Invoice Item") | |
| query = ( | |
| frappe.qb.from_(doc) | |
| .inner_join(doc_item) | |
| .on(doc.name == doc_item.parent) | |
| .select( | |
| doc.gst_category, | |
| doc.itc_classification, | |
| doc_item.is_fixed_asset, | |
| doc.is_reverse_charge, | |
| doc_item.gst_hsn_code, | |
| doc_item.cgst_amount, | |
| doc_item.sgst_amount, | |
| doc_item.igst_amount, | |
| (doc_item.cess_amount + doc_item.cess_non_advol_amount).as_( | |
| "cess_amount" | |
| ), | |
| ) | |
| .where( | |
| (doc.docstatus == 1) | |
| & (doc.posting_date[filters.get("from_date") : filters.get("to_date")]) | |
| & (doc.company == filters.get("company")) | |
| & (doc.company_gstin != doc.supplier_gstin) | |
| & (doc.is_opening == "No") | |
| ) | |
| ) | |
| if filters.get("company_gstin"): | |
| query = query.where(doc.company_gstin == filters.get("company_gstin")) | |
| return query.run(as_dict=True) | |
| # at top of file, add: | |
| from frappe.query_builder.custom import ConstantColumn | |
| from frappe.qb.functions import IfNull | |
| def _get_purchase_invoice_data(self, filters: dict) -> list[dict]: | |
| doc = frappe.qb.DocType("Purchase Invoice") | |
| doc_item = frappe.qb.DocType("Purchase Invoice Item") | |
| query = ( | |
| frappe.qb.from_(doc) | |
| .inner_join(doc_item) | |
| .on(doc.name == doc_item.parent) | |
| .select( | |
| doc.gst_category, | |
| doc.itc_classification, | |
| doc_item.is_fixed_asset, | |
| doc.is_reverse_charge, | |
| doc_item.gst_hsn_code, | |
| doc_item.cgst_amount, | |
| doc_item.sgst_amount, | |
| doc_item.igst_amount, | |
| (doc_item.cess_amount + doc_item.cess_non_advol_amount).as_( | |
| "cess_amount" | |
| ), | |
| ) | |
| .where( | |
| (doc.docstatus == 1) | |
| & (doc.posting_date[filters.get("from_date") : filters.get("to_date")]) | |
| & (doc.company == filters.get("company")) | |
| & (doc.company_gstin != IfNull(doc.supplier_gstin, "")) | |
| & (doc.is_opening == "No") | |
| ) | |
| ) | |
| if filters.get("company_gstin"): | |
| query = query.where(doc.company_gstin == filters.get("company_gstin")) | |
| return query.run(as_dict=True) |
🤖 Prompt for AI Agents
In
india_compliance/gst_india/report/summary_of_inward_supplies/summary_of_inward_supplies.py
between lines 129 and 162, the condition comparing company_gstin and
supplier_gstin excludes rows where supplier_gstin is NULL, which unintentionally
filters out unregistered parties. Modify the query to use IFNULL (or equivalent
coalesce function) on supplier_gstin to treat NULL or empty values properly,
ensuring only true internal transfers are excluded while including unregistered
parties.
Filters:
Summary by CodeRabbit