feat: Align Excel Report with Government Format #3451
Conversation
WalkthroughThe updates introduce conditional logic for Excel template selection based on GST return periods, enhance the Excel exporting utility to support loading from templates and inserting data into existing sheets, and adjust an enum value for sheet name consistency. Method signatures and a class constructor are updated to support these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GovExcel
participant ExcelExporter
User->>GovExcel: generate(gstin, period)
GovExcel->>GovExcel: Extract filing month/year from period
GovExcel->>GovExcel: Determine bifurcate_hsn flag
GovExcel->>GovExcel: Select Excel template (V2.0 or V2.1)
GovExcel->>ExcelExporter: build_excel(data, bifurcate_hsn)
ExcelExporter->>ExcelExporter: Load template or create workbook
GovExcel->>ExcelExporter: Insert or create sheets with data
ExcelExporter-->>GovExcel: Return Excel file
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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: 2
🧹 Nitpick comments (1)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (1)
192-221: Template sheet removal executed in every loop iteration
excel.remove_sheet("Sheet")is inside theelsebranch, so if multiple unmapped
categories exist we call it repeatedly; subsequent calls are no-ops but waste cycles.@@ - excel.create_sheet( + if "Sheet" in excel.wb.sheetnames: + excel.remove_sheet("Sheet") + + excel.create_sheet(Moving the removal outside the loop (just after
ExcelExporter(file)construction) is cleaner and avoids redundant map look-ups.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
india_compliance/gst_india/data/GSTR1_Excel_Workbook_Template_V2.0.xlsxis excluded by!**/*.xlsxindia_compliance/gst_india/data/GSTR1_Excel_Workbook_Template_V2.1.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (3)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py(3 hunks)india_compliance/gst_india/utils/exporter.py(2 hunks)india_compliance/gst_india/utils/gstr_1/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
india_compliance/gst_india/utils/gstr_1/__init__.py (1)
india_compliance/gst_india/utils/gstr_1/gstr_1_json_map.py (1)
B2B(135-301)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (3)
india_compliance/gst_india/utils/__init__.py (2)
get_data_file_path(327-328)get_period(1032-1041)india_compliance/gst_india/utils/exporter.py (4)
ExcelExporter(11-66)insert_data(33-40)create_sheet(18-31)remove_sheet(52-55)india_compliance/gst_india/doctype/gst_return_log/gst_return_log.py (1)
load_data(35-50)
🪛 Pylint (3.3.7)
india_compliance/gst_india/utils/exporter.py
[refactor] 33-33: Too many arguments (6/5)
(R0913)
[refactor] 33-33: Too many positional arguments (6/5)
(R0917)
🪛 GitHub Actions: Linters
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
[error] 251-251: flake8: undefined name 'df' (F821)
[error] 1050-1050: flake8: undefined name 'df' (F821)
⏰ 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 (2)
india_compliance/gst_india/utils/gstr_1/__init__.py (1)
298-298: Enum value change acknowledgedString trimmed to
b2b,sez,de. No functional issues spotted; mapping dicts below already reference.value, so nothing else to update.india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (1)
144-147: Period slicing is brittle for future period formats
return_period[:2]/return_period[2:]assumes a fixedMMYYYYlayout.
Quarterly periods (042024Q1,062024, etc.) or any future format will break the date parsing and the HSN bifurcation logic.- month, year = gstr_1_log.return_period[:2], gstr_1_log.return_period[2:] - filing_from = getdate(f"{year}-{month}-01") + try: + filing_from = datetime.strptime(gstr_1_log.return_period, "%m%Y").date() + except ValueError: + # fallback for quarter/other formats – default to template V2.1 + filing_from = HSN_BIFURCATION_FROMPlease verify all period variants used in production (monthly & quarterly) still hit the intended template.
| def insert_data(self, sheet_name, fields, data, start_row=1, start_column=1): | ||
| sheet = self.wb[sheet_name] | ||
|
|
||
| for i, row in enumerate(data, start=start_row): | ||
| for j, field in enumerate(fields, start=start_column): | ||
| value = row.get(field) | ||
| if value is not None: | ||
| sheet.cell(row=i, column=j, value=value) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate sheet existence & propagate data shape errors
self.wb[sheet_name] throws a KeyError if the template doesn’t contain the sheet – easy to hit when template versions drift.
Additionally, silently skipping None values can leave stale data in reused rows.
- sheet = self.wb[sheet_name]
+ try:
+ sheet = self.wb[sheet_name]
+ except KeyError as exc:
+ raise ValueError(f"Worksheet '{sheet_name}' not found in workbook") from excConsider:
- Clearing the destination row before writing, or writing explicit
""to keep column alignment. - Using
sheet.max_row + 1whenstart_rowis omitted to append instead of overwrite.
These small checks will prevent corrupt exports and make debugging easier.
📝 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 insert_data(self, sheet_name, fields, data, start_row=1, start_column=1): | |
| sheet = self.wb[sheet_name] | |
| for i, row in enumerate(data, start=start_row): | |
| for j, field in enumerate(fields, start=start_column): | |
| value = row.get(field) | |
| if value is not None: | |
| sheet.cell(row=i, column=j, value=value) | |
| def insert_data(self, sheet_name, fields, data, start_row=1, start_column=1): | |
| - sheet = self.wb[sheet_name] | |
| + try: | |
| + sheet = self.wb[sheet_name] | |
| + except KeyError as exc: | |
| + raise ValueError(f"Worksheet '{sheet_name}' not found in workbook") from exc | |
| + | |
| for i, row in enumerate(data, start=start_row): | |
| for j, field in enumerate(fields, start=start_column): | |
| value = row.get(field) | |
| if value is not None: | |
| sheet.cell(row=i, column=j, value=value) |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 33-33: Too many arguments (6/5)
(R0913)
[refactor] 33-33: Too many positional arguments (6/5)
(R0917)
🤖 Prompt for AI Agents
In india_compliance/gst_india/utils/exporter.py around lines 33 to 41, add a
check to verify if sheet_name exists in self.wb before accessing it to avoid
KeyError. Modify the code to clear or overwrite the entire destination row to
prevent stale data by writing empty strings for None values instead of skipping
them. Also, if start_row is not provided, default it to sheet.max_row + 1 to
append data rather than overwrite existing rows. These changes will ensure safer
data insertion and better alignment.
| class ExcelExporter: | ||
| def __init__(self): | ||
| self.wb = openpyxl.Workbook() | ||
| def __init__(self, file=None): | ||
| if file: | ||
| self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False) | ||
| else: | ||
| self.wb = openpyxl.Workbook() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against missing template & avoid masking built-in file
openpyxl.load_workbook will raise FileNotFoundError / InvalidFileException when the path is wrong or points to a non-xlsx.
Right now we let that bubble as an unhandled traceback.
Also, the parameter is named file, shadowing Python’s built-in file type.
- def __init__(self, file=None):
- if file:
- self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False)
+ def __init__(self, template_path: str | BytesIO | None = None):
+ """
+ :param template_path: Existing workbook (path or file-like).
+ If None, a new workbook is created.
+ """
+ try:
+ if template_path:
+ self.wb = openpyxl.load_workbook(
+ template_path, read_only=False, keep_links=False
+ )
+ else:
+ self.wb = openpyxl.Workbook()
+ except FileNotFoundError:
+ frappe.throw(_("Excel template not found: {0}").format(template_path))
+ except Exception as exc:
+ frappe.throw(_("Failed to load Excel template: {0}").format(exc))This makes the API safer and eliminates the name clash.
📝 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 ExcelExporter: | |
| def __init__(self): | |
| self.wb = openpyxl.Workbook() | |
| def __init__(self, file=None): | |
| if file: | |
| self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False) | |
| else: | |
| self.wb = openpyxl.Workbook() | |
| class ExcelExporter: | |
| def __init__(self, template_path: str | BytesIO | None = None): | |
| """ | |
| :param template_path: Existing workbook (path or file-like). | |
| If None, a new workbook is created. | |
| """ | |
| try: | |
| if template_path: | |
| self.wb = openpyxl.load_workbook( | |
| template_path, read_only=False, keep_links=False | |
| ) | |
| else: | |
| self.wb = openpyxl.Workbook() | |
| except FileNotFoundError: | |
| frappe.throw(_("Excel template not found: {0}").format(template_path)) | |
| except Exception as exc: | |
| frappe.throw(_("Failed to load Excel template: {0}").format(exc)) |
🤖 Prompt for AI Agents
In india_compliance/gst_india/utils/exporter.py around lines 11 to 17, rename
the constructor parameter from 'file' to something like 'filepath' to avoid
shadowing the built-in 'file' type. Add a try-except block around the call to
openpyxl.load_workbook to catch FileNotFoundError and InvalidFileException, and
handle these exceptions gracefully, such as by logging an error or raising a
custom exception, to prevent unhandled tracebacks when the file is missing or
invalid.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3451 +/- ##
===========================================
- Coverage 59.43% 59.34% -0.10%
===========================================
Files 136 136
Lines 13930 13955 +25
===========================================
+ Hits 8279 8281 +2
- Misses 5651 5674 +23
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
india_compliance/gst_india/utils/exporter.py (1)
12-17: Constructor still shadows built-infileand lacks safe load handlingPrevious review already pointed this out, but the argument is still named
fileandopenpyxl.load_workbookis still un-guarded.
UncaughtFileNotFoundError,InvalidFileException, etc. will propagate as a traceback.- def __init__(self, file=None): - if file: - self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False) + def __init__(self, template_path: str | BytesIO | None = None): + try: + if template_path: + self.wb = openpyxl.load_workbook( + template_path, read_only=False, keep_links=False + ) + else: + self.wb = openpyxl.Workbook() + except FileNotFoundError as exc: + frappe.throw(_("Excel template not found: {0}").format(template_path)) + except Exception as exc: + frappe.throw(_("Failed to load Excel template: {0}").format(exc))This also removes the name clash with the built-in
file.
🧹 Nitpick comments (1)
india_compliance/gst_india/utils/exporter.py (1)
147-156: Signature is getting unwieldy – eight positional params trigger pylint R0913/R0917The new helper already hits the “too many arguments” warning. Group related arguments into a dataclass or accept
**optionsto keep the public API stable while remaining lint-clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
india_compliance/gst_india/data/GSTR1_Excel_Template_V2.0.xlsxis excluded by!**/*.xlsxindia_compliance/gst_india/data/GSTR1_Excel_Template_V2.1.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (2)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py(5 hunks)india_compliance/gst_india/utils/exporter.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
🧰 Additional context used
🪛 Pylint (3.3.7)
india_compliance/gst_india/utils/exporter.py
[refactor] 147-147: Too many arguments (8/5)
(R0913)
[refactor] 147-147: Too many positional arguments (8/5)
(R0917)
[refactor] 147-147: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
india_compliance/gst_india/utils/exporter.py (1)
246-248:custom_styleslookup silently fails ifstyle_nameisNoneThis is a corollary of the bug above – once fixed, ensure
style_nameis neverNonebefore dereferencing:if style_name: header = kwargs.get("header") or self.headers[column - 1] custom_styles = header.get(style_name) if custom_styles: style.update(custom_styles)Fail fast with a clear error when an unknown flag is supplied.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
india_compliance/gst_india/utils/exporter.py (1)
233-243:apply_formatstill non-deterministic –headerkey competes with style flag
headeris fetched but not removed beforepopitem(), so it may be the item popped, breaking style resolution (style_name = None).- key, value = kwargs.popitem() + # remove meta-parameter first + header_cfg = kwargs.pop("header", None) + if len(kwargs) != 1: + raise ValueError( + "Exactly one style flag (is_data / is_header / is_total / is_filter) " + "must be supplied" + ) + key, value = kwargs.popitem()
♻️ Duplicate comments (3)
india_compliance/gst_india/utils/exporter.py (3)
12-17: Still shadowing the built-infileand no guard for invalid pathsThe constructor continues to receive a parameter named
file, overriding Python’s built-in and missing the previously-requestedtry/exceptaroundopenpyxl.load_workbook. Any wrong path will raise an unhandled stack-trace.- def __init__(self, file=None): - if file: - self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False) + def __init__(self, template_path: str | BytesIO | None = None): + """ + :param template_path: Path / file-like object pointing to an .xlsx template. + If None, a new blank workbook is created. + """ + try: + if template_path: + self.wb = openpyxl.load_workbook( + template_path, read_only=False, keep_links=False + ) + self.wb.calculation.fullCalcOnLoad = False + else: + self.wb = openpyxl.Workbook() + except FileNotFoundError as exc: + frappe.throw(_("Excel template not found: {0}").format(template_path)) + except Exception as exc: # noqa: BLE001 – propagate a user-friendly message + frappe.throw(_("Failed to load Excel template: {0}").format(exc)) - self.wb.calculation.fullCalcOnLoad = False - else: - self.wb = openpyxl.Workbook()
34-45:Worksheet()re-instantiated on every call & no sheet existence checkSpinning up a fresh
Worksheet()each time discards state and still hitsKeyErrorwhensheet_nameis missing. This is exactly the issue pointed out in the previous review.- Worksheet().insert_data(workbook=self.wb, **kwargs) + ws_util = Worksheet() + try: + ws_util.insert_data(workbook=self.wb, **kwargs) + except KeyError as exc: + raise ValueError( + f"Worksheet '{kwargs.get('sheet_name')}' not found in template" + ) from exc
157-160: Unprotected access toworkbook[sheet_name]will crash on mismatched templatesA missing sheet name detonates the export flow with a
KeyError. Wrap it and raise a descriptive error as suggested earlier.
🧹 Nitpick comments (2)
india_compliance/gst_india/utils/exporter.py (2)
147-156: API surface is growing unwieldy – 8 positional params trigger Pylint R0913/R0917
insert_datanow takes 8 positional arguments. Consider collapsing rarely-used ones into**optionsor using a small dataclass/@dataclassfor clarity.
233-243: Style override lookup can crash whenheaderisNoneIf the caller does not pass a
headerkwarg (usual path),header = None or self.headers[column-1]is fine.
But whenheaderis passed asNone,header.get()will raise. Add a guard.- header = kwargs.get("header") or self.headers[column - 1] - custom_styles = header.get(style_name) + header_dict = kwargs.get("header") or self.headers[column - 1] + custom_styles = header_dict.get(style_name) if header_dict else None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
india_compliance/gst_india/data/GSTR1_Excel_Template_V2.0.xlsxis excluded by!**/*.xlsxindia_compliance/gst_india/data/GSTR1_Excel_Template_V2.1.xlsxis excluded by!**/*.xlsx
📒 Files selected for processing (2)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py(17 hunks)india_compliance/gst_india/utils/exporter.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
🧰 Additional context used
🪛 Pylint (3.3.7)
india_compliance/gst_india/utils/exporter.py
[refactor] 147-147: Too many arguments (8/5)
(R0913)
[refactor] 147-147: Too many positional arguments (8/5)
(R0917)
[refactor] 147-147: Too many local variables (16/15)
(R0914)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
| for i, row in enumerate(data, start=start_row): | ||
| for j, header in enumerate(headers, start=start_column): | ||
| fieldname, transform = header.get("fieldname"), header.get("transform") | ||
| value = row.get(fieldname) | ||
|
|
||
| if transform: | ||
| value = transform(value) | ||
|
|
||
| if value is not None: | ||
| sheet.cell(row=i, column=j, value=value) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Row data with None leaves stale values & no style is applied
- Skipping
Nonewrites means reused template rows may keep old values. - Inserting without calling
apply_styleleads to inconsistent fonts/number-formats compared tocreate().
At minimum, write an empty string for None and optionally call a light apply_style for consistency.
- if value is not None:
- sheet.cell(row=i, column=j, value=value)
+ cell = sheet.cell(row=i, column=j, value=value or "")
+ # optional: propagate default data style
+ # self.apply_style(i, j, self.data_format)📝 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.
| for i, row in enumerate(data, start=start_row): | |
| for j, header in enumerate(headers, start=start_column): | |
| fieldname, transform = header.get("fieldname"), header.get("transform") | |
| value = row.get(fieldname) | |
| if transform: | |
| value = transform(value) | |
| if value is not None: | |
| sheet.cell(row=i, column=j, value=value) | |
| for i, row in enumerate(data, start=start_row): | |
| for j, header in enumerate(headers, start=start_column): | |
| fieldname, transform = header.get("fieldname"), header.get("transform") | |
| value = row.get(fieldname) | |
| if transform: | |
| value = transform(value) | |
| - if value is not None: | |
| - sheet.cell(row=i, column=j, value=value) | |
| + # always write something (empty string for None) to avoid stale cells | |
| + cell = sheet.cell(row=i, column=j, value=value if value is not None else "") | |
| + # optional: propagate default data style | |
| + # self.apply_style(i, j, self.data_format) |
🤖 Prompt for AI Agents
In india_compliance/gst_india/utils/exporter.py around lines 162 to 172, the
code skips writing cells when the value is None, causing stale values to remain
in reused template rows and styles not being applied consistently. To fix this,
modify the code to write an empty string to the cell when the value is None
instead of skipping it, and ensure that apply_style is called on the cell to
maintain consistent fonts and number formats as done in the create() method.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
india_compliance/gst_india/utils/exporter.py (1)
228-239:apply_formatis still non-deterministic when bothheaderand a style flag are passed
kwargs.popitem()pops an arbitrary key; if it removesheader,style_namebecomesNone→AttributeError. Past feedback highlighted this.- key, value = kwargs.popitem() + # Separate meta-parameter `header` + header = kwargs.pop("header", None) + if len(kwargs) != 1: + raise ValueError("Exactly one style flag (is_data / is_header / …) must be passed") + key, value = kwargs.popitem()
♻️ Duplicate comments (3)
india_compliance/gst_india/utils/exporter.py (3)
155-167: Stale template values & missing style propagation
- Skipping cells when
value is Nonekeeps old data in reused rows.- No call to
apply_format, so rows inserted into templates lose consistent styling.- if value is not None: - sheet.cell(row=i, column=j, value=value) + cell = sheet.cell(row=i, column=j, value="" if value is None else value) + # Propagate default data style for consistency + # self.apply_format(row=i, column=j, is_data=True, header=header)
34-44:insert_dataduplicatesWorksheet()per call & still risksKeyErrorEvery invocation creates a new
Worksheet()instance, losing any internal state, andWorkbook[sheet_name]is accessed without validation.- Worksheet().insert_data(workbook=self.wb, **kwargs) + ws_util = Worksheet() + try: + ws_util.insert_data(workbook=self.wb, **kwargs) + except KeyError as exc: + raise ValueError(f"Worksheet '{kwargs.get('sheet_name')}' not found in template") from exc
11-18: Renamefileparameter and add defensive load handling– Shadows the built-in
filetype.
–openpyxl.load_workbookexceptions (FileNotFoundError,InvalidFileException) are still unhandled, bubbling a raw traceback to users.- def __init__(self, file=None): - if file: - self.wb = openpyxl.load_workbook(file, read_only=False, keep_links=False) + def __init__(self, template_path: str | BytesIO | None = None): + try: + if template_path: + self.wb = openpyxl.load_workbook( + template_path, read_only=False, keep_links=False + ) + else: + self.wb = openpyxl.Workbook() + except FileNotFoundError: + frappe.throw(_("Excel template not found: {0}").format(template_path)) + except Exception as exc: + frappe.throw(_("Failed to load Excel template: {0}").format(exc))
🧹 Nitpick comments (1)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (1)
261-272:get_fieldshelper is unused – dead codeThe method is never referenced after introduction; consider removing or wiring it up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py(17 hunks)india_compliance/gst_india/utils/exporter.py(4 hunks)india_compliance/gst_india/utils/gstr_1/__init__.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- india_compliance/gst_india/utils/gstr_1/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (3)
india_compliance/gst_india/utils/__init__.py (2)
get_data_file_path(327-328)get_period(1032-1041)india_compliance/gst_india/utils/exporter.py (5)
ExcelExporter(11-71)insert_data(34-43)insert_data(146-166)create_sheet(19-32)remove_sheet(57-60)india_compliance/gst_india/doctype/gst_return_log/gst_return_log.py (1)
load_data(35-50)
🪛 Pylint (3.3.7)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
[error] 468-468: Instance of 'str' has no 'value' member
(E1101)
[error] 469-469: Class 'GovExcelField' has no 'TRANSACTION_TYPE' member
(E1101)
india_compliance/gst_india/utils/exporter.py
[refactor] 146-146: Too many arguments (7/5)
(R0913)
[refactor] 146-146: Too many positional arguments (7/5)
(R0917)
⏰ 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 (1)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py (1)
199-204: Template path may beNone– guard before loading
self.TEMPLATE_EXCEL_FILE.get(version)returnsNoneif the dict key is mistyped.
ExcelExporter(None)falls back to an empty workbook, silently ignoring the missing template.- file = self.TEMPLATE_EXCEL_FILE.get(version) - excel = ExcelExporter(file) + template_path = self.TEMPLATE_EXCEL_FILE.get(version) + if not template_path: + frappe.throw(_(f"Excel template for version {version} not found.")) + + excel = ExcelExporter(template_path)
| "transform": lambda x: x.strftime("%d-%b-%y"), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
strftime without None guard – crashes on blank dates
Several headers use lambda x: x.strftime("%d-%b-%y").
If the source field is None, this raises AttributeError.
Example fix (apply to all similar lambdas):
-"transform": lambda x: x.strftime("%d-%b-%y"),
+"transform": lambda x: x.strftime("%d-%b-%y") if x else "",Also applies to: 363-363, 466-471, 533-533, 589-589, 610-610
🤖 Prompt for AI Agents
In india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py at lines
299-300 and similarly at lines 363, 466-471, 533, 589, and 610, the lambda
functions use x.strftime("%d-%b-%y") without checking if x is None, causing
AttributeError on blank dates. Update these lambdas to first check if x is not
None before calling strftime, returning an empty string or None otherwise to
prevent crashes.
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
Outdated
Show resolved
Hide resolved
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_export.py
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit
Summary by CodeRabbit