chore: add type hints to frappe.whitelist function; #3628
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds explicit type annotations to many public/whitelisted functions across india_compliance (Audit Trail, GST, VAT, utils, reports). A few functional adjustments were introduced: GSTR-1 export filtering/HSN validation, enqueue/token-check for Purchase Reconciliation Tool download, input-sanitization decorators, and an api_secret logout guard. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client/UI
participant GSTR1 as GSTR-1 Export API
participant Log as GSTR1 Log
UI->>GSTR1: get_gstr_1_json(company_gstin, year, mo_qtr, include_uploaded, delete_missing)
GSTR1->>GSTR1: Coerce include_uploaded/delete_missing to bool
GSTR1->>Log: fetch data
Note over GSTR1,Log: Exclude NIL_EXEMPT, HSN_B2B/B2C, HSN, DOC_ISSUE, QUARTERLY_KEYS, rounding_difference
alt Subcategory == HSN
GSTR1->>GSTR1: Validate HSN per row
GSTR1-->>UI: throw if missing HSN
end
GSTR1->>GSTR1: Filter by upload_status (include/delete flags)
GSTR1->>Log: normalize_data(data)
GSTR1-->>UI: dict {data, filename}
sequenceDiagram
autonumber
participant UI as Client/UI
participant PRT as Purchase Reconciliation Tool
participant TaxAPI as TaxpayerBaseAPI
participant Queue as Background Queue
UI->>PRT: download_gstr(company_gstin, date_range, ...)
PRT->>TaxAPI: validate auth token
alt token invalid
PRT-->>UI: error
else
PRT->>Queue: enqueue download_gstr task
Queue-->>PRT: job id
PRT-->>UI: ack/job id
end
sequenceDiagram
autonumber
participant UI as Client/UI
participant Page as india_compliance_account
participant Auth as Auth Manager
UI->>Page: set_api_secret(api_secret)
alt api_secret is falsy
Page->>Auth: logout()
Page-->>UI: None
else
Page->>Auth: store secret
Page-->>UI: None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Connected to Huly®: IC-3768 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (30)
india_compliance/vat_india/doctype/c_form/c_form.py (1)
89-104: Fix incorrect return type hint (function returns a dict but annotated as None).The method returns a dictionary on success and None on empty input. The current annotation
-> Noneis wrong and will mislead both tooling and callers.Apply this minimal, safe change:
- def get_invoice_details(self, invoice_no: str) -> None: + def get_invoice_details(self, invoice_no: str) -> dict | None:india_compliance/gst_india/doctype/gst_return_log/gst_return_log.py (1)
181-191: Harden download_file against IDOR, header injection, and None deref; keep -> None.Current checks validate only DocType-level read and trust client-supplied doctype/name/filename. This allows downloading files from other doctypes if the caller has read on GST Return Log (classic IDOR). Also, filename is echoed back unsanitized, and missing files raise AttributeError.
Apply this diff to enforce record-level permission, lock doctype, sanitize filename, validate inputs, and handle missing files:
@frappe.whitelist() -def download_file() -> None: - frappe.has_permission("GST Return Log", "read", throw=True) - - data = frappe._dict(frappe.local.form_dict) - frappe.response["filename"] = data["file_name"] - - file = get_file_doc(data["doctype"], data["name"], data["file_field"]) - frappe.response["filecontent"] = file.get_content(encodings=[]) - - frappe.response["type"] = "download" +def download_file() -> None: + frappe.has_permission("GST Return Log", "read", throw=True) + + data = frappe._dict(frappe.local.form_dict or {}) + required = {"doctype", "name", "file_field", "file_name"} + missing = required - set(data) + if missing: + frappe.throw( + _("Missing parameters: {0}").format(", ".join(sorted(missing))), + title=_("Invalid Request"), + ) + + # Enforce endpoint to GST Return Log and record-level permission + if data.doctype != DOCTYPE: + frappe.throw(_("Invalid doctype for this endpoint."), frappe.PermissionError) + doc = frappe.get_doc(DOCTYPE, data.name) + if not doc.has_permission("read"): + frappe.throw(_("Not permitted to read this document."), frappe.PermissionError) + + # Sanitize filename and fetch file + from pathlib import Path + frappe.response["filename"] = Path(f"{data['file_name']}").name + file = get_file_doc(DOCTYPE, data["name"], data["file_field"]) + if not file: + frappe.throw(_("File not found."), frappe.DoesNotExistError) + + frappe.response["filecontent"] = file.get_content(encodings=[]) + frappe.response["type"] = "download"india_compliance/audit_trail/report/audit_trail/audit_trail.py (1)
407-418: Usediscardto prevent KeyError and correct “Account Settings” to “Accounts Settings”The
removecalls inget_relevant_doctypeswill raise if the entry is missing; switching todiscardis safer. Also, we discovered an inconsistent DocType reference—most of the code uses “Accounts Settings” (plural), but in theno_name_field_doctypeslist it’s spelled “Account Settings” (singular). That mismatch will likely break lookups or audits. Please:• In
india_compliance/audit_trail/report/audit_trail/audit_trail.py
– Changeremove→discardfor both doctypes.
– Update theno_name_field_doctypeslist entry to use “Accounts Settings”.Diff suggestion:
--- a/india_compliance/audit_trail/report/audit_trail/audit_trail.py @@ line 29 - "Account Settings", + "Accounts Settings", @@ def get_relevant_doctypes() -> set[str]: - doctypes.remove("Accounts Settings") + doctypes.discard("Accounts Settings") @@ if frappe.get_cached_value( - doctypes.remove("Process Deferred Accounting") + doctypes.discard("Process Deferred Accounting")india_compliance/gst_india/doctype/gst_settings/gst_settings.py (2)
552-556: Bug: accidental reassignment of local variable instead of updating the query.This line overwrites company with the Query object, and the filter is not applied to query. It likely results in unfiltered updates for all companies.
Apply this fix:
- if company: - company = query.where(sales_invoice.company == company) + if company: + query = query.where(sales_invoice.company == company)
276-279: Typo in user-facing message (“counfigure”).End users see this string in a throw; please correct the spelling.
- "Please counfigure your India Compliance Account to " + "Please configure your India Compliance Account to "india_compliance/gst_india/report/gst_balance/gst_balance.py (1)
41-49: Refactorget_pending_voucher_typesfor precise list typing and consistent return shapeThe current implementation in
india_compliance/gst_india/report/gst_balance/gst_balance.py
- Uses a generic
-> tuplereturn annotation, hiding the actual element types.- Initializes
company_accountsas""when no company is passed, but aslistwhen one is—introducing astr | listunion.- Returns
Nonefromverify_gstin_updatewhen there are no pending vouchers, instead of an empty list.These inconsistencies can lead to runtime surprises in downstream code (e.g., front-end consumers expecting lists). Please apply the following mandatory refactor:
@@ -41,14 +41,21 @@ @frappe.whitelist() -def get_pending_voucher_types(company: str = None) -> tuple: +def get_pending_voucher_types(company: str | None = None) -> tuple[list[str], list[str]]: frappe.has_permission("GST Settings", "read", throw=True) - company_accounts = "" - if company: - company_accounts = get_all_gst_accounts(company) - - return verify_gstin_update(company_accounts), company_accounts + # Always use a list for company_accounts + company_accounts: list[str] = [] + if company: + # get_all_gst_accounts may return None or an iterable + company_accounts = list(get_all_gst_accounts(company) or []) + + # Guarantee a list return from verify_gstin_update (empty if no pending types) + pending = verify_gstin_update(company_accounts) or [] + return pending, company_accounts
- Changed return annotation to
tuple[list[str], list[str]]to document exactly what’s returned.- Ensured
company_accountsis always alist[str].- Wrapped
verify_gstin_update(...)inor []so it never returnsNone.india_compliance/gst_india/utils/taxes_controller.py (1)
170-181: item_name/tax_name filters are ineffective; code paths always return all rows.Using dict.get with a default doesn’t filter—defaults are ignored when the key exists. This means specifying item_name or tax_name has no effect.
Replace with explicit filtering:
- def get_rows_to_update(self, item_name=None, tax_name=None): + def get_rows_to_update(self, item_name=None, tax_name=None): @@ - items = ( - self.doc.get("items", {"name": item_name}) - if item_name - else self.doc.get("items") - ) - taxes = ( - self.doc.get("taxes", {"name": tax_name}) if tax_name else self.doc.taxes - ) + items = [i for i in self.doc.get("items", []) if not item_name or i.name == item_name] + taxes = [t for t in self.doc.get("taxes", []) if not tax_name or t.name == tax_name] return items, taxesindia_compliance/gst_india/report/hsn_wise_summary_of_outward_supplies/hsn_wise_summary_of_outward_supplies.py (2)
166-188: Validate and parse inputs defensively; return a typed dict
filtersanddataare JSON strings from the client. A malformed payload will raiseJSONDecodeError. Also,filters["company"]is used without presence check, which can raiseKeyError.+from typing import Any @@ -@frappe.whitelist() -def get_json(filters: str, report_name: str, data: str) -> dict: +@frappe.whitelist() +def get_json(filters: str, report_name: str, data: str) -> dict[str, Any]: @@ - filters = json.loads(filters) - report_data = json.loads(data) + try: + filters = json.loads(filters) + except json.JSONDecodeError: + frappe.throw(_("Invalid filters payload. Expecting JSON.")) + try: + report_data = json.loads(data) + except json.JSONDecodeError: + frappe.throw(_("Invalid report data payload. Expecting JSON.")) + + if not report_name or not report_name.strip(): + frappe.throw(_("Report Name is required")) + + if not filters.get("company"): + frappe.throw(_("Company is required to generate GST JSON")) @@ - gstin = filters.get("company_gstin") or get_company_gstin_number(filters["company"]) + gstin = filters.get("company_gstin") or get_company_gstin_number(filters["company"])Optional: validate that
report_datahas the expected structure before delegating.
191-199: Normalize and validate download payload; ensure valid JSON contentAt the moment, any string in
data["data"]will be sent as a JSON file without validation. Re-parse and re-dump to guarantee valid JSON, and add presence checks.@frappe.whitelist() -def download_json_file() -> None: +def download_json_file() -> None: """download json content in a file""" data = frappe._dict(frappe.local.form_dict) - frappe.response["filename"] = ( - frappe.scrub("{0}".format(data["report_name"])) + ".json" - ) - frappe.response["filecontent"] = data["data"] + report_name = data.get("report_name") + raw = data.get("data") + if not report_name or not raw: + frappe.throw(_("Missing report_name or data")) + + # Ensure the payload is valid JSON and consistently formatted + try: + parsed = json.loads(raw) + except json.JSONDecodeError: + frappe.throw(_("Invalid JSON content")) + + frappe.response["filename"] = f"{frappe.scrub(str(report_name))}.json" + frappe.response["filecontent"] = json.dumps(parsed, ensure_ascii=False) frappe.response["content_type"] = "application/json" frappe.response["type"] = "download"india_compliance/gst_india/overrides/party.py (1)
115-131: Reset the re-entrancy flag and harden input handling
frappe.flags.in_update_docs_with_previous_gstinis set but never reset; it may leak into subsequent handlers within the same request context.docs_with_previous_gstinis trusted JSON; parse with error handling.- Restrict doctypes to the known set to avoid unintended updates if a crafted payload injects other doctypes.
def update_docs_with_previous_gstin( gstin: str, gst_category: str, docs_with_previous_gstin: str ) -> None: - frappe.flags.in_update_docs_with_previous_gstin = True - docs_with_previous_gstin = json.loads(docs_with_previous_gstin) - - for doctype, docnames in docs_with_previous_gstin.items(): - for docname in docnames: - try: - doc = frappe.get_doc(doctype, docname) - doc.gstin = gstin - doc.gst_category = gst_category - doc.save() - except Exception as e: - frappe.clear_last_message() - frappe.throw( - "Error updating {0} {1}:<br/> {2}".format(doctype, docname, str(e)) - ) + frappe.flags.in_update_docs_with_previous_gstin = True + try: + try: + payload = json.loads(docs_with_previous_gstin) + except json.JSONDecodeError: + frappe.throw(_("Invalid docs_with_previous_gstin payload. Expecting JSON")) + + allowed_doctypes = {"Address", "Supplier", "Customer", "Company"} + for doctype, docnames in payload.items(): + if doctype not in allowed_doctypes: + frappe.throw(_("Unsupported doctype: {0}").format(doctype)) + for docname in docnames: + try: + doc = frappe.get_doc(doctype, docname) + doc.gstin = gstin + doc.gst_category = gst_category + doc.save() + except Exception as e: + frappe.clear_last_message() + frappe.throw( + _("Error updating {0} {1}:<br/>{2}").format( + doctype, docname, str(e) + ) + ) + finally: + frappe.flags.in_update_docs_with_previous_gstin = Falseindia_compliance/gst_india/utils/gstr_2/__init__.py (1)
334-334: Bug: update_import_history expects an iterable but receives a string
save_gstr_2bcallsupdate_import_history(return_period)with a string, butupdate_import_historyuses("in", return_periods)which expects a list/tuple. With a string, the query may behave unpredictably (char-wise membership).Two compatible fixes (pick one):
- Coerce inside the callee (preferred, backwards-compatible):
def update_import_history(return_periods): """Updates 2A data availability from 2B Import""" - if not ( + if isinstance(return_periods, str): + return_periods = [return_periods] + if not ( inward_supplies := frappe.get_all( "GST Inward Supply", filters={"return_period_2b": ("in", return_periods)},
- Or pass a list from the call site:
- update_import_history(return_period) + update_import_history([return_period])Also applies to: 374-401
india_compliance/gst_india/doctype/pan/pan.py (1)
124-128: Add a request timeout to avoid hanging server workersExternal calls without timeouts can hang the request thread/process, impacting availability. Set a sane timeout and consider handling
requests.Timeout.- response = requests.post(url, json=payload) + response = requests.post(url, json=payload, timeout=10)Optional:
- except Exception as e: + except (requests.Timeout, Exception) as e:india_compliance/gst_india/overrides/payment_entry.py (1)
23-33: Parse args when passed as JSON string to match the new type hint.The signature allows args: dict | str, but the value is forwarded without parsing. If a client sends JSON text (common via frappe.call), ERPNext’s get_outstanding_reference_documents will receive a str and may break.
Apply:
@frappe.whitelist() def get_outstanding_reference_documents( - args: dict | str, validate: bool = False + args: dict | str, validate: bool = False ) -> list: from erpnext.accounts.doctype.payment_entry.payment_entry import ( get_outstanding_reference_documents, ) - reference_documents = get_outstanding_reference_documents(args, validate) + if isinstance(args, str): + args = frappe.parse_json(args) + + reference_documents = get_outstanding_reference_documents(args, validate)india_compliance/gst_india/overrides/transaction.py (1)
846-852: Bug: returning the result of dict.update() yields None.
dict.update()returns None, so the whitelisted method serializesnullto the client instead of the enriched dict.Apply this diff to return the updated mapping:
- return party_details.update( - { - **get_gst_details( - party_details, doctype, company, update_place_of_supply=True - ), - } - ) + party_details.update( + get_gst_details(party_details, doctype, company, update_place_of_supply=True) + ) + return party_detailsindia_compliance/gst_india/overrides/subcontracting_transaction.py (2)
424-424: Guard against None filters to avoid TypeError in frappe._dict.When
filtersis omitted,frappe._dict(filters)may raise. Default to{}.-filters = frappe._dict(filters) +filters = frappe._dict(filters or {})
462-462: Guard against None filters to avoid TypeError in frappe._dict.Mirror the fix applied to the receipts search.
-filters = frappe._dict(filters) +filters = frappe._dict(filters or {})india_compliance/gst_india/doctype/bill_of_entry/bill_of_entry.py (3)
309-314: Fix Semgrep blocker: avoid mutatingself.company_currencyinside on_submit pathCI flags that
self.company_currencyis being modified inget_gl_entries, which is invoked fromon_submit. This mutation is not persisted and trips the rulefrappe-modifying-but-not-comitting-other-method. You can safely rely onAccountsController.get_gl_dictto resolve company currency when the attribute is absent, so remove the assignment.Apply this diff:
- # company_currency is required by get_gl_dict - self.company_currency = erpnext.get_company_currency(self.company) # nosemgrep + # Let AccountsController.get_gl_dict resolve company currency internally
259-266: Wrong variable in error message:row.idxshould betax.idxInside
validate_taxes, when the calculated tax differs, the thrown message interpolatesrow.idx. Hererowrefers to the last loop variable from a previous loop and is incorrect. Usetax.idxto reference the current tax row.Apply this diff:
- ).format(row.idx, tax.tax_amount, column) + ).format(tax.idx, tax.tax_amount, column)
394-401: Whitelisted method should accept JSON string or list inputThe signature now advertises
list[str], but whitelisted methods often receive JSON strings from the client. Access patterns below assume a list. Make the method robust to both forms.Apply this diff:
- def get_items_from_purchase_invoice(self, purchase_invoices: list[str]) -> None: + def get_items_from_purchase_invoice(self, purchase_invoices: list[str] | str) -> None: if not purchase_invoices: frappe.msgprint(_("No Purchase Invoices selected")) return + if isinstance(purchase_invoices, str): + purchase_invoices = frappe.parse_json(purchase_invoices)india_compliance/gst_india/doctype/gstr_1/gstr_1_export.py (5)
86-90: Transformation guard skips legitimate zero values
apply_transformationsusesif row.get(field), so zeros (e.g., 0% differential) won’t be transformed and will remain as 0 instead of the intended None/format. Check field presence, not truthiness.Apply this diff:
- for field, modifier in self.FIELD_TRANSFORMATIONS.items(): - if row.get(field): - row[field] = modifier(row[field]) + for field, modifier in self.FIELD_TRANSFORMATIONS.items(): + if field in row: + row[field] = modifier(row[field])
162-166: Bug:dict.updatereturn value assigned back todata(becomes None)
data = data.update(data.pop("aggregate_data", {}))setsdatatoNone. Merge aggregate data without overwriting the dict reference.Apply this diff:
- data = data.update(data.pop("aggregate_data", {})) + data.update(data.pop("aggregate_data", {}))
1152-1161: Typo in user-facing header: "Documenrt Date"Excel header has a spelling mistake.
Apply this diff:
- "label": "Documenrt Date", + "label": "Document Date",
2139-2142: Bug:dict.updateassigned back (setsdatato None) in JSON builderSame
updatemisuse here will break JSON generation.Apply this diff:
- data = gstr1_log.get_json_for("books") - data = data.update(data.pop("aggregate_data", {})) + data = gstr1_log.get_json_for("books") + data.update(data.pop("aggregate_data", {}))
2143-2167: Unreachable HSN validation blockYou
continuefor HSN categories before checkingsubcategory == HSNso the validation never runs. Move the HSN check before the skip set or remove HSN from that set.Apply this diff (move HSN validation up and keep skip list without HSN):
- for subcategory, subcategory_data in data.items(): - if subcategory in { - GSTR1_SubCategory.NIL_EXEMPT.value, - GSTR1_SubCategory.HSN_B2B.value, - GSTR1_SubCategory.HSN_B2C.value, - GSTR1_SubCategory.HSN.value, # Backwards compatibility - GSTR1_SubCategory.DOC_ISSUE.value, - *QUARTERLY_KEYS, - "rounding_difference", - }: - continue - - if subcategory == GSTR1_SubCategory.HSN.value: + for subcategory, subcategory_data in data.items(): + # Validate HSN rows first + if subcategory == GSTR1_SubCategory.HSN.value: for row in subcategory_data.values(): if row.get(inv_f.HSN_CODE): continue frappe.throw( _( "GST HSN Code is missing in one or more invoices. Please ensure all invoices include the HSN Code, as it is Mandatory for filing GSTR-1." ) ) - continue + continue + + if subcategory in { + GSTR1_SubCategory.NIL_EXEMPT.value, + GSTR1_SubCategory.HSN_B2B.value, + GSTR1_SubCategory.HSN_B2C.value, + GSTR1_SubCategory.DOC_ISSUE.value, + *QUARTERLY_KEYS, + "rounding_difference", + }: + continueindia_compliance/gst_india/utils/e_invoice.py (2)
59-79: Handledocnamespassed as list or JSON stringThe new signature allows
list[str] | str, but the implementation still assumesstrand calls.startswith. This will raise for lists. Make parsing robust.Apply this diff:
-def enqueue_bulk_e_invoice_generation(docnames: list[str] | str) -> str: +def enqueue_bulk_e_invoice_generation(docnames: list[str] | str) -> str: @@ - docnames = frappe.parse_json(docnames) if docnames.startswith("[") else [docnames] + if isinstance(docnames, str): + s = docnames.strip() + docnames = frappe.parse_json(s) if s.startswith("[") else [s] + elif isinstance(docnames, list): + # already a list of names + pass + else: + docnames = [cstr(docnames)]
433-451:mark_e_invoice_as_cancelledmay return None; adjust annotationEarly return on Line 436 returns
None, but the signature is-> dict. Align the type hint.Apply this diff:
-def mark_e_invoice_as_cancelled(doctype: str, docname: str, values: dict | str) -> dict: +def mark_e_invoice_as_cancelled(doctype: str, docname: str, values: dict | str) -> dict | None: @@ - if doc.docstatus != 2: - return + if doc.docstatus != 2: + return Noneindia_compliance/gst_india/doctype/gstr_1/gstr_1.py (1)
65-92: Don’t clobber the incomingmessageparameter
generate_gstr1accepts an optionalmessage, but Line 78 resets it toNone, discarding caller intent. Respect the provided message.Apply this diff:
- message = None + # Preserve provided `message` if anyindia_compliance/gst_india/utils/e_waybill.py (1)
97-119: Bug:docnamescan be a list, but code assumes.startswithexistsThe signature allows
list[str] | str, but later logic usesdocnames.startswith("[")(Line 110). If a list is passed directly, this raises an AttributeError. Also, timeout calculation relies on a list length.Patch to accept both list and JSON string:
- docnames = frappe.parse_json(docnames) if docnames.startswith("[") else [docnames] + # Accept list, tuple, set, or JSON string/single name + if isinstance(docnames, str): + docnames = ( + frappe.parse_json(docnames) + if docnames.strip().startswith("[") + else [docnames] + ) + elif isinstance(docnames, (list, tuple, set)): + docnames = list(docnames) + else: + frappe.throw(_("Invalid 'docnames' type. Expected JSON string or list."), title=_("Invalid Arguments"))india_compliance/gst_india/doctype/purchase_reconciliation_tool/purchase_reconciliation_tool.py (2)
126-151: Prevent enqueueing withreturn_type=None(background job will crash)You allow
return_type: str | None, but the enqueueddownload_gstrcasts withReturnType(return_type)and will raise if None. This fails in background with poor UX.Minimal fix: assert a valid return type before enqueueing.
TaxpayerBaseAPI(company_gstin).validate_auth_token() + if return_type is None: + frappe.throw(_("Please select a Return Type (GSTR 2A or GSTR 2B) before downloading."), title=_("Missing Return Type")) + frappe.enqueue( download_gstr,Alternatively, derive
return_typefromself.gst_return(and enqueue both when “Both GSTR 2A & 2B” is selected).
258-268: Type hint mismatch: returns a list, not dict
self.ReconciledData.get(purchases, inward_supplies)returns a list of rows; the method is annotated-> dict.Adjust hint:
- ) -> dict: + ) -> list[dict]:Or wrap the list into a dict structure (e.g.,
{"rows": ...}) consistently across the API.
🧹 Nitpick comments (41)
india_compliance/vat_india/doctype/c_form/c_form.py (3)
92-95: Optionally sanitize and reject whitespace-only inputs.Guard against inputs that are only spaces and normalize the value before usage. Keeps behavior (returns None on empty) while hardening input handling.
- if not invoice_no: + invoice_no = (invoice_no or "").strip() + if not invoice_no: return
96-97: Be explicit about permission type.Clarity/readability nit: make the permission type explicit to avoid ambiguity and future refactor surprises.
- doc.check_permission() + doc.check_permission(permtype="read")
98-103: Stronger typing via TypedDict (optional).If you want stricter, self-documenting typing for API consumers, define a TypedDict and use it in the signature. This improves editor hints and mypy coverage.
Add near the imports (illustrative code, not a diff since it’s outside the changed hunk):
from typing import TypedDict class CFormInvoiceDetails(TypedDict): invoice_date: "date" # or datetime.date if imported explicitly territory: str net_total: float grand_total: floatThen update the signature:
def get_invoice_details(self, invoice_no: str) -> CFormInvoiceDetails | None: ...india_compliance/gst_india/overrides/item_tax_template.py (1)
71-77: Tighten the return type to reflect structure; consider annotating helper too.get_valid_gst_accounts returns four lists (three from get_valid_accounts + one negative-rate list). A bare list return type is too loose and weakens static checks.
Apply this diff to document intent:
-@frappe.whitelist() -def get_valid_gst_accounts(company: str) -> list: +@frappe.whitelist() +def get_valid_gst_accounts(company: str) -> list[list[str]]: frappe.has_permission("Item Tax Template", "read", throw=True)Optionally, add a type hint to the helper for clarity (outside this hunk):
def get_accounts_with_negative_rate(company: str) -> list[str]: ...india_compliance/gst_india/utils/itc_04/itc_04_export.py (1)
21-39: Accept dict payloads and validate required filtersCallers—such as in india_compliance/gst_india/utils/itc_04/test_itc_04_export.py—already pass a Python dict to download_itc_04_json instead of a JSON string. To make this explicit and guard against missing keys (which currently surface as errors in get_return_period), broaden the signature to accept both str and dict, and validate that the minimum set of filters is present.
• No other call sites invoke download_itc_04_json in the codebase, so this change won’t break existing consumers.
• Required filters for computing the return period are from_date, to_date and company_gstin.Suggested diff:
@frappe.whitelist() -def download_itc_04_json(filters: str) -> dict: +def download_itc_04_json(filters: str | dict) -> dict: frappe.has_permission("GST Job Work Stock Movement", "export", throw=True) - filters = frappe.parse_json(filters) + if isinstance(filters, str): + filters = frappe.parse_json(filters) + elif not isinstance(filters, dict): + frappe.throw(_("Invalid filters payload.")) + required = {"company_gstin", "from_date", "to_date"} + missing = required - set(filters) + if missing: + frappe.throw( + _("Missing required filters: {0}").format(", ".join(sorted(missing))), + title=_("Invalid Filters"), + ) company_gstin = filters.get("company_gstin") ret_period = get_return_period(filters)india_compliance/gst_india/report/gst_balance/gst_balance.py (1)
52-55: Specify the element type in the return annotation.update_company_gstin returns a list; clarify its contents (e.g., list[str] or list[dict]) to improve downstream type checking.
Example:
-@frappe.whitelist() -def update_company_gstin() -> list: +@frappe.whitelist() +def update_company_gstin() -> list[str]:If the underlying function returns structured rows, use list[dict[str, Any]] instead.
india_compliance/gst_india/doctype/gstr_3b_report/gstr_3b_report.py (2)
736-741: Defensive handling for missing/invalid JSON would prevent runtime errors.view_report returns json.loads(json_data) without guarding against None or invalid JSON, which can raise exceptions if the report hasn’t been generated yet.
Apply:
@frappe.whitelist() -def view_report(name: str) -> dict: +def view_report(name: str) -> dict: frappe.has_permission("GSTR 3B Report", throw=True) json_data = frappe.get_value("GSTR 3B Report", name, "json_output") - return json.loads(json_data) + if not json_data: + return {} + try: + return json.loads(json_data) + except Exception: + # Return empty dict to avoid client-side crashes; optionally log + return {}
744-752: Gracefully handle absent JSON before setting download response.When json_output is empty, filecontent becomes None. Some clients expect a string/bytes.
@frappe.whitelist() -def make_json(name: str) -> None: +def make_json(name: str) -> None: frappe.has_permission("GSTR 3B Report", throw=True) json_data = frappe.get_value("GSTR 3B Report", name, "json_output") file_name = "GST3B.json" frappe.local.response.filename = file_name - frappe.local.response.filecontent = json_data + frappe.local.response.filecontent = json_data or "{}" frappe.local.response.type = "download"india_compliance/gst_india/utils/taxes_controller.py (1)
136-168: Minor: clarify types in get_item_tax_map() for better readability and static analysis.tax_templates and tax_accounts are sets; consider normalizing to list to match frappe.get_all filters’ expectations and typing.
- tax_templates = frappe.parse_json(tax_templates) - tax_accounts = frappe.parse_json(tax_accounts) + tax_templates = list(frappe.parse_json(tax_templates) or []) + tax_accounts = list(frappe.parse_json(tax_accounts) or [])india_compliance/audit_trail/overrides/customize_form.py (2)
15-23: Tighten return typing for fetch_to_customizeThe method returns a mapping of mixed values. Prefer a precise return type to aid callers and mypy/pyright.
Apply:
+from typing import Any @@ - def fetch_to_customize(self) -> dict: + def fetch_to_customize(self) -> dict[str, Any]:
26-28: Align save_customization return type with base implementationDepending on Frappe version,
CustomizeForm.save_customization()may return a status string orNone. To avoid false-positive type errors for callers, widen the hint.- def save_customization(self) -> str: + def save_customization(self) -> str | None:If you're certain your framework version always returns a string, feel free to keep
-> str. Otherwise, the wider type is safer.india_compliance/gst_india/utils/gstr_2/__init__.py (2)
462-473: Type hint improvement: be explicit about the response shapeThe API returns a JSON-like mapping. Prefer
dict[str, Any]to avoid losing key/value type info.-from typing import Any +from typing import Any @@ -@frappe.whitelist() -@otp_handler -def regenerate_gstr_2b(gstin: str, return_period: str, doctype: str) -> dict: +@frappe.whitelist() +@otp_handler +def regenerate_gstr_2b(gstin: str, return_period: str, doctype: str) -> dict[str, Any]:
476-490: Mirror the same explicit typing for status checkMatch the regenerate variant for consistency and better tooling support.
-@frappe.whitelist() -def check_regenerate_status(gstin: str, reference_id: str, doctype: str) -> dict | None: +@frappe.whitelist() +def check_regenerate_status( + gstin: str, reference_id: str, doctype: str +) -> dict[str, Any] | None:india_compliance/gst_india/doctype/pan/pan.py (1)
51-62: Simplify return type; normalize input for consistent lookups
- The function always returns a 2-tuple (or
("", "")), so| Noneis unnecessary.- Normalizing
panearly helps avoid collation surprises across DBs.-@frappe.whitelist() -def get_pan_status(pan: str, force_update: bool = False) -> tuple[str, str] | None: +@frappe.whitelist() +def get_pan_status(pan: str, force_update: bool = False) -> tuple[str, str]: + pan = pan.upper().strip()india_compliance/gst_india/utils/gstr_utils.py (2)
29-34: Adjustrequest_otpreturn type to include NoneThe
TaxpayerBaseAPI.request_otp()implementation returnsNonewhenstatus_cd != 1and raisesOTPRequestedErrorfor OTP flows, so the current-> dictannotation is inaccurate. For minimal change, update the return type to reflect the possibleNone:@frappe.whitelist() -def request_otp(company_gstin: str) -> dict: +def request_otp(company_gstin: str) -> dict | None: frappe.has_permission("GST Settings", throw=True) return TaxpayerBaseAPI(company_gstin).request_otp()
36-44: Correct OTP method spelling across definition and usagesThe method is currently defined as
autheticate_with_otpinindia_compliance/gst_india/api_classes/taxpayer_base.py(line 143) and invoked with the same typo ingstr_utils.py. Changing only the call toauthenticate_with_otpwill break the code, since no such method exists. If you’d like to fix the spelling, you should rename both the definition and all call sites.Locations to update:
- Definition in india_compliance/gst_india/api_classes/taxpayer_base.py
- Invocation in india_compliance/gst_india/utils/gstr_utils.py
Proposed diffs:
--- india_compliance/gst_india/api_classes/taxpayer_base.py @@ -141,7 +141,7 @@ def request_otp(self): … - def autheticate_with_otp(self, otp=None): + def authenticate_with_otp(self, otp=None): if not otp: …--- india_compliance/gst_india/utils/gstr_utils.py @@ -39,7 +39,7 @@ api = TaxpayerBaseAPI(company_gstin) - response = api.autheticate_with_otp(otp) + response = api.authenticate_with_otp(otp) return api.process_response(response)india_compliance/gst_india/page/india_compliance_account/__init__.py (3)
28-41: Minor clarity: avoid returning the result of logout() in a function annotated to return None.No functional change, but being explicit reads cleaner and avoids confusing “return value of a void helper.”
@frappe.whitelist() def set_api_secret(api_secret: str) -> None: has_permission_of_page(page_name, throw=True) if not api_secret: - return logout() + logout() + return set_encrypted_password( "GST Settings", "GST Settings", api_secret, fieldname="api_secret" )
54-60: Harden JSON parsing to avoid 500s on bad globals.If ic_auth_session contains malformed JSON, json.loads(session) will raise. Gracefully handle it and clear the bad value.
@frappe.whitelist() def get_auth_session() -> dict | None: has_permission_of_page(page_name, throw=True) session = frappe.db.get_global("ic_auth_session") - return session and json.loads(session) + if not session: + return None + try: + return json.loads(session) + except Exception: + # clear corrupted value and return None + _set_auth_session(None) + return None
62-74: Widen thesessionparameter’s type hint to match supported payloadsThe server-side function accepts any non-string input (dicts, lists, etc.) by running
json.dumpson it. The current annotation (session: str | None) is therefore too restrictive. Please update the signature in
• india_compliance/gst_india/page/india_compliance_account/init.pySuggested diff:
@frappe.whitelist() -def set_auth_session(session: str = None) -> None: +def set_auth_session( + session: str + | dict[str, Any] + | list[Any] + | None = None +) -> None: has_permission_of_page(page_name, throw=True) if not session: _set_auth_session(None) return if not isinstance(session, str): session = json.dumps(session) _set_auth_session(session)• This makes it clear that any JSON-serializable payload is accepted.
• After updating, ensure that no existing callers rely on a narrower type.india_compliance/gst_india/overrides/company.py (1)
254-260: Broaden for_bank annotation or coerce early for whitelisted calls.Whitelisted endpoints often receive "1"/"0" or booleans as strings. You already coerce with int(for_bank), but the type hint int is stricter than reality. Either broaden the annotation or normalize to bool first.
Option A — widen type:
-def get_default_print_options(for_bank: int = 1) -> list: +def get_default_print_options(for_bank: int | str | bool = 1) -> list:Option B — keep int annotation but normalize explicitly:
def get_default_print_options(for_bank: int = 1) -> list: - if int(for_bank): + if int(for_bank): # accepts "1"/True as well return ["Account No.", "Bank Name", "Branch", "IFSC Code", "UPI ID"]india_compliance/audit_trail/utils.py (1)
10-11: Return a JSON-serializable list from the whitelisted methodThe client-facing
@frappe.whitelistendpoint must return a JSON-serializable type (sets aren’t serialized by default). We ran a global search and found only Python callers—no JS/TS usage—so converting tolist[str]is safe for iteration, membership checks, and removals.• Update
india_compliance/audit_trail/utils.py:-@frappe.whitelist() -def get_audit_trail_doctypes() -> set: - return set(frappe.get_hooks("audit_trail_doctypes")) +@frappe.whitelist() +def get_audit_trail_doctypes() -> list[str]: + # Preserve hook order while deduplicating + return list(dict.fromkeys(frappe.get_hooks("audit_trail_doctypes")))• Python consumers (setup, overrides, report) use iteration and
in/remove, all supported on lists.• Optional: other whitelisted functions like
get_relevant_doctypes()also returnset[str]—consider aligning their return types tolist[str]for consistency.india_compliance/gst_india/overrides/transaction.py (1)
856-863: Type hints improve API contract; consider stricter input typing.Current
party_details: dict | strcovers usage, but much of the function relies on attribute-style access viafrappe._dict. Iffrappe.parse_jsonever returns a plaindict, attribute access would fail. Consider normalizing withfrappe._dict(party_details)after parsing to guarantee dot access.india_compliance/gst_india/utils/__init__.py (3)
95-97: Prefer precise generics in return type.Use
list[str]to document element type.-def get_gstin_list( - party: str, party_type: str = "Company", exclude_isd: bool = False -) -> list: +def get_gstin_list( + party: str, party_type: str = "Company", exclude_isd: bool = False +) -> list[str]:
154-154: Optional: narrow the return type for better DX.Consider annotating as
dict[str, typing.Any] | Nonefor clarity. Currentdict | Noneis acceptable if you prefer brevity.
619-620: Prefer precise generics in return type.These are account names; annotate element type.
-@frappe.whitelist() -def get_all_gst_accounts(company: str) -> list: +@frappe.whitelist() +def get_all_gst_accounts(company: str) -> list[str]:india_compliance/gst_india/doctype/gst_invoice_management_system/gst_invoice_management_system.py (2)
228-246: Optional: escape LIKE pattern for supplier GSTINWhile QB parameterizes values, users can still inject
%/_wildcards. If that’s undesirable, escape them to reduce fuzzy matches.Consider:
- .where(PI.supplier_gstin.like(f"%{filters.supplier_gstin}%")) + .where(PI.supplier_gstin.like(f"%{frappe.db.escape_like(filters.supplier_gstin)}%"))
312-345: Double-check the “six months ago” fallback calculation
add_to_date(None, months=-7)labeled “six months ago” is off by one month. If intentional, add a clarifying comment; otherwise change to-6.Would you like me to adjust this and add unit coverage for boundary periods?
india_compliance/gst_india/utils/e_waybill.py (10)
85-93: Harden parsing ofvaluesto ensure attribute access worksDownstream code accesses
values.*attributes.frappe.parse_json(values)may return a plain dict; prefer coercing tofrappe._dictto guarantee attribute access.Apply:
- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values))
146-154: Parsevaluesinto_dictfor consistent attribute accessSame rationale as earlier: subsequent code expects attribute-style access.
- if values: - update_transaction(doc, frappe.parse_json(values)) + if values: + update_transaction(doc, frappe._dict(frappe.parse_json(values)))
271-276: Normalizevaluesto_dictKeeps attribute access (
values.reason, etc.) safe regardless of whether a dict or JSON string is passed.- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values))
332-399: Normalizevaluesto_dictand minor sanitization nit
- Normalize to
_dictto keepvalues.lr_noetc. reliable.- Optional:
lr_nomight benefit from.strip()likevehicle_nosanitization (consistency).- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values)) @@ - "lr_no": values.lr_no, + "lr_no": (values.lr_no or "").strip(),
461-489: Normalizevaluesto_dictbefore usePrevents surprises if a plain dict is provided.
- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values))
513-543: Normalizevaluesto_dictand sanitizelr_noSame consistency improvements as above.
- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values)) @@ - "lr_no": values.lr_no, + "lr_no": (values.lr_no or "").strip(),
609-645: Normalizevaluesto_dictand guard early-returnCoerce
valuesto_dictand retain the early return for falsy payloads.- values = frappe.parse_json(values) + values = frappe._dict(frappe.parse_json(values))
731-744: LGTM with a minor nitManual mark-as-generated path is clear and validates the EWB number. Optional nit: strip/normalize
e_waybill_dateandvalid_uptoif inputs may contain surrounding whitespace.
815-876: Avoid querying e-Waybill Log with None values; usefrappe.parse_jsonfordocs
- If any document lacks
ewaybill,e_waybill_map.values()can containNone, which then becomes part of the["in", ...]filter and can produce odd results.- Prefer
frappe.parse_jsonover rawjson.loadsfor consistency.- docs = json.loads(docs) if isinstance(docs, str) else docs + docs = frappe.parse_json(docs) if isinstance(docs, str) else docs @@ - e_waybill_log = { - log.name: log - for log in frappe.get_all( - "e-Waybill Log", - filters={"name": ["in", e_waybill_map.values()]}, - fields=["name", "is_latest_data"], - ) - } + ewb_names = [n for n in e_waybill_map.values() if n] + e_waybill_log = {} + if ewb_names: + e_waybill_log = { + log.name: log + for log in frappe.get_all( + "e-Waybill Log", + filters={"name": ["in", ewb_names]}, + fields=["name", "is_latest_data"], + ) + }
1088-1104: Guard against missing address and return a user-friendly errorIf the resolved address name is empty/None,
frappe.get_doc("Address", address_name)will throw an unhelpful DoesNotExist. Surface a clearer error.@@ - if address_type == "source_address": + if address_type == "source_address": address_name = address_map.ship_from or address_map.bill_from @@ - elif address_type == "destination_address": + elif address_type == "destination_address": address_name = address_map.ship_to or address_map.bill_to @@ - else: + else: frappe.throw(_("Invalid address type")) - return frappe.get_doc("Address", address_name) + if not address_name: + frappe.throw(_("No Address found for the selected type"), title=_("Missing Address")) + return frappe.get_doc("Address", address_name)india_compliance/gst_india/doctype/purchase_reconciliation_tool/purchase_reconciliation_tool.py (4)
96-110: Return type hint likely incorrect; method returns a list
self.ReconciledData.get()typically returns a list of reconciliation dicts. Declaring-> dictis misleading and breaks static analysis.If
get()still returns a list, adjust the hint:- def reconcile_and_generate_data(self) -> dict: + def reconcile_and_generate_data(self) -> list[dict]:If
get()has been changed elsewhere to return a dict, consider renaming for clarity and add a note.
112-122: Return type isNonebut function returns values; remove returns for clarityThe function is annotated
-> Noneyet returns the results ofsave_gstr_2a/2b. These return values are unused by callers and create confusion.- return save_gstr_2a(self.company_gstin, period, json_data) + save_gstr_2a(self.company_gstin, period, json_data) @@ - return save_gstr_2b(self.company_gstin, period, json_data) + save_gstr_2b(self.company_gstin, period, json_data)
278-313: Validateactionearly to avoid writing unexpected statusesIf
actionisn’t inSTATUS_MAP,statusbecomesNoneand is passed toset_reconciliation_status, potentially writing null/invalid statuses.- status = STATUS_MAP.get(action) + status = STATUS_MAP.get(action) + if status is None and action not in ("Ignore", "Accept"): + frappe.throw(_("Unsupported action: {0}").format(action), title=_("Invalid Action"))
314-329: Consider explicit else branch for unsupported doctypesA small guard that surfaces misuse earlier.
elif doctype == "Bill of Entry": return self.get_bill_of_entry_options(filters) + else: + frappe.throw(_("Unsupported doctype for linking: {0}").format(doctype))
| def update_taxes_in_item_master(taxes: list[dict] | str, hsn_code: str) -> int: | ||
| frappe.enqueue(update_item_document, taxes=taxes, hsn_code=hsn_code, queue="long") | ||
| return 1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate mass item updates behind write permission.
This whitelisted endpoint enqueues updates to all Item records matching an HSN—privileged operation. Add an explicit write-permission check on Item before enqueue.
@frappe.whitelist()
def update_taxes_in_item_master(taxes: list[dict] | str, hsn_code: str) -> int:
+ frappe.has_permission("Item", "write", throw=True)
frappe.enqueue(update_item_document, taxes=taxes, hsn_code=hsn_code, queue="long")
return 1📝 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 update_taxes_in_item_master(taxes: list[dict] | str, hsn_code: str) -> int: | |
| frappe.enqueue(update_item_document, taxes=taxes, hsn_code=hsn_code, queue="long") | |
| return 1 | |
| @frappe.whitelist() | |
| def update_taxes_in_item_master(taxes: list[dict] | str, hsn_code: str) -> int: | |
| # enforce write permission for Item before enqueueing a potentially mass update | |
| frappe.has_permission("Item", "write", throw=True) | |
| frappe.enqueue(update_item_document, taxes=taxes, hsn_code=hsn_code, queue="long") | |
| return 1 |
🤖 Prompt for AI Agents
In india_compliance/gst_india/doctype/gst_hsn_code/gst_hsn_code.py around lines
21-23, the function enqueues mass updates to Item records without verifying the
caller has write permission on the Item doctype; add an explicit permission
check before enqueueing: call frappe.has_permission("Item", ptype="write") and
if it fails either raise a PermissionError (frappe.PermissionError or
frappe.throw with appropriate HTTP 403 message) or return an error/zero, and
only enqueue update_item_document when the permission check passes.
| def unlink_documents(self, data: dict | str) -> dict: | ||
| frappe.has_permission("GST Invoice Management System", "write", throw=True) | ||
|
|
||
| purchases, inward_supplies = _unlink_documents(data) | ||
|
|
||
| return self.get_invoice_data(inward_supplies, purchases) | ||
|
|
There was a problem hiding this comment.
Return type mismatch for unlink_documents
This returns self.get_invoice_data(...) (list), but annotation says -> dict.
Apply this diff:
- def unlink_documents(self, data: dict | str) -> dict:
+ def unlink_documents(self, data: dict | str) -> list[dict]:📝 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 unlink_documents(self, data: dict | str) -> dict: | |
| frappe.has_permission("GST Invoice Management System", "write", throw=True) | |
| purchases, inward_supplies = _unlink_documents(data) | |
| return self.get_invoice_data(inward_supplies, purchases) | |
| def unlink_documents(self, data: dict | str) -> list[dict]: | |
| frappe.has_permission("GST Invoice Management System", "write", throw=True) | |
| purchases, inward_supplies = _unlink_documents(data) | |
| return self.get_invoice_data(inward_supplies, purchases) |
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gst_invoice_management_system/gst_invoice_management_system.py
around lines 220 to 226, the method unlink_documents is annotated to return dict
but actually returns the result of self.get_invoice_data(...) which is a list;
update the signature to reflect the correct return type (e.g., -> list[dict] or
-> list) to match get_invoice_data, or alternatively change the method to return
a dict by wrapping or converting the list into the expected dict structure;
ensure imports/typing are adjusted if using list[dict].
| def unlink_documents(self, data: str) -> dict: | ||
| frappe.has_permission("Purchase Reconciliation Tool", "write", throw=True) | ||
|
|
||
| purchases, inward_supplies = _unlink_documents(data) | ||
|
|
||
| return self.ReconciledData.get(purchases, inward_supplies) | ||
|
|
There was a problem hiding this comment.
Same type hint mismatch as link_documents
This also returns a list from ReconciledData.get(...) while annotated -> dict.
- def unlink_documents(self, data: str) -> dict:
+ def unlink_documents(self, data: str) -> list[dict]:📝 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 unlink_documents(self, data: str) -> dict: | |
| frappe.has_permission("Purchase Reconciliation Tool", "write", throw=True) | |
| purchases, inward_supplies = _unlink_documents(data) | |
| return self.ReconciledData.get(purchases, inward_supplies) | |
| def unlink_documents(self, data: str) -> list[dict]: | |
| frappe.has_permission("Purchase Reconciliation Tool", "write", throw=True) | |
| purchases, inward_supplies = _unlink_documents(data) | |
| return self.ReconciledData.get(purchases, inward_supplies) |
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/purchase_reconciliation_tool/purchase_reconciliation_tool.py
around lines 270–276, the method unlink_documents is annotated to return dict
but actually returns whatever ReconciledData.get(...) yields (a list); update
the signature to the correct type (e.g., -> list or -> list[dict] /
typing.List[dict]) or, if a dict is required, transform the returned list into
the expected dict structure before returning; also add any needed typing imports
(from typing import List) and keep permission check and logic unchanged.
| def set_item_wise_tax_rates( | ||
| doc: str, item_name: str | None = None, tax_name: str | None = None | ||
| ) -> dict: | ||
| doc = json.loads(doc, object_hook=frappe._dict) |
There was a problem hiding this comment.
Return type annotation doesn’t match behavior; function returns None.
The whitelisted set_item_wise_tax_rates is annotated to return dict but doesn’t return anything. Either return the mutated doc or change the annotation to -> None. Given many RPC clients expect a return payload, returning the doc is preferable.
Apply one of the following:
Option A (return the doc):
@frappe.whitelist()
-def set_item_wise_tax_rates(
- doc: str, item_name: str | None = None, tax_name: str | None = None
-) -> dict:
+def set_item_wise_tax_rates(
+ doc: str, item_name: str | None = None, tax_name: str | None = None
+) -> dict:
doc = json.loads(doc, object_hook=frappe._dict)
CustomTaxController(doc).set_item_wise_tax_rates(item_name, tax_name)
frappe.response.docs.append(doc)
+ return docOption B (pure side-effect API):
-) -> dict:
+) -> None:
@@
- frappe.response.docs.append(doc)
+ frappe.response.docs.append(doc)Pick one and ensure callers are aligned.
📝 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 set_item_wise_tax_rates( | |
| doc: str, item_name: str | None = None, tax_name: str | None = None | |
| ) -> dict: | |
| doc = json.loads(doc, object_hook=frappe._dict) | |
| @frappe.whitelist() | |
| def set_item_wise_tax_rates( | |
| doc: str, item_name: str | None = None, tax_name: str | None = None | |
| ) -> dict: | |
| doc = json.loads(doc, object_hook=frappe._dict) | |
| CustomTaxController(doc).set_item_wise_tax_rates(item_name, tax_name) | |
| frappe.response.docs.append(doc) | |
| return doc |
🤖 Prompt for AI Agents
In india_compliance/gst_india/utils/taxes_controller.py around lines 48 to 51,
the whitelisted function set_item_wise_tax_rates is annotated to return dict but
currently returns None; change it to actually return the mutated doc (preferred
Option A): after loading and mutating doc, ensure you return the resulting dict
(or JSON-decoded frappe._dict) and keep the -> dict annotation, update any
callers if they expect no return, and add a short docstring comment indicating
the function returns the updated doc for RPC consumers.
…l, and itc_04_export modules
karm1000
changed the title
1 participant
Fixes: #3625
Summary by CodeRabbit