Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Add chart of accounts type when importing accounts to category in Fyle #177

Merged
merged 7 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions apps/business_central/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
import logging
from typing import Dict, List

from openpyxl.utils.escape import unescape

from datetime import datetime
from django.utils import timezone

Expand All @@ -13,6 +15,7 @@
from apps.workspaces.models import BusinessCentralCredentials, ExportSetting, Workspace
from ms_business_central_api import settings


logger = logging.getLogger(__name__)
logger.level = logging.INFO

Expand Down Expand Up @@ -114,6 +117,11 @@ def _sync_data(self, data, attribute_type, display_name, workspace_id, field_nam
else:
active = False
if attribute_type == 'ACCOUNT':
if attribute_type == 'ACCOUNT' and 'category' in detail:
if detail['category'] == '_x0020_':
detail['category'] = 'Others'
else:
detail['category'] = unescape(detail['category'])
if item.get('accountType') != 'Posting' or not item.get('directPosting'):
continue
destination_attributes.append(self._create_destination_attribute(
Expand Down Expand Up @@ -365,3 +373,4 @@ def post_attachments(
responses.append(post_response)

return post_response

19 changes: 14 additions & 5 deletions apps/mappings/imports/modules/base.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import math
import copy
from datetime import datetime, timedelta, timezone
from typing import List

from django.db.models import Q

from fyle_accounting_mappings.models import CategoryMapping, DestinationAttribute, ExpenseAttribute, Mapping
from fyle_integrations_platform_connector import PlatformConnector

Expand Down Expand Up @@ -57,15 +60,21 @@ def construct_attributes_filter(self, attribute_type: str, paginated_destination
:return: dict
"""
filters = {
'attribute_type': attribute_type,
'workspace_id': self.workspace_id
"attribute_type": attribute_type,
"workspace_id": self.workspace_id,
}

if self.sync_after and self.platform_class_name != 'expense_custom_fields':
filters['updated_at__gte'] = self.sync_after
filters["updated_at__gte"] = self.sync_after

if paginated_destination_attribute_values:
filters['value__in'] = paginated_destination_attribute_values
filters["value__in"] = paginated_destination_attribute_values

account_filters = filters.copy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we doing this copy thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are doing this QBO as well, thought must be for some reason, like mutability

if attribute_type != 'CATEGORY':
if hasattr(self, 'charts_of_accounts') and len(self.charts_of_accounts) > 0:
account_filters["detail__category__in"] = self.charts_of_accounts
filters = account_filters

return filters

Expand Down Expand Up @@ -205,7 +214,7 @@ def construct_payload_and_import_to_fyle(
is_auto_sync_status_allowed = self.get_auto_sync_permission()

filters = self.construct_attributes_filter(self.destination_field)

destination_attributes = DestinationAttribute.objects.filter(**filters)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove redundant database query

The destination_attributes variable is assigned but never used, creating an unnecessary database query. The same filter is applied later for the count and in get_destination_attributes_generator.

-        destination_attributes = DestinationAttribute.objects.filter(**filters)
         destination_attributes_count = DestinationAttribute.objects.filter(**filters).count()
📝 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.

Suggested change
destination_attributes = DestinationAttribute.objects.filter(**filters)
🧰 Tools
🪛 Ruff (0.7.0)

217-217: Local variable destination_attributes is assigned to but never used

Remove assignment to unused variable destination_attributes

(F841)

destination_attributes_count = DestinationAttribute.objects.filter(**filters).count()

is_auto_sync_status_allowed = self.get_auto_sync_permission()
Expand Down
5 changes: 4 additions & 1 deletion apps/mappings/imports/modules/categories.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ class Category(Base):
Class for Category module
"""

def __init__(self, workspace_id: int, destination_field: str, sync_after: datetime):
def __init__(self, workspace_id: int, destination_field: str, sync_after: datetime, charts_of_accounts: List[str]):

self.charts_of_accounts = charts_of_accounts

super().__init__(
workspace_id=workspace_id,
source_field="CATEGORY",
Expand Down
4 changes: 3 additions & 1 deletion apps/mappings/imports/queues.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ def chain_import_fields_to_fyle(workspace_id):
'apps.mappings.imports.tasks.trigger_import_via_schedule',
workspace_id,
'ACCOUNT',
'CATEGORY'
'CATEGORY',
import_settings.charts_of_accounts
)

if import_settings.import_vendors_as_merchants:
Expand All @@ -45,6 +46,7 @@ def chain_import_fields_to_fyle(workspace_id):
workspace_id,
custom_fields_mapping_setting.destination_field,
custom_fields_mapping_setting.source_field,
[],
True
)

Expand Down
19 changes: 16 additions & 3 deletions apps/mappings/imports/tasks.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from typing import List
from apps.mappings.imports.modules.categories import Category
from apps.mappings.imports.modules.cost_centers import CostCenter
from apps.mappings.imports.modules.expense_custom_fields import ExpenseCustomField
Expand All @@ -13,7 +14,7 @@
}


def trigger_import_via_schedule(workspace_id: int, destination_field: str, source_field: str, is_custom: bool = False):
def trigger_import_via_schedule(workspace_id: int, destination_field: str, source_field: str, charts_of_accounts: List[str] = None, is_custom: bool = False):
"""
Trigger import via schedule
:param workspace_id: Workspace id
Expand All @@ -23,10 +24,22 @@ def trigger_import_via_schedule(workspace_id: int, destination_field: str, sourc
import_log = ImportLog.objects.filter(workspace_id=workspace_id, attribute_type=source_field).first()
sync_after = import_log.last_successful_run_at if import_log else None

args = {
'workspace_id': workspace_id,
'destination_field': destination_field,
'sync_after': sync_after,
}

if is_custom:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two is_custom coditions are here

item = ExpenseCustomField(workspace_id, source_field, destination_field, sync_after)
args['source_field'] = source_field

if source_field == 'CATEGORY':
args['charts_of_accounts'] = charts_of_accounts

if is_custom:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix this

item = ExpenseCustomField(**args)
item.trigger_import()
else:
module_class = SOURCE_FIELD_CLASS_MAP[source_field]
item = module_class(workspace_id, destination_field, sync_after)
item = module_class(**args)
item.trigger_import()
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Generated by Django 4.1.2 on 2024-11-25 09:47

import apps.workspaces.models
import django.contrib.postgres.fields
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('workspaces', '0003_remove_advancedsetting_schedule_id_and_more'),
]

operations = [
migrations.AddField(
model_name='importsetting',
name='charts_of_accounts',
field=django.contrib.postgres.fields.ArrayField(base_field=models.CharField(max_length=100), default=apps.workspaces.models.get_default_chart_of_accounts, help_text='list of chart of account types to be imported into Fyle', size=None),
),
]
4 changes: 4 additions & 0 deletions apps/workspaces/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ class Meta:
('EMAIL', 'EMAIL')
)

def get_default_chart_of_accounts():
return ['Expense']


class BusinessCentralCredentials(BaseModel):
"""
Expand Down Expand Up @@ -229,6 +232,7 @@ class ImportSetting(BaseModel):
import_categories = BooleanFalseField(help_text='toggle for import of chart of accounts from Business Central')
import_vendors_as_merchants = BooleanFalseField(help_text='toggle for import of vendors as merchant from Business Central')
workspace = models.OneToOneField(Workspace, on_delete=models.PROTECT, help_text='Reference to Workspace model', related_name="import_settings")
charts_of_accounts = ArrayField(base_field=models.CharField(max_length=100), default=get_default_chart_of_accounts, help_text='list of chart of account types to be imported into Fyle')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to import_charts_of_accounts, just to maintain consistency


class Meta:
db_table = 'import_settings'
Expand Down
13 changes: 12 additions & 1 deletion apps/workspaces/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from apps.accounting_exports.models import AccountingExportSummary
from apps.business_central.utils import BusinessCentralConnector
from apps.fyle.helpers import get_cluster_domain
from apps.mappings.models import ImportLog
from apps.users.models import User
from apps.workspaces.models import (
AdvancedSetting,
Expand Down Expand Up @@ -182,6 +183,7 @@ class Meta:
fields = [
'import_categories',
'import_vendors_as_merchants',
'charts_of_accounts'
]


Expand Down Expand Up @@ -213,15 +215,24 @@ def update(self, instance, validated):
mapping_settings = validated.pop('mapping_settings')
import_settings = validated.pop('import_settings')

if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts:
category_import_log = ImportLog.objects.filter(workspace_id=instance.id, attribute_type='CATEGORY').first()

if category_import_log:
category_import_log.last_successful_run_at = None
category_import_log.save()

Comment on lines +218 to +224
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null checks and optimize database operations

The current implementation has potential issues:

  1. Missing null checks before accessing instance.import_settings.charts_of_accounts
  2. The ImportLog update should be within the transaction block

Consider this safer implementation:

-    if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts:
-        category_import_log = ImportLog.objects.filter(workspace_id=instance.id, attribute_type='CATEGORY').first()
-
-        if category_import_log:
-            category_import_log.last_successful_run_at = None
-            category_import_log.save()

+    with transaction.atomic():
+        old_charts_of_accounts = getattr(instance.import_settings, 'charts_of_accounts', None)
+        new_charts_of_accounts = import_settings.get('charts_of_accounts')
+        
+        if old_charts_of_accounts != new_charts_of_accounts:
+            ImportLog.objects.filter(
+                workspace_id=instance.id,
+                attribute_type='CATEGORY'
+            ).update(last_successful_run_at=None)
📝 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.

Suggested change
if import_settings.get('charts_of_accounts') != instance.import_settings.charts_of_accounts:
category_import_log = ImportLog.objects.filter(workspace_id=instance.id, attribute_type='CATEGORY').first()
if category_import_log:
category_import_log.last_successful_run_at = None
category_import_log.save()
with transaction.atomic():
old_charts_of_accounts = getattr(instance.import_settings, 'charts_of_accounts', None)
new_charts_of_accounts = import_settings.get('charts_of_accounts')
if old_charts_of_accounts != new_charts_of_accounts:
ImportLog.objects.filter(
workspace_id=instance.id,
attribute_type='CATEGORY'
).update(last_successful_run_at=None)

with transaction.atomic():
ImportSetting.objects.update_or_create(
workspace_id=instance.id,
defaults={
'import_categories': import_settings.get('import_categories'),
'import_vendors_as_merchants': import_settings.get('import_vendors_as_merchants')
'import_vendors_as_merchants': import_settings.get('import_vendors_as_merchants'),
'charts_of_accounts': import_settings.get('charts_of_accounts'),
}
)


trigger: ImportSettingsTrigger = ImportSettingsTrigger(
mapping_settings=mapping_settings,
workspace_id=instance.id
Expand Down
2 changes: 2 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,5 @@ pytest-mock==3.8.2
# Sendgrid for sending emails_selected
sendgrid==6.9.7
sentry-sdk==1.19.1

openpyxl
Loading