Feat: Adding new Journal Entry changes #175
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Outside diff range and nitpick comments (3)
apps/business_central/exports/journal_entry/models.py (1)
125-140: Consider adding transaction handling for line item creationThe line item creation involves multiple database operations (getting journal entry, category mapping, and creating line item). Consider wrapping this in a transaction to ensure data consistency.
+from django.db import transaction + @classmethod def create_or_update_object(self, accounting_export: AccountingExport, advance_setting: AdvancedSetting, export_settings: ExportSetting): + with transaction.atomic(): # existing codeapps/business_central/exports/journal_entry/tasks.py (2)
Line range hint
161-176: Fix typo and improve documentation.The function has a typo in the variable name and needs better documentation:
Apply this diff to improve the code:
@handle_business_central_exceptions() def create_journal_entry(accounting_export: AccountingExport, last_export: bool): ''' Helper function to create and export a journal entry. + + Args: + accounting_export (AccountingExport): The accounting export instance to process + last_export (bool): Flag indicating if this is the last export in the batch + + Returns: + dict: The exported journal entry response from Business Central ''' export_journal_entry_instance = ExportJournalEntry() # Create and export the journal entry using the base class method - exported_jornal_entry = export_journal_entry_instance.create_business_central_object(accounting_export=accounting_export) + exported_journal_entry = export_journal_entry_instance.create_business_central_object(accounting_export=accounting_export) if last_export: update_accounting_export_summary(accounting_export.workspace_id) - return exported_jornal_entry + return exported_journal_entry
Line range hint
133-157: Improve dimension handling robustness.The current dimension handling has potential issues:
- No validation for empty dimensions list
- Hard-coded array indices make the code fragile to changes in response structure
Consider applying this safer approach:
def construct_dimension_set_line_payload(self, dimensions: list, exported_response: dict): """ construct payload for setting dimension for Purchase Invoice """ + if not dimensions: + return [] dimension = dimensions[0] + if len(exported_response) < 2: + logger.error(f"Unexpected response structure: {exported_response}") + return [] dimension_payload = [ { "id": dimension['id'], "code": dimension['code'], "parentId": exported_response[0]['body']['id'], "valueId": dimension['valueId'], "valueCode": dimension['valueCode'], "exported_module_id": dimension['exported_module_id'] }, { "id": dimension['id'], "code": dimension['code'], "parentId": exported_response[1]['body']['id'], "valueId": dimension['valueId'], "valueCode": dimension['valueCode'], "exported_module_id": dimension['exported_module_id'] }] return dimension_payload
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/business_central/exports/journal_entry/models.py(2 hunks)apps/business_central/exports/journal_entry/tasks.py(4 hunks)
🧰 Additional context used
🪛 Ruff
apps/business_central/exports/journal_entry/models.py
108-108: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (4)
apps/business_central/exports/journal_entry/models.py (2)
44-44: Verify the impact of using only the first expense
The change from handling multiple expenses to using only first() could potentially miss important data from other expenses in the group.
✅ Verification successful
Based on the search results, I can now generate a final response as we have a clear picture of how expenses are handled across the codebase.
Using first() expense is consistent with the implementation
The change to use first() expense is correct and consistent with the journal entry implementation. The codebase shows that while some modules (like purchase_invoice) handle multiple expenses, the journal entry module is specifically designed to work with a single expense, as evidenced by:
- All journal entry related code consistently uses
first()when accessing expenses - The test cases validate this behavior
- Other parts of the journal entry module (
JournalEntryLineItems) also operate on single expense model
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are cases where accounting_export has multiple expenses
rg -A 5 "accounting_export\.expenses\.count|len\(.*expenses\)"
Length of output: 124
Script:
#!/bin/bash
# Let's check how expenses are being used in the codebase
rg -A 5 "accounting_export\.expenses"
Length of output: 4917
Line range hint 44-141: Verify consistency with ExportJournalEntry changes
The changes to handle single expenses should align with the modifications in tasks.py where ExportJournalEntry class was updated to determine balance_account_type dynamically.
✅ Verification successful
Let me gather more information about the journal entry handling to verify the consistency.
No inconsistency found in journal entry handling
The changes in the models are consistent with the journal entry export implementation. The balance_account_type is correctly determined in tasks.py based on the accounts_payable_account_id that's being set in both JournalEntry and JournalEntryLineItems models. The account type resolution follows the same pattern where it defaults to 'G/L Account' and switches to 'Bank Account' based on the destination attribute type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistency in expense handling across related files
rg -A 5 "ExportJournalEntry.*balance_account_type|__construct_journal_entry"
Length of output: 2720
Script:
#!/bin/bash
# Check the implementation of journal entry construction
ast-grep --pattern 'def __construct_journal_entry($$$)'
# Check for balance_account_type usage
rg "balance_account_type" -B 2 -A 2
Length of output: 1878
🧰 Tools
🪛 Ruff
108-108: Comparison to None should be cond is None
Replace with cond is None
(E711)
apps/business_central/exports/journal_entry/tasks.py (2)
14-14: LGTM!
The import statement is correctly placed and necessary for the new functionality.
47-52:
Add explicit None handling and fix spacing.
The account attribute type query needs more robust error handling and has a minor formatting issue:
Apply this diff to improve the code:
- account_attribute_type = DestinationAttribute.objects.filter(workspace_id=body.workspace_id, destination_id=body.accounts_payable_account_id).first()
+ account_attribute_type = DestinationAttribute.objects.filter(
+ workspace_id=body.workspace_id,
+ destination_id=body.accounts_payable_account_id
+ ).first()
balance_account_type = 'G/L Account'
- if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT':
+ if account_attribute_type and account_attribute_type.attribute_type == 'BANK_ACCOUNT':
balance_account_type = 'Bank Account'Let's verify if there's an index on the queried fields:
|
|
||
| accounts_payable_account_id = export_settings.default_bank_account_id | ||
|
|
||
| document_number = accounting_export.description['claim_number'] if accounting_export.description and accounting_export.description.get('claim_number') else accounting_export.description['expense_number'] | ||
| advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameter naming inconsistency with implementation
The method parameter is named _: AdvancedSetting but the implementation retrieves and uses advance_setting. This suggests the parameter should be utilized instead of making a new database query.
-def create_or_update_object(self, accounting_export: AccountingExport, _: AdvancedSetting, export_settings: ExportSetting):
+def create_or_update_object(self, accounting_export: AccountingExport, advance_setting: AdvancedSetting, export_settings: ExportSetting):
- advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id)Also applies to: 50-52
| account = CategoryMapping.objects.filter( | ||
| source_category__value=category, | ||
| workspace_id=accounting_export.workspace_id | ||
| ).first() | ||
|
|
There was a problem hiding this comment.
Add error handling for missing category mapping
The code assumes the category mapping will exist but doesn't handle the case where account might be None. This could lead to a NullPointerException when accessing account.destination_account.destination_id.
account = CategoryMapping.objects.filter(
source_category__value=category,
workspace_id=accounting_export.workspace_id
).first()
+ if not account or not account.destination_account:
+ raise ValueError(f"No category mapping found for category: {category} in workspace: {accounting_export.workspace_id}")Also applies to: 133-133
|
|
||
| comment = "Consolidated Credit Entry For Report/Expense {}".format(document_number) | ||
| document_number = accounting_export.description['expense_number'] if accounting_export.description and accounting_export.description.get('expense_number') else None |
There was a problem hiding this comment.
get expense number from expense
|
|
||
| account_type, account_id = self.get_account_id_type(accounting_export=accounting_export, export_settings=export_settings, merchant=lineitem.vendor) | ||
|
|
||
| document_number = accounting_export.description['expense_number'] if accounting_export.description and accounting_export.description.get('expense_number') else None |
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/business_central/exports/journal_entry/models.py (2)
109-110: Modernize string formatting and fix None comparisonThe category formatting can be improved using f-strings and proper None comparison.
- category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format(lineitem.category, lineitem.sub_category) + category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) else f'{lineitem.category} / {lineitem.sub_category}'🧰 Tools
🪛 Ruff (0.7.0)
109-109: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
126-142: Simplify return value for single line itemSince we're only processing a single line item, maintaining a list is unnecessary.
- journal_entry_lineitems = [] - journal_entry_lineitems_object, _ = JournalEntryLineItems.objects.update_or_create( + return JournalEntryLineItems.objects.update_or_create( journal_entry_id = journal_entry.id, expense_id=lineitem.id, defaults={ 'amount': lineitem.amount * -1, 'account_id': account_id, 'account_type': account_type, 'document_number': document_number, 'accounts_payable_account_id': account.destination_account.destination_id, 'comment': comment, 'workspace_id': accounting_export.workspace_id, 'invoice_date': invoice_date, 'description': lineitem.purpose if lineitem.purpose else None, 'dimensions': dimensions } - ) - journal_entry_lineitems.append(journal_entry_lineitems_object) - - return journal_entry_lineitems + )[0]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/business_central/exports/journal_entry/models.py(2 hunks)apps/business_central/exports/journal_entry/tasks.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/business_central/exports/journal_entry/tasks.py
🧰 Additional context used
🪛 Ruff (0.7.0)
apps/business_central/exports/journal_entry/models.py
109-109: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (2)
apps/business_central/exports/journal_entry/models.py (2)
Line range hint 61-68: LGTM: Defaults properly updated for single expense
The changes correctly reflect the switch to single expense processing.
116-125: LGTM: Line item processing logic is well-structured
The processing logic for line items is properly implemented with all necessary components.
| @@ -41,22 +41,24 @@ def create_or_update_object(self, accounting_export: AccountingExport, _: Advanc | |||
| :param accounting_export: expense group | |||
| :return: purchase invoices object | |||
| """ | |||
| expenses = accounting_export.expenses.all() | |||
| expense = accounting_export.expenses.first() | |||
There was a problem hiding this comment.
Add error handling and optimize database queries
Several issues need to be addressed:
- The method accepts
advance_settingas a parameter but retrieves it again from the database. This is inefficient and inconsistent with the method signature. - No error handling for the case when no expenses exist.
- The document_number logic could be simplified.
Apply these changes:
- expense = accounting_export.expenses.first()
+ expense = accounting_export.expenses.first()
+ if not expense:
+ raise ValueError(f"No expenses found for accounting_export: {accounting_export.id}")
- advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id)
- document_number = accounting_export.description['expense_number'] if accounting_export.description and accounting_export.description.get('expense_number') else None
+ document_number = accounting_export.description.get('expense_number') if accounting_export.description else None
comment = self.get_expense_comment(accounting_export.workspace_id, expense, expense.category, advance_setting)Also applies to: 48-52
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/business_central/exports/journal_entry/models.py (1)
126-126: Add validation for dimension objectThe dimensions are retrieved and stored without validation. Consider adding validation to ensure the dimension object meets the expected format.
dimensions = self.get_dimension_object(accounting_export, lineitem) + if dimensions: + self.validate_dimensions(dimensions) # Add this method to validate dimension structure # Add this method to the class + @classmethod + def validate_dimensions(cls, dimensions): + """ + Validate the structure of dimensions object + :param dimensions: list of dimension objects + :raises ValueError: if dimensions are invalid + """ + if not isinstance(dimensions, list): + raise ValueError("Dimensions must be a list") + for dimension in dimensions: + if not isinstance(dimension, dict) or 'id' not in dimension: + raise ValueError("Each dimension must be a dictionary with an 'id' field")Also applies to: 141-141
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apps/business_central/exports/journal_entry/models.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
apps/business_central/exports/journal_entry/models.py
111-111: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (3)
apps/business_central/exports/journal_entry/models.py (3)
Line range hint 63-70: LGTM: Journal entry defaults are properly configured
The defaults for the journal entry creation are correctly set up to use the single expense's properties.
113-117:
Add error handling for missing category mapping
The code assumes the category mapping will always exist, which could lead to NullPointerException later when accessing account.destination_account.destination_id.
account = CategoryMapping.objects.filter(
source_category__value=category,
workspace_id=accounting_export.workspace_id
).first()
+ if not account or not account.destination_account:
+ raise ValueError(f"No category mapping found for category: {category} in workspace: {accounting_export.workspace_id}")Likely invalid or redundant comment.
132-132: Review negative amount multiplication
The line 'amount': lineitem.amount * -1, inverts the amount. Please verify if this is the intended behavior for journal entry line items.
✅ Verification successful
Amount negation is correct for double-entry accounting
The amount negation in 'amount': lineitem.amount * -1 is the intended behavior. This is part of a double-entry accounting system where:
- The main
JournalEntryclass records the positive amount (expense.amount) - The
JournalEntryLineItemsclass records the corresponding negative amount (lineitem.amount * -1)
This follows the fundamental accounting principle where every debit must have a corresponding credit of equal value. The negation ensures the books remain balanced.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of amount negation in journal entries
rg -B 2 -A 2 "amount.*-1" --type py
Length of output: 502
Script:
#!/bin/bash
# Let's look at the broader context of this journal entry model to understand the purpose
ast-grep --pattern 'class $_ {
$$$
amount
$$$
}'
Length of output: 56
Script:
#!/bin/bash
# Let's look at the broader context of this file to understand the journal entry logic
cat apps/business_central/exports/journal_entry/models.py
Length of output: 6626
| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format(lineitem.category, lineitem.sub_category) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix None comparison and simplify category formatting
The None comparison should use the is operator as per Python best practices.
- category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format(lineitem.category, lineitem.sub_category)
+ category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) else f'{lineitem.category} / {lineitem.sub_category}'📝 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.
| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category == None) else '{0} / {1}'.format(lineitem.category, lineitem.sub_category) | |
| category = lineitem.category if (lineitem.category == lineitem.sub_category or lineitem.sub_category is None) else f'{lineitem.category} / {lineitem.sub_category}' |
🧰 Tools
🪛 Ruff (0.7.0)
111-111: Comparison to None should be cond is None
Replace with cond is None
(E711)
| expense = accounting_export.expenses.first() | ||
|
|
||
| accounts_payable_account_id = export_settings.default_bank_account_id | ||
|
|
||
| document_number = accounting_export.description['claim_number'] if accounting_export.description and accounting_export.description.get('claim_number') else accounting_export.description['expense_number'] | ||
| advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id) | ||
|
|
||
| comment = "Consolidated Credit Entry For Report/Expense {}".format(document_number) | ||
| print(accounting_export.__dict__) | ||
|
|
||
| document_number = expense.expense_number | ||
|
|
||
| comment = self.get_expense_comment(accounting_export.workspace_id, expense, expense.category, advance_setting) |
There was a problem hiding this comment.
Clean up implementation and add error handling
Several issues need to be addressed:
- Redundant database query for advance_setting
- Missing error handling for the case when no expenses exist
- Debug print statement should be removed
- Direct access to expense_number without validation
Apply these changes:
def create_or_update_object(self, accounting_export: AccountingExport, advance_setting: AdvancedSetting, export_settings: ExportSetting):
expense = accounting_export.expenses.first()
+ if not expense:
+ raise ValueError(f"No expenses found for accounting_export: {accounting_export.id}")
accounts_payable_account_id = export_settings.default_bank_account_id
- advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id)
- print(accounting_export.__dict__)
- document_number = expense.expense_number
+ document_number = getattr(expense, 'expense_number', None)
+ if not document_number:
+ raise ValueError(f"No expense number found for expense: {expense.id}")📝 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.
| expense = accounting_export.expenses.first() | |
| accounts_payable_account_id = export_settings.default_bank_account_id | |
| document_number = accounting_export.description['claim_number'] if accounting_export.description and accounting_export.description.get('claim_number') else accounting_export.description['expense_number'] | |
| advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id) | |
| comment = "Consolidated Credit Entry For Report/Expense {}".format(document_number) | |
| print(accounting_export.__dict__) | |
| document_number = expense.expense_number | |
| comment = self.get_expense_comment(accounting_export.workspace_id, expense, expense.category, advance_setting) | |
| expense = accounting_export.expenses.first() | |
| if not expense: | |
| raise ValueError(f"No expenses found for accounting_export: {accounting_export.id}") | |
| accounts_payable_account_id = export_settings.default_bank_account_id | |
| document_number = getattr(expense, 'expense_number', None) | |
| if not document_number: | |
| raise ValueError(f"No expense number found for expense: {expense.id}") | |
| comment = self.get_expense_comment(accounting_export.workspace_id, expense, expense.category, advance_setting) |
Ashutosh619-sudo
merged commit ca40efc
into
export-dimensions-je-pi
…Invoice (#174) * Feat: Support for dimension during export Journal Entry and purchase Invoice * comment resolved * added dimension support for JE and bug fix * implementation of exporting dimensions for JE and PI * bug fixed * comment resolved * flake fixed * Feat: Adding new Journal Entry changes (#175) * Feat: Adding new Journal Entry changes * comment resolved
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901604887942-1
Summary by CodeRabbit
New Features
Bug Fixes
Documentation