feat: add rounding difference row in gstr-1 beta #3418
Conversation
WalkthroughThe GST India module's GSTR-1 Beta feature was enhanced with functionality to handle rounding differences. This includes new methods to display rounding difference rows in summary tables, create journal entries for rounding adjustments, and fetch relevant accounts and posting dates via API. Currency precision for rounding was also made dynamic. Test cases were updated to reflect more precise rounding values. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileGSTR1Dialog
participant GSTR1
participant BackendAPI
participant BooksTab
participant DataTable
User->>FileGSTR1Dialog: Initiate GSTR-1 filing
FileGSTR1Dialog->>GSTR1: load_gstr_1_data()
GSTR1->>FileGSTR1Dialog: show_suggested_jv_dialog()
FileGSTR1Dialog->>GSTR1: show_round_off_jv_dialog()
GSTR1->>BackendAPI: get_accounts()
BackendAPI-->>GSTR1: accounts + posting_date
GSTR1->>GSTR1: create_rounding_journal_entry()
GSTR1->>User: open_journal_entry_dialog()
User->>BooksTab: refresh_data(data, summary_data, status)
BooksTab->>BooksTab: store rounding_difference
BooksTab->>DataTable: trigger footer render
User->>BooksTab: refresh_view("Summary", category, filters)
BooksTab->>BooksTab: render_rounding_difference()
BooksTab->>DataTable: insert rounding difference row
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 (5)
✨ 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 (2)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js (2)
1703-1711: Consider a more maintainable approach for extending datatable functionality.The current implementation directly overrides the datatable's
renderFootermethod, which is somewhat fragile as it depends on internal implementation details of the datatable library. This approach could break if the datatable library is updated.Consider using a more maintainable approach:
constructor(instance, wrapper, summary_view_callback, detailed_view_callback) { super(instance, wrapper, summary_view_callback, detailed_view_callback); - const datatable = this.datatable.datatable; - const renderFooter = datatable.bodyRenderer.renderFooter; - datatable.bodyRenderer.renderFooter = () => { - renderFooter.call(datatable.bodyRenderer); - this.render_rounding_difference(); - }; + // Store reference to original method for proper cleanup if needed + this.originalRenderFooter = this.datatable.datatable.bodyRenderer.renderFooter; + this.datatable.datatable.bodyRenderer.renderFooter = () => { + this.originalRenderFooter.call(this.datatable.datatable.bodyRenderer); + this.render_rounding_difference(); + }; }Alternatively, consider using event-based approach or hooks if the datatable library supports them.
1799-1829: Enhance data transformation logic and add validation.The method creates row data by mapping columns, but could benefit from better validation and more explicit column type handling.
Apply this diff to improve the implementation:
get_rounding_difference() { - const datatable = this.datatable.datatable; - const columns = datatable.getColumns(); - const rounding_difference_row_template = columns.map(col => { - let content = null; - if (["_rowIndex", "_checkbox", "no_of_records"].includes(col.id)) { - content = ""; - } - return { - content, - isTotalRow: 1, - colIndex: col.colIndex, - column: col, - }; - }); + try { + const datatable = this.datatable.datatable; + const columns = datatable.getColumns(); + + if (!columns || columns.length === 0) { + console.warn("No columns found for rounding difference calculation"); + return []; + } + + const excludedColumns = ["_rowIndex", "_checkbox", "no_of_records"]; + const rounding_difference_row_template = columns.map(col => { + let content = null; + if (excludedColumns.includes(col.id)) { + content = ""; + } + return { + content, + isTotalRow: 1, + colIndex: col.colIndex, + column: col, + }; + }); - const rounding_difference = rounding_difference_row_template.map(cell => { - if (cell.content === "") return cell; + const rounding_difference = rounding_difference_row_template.map(cell => { + if (cell.content === "") return cell; - if (cell.column.id === "description") { - cell.content = "Rounding Difference"; - } else if (cell.column._fieldtype == "Float") { - const fieldname = cell.column.id; - cell.content = this.rounding_difference[fieldname] || 0.0; - } + if (cell.column.id === "description") { + cell.content = "Rounding Difference"; + } else if (cell.column._fieldtype === "Float") { + const fieldname = cell.column.id; + cell.content = this.rounding_difference[fieldname] || 0.0; + } else { + // Handle other column types explicitly + cell.content = ""; + } - return cell; - }); + return cell; + }); - return rounding_difference; + return rounding_difference; + } catch (error) { + console.error("Error creating rounding difference row:", error); + return []; + } }Note the strict equality operator change: Use
===instead of==for better type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js(2 hunks)
⏰ 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/doctype/gstr_1_beta/gstr_1_beta.js (1)
1767-1772: LGTM! Clean conditional rendering logic.The implementation correctly ensures that rounding difference is only rendered in the Summary view, which is appropriate for this type of aggregate information.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js (2)
1831-1842: Previous review concern partially addressed but needs improvement.The defensive checks have been added but could be more comprehensive as suggested in the past review comments.
The validation logic should be enhanced as per the previous review suggestion to check if
data.rounding_differenceis an array and has at least one element.
1853-1878: Error handling improved but DOM cleanup still needed.The try-catch block addresses the previous review concern, but there's still a missing feature for removing existing rounding difference rows.
The previous review suggestion to remove existing rounding difference rows before adding new ones should still be implemented to prevent duplicate rows.
🧹 Nitpick comments (2)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js (2)
1776-1784: Review constructor override pattern for potential memory leaks.The constructor wraps the datatable's footer render function, but there's no cleanup mechanism if the instance is destroyed or recreated.
Consider adding a cleanup mechanism:
constructor(instance, wrapper, summary_view_callback, detailed_view_callback) { super(instance, wrapper, summary_view_callback, detailed_view_callback); const datatable = this.datatable.datatable; - const renderFooter = datatable.bodyRenderer.renderFooter; + this.originalRenderFooter = datatable.bodyRenderer.renderFooter; datatable.bodyRenderer.renderFooter = () => { - renderFooter.call(datatable.bodyRenderer); + this.originalRenderFooter.call(datatable.bodyRenderer); this.render_rounding_difference(); }; }Additionally, consider adding a destroy method to restore the original function if needed.
2804-2804: Ensure proper error handling for chained dialog calls.The addition of the round-off JV dialog call is good, but consider that if either dialog fails, it shouldn't break the entire flow.
Consider wrapping in a try-catch to prevent one dialog failure from affecting the other:
.then(r => { this.frm.doc.__gst_data = r.message; this.frm.trigger("load_gstr1_data"); - this.frm.gstr1.show_suggested_jv_dialog(); - this.frm.gstr1.show_round_off_jv_dialog(); + try { + this.frm.gstr1.show_suggested_jv_dialog(); + this.frm.gstr1.show_round_off_jv_dialog(); + } catch (error) { + console.error("Error showing JV dialogs:", error); + } });
📜 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_beta.js(4 hunks)india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.py(1 hunks)india_compliance/gst_india/utils/gstr_1/gstr_1_json_map.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.py
[refactor] 307-307: Either all return statements in a function should return an expression, or none of them should.
(R1710)
⏰ 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 (5)
india_compliance/gst_india/utils/gstr_1/gstr_1_json_map.py (2)
6-6: LGTM: Import addition supports dynamic precision functionality.The addition of
cintimport is correctly used in theupdate_rounding_differencemethod for handling the currency precision parameter.
377-380: Excellent improvement: Dynamic currency precision implementation.The change from hardcoded precision (2) to dynamic precision fetched from system defaults is a great enhancement for configurability. The implementation correctly handles the case where
currency_precisionmight be None by using it as a fallback to thefltfunction's default behavior.india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js (3)
829-834: Good mapping structure for tax accounts.The
TAX_TO_ACCOUNT_MAPconstant provides a clean mapping between tax fields and account fields, properly handling the cess accounts as arrays to support multiple account types.
1844-1849: Good integration with view refresh logic.The method properly triggers rounding difference rendering when switching to Summary view, maintaining consistency with the datatable state.
1880-1910: Well-structured helper method with good logic.The
get_rounding_differencemethod properly maps datatable columns and handles different field types appropriately. The logic for filtering out specific columns and setting content based on field types is sound.
| @frappe.whitelist() | ||
| def get_accounts(month_or_quarter, year, company, filing_preference): | ||
| if not frappe.has_permission("Journal Entry", "create"): | ||
| return | ||
|
|
||
| _, to_date = get_gstr_1_from_and_to_date(month_or_quarter, year, filing_preference) | ||
|
|
||
| # For Output Account | ||
| gst_settings = frappe.get_cached_doc("GST Settings") | ||
|
|
||
| output_gst_account = [ | ||
| account | ||
| for account in gst_settings.gst_accounts | ||
| if account.account_type == "Output" | ||
| ] | ||
|
|
||
| if not output_gst_account: | ||
| return | ||
|
|
||
| output_gst_account = output_gst_account[0] | ||
|
|
||
| # For Round Off Account | ||
| round_off_account = frappe.get_all( | ||
| "Account", | ||
| filters={ | ||
| "company": company, | ||
| "account_type": "Round Off", | ||
| }, | ||
| pluck="name", | ||
| ) | ||
|
|
||
| if not round_off_account: | ||
| return | ||
|
|
||
| round_off_account = round_off_account[0] | ||
|
|
||
| account = { | ||
| "igst_account": output_gst_account.igst_account, | ||
| "cgst_account": output_gst_account.cgst_account, | ||
| "sgst_account": output_gst_account.sgst_account, | ||
| "cess_account": output_gst_account.cess_account, | ||
| "cess_non_advol_account": output_gst_account.cess_non_advol_account, | ||
| "round_off_account": round_off_account, | ||
| } | ||
|
|
||
| return { | ||
| "account": account, | ||
| "posting_date": to_date, | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix inconsistent return statements for better API contract.
The function has inconsistent return behavior that could cause issues for callers:
- Lines 309, 323, 338: Return
Noneimplicitly - Lines 351-354: Return a dictionary explicitly
This violates the principle of consistent return types and could lead to runtime errors.
Apply this diff to make return statements consistent:
@frappe.whitelist()
def get_accounts(month_or_quarter, year, company, filing_preference):
if not frappe.has_permission("Journal Entry", "create"):
- return
+ return None
_, to_date = get_gstr_1_from_and_to_date(month_or_quarter, year, filing_preference)
# For Output Account
gst_settings = frappe.get_cached_doc("GST Settings")
output_gst_account = [
account
for account in gst_settings.gst_accounts
if account.account_type == "Output"
]
if not output_gst_account:
- return
+ return None
output_gst_account = output_gst_account[0]
# For Round Off Account
round_off_account = frappe.get_all(
"Account",
filters={
"company": company,
"account_type": "Round Off",
},
pluck="name",
)
if not round_off_account:
- return
+ return None
round_off_account = round_off_account[0]
account = {
"igst_account": output_gst_account.igst_account,
"cgst_account": output_gst_account.cgst_account,
"sgst_account": output_gst_account.sgst_account,
"cess_account": output_gst_account.cess_account,
"cess_non_advol_account": output_gst_account.cess_non_advol_account,
"round_off_account": round_off_account,
}
return {
"account": account,
"posting_date": to_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.
| @frappe.whitelist() | |
| def get_accounts(month_or_quarter, year, company, filing_preference): | |
| if not frappe.has_permission("Journal Entry", "create"): | |
| return | |
| _, to_date = get_gstr_1_from_and_to_date(month_or_quarter, year, filing_preference) | |
| # For Output Account | |
| gst_settings = frappe.get_cached_doc("GST Settings") | |
| output_gst_account = [ | |
| account | |
| for account in gst_settings.gst_accounts | |
| if account.account_type == "Output" | |
| ] | |
| if not output_gst_account: | |
| return | |
| output_gst_account = output_gst_account[0] | |
| # For Round Off Account | |
| round_off_account = frappe.get_all( | |
| "Account", | |
| filters={ | |
| "company": company, | |
| "account_type": "Round Off", | |
| }, | |
| pluck="name", | |
| ) | |
| if not round_off_account: | |
| return | |
| round_off_account = round_off_account[0] | |
| account = { | |
| "igst_account": output_gst_account.igst_account, | |
| "cgst_account": output_gst_account.cgst_account, | |
| "sgst_account": output_gst_account.sgst_account, | |
| "cess_account": output_gst_account.cess_account, | |
| "cess_non_advol_account": output_gst_account.cess_non_advol_account, | |
| "round_off_account": round_off_account, | |
| } | |
| return { | |
| "account": account, | |
| "posting_date": to_date, | |
| } | |
| @frappe.whitelist() | |
| def get_accounts(month_or_quarter, year, company, filing_preference): | |
| if not frappe.has_permission("Journal Entry", "create"): | |
| return None | |
| _, to_date = get_gstr_1_from_and_to_date( | |
| month_or_quarter, year, filing_preference | |
| ) | |
| # For Output Account | |
| gst_settings = frappe.get_cached_doc("GST Settings") | |
| output_gst_account = [ | |
| account | |
| for account in gst_settings.gst_accounts | |
| if account.account_type == "Output" | |
| ] | |
| if not output_gst_account: | |
| return None | |
| output_gst_account = output_gst_account[0] | |
| # For Round Off Account | |
| round_off_account = frappe.get_all( | |
| "Account", | |
| filters={ | |
| "company": company, | |
| "account_type": "Round Off", | |
| }, | |
| pluck="name", | |
| ) | |
| if not round_off_account: | |
| return None | |
| round_off_account = round_off_account[0] | |
| account = { | |
| "igst_account": output_gst_account.igst_account, | |
| "cgst_account": output_gst_account.cgst_account, | |
| "sgst_account": output_gst_account.sgst_account, | |
| "cess_account": output_gst_account.cess_account, | |
| "cess_non_advol_account": output_gst_account.cess_non_advol_account, | |
| "round_off_account": round_off_account, | |
| } | |
| return { | |
| "account": account, | |
| "posting_date": to_date, | |
| } |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 307-307: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🤖 Prompt for AI Agents
In india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.py between lines
306 and 355, the function get_accounts has inconsistent return types, returning
None in some cases and a dictionary in others. To fix this, ensure all return
statements return a consistent type by replacing the implicit None returns with
explicit returns of an empty dictionary or a dictionary with default values
matching the successful return structure. This will maintain a consistent API
contract and prevent runtime errors.
| async show_round_off_jv_dialog() { | ||
| if (!frappe.perm.has_perm("Journal Entry")) return; | ||
|
|
||
| const { month_or_quarter, year, company, filing_preference } = this.frm.doc; | ||
| const { message: data } = await frappe.call({ | ||
| method: "india_compliance.gst_india.doctype.gstr_1_beta.gstr_1_beta.get_accounts", | ||
| args: { month_or_quarter, year, company, filing_preference }, | ||
| }); | ||
|
|
||
| if (!data) return; | ||
|
|
||
| this.create_rounding_journal_entry(data.account, data.posting_date); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling.
The method assumes the backend API will always return valid data and that permissions are properly checked. Consider adding defensive programming measures.
Apply this diff to improve robustness:
async show_round_off_jv_dialog() {
- if (!frappe.perm.has_perm("Journal Entry")) return;
+ if (!frappe.perm.has_perm("Journal Entry")) {
+ frappe.show_alert(__("You don't have permission to create Journal Entry"));
+ return;
+ }
const { month_or_quarter, year, company, filing_preference } = this.frm.doc;
+ if (!month_or_quarter || !year || !company) {
+ console.warn("Missing required fields for round-off JV dialog");
+ return;
+ }
+
+ try {
const { message: data } = await frappe.call({
method: "india_compliance.gst_india.doctype.gstr_1_beta.gstr_1_beta.get_accounts",
args: { month_or_quarter, year, company, filing_preference },
});
- if (!data) return;
+ if (!data || !data.account || !data.posting_date) {
+ console.warn("Invalid or missing account data from backend");
+ return;
+ }
this.create_rounding_journal_entry(data.account, data.posting_date);
+ } catch (error) {
+ console.error("Error fetching accounts for rounding JV:", error);
+ frappe.show_alert(__("Failed to fetch account information"));
+ }
}📝 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.
| async show_round_off_jv_dialog() { | |
| if (!frappe.perm.has_perm("Journal Entry")) return; | |
| const { month_or_quarter, year, company, filing_preference } = this.frm.doc; | |
| const { message: data } = await frappe.call({ | |
| method: "india_compliance.gst_india.doctype.gstr_1_beta.gstr_1_beta.get_accounts", | |
| args: { month_or_quarter, year, company, filing_preference }, | |
| }); | |
| if (!data) return; | |
| this.create_rounding_journal_entry(data.account, data.posting_date); | |
| } | |
| async show_round_off_jv_dialog() { | |
| if (!frappe.perm.has_perm("Journal Entry")) { | |
| frappe.show_alert(__("You don't have permission to create Journal Entry")); | |
| return; | |
| } | |
| const { month_or_quarter, year, company, filing_preference } = this.frm.doc; | |
| if (!month_or_quarter || !year || !company) { | |
| console.warn("Missing required fields for round-off JV dialog"); | |
| return; | |
| } | |
| try { | |
| const { message: data } = await frappe.call({ | |
| method: "india_compliance.gst_india.doctype.gstr_1_beta.gstr_1_beta.get_accounts", | |
| args: { month_or_quarter, year, company, filing_preference }, | |
| }); | |
| if (!data || !data.account || !data.posting_date) { | |
| console.warn("Invalid or missing account data from backend"); | |
| return; | |
| } | |
| this.create_rounding_journal_entry(data.account, data.posting_date); | |
| } catch (error) { | |
| console.error("Error fetching accounts for rounding JV:", error); | |
| frappe.show_alert(__("Failed to fetch account information")); | |
| } | |
| } |
🤖 Prompt for AI Agents
In india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js around lines
815 to 827, the show_round_off_jv_dialog method lacks input validation and error
handling for the backend API call and permission check. To fix this, add checks
to ensure the required fields (month_or_quarter, year, company,
filing_preference) are valid before making the call, handle possible errors from
frappe.call with try-catch, verify that data.account and data.posting_date exist
before calling create_rounding_journal_entry, and confirm permissions explicitly
to prevent unauthorized access.
| create_rounding_journal_entry(account, posting_date) { | ||
| let rounding_difference = this.data.books?.rounding_difference[0]; | ||
| if (!rounding_difference) return; | ||
|
|
||
| const je_details = { | ||
| posting_date: posting_date, | ||
| data: [], | ||
| }; | ||
|
|
||
| const get_account_name = account_field => { | ||
| if (!Array.isArray(account_field)) return account[account_field]; | ||
| for (const acc of account_field) { | ||
| if (account[acc]) { | ||
| return account[acc]; | ||
| } | ||
| } | ||
| return null; | ||
| }; | ||
|
|
||
| let total = 0; | ||
|
|
||
| for (const [tax_field, account_field] of Object.entries( | ||
| this.TAX_TO_ACCOUNT_MAP | ||
| )) { | ||
| let value = rounding_difference[tax_field]; | ||
|
|
||
| if (!value) continue; | ||
|
|
||
| let account_name = get_account_name(account_field); | ||
|
|
||
| if (!account_name) continue; | ||
|
|
||
| total += value; | ||
|
|
||
| je_details.data.push({ | ||
| account: account_name, | ||
| debit_in_account_currency: value > 0 ? value : 0, | ||
| credit_in_account_currency: value < 0 ? Math.abs(value) : 0, | ||
| }); | ||
| } | ||
|
|
||
| if (total !== 0) { | ||
| je_details.data.push({ | ||
| account: account.round_off_account, | ||
| debit_in_account_currency: total < 0 ? Math.abs(total) : 0, | ||
| credit_in_account_currency: total > 0 ? total : 0, | ||
| }); | ||
| } | ||
|
|
||
| this.create_journal_entry_dialog(je_details); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and validation in journal entry creation.
The method has good logic but lacks comprehensive error handling and validation for edge cases.
Apply this diff to improve robustness:
create_rounding_journal_entry(account, posting_date) {
let rounding_difference = this.data.books?.rounding_difference[0];
- if (!rounding_difference) return;
+ if (!rounding_difference) {
+ console.warn("No rounding difference data available");
+ return;
+ }
+
+ if (!account || !posting_date) {
+ console.error("Missing account or posting_date for journal entry");
+ return;
+ }
const je_details = {
posting_date: posting_date,
data: [],
};
const get_account_name = account_field => {
if (!Array.isArray(account_field)) return account[account_field];
for (const acc of account_field) {
if (account[acc]) {
return account[acc];
}
}
return null;
};
let total = 0;
for (const [tax_field, account_field] of Object.entries(
this.TAX_TO_ACCOUNT_MAP
)) {
let value = rounding_difference[tax_field];
- if (!value) continue;
+ if (!value || value === 0) continue;
let account_name = get_account_name(account_field);
- if (!account_name) continue;
+ if (!account_name) {
+ console.warn(`No account found for ${tax_field}`);
+ continue;
+ }
total += value;
je_details.data.push({
account: account_name,
debit_in_account_currency: value > 0 ? value : 0,
credit_in_account_currency: value < 0 ? Math.abs(value) : 0,
});
}
+ if (je_details.data.length === 0) {
+ console.info("No journal entry data to process");
+ return;
+ }
if (total !== 0) {
+ if (!account.round_off_account) {
+ console.error("Round off account not configured");
+ frappe.show_alert(__("Round off account not configured"));
+ return;
+ }
+
je_details.data.push({
account: account.round_off_account,
debit_in_account_currency: total < 0 ? Math.abs(total) : 0,
credit_in_account_currency: total > 0 ? total : 0,
});
}
this.create_journal_entry_dialog(je_details);
}📝 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.
| create_rounding_journal_entry(account, posting_date) { | |
| let rounding_difference = this.data.books?.rounding_difference[0]; | |
| if (!rounding_difference) return; | |
| const je_details = { | |
| posting_date: posting_date, | |
| data: [], | |
| }; | |
| const get_account_name = account_field => { | |
| if (!Array.isArray(account_field)) return account[account_field]; | |
| for (const acc of account_field) { | |
| if (account[acc]) { | |
| return account[acc]; | |
| } | |
| } | |
| return null; | |
| }; | |
| let total = 0; | |
| for (const [tax_field, account_field] of Object.entries( | |
| this.TAX_TO_ACCOUNT_MAP | |
| )) { | |
| let value = rounding_difference[tax_field]; | |
| if (!value) continue; | |
| let account_name = get_account_name(account_field); | |
| if (!account_name) continue; | |
| total += value; | |
| je_details.data.push({ | |
| account: account_name, | |
| debit_in_account_currency: value > 0 ? value : 0, | |
| credit_in_account_currency: value < 0 ? Math.abs(value) : 0, | |
| }); | |
| } | |
| if (total !== 0) { | |
| je_details.data.push({ | |
| account: account.round_off_account, | |
| debit_in_account_currency: total < 0 ? Math.abs(total) : 0, | |
| credit_in_account_currency: total > 0 ? total : 0, | |
| }); | |
| } | |
| this.create_journal_entry_dialog(je_details); | |
| } | |
| create_rounding_journal_entry(account, posting_date) { | |
| let rounding_difference = this.data.books?.rounding_difference[0]; | |
| if (!rounding_difference) { | |
| console.warn("No rounding difference data available"); | |
| return; | |
| } | |
| if (!account || !posting_date) { | |
| console.error("Missing account or posting_date for journal entry"); | |
| return; | |
| } | |
| const je_details = { | |
| posting_date: posting_date, | |
| data: [], | |
| }; | |
| const get_account_name = account_field => { | |
| if (!Array.isArray(account_field)) return account[account_field]; | |
| for (const acc of account_field) { | |
| if (account[acc]) { | |
| return account[acc]; | |
| } | |
| } | |
| return null; | |
| }; | |
| let total = 0; | |
| for (const [tax_field, account_field] of Object.entries( | |
| this.TAX_TO_ACCOUNT_MAP | |
| )) { | |
| let value = rounding_difference[tax_field]; | |
| if (!value || value === 0) continue; | |
| let account_name = get_account_name(account_field); | |
| if (!account_name) { | |
| console.warn(`No account found for ${tax_field}`); | |
| continue; | |
| } | |
| total += value; | |
| je_details.data.push({ | |
| account: account_name, | |
| debit_in_account_currency: value > 0 ? value : 0, | |
| credit_in_account_currency: value < 0 ? Math.abs(value) : 0, | |
| }); | |
| } | |
| if (je_details.data.length === 0) { | |
| console.info("No journal entry data to process"); | |
| return; | |
| } | |
| if (total !== 0) { | |
| if (!account.round_off_account) { | |
| console.error("Round off account not configured"); | |
| frappe.show_alert(__("Round off account not configured")); | |
| return; | |
| } | |
| je_details.data.push({ | |
| account: account.round_off_account, | |
| debit_in_account_currency: total < 0 ? Math.abs(total) : 0, | |
| credit_in_account_currency: total > 0 ? total : 0, | |
| }); | |
| } | |
| this.create_journal_entry_dialog(je_details); | |
| } |
🤖 Prompt for AI Agents
In india_compliance/gst_india/doctype/gstr_1_beta/gstr_1_beta.js around lines
836 to 886, the create_rounding_journal_entry method lacks error handling and
validation. Add checks to ensure this.data.books and
this.data.books.rounding_difference exist before accessing them. Validate that
rounding_difference fields are numbers before using them. Add try-catch around
the main logic to catch unexpected errors and log or handle them appropriately.
Also, verify that account and account.round_off_account are valid before use to
prevent runtime errors.
There was a problem hiding this comment.
@karm1000 Got it—since get_accounts returns None when the round-off account isn’t configured, the UI never invokes create_rounding_journal_entry, so the extra round_off_account guard I proposed is indeed redundant.
If you’re happy with the current behaviour, feel free to disregard that part of the suggestion. The smaller checks (like logging when rounding_difference is missing or zero and early-returning when no JE lines get built) can still help with debugging edge cases, but they’re entirely optional.
Thanks for clarifying!
karm1000
force-pushed
the
add-rounding-difference
branch
from
45a4bf0 to
a7ef6d6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3418 +/- ##
===========================================
- Coverage 59.55% 59.37% -0.19%
===========================================
Files 136 136
Lines 13885 13941 +56
===========================================
+ Hits 8269 8277 +8
- Misses 5616 5664 +48
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Summary by CodeRabbit