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

Imports refactor master #322

Merged
merged 14 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 3 additions & 1 deletion .github/workflows/codecov.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ jobs:
environment: CI Environment
steps:
- uses: actions/checkout@v2
with:
submodules: recursive
- name: Bring up Services and Run Tests
run: |
docker-compose -f docker-compose-pipeline.yml build
docker-compose -f docker-compose-pipeline.yml up -d
docker-compose -f docker-compose-pipeline.yml exec -T api pytest tests/ --cov --cov-report=xml --cov-fail-under=94
docker-compose -f docker-compose-pipeline.yml exec -T api pytest tests/ --cov --cov-report=xml --cov-fail-under=88
echo "STATUS=$(cat pytest-coverage.txt | grep 'Required test' | awk '{ print $1 }')" >> $GITHUB_ENV
echo "FAILED=$(cat test-reports/report.xml | awk -F'=' '{print $5}' | awk -F' ' '{gsub(/"/, "", $1); print $1}')" >> $GITHUB_ENV
env:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/production_deployment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ jobs:
environment: Production
steps:
- uses: actions/checkout@v2
with:
submodules: recursive
- name: push to dockerhub
uses: fylein/docker-release-action@master
env:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/pytest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ jobs:
environment: CI Environment
steps:
- uses: actions/checkout@v2
with:
submodules: recursive
- name: Bring up Services and test for token health
run: |
docker-compose -f docker-compose-pipeline.yml build
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/staging_deployment.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ jobs:
environment: Staging
steps:
- uses: actions/checkout@v2
with:
submodules: recursive
- name: push to dockerhub
uses: fylein/docker-release-action@master
env:
Expand Down
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[submodule "fyle_integrations_imports"]
path = fyle_integrations_imports
url = [email protected]:fylein/fyle_integrations_imports.git
8 changes: 8 additions & 0 deletions apps/mappings/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,11 @@
"paid date",
"expense created date",
]

SYNC_METHODS = {
'ACCOUNT': 'accounts',
'ITEM': 'items',
'TAX_CODE': 'tax_codes',
'CONTACT': 'contacts',
'CUSTOMER': 'customers',
}
54 changes: 54 additions & 0 deletions apps/mappings/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from xerosdk.exceptions import WrongParamsError as XeroWrongParamsError

from apps.workspaces.models import XeroCredentials
from fyle_integrations_imports.models import ImportLog

logger = logging.getLogger(__name__)
logger.level = logging.INFO
Expand Down Expand Up @@ -77,3 +78,56 @@ def new_fn(workspace_id, *args):
return new_fn

return decorator


def handle_import_exceptions_v2(func):
def new_fn(expense_attribute_instance, *args):
import_log: ImportLog = args[0]
workspace_id = import_log.workspace_id
attribute_type = import_log.attribute_type
error = {
'task': 'Import {0} to Fyle and Auto Create Mappings'.format(attribute_type),
'workspace_id': workspace_id,
'message': None,
'response': None
}
try:
return func(expense_attribute_instance, *args)
except WrongParamsError as exception:
error['message'] = exception.message
error['response'] = exception.response
error['alert'] = True
import_log.status = 'FAILED'

except InvalidTokenError:
error['message'] = 'Invalid Token for fyle'
error['alert'] = False
import_log.status = 'FAILED'

except InternalServerError:
error['message'] = 'Internal server error while importing to Fyle'
error['alert'] = True
import_log.status = 'FAILED'

except (XeroWrongParamsError, XeroInvalidTokenError, XeroCredentials.DoesNotExist) as exception:
error['message'] = 'Invalid Token or Xero credentials does not exist workspace_id - {0}'.format(workspace_id)
error['alert'] = False
error['response'] = exception.__dict__
import_log.status = 'FAILED'

except Exception:
response = traceback.format_exc()
error['message'] = 'Something went wrong'
error['response'] = response
error['alert'] = False
import_log.status = 'FATAL'

if error['alert']:
logger.error(error)
else:
logger.info(error)

import_log.error_log = error
import_log.save()

Comment on lines +83 to +132
Copy link

Choose a reason for hiding this comment

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

The introduction of handle_import_exceptions_v2 in apps/mappings/exceptions.py is a valuable addition for handling and logging errors during the import process. A few suggestions for improvement:

  • The function does a good job of handling various exceptions and updating the ImportLog status accordingly. Ensure that all relevant exceptions are covered and that the error messages are clear and informative.

  • The use of error['alert'] to determine whether to log an error as info or error is a good practice. However, consider standardizing the conditions under which error['alert'] is set to True or False to ensure consistency across different types of errors.

  • The function updates the ImportLog with error details, which is crucial for debugging. Ensure that the ImportLog model has appropriate fields to store all necessary error information and that this information is easily accessible for troubleshooting.

  • Consider adding unit tests specifically for handle_import_exceptions_v2 to verify that it behaves as expected for different types of exceptions. This can help ensure robustness and ease future maintenance.

  • Verify coverage of all relevant exceptions and clarity of error messages.

  • Standardize conditions for setting error['alert'].

  • Ensure the ImportLog model adequately supports error logging.

  • Add unit tests for handle_import_exceptions_v2.

return new_fn
44 changes: 8 additions & 36 deletions apps/mappings/helpers.py
Original file line number Diff line number Diff line change
@@ -1,42 +1,14 @@
from datetime import datetime

from django_q.models import Schedule
from fyle_accounting_mappings.models import MappingSetting

from apps.fyle.enums import FyleAttributeEnum
from apps.workspaces.models import WorkspaceGeneralSettings


def schedule_or_delete_fyle_import_tasks(configuration: WorkspaceGeneralSettings):
def is_auto_sync_allowed(workspace_general_settings: WorkspaceGeneralSettings, mapping_setting: MappingSetting = None):
"""
:param configuration: WorkspaceGeneralSettings Instance
:return: None
Get the auto sync permission
:return: bool
"""
project_mapping = MappingSetting.objects.filter(
source_field=FyleAttributeEnum.PROJECT, workspace_id=configuration.workspace_id
).first()
if (
configuration.import_categories
or (project_mapping and project_mapping.import_to_fyle)
or configuration.import_suppliers_as_merchants
):
start_datetime = datetime.now()
Schedule.objects.update_or_create(
func="apps.mappings.tasks.auto_import_and_map_fyle_fields",
cluster='import',
args="{}".format(configuration.workspace_id),
defaults={
"schedule_type": Schedule.MINUTES,
"minutes": 24 * 60,
"next_run": start_datetime,
},
)
elif (
not configuration.import_categories
and not (project_mapping and project_mapping.import_to_fyle)
and not configuration.import_suppliers_as_merchants
):
Schedule.objects.filter(
func="apps.mappings.tasks.auto_import_and_map_fyle_fields",
args="{}".format(configuration.workspace_id),
).delete()
is_auto_sync_status_allowed = False
if (mapping_setting and mapping_setting.destination_field == 'CUSTOMER' and mapping_setting.source_field == 'PROJECT') or workspace_general_settings.import_categories:
is_auto_sync_status_allowed = True

return is_auto_sync_status_allowed
134 changes: 73 additions & 61 deletions apps/mappings/queue.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
from datetime import datetime, timedelta
from datetime import datetime

from django_q.models import Schedule
from fyle_accounting_mappings.models import MappingSetting
from apps.fyle.enums import FyleAttributeEnum
from apps.mappings.constants import SYNC_METHODS
from apps.mappings.helpers import is_auto_sync_allowed
from fyle_integrations_imports.dataclasses import TaskSetting
from fyle_integrations_imports.queues import chain_import_fields_to_fyle
from apps.workspaces.models import WorkspaceGeneralSettings, XeroCredentials


def schedule_auto_map_employees(employee_mapping_preference: str, workspace_id: str):
Expand All @@ -26,71 +32,77 @@
schedule.delete()


def schedule_cost_centers_creation(import_to_fyle, workspace_id: int):
if import_to_fyle:
schedule, _ = Schedule.objects.update_or_create(
func="apps.mappings.tasks.auto_create_cost_center_mappings",
cluster='import',
args="{}".format(workspace_id),
defaults={
"schedule_type": Schedule.MINUTES,
"minutes": 24 * 60,
"next_run": datetime.now(),
},
)
else:
schedule: Schedule = Schedule.objects.filter(
func="apps.mappings.tasks.auto_create_cost_center_mappings",
args="{}".format(workspace_id),
).first()

if schedule:
schedule.delete()
def construct_tasks_and_chain_import_fields_to_fyle(workspace_id: int):
"""
Construct tasks and chain import fields to fyle
:param workspace_id: Workspace Id
"""
mapping_settings = MappingSetting.objects.filter(
workspace_id=workspace_id,
import_to_fyle=True
)
workspace_general_settings = WorkspaceGeneralSettings.objects.get(
workspace_id=workspace_id
)
credentials = XeroCredentials.objects.get(
workspace_id=workspace_id
)

# import_vendors_as_merchants is not used in xero, placeholder to avoid KeyError
task_settings: TaskSetting = {
'import_tax': None,
'import_vendors_as_merchants': None,
'import_suppliers_as_merchants': None,
'import_categories': None,
'import_items': None,
'mapping_settings': [],
'credentials': credentials,
'sdk_connection_string': 'apps.xero.utils.XeroConnector',
'custom_properties': None
}

def schedule_tax_groups_creation(import_tax_codes, workspace_id):
if import_tax_codes:
schedule, _ = Schedule.objects.update_or_create(
func="apps.mappings.tasks.auto_create_tax_codes_mappings",
cluster='import',
args="{}".format(workspace_id),
defaults={
"schedule_type": Schedule.MINUTES,
"minutes": 24 * 60,
"next_run": datetime.now(),
},
)
else:
schedule: Schedule = Schedule.objects.filter(
func="apps.mappings.tasks.auto_create_tax_codes_mappings",
args="{}".format(workspace_id),
).first()
# For now adding only for PROJECT
ALLOWED_SOURCE_FIELDS = [
FyleAttributeEnum.PROJECT,
FyleAttributeEnum.COST_CENTER,
]

if schedule:
schedule.delete()
if workspace_general_settings.import_tax_codes:
task_settings['import_tax'] = {
'destination_field': 'TAX_CODE',
'destination_sync_methods': [SYNC_METHODS['TAX_CODE']],
'is_auto_sync_enabled': False,
'is_3d_mapping': False
}

if workspace_general_settings.import_categories:
task_settings['import_categories'] = {
'destination_field': 'ACCOUNT',
'destination_sync_methods': [SYNC_METHODS['ACCOUNT']],
'is_auto_sync_enabled': True,
'is_3d_mapping': False,
'charts_of_accounts': workspace_general_settings.charts_of_accounts
}

def schedule_fyle_attributes_creation(workspace_id: int):
mapping_settings = MappingSetting.objects.filter(
is_custom=True, import_to_fyle=True, workspace_id=workspace_id
).all()
if workspace_general_settings.import_suppliers_as_merchants:
task_settings['custom_properties'] = {
'func': 'apps.mappings.tasks.auto_create_suppliers_as_merchants',
'args': {
'workspace_id': workspace_id
}
}

if mapping_settings:
schedule, _ = Schedule.objects.get_or_create(
func="apps.mappings.tasks.async_auto_create_custom_field_mappings",
cluster='import',
args="{0}".format(workspace_id),
defaults={
"schedule_type": Schedule.MINUTES,
"minutes": 24 * 60,
"next_run": datetime.now() + timedelta(hours=24),
},
)
else:
schedule: Schedule = Schedule.objects.filter(
func="apps.mappings.tasks.async_auto_create_custom_field_mappings",
args=workspace_id,
).first()
for mapping_setting in mapping_settings:
if mapping_setting.source_field in ALLOWED_SOURCE_FIELDS or mapping_setting.is_custom:
task_settings['mapping_settings'].append(
{
'source_field': mapping_setting.source_field,
'destination_field': mapping_setting.destination_field,

Check warning on line 101 in apps/mappings/queue.py

View check run for this annotation

Codecov / codecov/patch

apps/mappings/queue.py#L96-L101

Added lines #L96 - L101 were not covered by tests
'is_custom': mapping_setting.is_custom,
'destination_sync_methods': [SYNC_METHODS.get(mapping_setting.destination_field.upper(), 'tracking_categories')],
'is_auto_sync_enabled': is_auto_sync_allowed(workspace_general_settings, mapping_setting)
}
)

if schedule:
schedule.delete()
chain_import_fields_to_fyle(workspace_id, task_settings)
Comment on lines +35 to +108
Copy link

Choose a reason for hiding this comment

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

The refactoring in apps/mappings/queue.py to construct_tasks_and_chain_import_fields_to_fyle is a significant improvement in terms of clarity and functionality. However, there are a few areas that could be further refined:

  • The function now handles a broader range of tasks, which is a positive change. Ensure that the function's documentation is updated to accurately describe its new responsibilities and the parameters it accepts.

  • The construction of task_settings is a critical part of this function. Consider breaking down this construction into smaller, more focused functions or methods to improve readability and maintainability.

  • The use of hard-coded values and conditions (e.g., ALLOWED_SOURCE_FIELDS, import_tax, import_categories) should be carefully reviewed to ensure they are flexible and maintainable. Where possible, consider externalizing these configurations or making them more dynamic.

  • Ensure comprehensive testing of this function, particularly because it now encompasses a wider range of functionality. This includes testing for correct task construction, handling of various settings, and interaction with the chain_import_fields_to_fyle function.

  • Update the function's documentation to reflect its expanded responsibilities.

  • Consider refactoring the construction of task_settings to improve readability.

  • Review and possibly externalize hard-coded values and conditions for better flexibility.

  • Ensure comprehensive testing of the function's expanded functionality.

50 changes: 50 additions & 0 deletions apps/mappings/schedules.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
from datetime import datetime
from typing import List, Dict
from apps.workspaces.models import WorkspaceGeneralSettings
from django_q.models import Schedule
from fyle_accounting_mappings.models import MappingSetting


def new_schedule_or_delete_fyle_import_tasks(
workspace_general_settings_instance: WorkspaceGeneralSettings,
mapping_settings: List[Dict]
):
"""
Schedule or delete fyle import tasks based on the
workspace general settings and mapping settings
:param workspace_general_settings_instance: WorkspaceGeneralSettings instance
:param mapping_settings: List of mapping settings
:return: None
"""
# short-hand notation, it returns True as soon as it encounters import_to_fyle as True
task_to_be_scheduled = any(mapping_setting['import_to_fyle'] for mapping_setting in mapping_settings)

if (
task_to_be_scheduled
or workspace_general_settings_instance.import_customers
or workspace_general_settings_instance.import_tax_codes
or workspace_general_settings_instance.import_categories
or workspace_general_settings_instance.import_suppliers_as_merchants
):
Schedule.objects.update_or_create(
func='apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle',
args='{}'.format(workspace_general_settings_instance.workspace_id),
defaults={
'schedule_type':Schedule.MINUTES,
'minutes': 24 * 60,
'next_run': datetime.now(),
'cluster': 'import'
}
)
else:
import_fields_count = MappingSetting.objects.filter(
workspace_id=workspace_general_settings_instance.workspace_id,
import_to_fyle=True
).count()

# if there are no import fields, delete the schedule
if import_fields_count == 0:
Schedule.objects.filter(
func='apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle',
args='{}'.format(workspace_general_settings_instance.workspace_id)
).delete()
Loading
Loading