feat: add GSTR-1 Reconciliation #3388
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new "GSTR-1 Reconciliation" feature is introduced for the GST India module, including a DocType definition, client-side event handlers, backend reconciliation logic, and test scaffolding. The implementation covers form setup, data fetching, reconciliation orchestration, and placeholders for unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClientForm
participant ServerAPI
participant GSTNAPI
participant Database
User->>ClientForm: Open GSTR-1 Reconciliation form
ClientForm->>ServerAPI: Fetch GSTINs for selected company
ServerAPI->>Database: Query GSTINs
Database-->>ServerAPI: Return GSTIN list
ServerAPI-->>ClientForm: Return GSTINs
ClientForm->>User: Display form with default GSTIN
User->>ClientForm: Click "Generate"
ClientForm->>ServerAPI: Call get_gstr_1_reconciliation
ServerAPI->>Database: Validate fiscal year, fetch logs
loop For each month in fiscal year
ServerAPI->>GSTNAPI: Fetch government GSTR-1 data
ServerAPI->>Database: Fetch books data
ServerAPI->>ServerAPI: Reconcile and summarize data
end
ServerAPI-->>ClientForm: Return reconciliation summary
ClientForm->>User: Display reconciliation results
Poem
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 (5)
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py (1)
63-67: Unusedis_queuedflag – lost information
get_gov_gstr1_data()returns(gov_data, is_queued)butis_queuedis never examined.
Consider short-circuiting further processing (or informing the user) when the data is still queued.india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.js (2)
31-38: Optional chaining cleans up null-checks
The static analysis hint is valid. Modern JS allowsgstin_list?.lengthandgstin_list?.[0], reducing branching noise.- if (gstin_list && gstin_list.length) { - frm.set_value("company_gstin", gstin_list[0]); + if (gstin_list?.length) { + frm.set_value("company_gstin", gstin_list[0]); }Ensure Babel/TS config targets ES2020+.
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
19-23: No error feedback fromtaxpayer_api_call
If the backend throws (e.g., missing OTP), the user receives no notification. Wrap the promise infrm.callstyle or attach.then/.catchto surface messages and optionally show a progress indicator.india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.json (2)
20-24: Fields marked “In List View” have no effect on Single DocTypes
Becauseissingle = 1, the framework never shows a list view. Settingin_list_viewis redundant noise—consider removing for clarity.
30-35: GSTIN as “Autocomplete” may bypass validation
A GSTIN is a fixed-length 15-char identifier. Usingfieldtype: Datawith a regex or a dedicated GSTIN Link DocType enforces correctness; free-text autocomplete could store invalid values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.js(1 hunks)india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.json(1 hunks)india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py(1 hunks)india_compliance/gst_india/doctype/gstr_1_reconciliation/test_gstr_1_reconciliation.py(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.js
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 Ruff (0.11.9)
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
9-9: india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI imported but unused
Remove unused import: india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI
(F401)
🪛 GitHub Actions: Linters
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
[error] 8-8: ESLint: 'india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI' imported but unused. (flake8 F401)
⏰ 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
| books_data = gstr_log.get_books_gstr1_data(gstr_log) | ||
|
|
||
| try: | ||
| gov_data, is_queued = gstr_log.get_gov_gstr1_data() | ||
| except frappe.ValidationError as error: | ||
| frappe.throw(title="GSTR-1 Generation Failed", msg=str(error)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suspicious extra argument in get_books_gstr1_data call
gstr_log.get_books_gstr1_data(gstr_log) passes the log instance to its own method. Unless the method signature explicitly requires an external log object, this is likely redundant and hides bugs if the API changes.
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
around lines 60 to 66, the method get_books_gstr1_data is called with gstr_log
passed as an argument, which is likely unnecessary since it is an instance
method. Remove the gstr_log argument from the call so it is invoked as
gstr_log.get_books_gstr1_data() to avoid redundant or incorrect parameter
passing.
| def summarize_data(self, data): | ||
| """ | ||
| Summarize the data for the given filters (Aggregate the data) | ||
| """ | ||
| pass |
There was a problem hiding this comment.
🛠️ Refactor suggestion
summarize_data stub blocks feature completion
The public method returns pass, so the overall API already advertised by the UI will always return empty summaries. Either implement the logic now or raise NotImplementedError to avoid silent failures.
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
at lines 112 to 116, the summarize_data method is currently a stub that returns
pass, causing the API to return empty summaries silently. To fix this, either
implement the actual data summarization logic as intended or replace the pass
statement with a raise NotImplementedError to explicitly indicate the method is
not yet implemented and prevent silent failures.
| from india_compliance.gst_india.api_classes.taxpayer_base import ( | ||
| TaxpayerBaseAPI, | ||
| otp_handler, | ||
| ) | ||
| from india_compliance.gst_india.doctype.gst_return_log.gst_return_log import ( | ||
| get_gst_return_log, | ||
| ) |
There was a problem hiding this comment.
Remove unused import to make linters green
TaxpayerBaseAPI is imported but never referenced, which triggers Ruff / flake8 F401 and fails the pipeline.
-from india_compliance.gst_india.api_classes.taxpayer_base import (
- TaxpayerBaseAPI,
- otp_handler,
-)
+from india_compliance.gst_india.api_classes.taxpayer_base import otp_handler📝 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.
| from india_compliance.gst_india.api_classes.taxpayer_base import ( | |
| TaxpayerBaseAPI, | |
| otp_handler, | |
| ) | |
| from india_compliance.gst_india.doctype.gst_return_log.gst_return_log import ( | |
| get_gst_return_log, | |
| ) | |
| from india_compliance.gst_india.api_classes.taxpayer_base import otp_handler | |
| from india_compliance.gst_india.doctype.gst_return_log.gst_return_log import ( | |
| get_gst_return_log, | |
| ) |
🧰 Tools
🪛 Ruff (0.11.9)
9-9: india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI imported but unused
Remove unused import: india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI
(F401)
🪛 GitHub Actions: Linters
[error] 8-8: ESLint: 'india_compliance.gst_india.api_classes.taxpayer_base.TaxpayerBaseAPI' imported but unused. (flake8 F401)
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
between lines 8 and 14, remove the unused import TaxpayerBaseAPI from
india_compliance.gst_india.api_classes.taxpayer_base to fix the F401 linting
error and make the pipeline pass.
| current_date = year_start_date | ||
| now_date = getdate() | ||
| # Ensure we do not go beyond the current date in the current year | ||
| if year_end_date.year == now_date.year: | ||
| last_date = now_date | ||
|
|
||
| while current_date <= last_date: | ||
| period = current_date.strftime("%m%Y") | ||
| log_name = f"GSTR-1-{period}-{self.company_gstin}" |
There was a problem hiding this comment.
last_date may be undefined → possible UnboundLocalError
When the fiscal year is a past year (year_end_date.year != now_date.year), the else branch never sets last_date, yet it is referenced in the while loop.
- if year_end_date.year == now_date.year:
- last_date = now_date
+ if year_end_date.year == now_date.year:
+ last_date = now_date
+ else:
+ last_date = year_end_date📝 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.
| current_date = year_start_date | |
| now_date = getdate() | |
| # Ensure we do not go beyond the current date in the current year | |
| if year_end_date.year == now_date.year: | |
| last_date = now_date | |
| while current_date <= last_date: | |
| period = current_date.strftime("%m%Y") | |
| log_name = f"GSTR-1-{period}-{self.company_gstin}" | |
| current_date = year_start_date | |
| now_date = getdate() | |
| # Ensure we do not go beyond the current date in the current year | |
| if year_end_date.year == now_date.year: | |
| last_date = now_date | |
| else: | |
| last_date = year_end_date | |
| while current_date <= last_date: | |
| period = current_date.strftime("%m%Y") | |
| log_name = f"GSTR-1-{period}-{self.company_gstin}" |
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.py
around lines 95 to 103, the variable last_date is only set inside the if block
when year_end_date.year equals now_date.year, but it is used later in the while
loop regardless. To fix this, initialize last_date to year_end_date before the
if statement so it is always defined, ensuring the loop has a valid end date
whether the fiscal year is current or past.
| frm.disable_save(); | ||
| frm.page.set_primary_action(__("Generate"), () => frm.trigger("generate")); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Primary action added on every refresh → duplicate handlers
refresh runs on each form reload; set_primary_action will bind a new click callback every time, causing multiple identical listeners. Guard with a flag or use frm.page.clear_primary_action() first.
🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/gstr_1_reconciliation.js
around lines 15 to 17, the primary action is set on every form refresh, causing
duplicate click handlers. To fix this, clear existing primary actions first by
calling frm.page.clear_primary_action() before setting the new primary action,
or use a flag to ensure the handler is only bound once.
| class UnitTestGSTR1Reconciliation(UnitTestCase): | ||
| """ | ||
| Unit tests for GSTR1Reconciliation. | ||
| Use this class for testing individual functions and methods. | ||
| """ | ||
|
|
||
| pass | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests are placeholders – please add at least one sanity test
CI will mark the module as “collected 0 tests”, offering no regression guard. Suggest starting with:
def test_minimum_api_response(self):
doc = frappe.get_doc({
"doctype": "GSTR-1 Reconciliation",
"company": "Test Company",
"company_gstin": "27ABCDE1234F1Z5",
"fiscal_year": "2024-25",
})
with self.assertRaises(frappe.ValidationError):
doc.get_gstr_1_reconciliation()🤖 Prompt for AI Agents
In
india_compliance/gst_india/doctype/gstr_1_reconciliation/test_gstr_1_reconciliation.py
around lines 14 to 21, the test class is empty and contains no actual tests,
causing the CI to report zero tests collected. Add at least one sanity test
method inside the UnitTestGSTR1Reconciliation class that creates a GSTR-1
Reconciliation document with minimal required fields and asserts that calling
get_gstr_1_reconciliation raises a ValidationError. This will ensure the test
suite runs and provides basic regression coverage.
Summary by CodeRabbit