-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor job sync #193
Refactor job sync #193
Changes from 53 commits
f6df1ec
372c628
d0ad337
7825b9e
b79c74c
97779c6
601e3cb
fb78432
57a4ed4
19dfa7d
151e397
ffbe1f5
372e5ab
737c79d
da072df
e2f4cef
5bc9075
c6a4697
f667558
1c2e3c1
0e98a8a
d23f0cd
7dd6145
f1a31f1
505fabc
e85f6b2
b1f39ca
555a72c
675c37a
84ba2e9
6dd28a9
aa96f62
51e76e5
6cbcb7d
6c54fdc
1da8a7d
9aefa9f
173d31a
5dac65e
ef99211
771c939
0d688ac
53d90b1
766beb3
8393600
7a6d4fa
7a334fb
369009f
592dc6d
f6abaaa
0208231
4048f92
1b0209b
f384ba4
c3a0607
852d69f
352823b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,12 @@ | |
|
||
|
||
def handle_import_exceptions(func): | ||
def new_fn(expense_attribute_instance, *args): | ||
import_log: ImportLog = args[0] | ||
def new_fn(expense_attribute_instance, *args, **kwargs): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor: Use f-strings for better readability. The - 'Import {0} to Fyle and Auto Create Mappings'.format(attribute_type)
+ f'Import {attribute_type} to Fyle and Auto Create Mappings'
- 'Invalid Token or Sage 300 credentials does not exist workspace_id - {0}'.format(workspace_id)
+ f'Invalid Token or Sage 300 credentials does not exist workspace_id - {workspace_id}' Also applies to: 29-29, 43-43 |
||
import_log = None | ||
if isinstance(expense_attribute_instance, ImportLog): | ||
import_log: ImportLog = expense_attribute_instance | ||
else: | ||
import_log: ImportLog = args[0] | ||
workspace_id = import_log.workspace_id | ||
attribute_type = import_log.attribute_type | ||
error = { | ||
|
@@ -28,7 +32,7 @@ def new_fn(expense_attribute_instance, *args): | |
'response': None | ||
} | ||
try: | ||
return func(expense_attribute_instance, *args) | ||
return func(expense_attribute_instance, *args, **kwargs) | ||
except WrongParamsError as exception: | ||
error['message'] = exception.message | ||
error['response'] = exception.response | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||
import math | ||||||
import logging | ||||||
from typing import List | ||||||
from datetime import ( | ||||||
datetime, | ||||||
|
@@ -20,6 +21,10 @@ | |||||
from apps.accounting_exports.models import Error | ||||||
|
||||||
|
||||||
logger = logging.getLogger(__name__) | ||||||
logger.level = logging.INFO | ||||||
|
||||||
|
||||||
class Base: | ||||||
""" | ||||||
The Base class for all the modules | ||||||
|
@@ -299,6 +304,8 @@ def post_to_fyle_and_sync(self, fyle_payload: List[object], resource_class, is_l | |||||
:param is_last_batch: bool | ||||||
:param import_log: ImportLog object | ||||||
""" | ||||||
logger.info("| Importing {} to Fyle | Content: {{WORKSPACE_ID: {} Fyle Payload count: {} is_last_batch: {}}}".format(self.destination_field, self.workspace_id, len(fyle_payload), is_last_batch)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimize logging with f-strings. Convert the logging statement to use f-strings for better performance and readability. - logger.info("| Importing {} to Fyle | Content: {{WORKSPACE_ID: {} Fyle Payload count: {} is_last_batch: {}}}".format(self.destination_field, self.workspace_id, len(fyle_payload), is_last_batch))
+ logger.info(f"| Importing {self.destination_field} to Fyle | Content: {{WORKSPACE_ID: {self.workspace_id} Fyle Payload count: {len(fyle_payload)} is_last_batch: {is_last_batch}}}") Committable suggestion
Suggested change
ToolsRuff
|
||||||
|
||||||
if fyle_payload and self.platform_class_name in ['expense_custom_fields', 'merchants']: | ||||||
resource_class.post(fyle_payload) | ||||||
elif fyle_payload: | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||||||||||||||||
from datetime import datetime | ||||||||||||||||||
from typing import List | ||||||||||||||||||
from apps.mappings.imports.modules.base import Base | ||||||||||||||||||
from apps.sage300.models import CostCategory | ||||||||||||||||||
from fyle_accounting_mappings.models import DestinationAttribute | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
|
@@ -39,23 +40,29 @@ def construct_fyle_payload( | |||||||||||||||||
""" | ||||||||||||||||||
payload = [] | ||||||||||||||||||
|
||||||||||||||||||
job_ids_in_cost_category = CostCategory.objects.filter( | ||||||||||||||||||
workspace_id = self.workspace_id, | ||||||||||||||||||
job_id__in = [attribute.destination_id for attribute in paginated_destination_attributes] | ||||||||||||||||||
).values_list('job_id', flat=True).distinct() | ||||||||||||||||||
Comment on lines
+43
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a set comprehension for performance. The current list comprehension inside the filter might be less efficient for larger datasets. Using a set comprehension can improve performance since membership tests in sets are faster. - job_id__in=[attribute.destination_id for attribute in paginated_destination_attributes]
+ job_id__in={attribute.destination_id for attribute in paginated_destination_attributes} Committable suggestion
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
for attribute in paginated_destination_attributes: | ||||||||||||||||||
project = { | ||||||||||||||||||
'name': attribute.value, | ||||||||||||||||||
'code': attribute.destination_id, | ||||||||||||||||||
'description': 'Sage 300 Project - {0}, Id - {1}'.format( | ||||||||||||||||||
attribute.value, | ||||||||||||||||||
attribute.destination_id | ||||||||||||||||||
), | ||||||||||||||||||
'is_enabled': True if attribute.active is None else attribute.active | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
# Create a new project if it does not exist in Fyle | ||||||||||||||||||
if attribute.value.lower() not in existing_fyle_attributes_map: | ||||||||||||||||||
payload.append(project) | ||||||||||||||||||
# Disable the existing project in Fyle if auto-sync status is allowed and the destination_attributes is inactive | ||||||||||||||||||
elif is_auto_sync_status_allowed and not attribute.active: | ||||||||||||||||||
project['id'] = existing_fyle_attributes_map[attribute.value.lower()] | ||||||||||||||||||
payload.append(project) | ||||||||||||||||||
if attribute.destination_id in job_ids_in_cost_category: | ||||||||||||||||||
project = { | ||||||||||||||||||
'name': attribute.value, | ||||||||||||||||||
'code': attribute.destination_id, | ||||||||||||||||||
'description': 'Sage 300 Project - {0}, Id - {1}'.format( | ||||||||||||||||||
attribute.value, | ||||||||||||||||||
attribute.destination_id | ||||||||||||||||||
), | ||||||||||||||||||
'is_enabled': True if attribute.active is None else attribute.active | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+53
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optimize string formatting using f-strings. Using f-strings can make the code cleaner and potentially faster. - 'description': 'Sage 300 Project - {0}, Id - {1}'.format(attribute.value, attribute.destination_id)
+ 'description': f'Sage 300 Project - {attribute.value}, Id - {attribute.destination_id}' Committable suggestion
Suggested change
ToolsRuff
|
||||||||||||||||||
|
||||||||||||||||||
# Create a new project if it does not exist in Fyle | ||||||||||||||||||
if attribute.value.lower() not in existing_fyle_attributes_map: | ||||||||||||||||||
payload.append(project) | ||||||||||||||||||
# Disable the existing project in Fyle if auto-sync status is allowed and the destination_attributes is inactive | ||||||||||||||||||
elif is_auto_sync_status_allowed and not attribute.active: | ||||||||||||||||||
project['id'] = existing_fyle_attributes_map[attribute.value.lower()] | ||||||||||||||||||
payload.append(project) | ||||||||||||||||||
|
||||||||||||||||||
return payload |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
from apps.workspaces.models import Sage300Credential | ||
from apps.sage300.utils import SageDesktopConnector | ||
from apps.mappings.models import ImportLog | ||
|
||
|
||
def sync_sage300_attributes(sage300_attribute_type: str, workspace_id: int): | ||
def sync_sage300_attributes(sage300_attribute_type: str, workspace_id: int, import_log: ImportLog = None): | ||
sage300_credentials: Sage300Credential = Sage300Credential.objects.get(workspace_id=workspace_id) | ||
|
||
sage300_connection = SageDesktopConnector( | ||
|
@@ -12,8 +13,8 @@ def sync_sage300_attributes(sage300_attribute_type: str, workspace_id: int): | |
|
||
sync_functions = { | ||
'JOB': sage300_connection.sync_jobs, | ||
'COST_CODE': sage300_connection.sync_cost_codes, | ||
'COST_CATEGORY': sage300_connection.sync_cost_categories, | ||
'COST_CODE': lambda:sage300_connection.sync_cost_codes(import_log), | ||
'COST_CATEGORY': lambda:sage300_connection.sync_cost_categories(import_log), | ||
Comment on lines
+16
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification The functions
To address this, ensure that the Locations needing attention:
Analysis chainApproved: Proper use of The addition of Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the correct usage of `import_log` in the sync functions.
# Test: Search for the function implementations. Expect: Proper handling of `import_log`.
ast-grep --lang python --pattern $'def sync_cost_codes($_, $_, $_) { $$$ }' apps/sage300/utils.py
ast-grep --lang python --pattern $'def sync_cost_categories($_, $_, $_) { $$$ }' apps/sage300/utils.py
Length of output: 1131 Script: #!/bin/bash
# Description: Verify the correct usage of `import_log` in the sync functions.
# Corrected Test: Search for the function implementations. Expect: Proper handling of `import_log`.
ast-grep --lang python --pattern 'def sync_cost_codes(import_log, $$$):'
ast-grep --lang python --pattern 'def sync_cost_categories(import_log, $$$):'
Length of output: 154 Script: #!/bin/bash
# Description: Verify the correct usage of `import_log` in the sync functions across the entire codebase.
# Search for the function definitions of sync_cost_codes and sync_cost_categories.
rg 'def sync_cost_codes'
rg 'def sync_cost_categories'
Length of output: 208 Script: #!/bin/bash
# Description: Extract the full implementations of the sync functions to verify the correct usage of `import_log`.
# Extract the implementation of sync_cost_codes.
ast-grep --lang python --pattern 'def sync_cost_codes(self, _import_log = None): { $$$ }' apps/sage300/utils.py
# Extract the implementation of sync_cost_categories.
ast-grep --lang python --pattern 'def sync_cost_categories(self, import_log = None): { $$$ }' apps/sage300/utils.py
Length of output: 231 Script: #!/bin/bash
# Description: Extract the full content of apps/sage300/utils.py for manual inspection.
# Extract the full content of the file.
rg '' apps/sage300/utils.py
Length of output: 12752 |
||
'ACCOUNT': sage300_connection.sync_accounts, | ||
'VENDOR': sage300_connection.sync_vendors, | ||
'COMMITMENT': sage300_connection.sync_commitments, | ||
|
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.
Use proper exception chaining to clarify exception sources.
When re-raising exceptions, it's recommended to use
from
to provide more context and avoid masking other exceptions.