Feat: Add sync method for bank accounts, dimensions and dimension_values #173
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||
| from django.db import models | ||||||
| from django.db.models import JSONField | ||||||
|
|
||||||
| from fyle_accounting_mappings.models import CategoryMapping | ||||||
|
|
||||||
| from apps.accounting_exports.models import AccountingExport | ||||||
|
|
@@ -39,22 +41,24 @@ def create_or_update_object(self, accounting_export: AccountingExport, _: Advanc | |||||
| :param accounting_export: expense group | ||||||
| :return: purchase invoices object | ||||||
| """ | ||||||
| expense = accounting_export.expenses.first() | ||||||
|
There was a problem hiding this comment. Add error handling for missing expense. The code assumes an expense will always exist. Add proper error handling for cases where no expense is found. - expense = accounting_export.expenses.first()
+ expense = accounting_export.expenses.first()
+ if not expense:
+ raise ValueError('No expense found for this accounting export')
document_number = expense.expense_numberAlso applies to: 52-54 |
||||||
|
|
||||||
| accounts_payable_account_id = export_settings.default_bank_account_id | ||||||
|
|
||||||
| advance_setting = AdvancedSetting.objects.get(workspace_id=accounting_export.workspace_id) | ||||||
|
|
||||||
| document_number = expense.expense_number | ||||||
|
|
||||||
| comment = self.get_expense_comment(accounting_export.workspace_id, expense, expense.category, advance_setting) | ||||||
|
|
||||||
| invoice_date = self.get_invoice_date(accounting_export=accounting_export) | ||||||
|
|
||||||
| account_type, account_id = self.get_account_id_type(accounting_export=accounting_export, export_settings=export_settings, merchant=expense.vendor) | ||||||
|
|
||||||
| journal_entry_object, _ = JournalEntry.objects.update_or_create( | ||||||
| accounting_export= accounting_export, | ||||||
| defaults={ | ||||||
| 'amount': expense.amount, | ||||||
| 'document_number': document_number, | ||||||
| 'accounts_payable_account_id': accounts_payable_account_id, | ||||||
| 'account_id': account_id, | ||||||
|
|
@@ -83,6 +87,9 @@ class JournalEntryLineItems(BaseExportModel): | |||||
| invoice_date = CustomDateTimeField(help_text='date of invoice') | ||||||
| document_number = TextNotNullField(help_text='document number of the invoice') | ||||||
| journal_entry = models.ForeignKey(JournalEntry, on_delete=models.PROTECT, help_text='Journal Entry reference', related_name='journal_entry_lineitems') | ||||||
| dimensions = JSONField(default=list, help_text='Business Central dimensions') | ||||||
| dimension_error_log = JSONField(null=True, help_text='dimension set response log') | ||||||
| dimension_success_log = JSONField(null=True, help_text='dimension set success response log') | ||||||
|
|
||||||
| class Meta: | ||||||
| db_table = 'journal_entries_lineitems' | ||||||
|
|
@@ -94,42 +101,44 @@ def create_or_update_object(self, accounting_export: AccountingExport, advance_s | |||||
| :param accounting_export: expense group | ||||||
| :return: purchase invoices object | ||||||
| """ | ||||||
| lineitem = accounting_export.expenses.first() | ||||||
|
There was a problem hiding this comment. Add error handling for missing journal entry. Add proper error handling for cases where journal entry or category mapping is not found. lineitem = accounting_export.expenses.first()
+ if not lineitem:
+ raise ValueError('No expense found for this accounting export')
+
journal_entry = JournalEntry.objects.get(accounting_export=accounting_export)
+ if not journal_entry:
+ raise ValueError('No journal entry found for this accounting export')
# ... later in the code ...
account = CategoryMapping.objects.filter(
source_category__value=category,
workspace_id=accounting_export.workspace_id
).first()
+ if not account:
+ raise ValueError(f'No category mapping found for category: {category}')Also applies to: 128-143 |
||||||
| journal_entry = JournalEntry.objects.get(accounting_export=accounting_export) | ||||||
|
|
||||||
| journal_entry_lineitems = [] | ||||||
|
|
||||||
| 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. Fix None comparison style. Use Python's identity comparison for None. - 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 '{0} / {1}'.format(lineitem.category, lineitem.sub_category)📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.7.0)111-111: Comparison to Replace with (E711) |
||||||
|
|
||||||
| account = CategoryMapping.objects.filter( | ||||||
| source_category__value=category, | ||||||
| workspace_id=accounting_export.workspace_id | ||||||
| ).first() | ||||||
|
Comment on lines
+111
to
+114
There was a problem hiding this comment. 🛠️ Refactor suggestion Optimize database query for CategoryMapping. The current implementation might lead to N+1 query issues. Consider prefetching related fields and adding appropriate indexes. + from django.db.models import Index
class CategoryMapping(models.Model):
class Meta:
indexes = [
Index(fields=['source_category', 'workspace_id']),
]Also, consider caching the mapping results if they're frequently accessed: from django.core.cache import cache
def get_category_mapping(category, workspace_id):
cache_key = f'category_mapping_{workspace_id}_{category}'
mapping = cache.get(cache_key)
if mapping is None:
mapping = CategoryMapping.objects.filter(
source_category__value=category,
workspace_id=workspace_id
).first()
cache.set(cache_key, mapping, timeout=3600) # Cache for 1 hour
return mapping
Comment on lines
+104
to
+114
There was a problem hiding this comment. 🛠️ Refactor suggestion Optimize database queries. The current implementation makes multiple separate database queries which could lead to N+1 query issues. Consider these optimizations:
lineitem = accounting_export.expenses.select_related('expense_number', 'category', 'sub_category', 'vendor').first()
class CategoryMapping(models.Model):
class Meta:
indexes = [
models.Index(fields=['source_category', 'workspace_id']),
]🧰 Tools🪛 Ruff (0.7.0)109-109: Comparison to Replace with (E711) |
||||||
|
|
||||||
| comment = self.get_expense_comment(accounting_export.workspace_id, lineitem, lineitem.category, advance_setting) | ||||||
|
|
||||||
| invoice_date = self.get_invoice_date(accounting_export=accounting_export) | ||||||
|
|
||||||
| account_type, account_id = self.get_account_id_type(accounting_export=accounting_export, export_settings=export_settings, merchant=lineitem.vendor) | ||||||
|
|
||||||
| document_number = lineitem.expense_number | ||||||
|
|
||||||
| dimensions = self.get_dimension_object(accounting_export, lineitem) | ||||||
|
|
||||||
|
Comment on lines
+124
to
+125
There was a problem hiding this comment. Add error handling for dimension retrieval. The Apply this diff: - dimensions = self.get_dimension_object(accounting_export, lineitem)
+ try:
+ dimensions = self.get_dimension_object(accounting_export, lineitem)
+ except Exception as e:
+ dimensions = []
+ # Log the error or handle it appropriately
+ logger.error(f'Failed to get dimensions for expense {lineitem.id}: {str(e)}')
|
||||||
| journal_entry_lineitems_object, _ = 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 | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ | |
| from apps.business_central.utils import BusinessCentralConnector | ||
| from apps.workspaces.models import BusinessCentralCredentials | ||
|
|
||
| from fyle_accounting_mappings.models import DestinationAttribute | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
| logger.level = logging.INFO | ||
|
|
||
|
|
@@ -40,6 +42,13 @@ def __construct_journal_entry(self, body: JournalEntry, lineitems: List[JournalE | |
| :return: constructed expense_report | ||
| ''' | ||
| batch_journal_entry_payload = [] | ||
| dimensions = [] | ||
|
|
||
| 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': | ||
| balance_account_type = 'Bank Account' | ||
|
|
||
| journal_entry_payload = { | ||
| 'accountType': body.account_type, | ||
|
|
@@ -49,14 +58,18 @@ def __construct_journal_entry(self, body: JournalEntry, lineitems: List[JournalE | |
| 'amount': body.amount, | ||
| 'comment': body.comment, | ||
| 'description': body.description, | ||
| 'balanceAccountType': balance_account_type, | ||
| 'balancingAccountNumber': body.accounts_payable_account_id | ||
|
|
||
| } | ||
|
|
||
| batch_journal_entry_payload.append(journal_entry_payload) | ||
|
|
||
| for lineitem in lineitems: | ||
| for dimension in lineitem.dimensions: | ||
| dimension['exported_module_id'] = lineitem.id | ||
|
|
||
| dimensions.extend(lineitem.dimensions) | ||
|
Comment on lines
+69
to
+72
There was a problem hiding this comment. Handle potential If Apply this diff to handle possible for lineitem in lineitems:
+ if lineitem.dimensions:
for dimension in lineitem.dimensions:
dimension['exported_module_id'] = lineitem.id
dimensions.extend(lineitem.dimensions)
|
||
| journal_entry_lineitem_payload = { | ||
| 'accountType': lineitem.account_type, | ||
| 'accountNumber': lineitem.account_id, | ||
|
|
@@ -65,30 +78,39 @@ def __construct_journal_entry(self, body: JournalEntry, lineitems: List[JournalE | |
| 'amount': lineitem.amount, | ||
| 'comment': lineitem.comment, | ||
| 'description': lineitem.description if lineitem.description else '', | ||
| 'balanceAccountType': balance_account_type, | ||
| 'balancingAccountNumber': lineitem.accounts_payable_account_id | ||
| } | ||
|
|
||
| batch_journal_entry_payload.append(journal_entry_lineitem_payload) | ||
|
|
||
| return batch_journal_entry_payload, dimensions | ||
|
|
||
| def post(self, accounting_export, item, lineitem): | ||
| ''' | ||
| Export the Journal Entry to Business Central. | ||
| ''' | ||
|
|
||
| batch_journal_entry_payload, dimensions = self.__construct_journal_entry(item, lineitem) | ||
| logger.info('WORKSPACE_ID: {0}, ACCOUNTING_EXPORT_ID: {1}, JOURNAL_ENTRY_PAYLOAD: {2}'.format(accounting_export.workspace_id, accounting_export.id, batch_journal_entry_payload)) | ||
| business_central_credentials = BusinessCentralCredentials.get_active_business_central_credentials(accounting_export.workspace_id) | ||
| # Establish a connection to Business Central | ||
| business_central_connection = BusinessCentralConnector(business_central_credentials, accounting_export.workspace_id) | ||
|
|
||
| # Post the journal entry to Business Central | ||
| response = business_central_connection.bulk_post_journal_lineitems(batch_journal_entry_payload, accounting_export) | ||
|
|
||
| if dimensions: | ||
| dimension_set_line_payloads = self.construct_dimension_set_line_payload(dimensions, response['responses']) | ||
| logger.info('WORKSPACE_ID: {0}, ACCOUNTING_EXPORT_ID: {1}, DIMENSION_SET_LINE_PAYLOADS: {2}'.format(accounting_export.workspace_id, accounting_export.id, dimension_set_line_payloads)) | ||
| dimension_line_responses = ( | ||
| business_central_connection.post_dimension_lines( | ||
| dimension_set_line_payloads, "JOURNAL_ENTRY", item.id | ||
| ) | ||
| ) | ||
| response["dimension_line_responses"] = dimension_line_responses | ||
|
|
||
| expenses = accounting_export.expenses.all() | ||
| # Load attachments to Business Central | ||
| for i in range(1, len(response["responses"])): | ||
| load_attachments( | ||
|
|
@@ -100,6 +122,23 @@ def post(self, accounting_export, item, lineitem): | |
|
|
||
| return response | ||
|
|
||
| def construct_dimension_set_line_payload(self, dimensions: list, exported_response: dict): | ||
| """ | ||
| construct payload for setting dimension for Journal Entry | ||
| """ | ||
|
|
||
| dimension_payload = [] | ||
|
|
||
| for response in exported_response: | ||
| parent_id = response['body']['id'] | ||
| for dimension in dimensions: | ||
| dimension_copy = dimension.copy() | ||
| dimension_copy.pop('expense_number') | ||
| dimension_copy['parentId'] = parent_id | ||
| dimension_payload.append(dimension_copy) | ||
|
|
||
| return dimension_payload | ||
|
|
||
|
Comment on lines
+125
to
+141
There was a problem hiding this comment. Potential mismatch when associating dimensions with journal entries. In the Consider modifying the code to correctly associate each dimension with its corresponding journal entry. Here's a possible refactor: def construct_dimension_set_line_payload(self, dimensions: list, exported_response: list):
"""
Construct payload for setting dimensions for Journal Entry
"""
dimension_payload = []
+ # Assuming that dimensions and exported_response are in the same order
+ for response, dimension in zip(exported_response, dimensions):
parent_id = response['body']['id']
- for dimension in dimensions:
dimension_copy = dimension.copy()
dimension_copy.pop('expense_number', None)
dimension_copy['parentId'] = parent_id
dimension_payload.append(dimension_copy)
return dimension_payloadThis change ensures that each dimension is correctly linked to its corresponding journal entry.
|
||
|
|
||
| @handle_business_central_exceptions() | ||
| def create_journal_entry(accounting_export: AccountingExport, last_export: bool): | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| from typing import List | ||
|
|
||
| from django.db import models | ||
| from django.db.models import JSONField | ||
|
|
||
| from fyle_accounting_mappings.models import CategoryMapping | ||
|
|
||
| from apps.accounting_exports.models import AccountingExport | ||
|
|
@@ -61,6 +63,9 @@ class PurchaseInvoiceLineitems(BaseExportModel): | |
| amount = FloatNullField(help_text='Amount of the invoice') | ||
| description = TextNotNullField(help_text='description for the invoice') | ||
| location_id = StringNullField(help_text='location id of the invoice') | ||
| dimensions = JSONField(default=list, help_text='Business Central dimensions') | ||
| dimension_error_log = JSONField(null=True, help_text='dimension set response log') | ||
| dimension_success_log = JSONField(null=True, help_text='dimension set success response log') | ||
|
|
||
| class Meta: | ||
| db_table = 'purchase_invoice_lineitems' | ||
|
|
@@ -88,6 +93,7 @@ def create_or_update_object(self, accounting_export: AccountingExport, advance_s | |
|
|
||
| description = self.get_expense_purpose(lineitem, lineitem.category, advance_setting) | ||
| location_id = self.get_location_id(accounting_export, lineitem) | ||
| dimensions = self.get_dimension_object(accounting_export, lineitem) | ||
|
|
||
| purchase_invoice_lineitem_object, _ = PurchaseInvoiceLineitems.objects.update_or_create( | ||
| purchase_invoice_id=purchase_invoice.id, | ||
|
|
@@ -97,7 +103,8 @@ def create_or_update_object(self, accounting_export: AccountingExport, advance_s | |
| 'accounts_payable_account_id': account.destination_account.destination_id if account else None, | ||
| 'description': description, | ||
| 'workspace_id': accounting_export.workspace_id, | ||
| 'location_id': location_id, | ||
| 'dimensions': dimensions | ||
| } | ||
| ) | ||
| purchase_invoice_lineitem_objects.append(purchase_invoice_lineitem_object) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add type hints and documentation.
The method signature could be improved with type hints and documentation for better maintainability and clarity.
Apply this diff:
📝 Committable suggestion