Expenses Sync APIs #32
Conversation
ruuushhh
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## exportable-expense-api #32 +/- ##
==========================================================
- Coverage 87.74% 86.87% -0.88%
==========================================================
Files 25 28 +3
Lines 1126 1211 +85
==========================================================
+ Hits 988 1052 +64
- Misses 138 159 +21 ☔ View full report in Codecov by Sentry. |
WalkthroughThe changes involve enhancing a Django application with new functionalities for handling Fyle platform data. New exception handling, task queuing, and import mechanisms are introduced, alongside modifications to models, views, and utilities to support synchronization with Fyle's expense management system. The updates include new URL patterns, improved error handling, and refactoring to streamline the integration process. Changes
TipsChat with CodeRabbit Bot (
|
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (6)
- apps/fyle/exceptions.py (1 hunks)
- apps/fyle/queue.py (1 hunks)
- apps/fyle/tasks.py (1 hunks)
- apps/fyle/urls.py (2 hunks)
- apps/fyle/views.py (2 hunks)
- tests/test_fyle/test_views.py (1 hunks)
Additional comments: 7
apps/fyle/queue.py (2)
18-24: The
update_or_createmethod is used without error handling. Ensure that any potential exceptions are handled appropriately, possibly in the upstream code.27-30: The
async_taskfunction is called without error handling. Ensure that any potential exceptions are handled appropriately, possibly in the upstream code.apps/fyle/tasks.py (2)
21-66: The changes to the
import_expensesfunction, including the addition of theaccounting_exportparameter and the use of thehandle_exceptionsdecorator, are correctly implemented and follow best practices for error handling and database transactions. The logic for updating thelast_synced_attimestamp and creating expense objects and accounting exports is sound. The function also correctly updates theaccounting_exportstatus to 'COMPLETE' at the end of the process.21-66: The summary does not mention the addition of the
accounting_exportparameter to theimport_expensesfunction or the specific changes within the function. It would be beneficial to update the summary to reflect these important changes for clarity and completeness.apps/fyle/urls.py (1)
- 33-33: The new URL path for syncing accounting exports is correctly added. Ensure that the corresponding view
AccoutingExportSyncView(after correcting the typo) is implemented with the necessary logic and error handling as described in the pull request summary.apps/fyle/views.py (2)
9-9: The import statements for
queue_import_credit_card_expensesandqueue_import_reimbursable_expenseshave been correctly added.91-92: The
postmethod correctly callsqueue_import_reimbursable_expensesandqueue_import_credit_card_expenseswithsynchronous=True.
tests/test_fyle/test_views.py
Outdated
| def test_accounting_export_sync_view(api_client, test_connection, create_temp_workspace, add_fyle_credentials, add_export_settings): | ||
|
|
||
| access_token = test_connection.access_token | ||
| url = reverse('sync-accounting-exports', kwargs={'workspace_id': 1}) | ||
| api_client.credentials(HTTP_AUTHORIZATION='Bearer {}'.format(access_token)) | ||
|
|
||
| response = api_client.post(url) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
The test test_accounting_export_sync_view only checks for the response status code. It would be beneficial to also verify the expected side effects of the POST request, such as changes in the database or certain actions being triggered, to ensure the endpoint is functioning correctly beyond just returning a 200 status code.
| def handle_exceptions(func): | ||
| @wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| try: | ||
| return func(*args, **kwargs) | ||
| except FyleCredential.DoesNotExist: | ||
| logger.info('Fyle credentials not found %s', args[0]) # args[1] is workspace_id | ||
| args[1].detail = {'message': 'Fyle credentials do not exist in workspace'} | ||
| args[1].status = 'FAILED' | ||
| args[1].save() | ||
|
|
||
| except NoPrivilegeError: | ||
| logger.info('Invalid Fyle Credentials / Admin is disabled') | ||
| args[1].detail = {'message': 'Invalid Fyle Credentials / Admin is disabled'} | ||
| args[1].status = 'FAILED' | ||
| args[1].save() | ||
|
|
||
| except Exception: | ||
| error = traceback.format_exc() | ||
| args[1].detail = {'error': error} | ||
| args[1].status = 'FATAL' | ||
| args[1].save() | ||
| logger.exception('Something unexpected happened workspace_id: %s %s', args[0], args[1].detail) | ||
|
|
||
| return wrapper |
There was a problem hiding this comment.
The handle_exceptions decorator assumes a specific order and type of arguments, which could lead to issues if the decorated function does not follow this pattern. Consider making the decorator more flexible to handle different argument orders or types, and ensure that exceptions are re-raised after logging and updating the status to notify the calling code of the exception.
+ raiseCommittable 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.
| def handle_exceptions(func): | |
| @wraps(func) | |
| def wrapper(*args, **kwargs): | |
| try: | |
| return func(*args, **kwargs) | |
| except FyleCredential.DoesNotExist: | |
| logger.info('Fyle credentials not found %s', args[0]) # args[1] is workspace_id | |
| args[1].detail = {'message': 'Fyle credentials do not exist in workspace'} | |
| args[1].status = 'FAILED' | |
| args[1].save() | |
| except NoPrivilegeError: | |
| logger.info('Invalid Fyle Credentials / Admin is disabled') | |
| args[1].detail = {'message': 'Invalid Fyle Credentials / Admin is disabled'} | |
| args[1].status = 'FAILED' | |
| args[1].save() | |
| except Exception: | |
| error = traceback.format_exc() | |
| args[1].detail = {'error': error} | |
| args[1].status = 'FATAL' | |
| args[1].save() | |
| logger.exception('Something unexpected happened workspace_id: %s %s', args[0], args[1].detail) | |
| return wrapper | |
| def handle_exceptions(func): | |
| @wraps(func) | |
| def wrapper(*args, **kwargs): | |
| try: | |
| return func(*args, **kwargs) | |
| except FyleCredential.DoesNotExist: | |
| logger.info('Fyle credentials not found %s', args[0]) # args[1] is workspace_id | |
| args[1].detail = {'message': 'Fyle credentials do not exist in workspace'} | |
| args[1].status = 'FAILED' | |
| args[1].save() | |
| raise | |
| except NoPrivilegeError: | |
| logger.info('Invalid Fyle Credentials / Admin is disabled') | |
| args[1].detail = {'message': 'Invalid Fyle Credentials / Admin is disabled'} | |
| args[1].status = 'FAILED' | |
| args[1].save() | |
| raise | |
| except Exception: | |
| error = traceback.format_exc() | |
| args[1].detail = {'error': error} | |
| args[1].status = 'FATAL' | |
| args[1].save() | |
| logger.exception('Something unexpected happened workspace_id: %s %s', args[0], args[1].detail) | |
| raise | |
| return wrapper |
| @@ -19,6 +19,7 @@ | |||
| from django.urls import path | |||
|
|
|||
| from apps.fyle.views import ( | |||
| AccoutingExportSyncView, | |||
There was a problem hiding this comment.
There appears to be a typo in the class name AccoutingExportSyncView. It should be corrected to AccountingExportSyncView if that is the intended name.
- AccoutingExportSyncView,
+ AccountingExportSyncView,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.
| AccoutingExportSyncView, | |
| AccountingExportSyncView, |
| @@ -76,3 +77,20 @@ def get(self, request, *args, **kwargs): | |||
| data={'exportable_expense_group_ids': exportable_ids}, | |||
| status=status.HTTP_200_OK | |||
| ) | |||
|
|
|||
|
|
|||
| class AccoutingExportSyncView(generics.CreateAPIView): | |||
There was a problem hiding this comment.
The class name AccoutingExportSyncView contains a typo and should be corrected to AccountingExportSyncView.
- class AccoutingExportSyncView(generics.CreateAPIView):
+ class AccountingExportSyncView(generics.CreateAPIView):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.
| class AccoutingExportSyncView(generics.CreateAPIView): | |
| class AccountingExportSyncView(generics.CreateAPIView): |
| def post(self, request, *args, **kwargs): | ||
| """ | ||
| Post expense groups creation | ||
| """ | ||
|
|
||
| queue_import_reimbursable_expenses(kwargs['workspace_id'], synchronous=True) | ||
| queue_import_credit_card_expenses(kwargs['workspace_id'], synchronous=True) | ||
|
|
||
| return Response( | ||
| status=status.HTTP_200_OK | ||
| ) |
There was a problem hiding this comment.
The post method in AccoutingExportSyncView should use the handle_exceptions decorator to ensure consistent error handling across the application.
| ) | ||
| return | ||
|
|
||
| import_expenses(workspace_id, accounting_export, 'PERSONAL_CASH_ACCOUNT', 'PERSONAL') |
There was a problem hiding this comment.
The import_expenses function is called with hardcoded values 'PERSONAL_CASH_ACCOUNT' and 'PERSONAL'. Consider making these values configurable or ensuring they are appropriate for all use cases.
apps/fyle/queue.py
Outdated
| def queue_import_reimbursable_expenses(workspace_id: int, synchronous: bool = False): | ||
| """ | ||
| Queue Import of Reimbursable Expenses from Fyle | ||
| :param workspace_id: Workspace id | ||
| :return: None | ||
| """ | ||
| accounting_export, _ = AccountingExport.objects.update_or_create( | ||
| workspace_id=workspace_id, | ||
| type='FETCHING_REIMBURSABLE_EXPENSES', | ||
| defaults={ | ||
| 'status': 'IN_PROGRESS' | ||
| } | ||
| ) | ||
|
|
||
| if not synchronous: | ||
| async_task( | ||
| 'apps.fyle.tasks.import_reimbursable_expenses', | ||
| workspace_id, accounting_export, | ||
| ) | ||
| return | ||
|
|
||
| import_expenses(workspace_id, accounting_export, 'PERSONAL_CASH_ACCOUNT', 'PERSONAL') | ||
|
|
||
|
|
||
| def queue_import_credit_card_expenses(workspace_id: int, synchronous: bool = False): | ||
| """ | ||
| Queue Import of Credit Card Expenses from Fyle | ||
| :param workspace_id: Workspace id | ||
| :return: None | ||
| """ | ||
| accounting_export, _ = AccountingExport.objects.update_or_create( | ||
| workspace_id=workspace_id, | ||
| type='FETCHING_CREDIT_CARD_EXPENSES', | ||
| defaults={ | ||
| 'status': 'IN_PROGRESS' | ||
| } | ||
| ) | ||
|
|
||
| if not synchronous: | ||
| async_task( | ||
| 'apps.fyle.tasks.import_credit_card_expenses', | ||
| workspace_id, accounting_export, | ||
| ) | ||
| return | ||
|
|
||
| import_expenses(workspace_id, accounting_export, 'PERSONAL_CASH_ACCOUNT', 'PERSONAL') |
There was a problem hiding this comment.
The functions queue_import_reimbursable_expenses and queue_import_credit_card_expenses have duplicated logic. Consider refactoring to reduce code duplication, perhaps by extracting common functionality into a helper function.
* Get Fyle Orgs API * Get user profile api (#34) * Bug Fix: After testing (#35) * Get user profile api * Bug Fix: After testing * Bug Fix: After testing * Onboarding state bug fix * Onboarding state bug fix * Test case resolved * Import expenses fixed * Import expenses bug fixed * test cases resolved * test cases * logic fixed for date * Fix tests * logic fixed for date * Imports Folder Structure (#36) * Imports Folder Structure * Import base files added (#39) * Import base files added * Import Categories support added (#40) * Import Categories support added * Import merchants support added (#41) * Import merchants support added * Import cost centers support added (#42) * Import cost centers support added * Import project support added (#43) * Import project support added * Schedule, queues signals configured (#44) * Schedule, queues signals configured * Imports bugs fixed (#45) * Imports bugs fixed * Comments resolved
ruuushhh
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 30
Configuration used: CodeRabbit UI
Files selected for processing (37)
- apps/accounting_exports/models.py (3 hunks)
- apps/business_central/utils.py (6 hunks)
- apps/fyle/helpers.py (2 hunks)
- apps/fyle/queue.py (1 hunks)
- apps/fyle/serializers.py (2 hunks)
- apps/fyle/tasks.py (1 hunks)
- apps/fyle/views.py (3 hunks)
- apps/mappings/apps.py (1 hunks)
- apps/mappings/constants.py (1 hunks)
- apps/mappings/exceptions.py (1 hunks)
- apps/mappings/imports/modules/base.py (1 hunks)
- apps/mappings/imports/modules/categories.py (1 hunks)
- apps/mappings/imports/modules/cost_centers.py (1 hunks)
- apps/mappings/imports/modules/expense_custom_fields.py (1 hunks)
- apps/mappings/imports/modules/merchants.py (1 hunks)
- apps/mappings/imports/modules/projects.py (1 hunks)
- apps/mappings/imports/queues.py (1 hunks)
- apps/mappings/imports/schedules.py (1 hunks)
- apps/mappings/imports/tasks.py (1 hunks)
- apps/mappings/models.py (1 hunks)
- apps/mappings/signals.py (1 hunks)
- apps/mappings/tasks.py (1 hunks)
- apps/mappings/urls.py (1 hunks)
- apps/users/helpers.py (1 hunks)
- apps/users/urls.py (1 hunks)
- apps/users/views.py (1 hunks)
- apps/workspaces/helpers.py (2 hunks)
- apps/workspaces/models.py (3 hunks)
- apps/workspaces/serializers.py (3 hunks)
- apps/workspaces/urls.py (2 hunks)
- apps/workspaces/views.py (2 hunks)
- ms_business_central_api/urls.py (1 hunks)
- tests/conftest.py (2 hunks)
- tests/test_fyle/fixtures.py (2 hunks)
- tests/test_fyle/test_views.py (3 hunks)
- tests/test_users/test_views.py (1 hunks)
- tests/test_workspaces/test_view.py (2 hunks)
Files skipped from review due to trivial changes (4)
- apps/mappings/constants.py
- apps/mappings/signals.py
- apps/mappings/urls.py
- apps/workspaces/urls.py
Additional comments: 53
apps/accounting_exports/models.py (1)
- 37-83: The logic in
_group_expensesfunction seems to be correctly implemented. It groups expenses based on thefund_sourceand fields specified inexport_setting. The function also dynamically adjusts the fields used for grouping based on thefund_source. This is a good use of dynamic programming to handle different grouping requirements.apps/business_central/utils.py (5)
17-23: The parameter name correction from
enviromenttoenvironmentaligns with the summary and is a good fix for a typo.91-97: The addition of the
field_namesparameter in thesync_accountsmethod is consistent with the summary and is a good enhancement for flexibility.103-112: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [100-109]
The addition of the
field_namesparameter in thesync_vendorsmethod is consistent with the summary and is a good enhancement for flexibility.
- 115-124: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [112-121]
The addition of the
field_namesparameter in thesync_employeesmethod is consistent with the summary and is a good enhancement for flexibility.
- 127-134: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [124-133]
The addition of the
field_namesparameter in thesync_locationsmethod is consistent with the summary and is a good enhancement for flexibility.apps/fyle/helpers.py (1)
- 141-147: Ensure that no default parameters are needed for the
get_requestcall within theget_fyle_orgsfunction.apps/fyle/serializers.py (3)
8-8: The summary states that the import for
ExpenseAttributewas both added and removed, which is contradictory. The hunk shows that the import is present, so the summary is incorrect.112-126: The changes to
ExpenseFieldSerializeralign with the summary, introducing new fieldsfield_name,type, andis_custom, and updating theget_expense_fieldsmethod to acceptworkspace_idas an integer parameter.119-119: The method
get_expense_fieldshas been correctly updated to useworkspace_idas a parameter instead ofvalidated_data.apps/fyle/views.py (6)
9-9: The new imports for
queue_import_credit_card_expensesandqueue_import_reimbursable_expensesare correctly added.66-67: The
get_querysetmethod is correctly added toCustomFieldViewto customize the queryset.84-84: The class name
AccoutingExportSyncViewcontains a typo and should be corrected toAccountingExportSyncView.- class AccoutingExportSyncView(generics.CreateAPIView): + class AccountingExportSyncView(generics.CreateAPIView):
88-98: The
postmethod inAccoutingExportSyncViewshould use thehandle_exceptionsdecorator to ensure consistent error handling across the application.64-64: The
pagination_classis correctly set toNoneforCustomFieldView.61-67: The
querysetattribute is correctly removed fromCustomFieldView.apps/mappings/apps.py (1)
- 8-10: The implementation of the
readymethod inMappingsConfigclass follows the Django convention for importing signals. Ensure that the signals are defined correctly inapps.mappings.signalsto avoid any issues with signal registration.apps/mappings/exceptions.py (1)
- 16-65: The
handle_import_exceptionsdecorator is well-structured and covers a range of exceptions, logging them appropriately. Ensure that all functions using this decorator are compatible with its signature and error handling strategy.apps/mappings/imports/modules/categories.py (1)
- 14-85: The implementation of the
Categoryclass methods appears to be correct and follows the expected logic for importing categories and creating mappings. The use of type hints and clear method names enhances readability and maintainability.apps/mappings/imports/modules/cost_centers.py (3)
14-21: The constructor of the
CostCenterclass correctly sets up the necessary fields for the base class. Ensure that thedestination_fieldandsync_afterparameters are always provided when creating an instance ofCostCenter.23-27: The
trigger_importmethod correctly callscheck_import_log_and_start_import. Verify that this method is implemented in theBaseclass and does not require any parameters.29-59: The
construct_fyle_payloadmethod logic appears to be correct. However, verify that the default behavior foris_enabledshould beTruewhenattribute.activeisNone. Also, confirm thatexisting_fyle_attributes_mapcontains lowercased cost center names for the comparison in line 56 to work as intended.apps/mappings/imports/modules/merchants.py (3)
- 33-37: The
is_auto_sync_status_allowedparameter is still present in theconstruct_fyle_payloadmethod signature, which contradicts the summary stating it should be removed.- def construct_fyle_payload( - self, - paginated_destination_attributes: List[DestinationAttribute], - existing_fyle_attributes_map: object, - is_auto_sync_status_allowed: bool - ):
56-71: The use of the
handle_import_exceptionsdecorator aligns with the summary's mention of improved error handling.65-71: The
sync_expense_attributesmethod is called twice inimport_destination_attribute_to_fyle. Verify if this is intentional or an oversight, as it could lead to redundant operations.- self.sync_expense_attributes(platform) - self.sync_expense_attributes(platform)apps/mappings/imports/schedules.py (1)
- 28-28: Verify if the
next_runshould be set todatetime.now()which schedules the task to run immediately. Confirm if this is the intended behavior or if it should be scheduled for a later time.apps/users/urls.py (1)
- 18-22: The URL patterns for
UserProfileViewandFyleOrgsViewhave been added correctly and follow Django's URL configuration conventions.apps/users/views.py (2)
9-26: The implementation of
FyleOrgsViewand itsgetmethod aligns with the summary and follows best practices for Django REST framework views. Ensure that the helpers used within thegetmethod have proper error handling and logging mechanisms in place.29-41: The implementation of
UserProfileViewand itsgetmethod aligns with the summary and follows best practices for Django REST framework views. Ensure that the helperget_user_profileused within thegetmethod has proper error handling and logging mechanisms in place.apps/workspaces/helpers.py (2)
1-8: The reordering of import statements is a cosmetic change and does not affect functionality. It's good practice to follow PEP 8 guidelines for imports ordering.
121-124: The change in
onboarding_statefrom "COMPANY_SELECTION" to "EXPORT_SETTINGS" should be verified across the application to ensure it does not negatively impact other functionalities.apps/workspaces/models.py (4)
5-15: The summary mentions the addition of imports for
BooleanFalseFieldandBooleanTrueField, but these imports are not visible in the provided hunk. Please verify if the imports are correctly placed in the file.21-21: The addition of
'COMPANY_SELECTION', 'COMPANY_SELECTION'toONBOARDING_STATE_CHOICESis correct and aligns with the summary.29-30: The change in the default return value of
get_default_onboarding_stateto'CONNECTION'is correct and aligns with the summary.41-42: The renaming of fields
reimbursable_last_synced_atandcredit_card_last_synced_atis correct and aligns with the summary.apps/workspaces/serializers.py (4)
3-3: The summary indicates that the import statement for
rest_framework.serializerswas removed, but it is still present in the code.21-21: The summary indicates that the import statement for
ms_business_central_api.utils.assert_validwas removed, but it is still present in the code.12-12: The summary does not mention the addition of
connect_business_centralimport, but it is present in the code.210-214: The summary does not mention changes to
WorkspaceAdminSerializer, but the hunk shows the addition ofnamefields.apps/workspaces/views.py (2)
- 3-17: The import statements for
get_user_model,WorkspaceSerializer, andassert_validshould be removed according to the summary, but they are still present in the hunk. Please verify if they are indeed to be removed or if the summary is incorrect.- from django.contrib.auth import get_user_model - from apps.workspaces.serializers import WorkspaceSerializer - from ms_business_central_api.utils import assert_valid
- 116-122: The addition of
pagination_class = Noneand the newget_querysetmethod inWorkspaceAdminsViewclass are consistent with the summary and seem to be correctly implemented.ms_business_central_api/urls.py (2)
20-22: The summary mentions new URL patterns added at lines 7 and 9, but the hunk shows changes at lines 20 and 22. Please verify the line numbers and ensure the summary accurately reflects the changes made in the code.
20-22: The addition of URL patterns for 'api/auth/' and 'api/user/' is consistent with the summary and follows Django's URL configuration conventions.
tests/conftest.py (2)
115-121: The field names
reimbursable_last_synced_atandcredit_card_last_synced_athave been correctly updated in thecreate_temp_workspacefixture.307-311: The logic to set
reimbursable_expense_datebased on theworkspace_idhas been correctly implemented in theadd_export_settingsfixture.tests/test_fyle/fixtures.py (4)
27-45: The addition of the 'get_all_orgs' key with organization details is consistent with the summary.
224-284: The addition of the 'expenses' key with expense details is consistent with the summary.
27-45: Ensure that the new keys and their respective data structures are properly integrated and used in the tests that consume this fixture data.
224-284: Ensure that the new keys and their respective data structures are properly integrated and used in the tests that consume this fixture data.
tests/test_fyle/test_views.py (2)
3-10: The imports for
AccountingExportandExpenseare correctly added to support the new test functiontest_accounting_export_sync_view.119-139: The new test function
test_accounting_export_sync_viewcorrectly tests the synchronization of accounting exports, asserts the response status code, and checks theAccountingExportstatus andExpensecount as described in the summary.tests/test_users/test_views.py (1)
- 12-28: The new test function
test_get_all_orgs_viewcorrectly mocks theget_fyle_orgsfunction, sets up the API client with the necessary credentials, and asserts both the status code and the response data. This aligns with the summary provided.tests/test_workspaces/test_view.py (1)
- 19-22: The addition of the assertion for 'onboarding_state' is a good practice to ensure the integrity of the response data.
| sync_functions = { | ||
| 'ACCOUNT': business_central_connection.sync_accounts, | ||
| 'COMPANY': business_central_connection.sync_companies, | ||
| 'LOCATION': business_central_connection.sync_locations, | ||
| 'EMPLOYEE': business_central_connection.sync_employees, | ||
| 'VENDOR': business_central_connection.sync_vendors, | ||
| } | ||
|
|
||
| sync_function = sync_functions[business_central_attribute_type] | ||
| sync_function() |
There was a problem hiding this comment.
The function lacks error handling for an invalid business_central_attribute_type. Consider adding a try-except block to handle KeyError and provide a meaningful error message if an unsupported attribute type is passed.
+ try:
sync_function = sync_functions[business_central_attribute_type]
sync_function()
+ except KeyError:
+ # Handle the error or log a messageCommittable 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.
| sync_functions = { | |
| 'ACCOUNT': business_central_connection.sync_accounts, | |
| 'COMPANY': business_central_connection.sync_companies, | |
| 'LOCATION': business_central_connection.sync_locations, | |
| 'EMPLOYEE': business_central_connection.sync_employees, | |
| 'VENDOR': business_central_connection.sync_vendors, | |
| } | |
| sync_function = sync_functions[business_central_attribute_type] | |
| sync_function() | |
| try: | |
| sync_function = sync_functions[business_central_attribute_type] | |
| sync_function() | |
| except KeyError: | |
| # Handle the error or log a message |
| def test_get_user_profile(api_client, test_connection, create_temp_workspace, add_fyle_credentials): | ||
| """ | ||
| Test get user profile | ||
| """ | ||
| access_token = test_connection.access_token | ||
| url = reverse('user-profile') | ||
|
|
||
| api_client.credentials(HTTP_AUTHORIZATION='Bearer {}'.format(access_token)) | ||
|
|
||
| response = api_client.get(url) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
The new test function test_get_user_profile sets up the API client with the necessary credentials and asserts the status code of the response. However, it does not assert the response data. If the response data is important to the test, consider adding an assertion for it.
| @@ -23,6 +19,7 @@ def test_post_of_workspace(api_client, test_connection): | |||
| assert response.status_code == 201 | |||
| assert workspace.name == response.data['name'] | |||
| assert workspace.org_id == response.data['org_id'] | |||
| assert workspace.onboarding_state == response.data['onboarding_state'] | |||
|
|
|||
| response = json.loads(response.content) | |||
There was a problem hiding this comment.
The response should be converted to JSON before making assertions on response.data.
- response = json.loads(response.content)
+ response_data = json.loads(response.content)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.
| response = json.loads(response.content) | |
| response_data = json.loads(response.content) |
| """ | ||
| mapping_settings = MappingSetting.objects.filter(workspace_id=workspace_id, import_to_fyle=True) | ||
| custom_field_mapping_settings = MappingSetting.objects.filter(workspace_id=workspace_id, is_custom=True, import_to_fyle=True) | ||
| import_settings = ImportSetting.objects.get(workspace_id=workspace_id) |
There was a problem hiding this comment.
Consider adding error handling for the case where ImportSetting does not exist for the given workspace_id. This will prevent the function from raising an unhandled ImportSetting.DoesNotExist exception.
- import_settings = ImportSetting.objects.get(workspace_id=workspace_id)
+ try:
+ import_settings = ImportSetting.objects.get(workspace_id=workspace_id)
+ except ImportSetting.DoesNotExist:
+ # Handle the error or log it
+ returnCommittable 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.
| import_settings = ImportSetting.objects.get(workspace_id=workspace_id) | |
| try: | |
| import_settings = ImportSetting.objects.get(workspace_id=workspace_id) | |
| except ImportSetting.DoesNotExist: | |
| # Handle the error or log it | |
| return |
| item = ExpenseCustomField(workspace_id, source_field, destination_field, sync_after) | ||
| item.trigger_import() | ||
| else: | ||
| module_class = SOURCE_FIELD_CLASS_MAP[source_field] |
There was a problem hiding this comment.
Consider adding error handling for the case where source_field is not found in SOURCE_FIELD_CLASS_MAP to prevent a KeyError.
| refresh_token = AuthToken.objects.get(user__user_id=request.user).refresh_token | ||
| cluster_domain, _ = get_cluster_domain_and_refresh_token(request.user) | ||
|
|
||
| fyle_credentials = FyleCredential( | ||
| cluster_domain=cluster_domain, | ||
| refresh_token=refresh_token | ||
| ) | ||
|
|
||
| platform = PlatformConnector(fyle_credentials) | ||
| employee_profile = platform.connection.v1beta.spender.my_profile.get() | ||
|
|
||
| return employee_profile |
There was a problem hiding this comment.
Ensure that the AuthToken.DoesNotExist exception is handled when fetching the refresh token. This is important for robust error handling and to prevent the function from failing unexpectedly.
- refresh_token = AuthToken.objects.get(user__user_id=request.user).refresh_token
+ try:
+ refresh_token = AuthToken.objects.get(user__user_id=request.user).refresh_token
+ except AuthToken.DoesNotExist:
+ # Handle the exception, e.g., by logging and returning an appropriate responseCommittable 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.
| refresh_token = AuthToken.objects.get(user__user_id=request.user).refresh_token | |
| cluster_domain, _ = get_cluster_domain_and_refresh_token(request.user) | |
| fyle_credentials = FyleCredential( | |
| cluster_domain=cluster_domain, | |
| refresh_token=refresh_token | |
| ) | |
| platform = PlatformConnector(fyle_credentials) | |
| employee_profile = platform.connection.v1beta.spender.my_profile.get() | |
| return employee_profile | |
| try: | |
| refresh_token = AuthToken.objects.get(user__user_id=request.user).refresh_token | |
| except AuthToken.DoesNotExist: | |
| # Handle the exception, e.g., by logging and returning an appropriate response | |
| cluster_domain, _ = get_cluster_domain_and_refresh_token(request.user) | |
| fyle_credentials = FyleCredential( | |
| cluster_domain=cluster_domain, | |
| refresh_token=refresh_token | |
| ) | |
| platform = PlatformConnector(fyle_credentials) | |
| employee_profile = platform.connection.v1beta.spender.my_profile.get() | |
| return employee_profile |
| @staticmethod | ||
| def create_accounting_export(expense_objects: List[Expense], fund_source: str, workspace_id): | ||
| """ | ||
| Group expenses by report_id and fund_source, format date fields, and create AccountingExport objects. | ||
| """ | ||
| # Retrieve the ExportSetting for the workspace | ||
| export_setting = ExportSetting.objects.get(workspace_id=workspace_id) | ||
|
|
||
| # Group expenses based on specified fields and fund_source | ||
| accounting_exports = _group_expenses(expense_objects, export_setting, fund_source) | ||
|
|
||
| fund_source_map = { | ||
| 'PERSONAL': 'reimbursable', | ||
| 'CCC': 'credit_card' | ||
| } | ||
|
|
||
| for accounting_export in accounting_exports: | ||
| # Determine the date field based on fund_source | ||
| date_field = getattr(export_setting, f"{fund_source_map.get(fund_source)}_expense_date", None) | ||
|
|
||
| # Calculate and assign 'last_spent_at' based on the chosen date field | ||
| if date_field == 'last_spent_at': | ||
| latest_expense = Expense.objects.filter(id__in=accounting_export['expense_ids']).order_by('-spent_at').first() | ||
| accounting_export['last_spent_at'] = latest_expense.spent_at if latest_expense else None | ||
|
|
||
| # Store expense IDs and remove unnecessary keys | ||
| expense_ids = accounting_export['expense_ids'] | ||
| accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%dT%H:%M:%S') | ||
| accounting_export.pop('total') | ||
| accounting_export.pop('expense_ids') | ||
|
|
||
| # Create an AccountingExport object for the expense group | ||
| accounting_export_instance = AccountingExport.objects.create( | ||
| workspace_id=workspace_id, | ||
| fund_source=accounting_export['fund_source'], | ||
| description=accounting_export, | ||
| status='EXPORT_READY' | ||
| ) | ||
|
|
||
| # Add related expenses to the AccountingExport object | ||
| accounting_export_instance.expenses.add(*expense_ids) | ||
|
|
||
|
|
There was a problem hiding this comment.
In the create_accounting_export function, there is a potential issue with the date formatting logic. If the date_field is not 'last_spent_at', the code attempts to format the date without checking if the date is present or not. This could lead to an AttributeError if accounting_export[date_field] is None.
- accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%dT%H:%M:%S')
+ if accounting_export.get(date_field):
+ accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%dT%H:%M:%S')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.
| @staticmethod | |
| def create_accounting_export(expense_objects: List[Expense], fund_source: str, workspace_id): | |
| """ | |
| Group expenses by report_id and fund_source, format date fields, and create AccountingExport objects. | |
| """ | |
| # Retrieve the ExportSetting for the workspace | |
| export_setting = ExportSetting.objects.get(workspace_id=workspace_id) | |
| # Group expenses based on specified fields and fund_source | |
| accounting_exports = _group_expenses(expense_objects, export_setting, fund_source) | |
| fund_source_map = { | |
| 'PERSONAL': 'reimbursable', | |
| 'CCC': 'credit_card' | |
| } | |
| for accounting_export in accounting_exports: | |
| # Determine the date field based on fund_source | |
| date_field = getattr(export_setting, f"{fund_source_map.get(fund_source)}_expense_date", None) | |
| # Calculate and assign 'last_spent_at' based on the chosen date field | |
| if date_field == 'last_spent_at': | |
| latest_expense = Expense.objects.filter(id__in=accounting_export['expense_ids']).order_by('-spent_at').first() | |
| accounting_export['last_spent_at'] = latest_expense.spent_at if latest_expense else None | |
| # Store expense IDs and remove unnecessary keys | |
| expense_ids = accounting_export['expense_ids'] | |
| accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%dT%H:%M:%S') | |
| accounting_export.pop('total') | |
| accounting_export.pop('expense_ids') | |
| # Create an AccountingExport object for the expense group | |
| accounting_export_instance = AccountingExport.objects.create( | |
| workspace_id=workspace_id, | |
| fund_source=accounting_export['fund_source'], | |
| description=accounting_export, | |
| status='EXPORT_READY' | |
| ) | |
| # Add related expenses to the AccountingExport object | |
| accounting_export_instance.expenses.add(*expense_ids) | |
| @staticmethod | |
| def create_accounting_export(expense_objects: List[Expense], fund_source: str, workspace_id): | |
| """ | |
| Group expenses by report_id and fund_source, format date fields, and create AccountingExport objects. | |
| """ | |
| # Retrieve the ExportSetting for the workspace | |
| export_setting = ExportSetting.objects.get(workspace_id=workspace_id) | |
| # Group expenses based on specified fields and fund_source | |
| accounting_exports = _group_expenses(expense_objects, export_setting, fund_source) | |
| fund_source_map = { | |
| 'PERSONAL': 'reimbursable', | |
| 'CCC': 'credit_card' | |
| } | |
| for accounting_export in accounting_exports: | |
| # Determine the date field based on fund_source | |
| date_field = getattr(export_setting, f"{fund_source_map.get(fund_source)}_expense_date", None) | |
| # Calculate and assign 'last_spent_at' based on the chosen date field | |
| if date_field == 'last_spent_at': | |
| latest_expense = Expense.objects.filter(id__in=accounting_export['expense_ids']).order_by('-spent_at').first() | |
| accounting_export['last_spent_at'] = latest_expense.spent_at if latest_expense else None | |
| # Store expense IDs and remove unnecessary keys | |
| expense_ids = accounting_export['expense_ids'] | |
| if accounting_export.get(date_field): | |
| accounting_export[date_field] = accounting_export[date_field].strftime('%Y-%m-%dT%H:%M:%S') | |
| accounting_export.pop('total') | |
| accounting_export.pop('expense_ids') | |
| # Create an AccountingExport object for the expense group | |
| accounting_export_instance = AccountingExport.objects.create( | |
| workspace_id=workspace_id, | |
| fund_source=accounting_export['fund_source'], | |
| description=accounting_export, | |
| status='EXPORT_READY' | |
| ) | |
| # Add related expenses to the AccountingExport object | |
| accounting_export_instance.expenses.add(*expense_ids) | |
|
|
||
| destination_attributes_count = DestinationAttribute.objects.filter(**filters).count() | ||
|
|
||
| is_auto_sync_status_allowed = self.get_auto_sync_permission() |
There was a problem hiding this comment.
The call to get_auto_sync_permission is redundant as it is already called and stored in is_auto_sync_status_allowed earlier in the construct_payload_and_import_to_fyle method.
- is_auto_sync_status_allowed = self.get_auto_sync_permission()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.
| is_auto_sync_status_allowed = self.get_auto_sync_permission() |
| :param import_log: ImportLog object | ||
| """ | ||
| if is_last_batch: | ||
| import_log.last_successful_run_at = datetime.now() |
There was a problem hiding this comment.
The datetime.now() method is used without timezone awareness. It is recommended to use datetime.now(timezone.utc) to ensure that the time is stored in a consistent and timezone-aware format.
- import_log.last_successful_run_at = datetime.now()
+ import_log.last_successful_run_at = datetime.now(timezone.utc)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.
| import_log.last_successful_run_at = datetime.now() | |
| import_log.last_successful_run_at = datetime.now(timezone.utc) |
| 'status': 'IN_PROGRESS' | ||
| } | ||
| ) | ||
| time_difference = datetime.now() - timedelta(minutes=30) |
There was a problem hiding this comment.
The datetime.now() method is used without timezone awareness. It is recommended to use datetime.now(timezone.utc) to ensure that the time is stored in a consistent and timezone-aware format.
- time_difference = datetime.now() - timedelta(minutes=30)
+ time_difference = datetime.now(timezone.utc) - timedelta(minutes=30)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.
| time_difference = datetime.now() - timedelta(minutes=30) | |
| time_difference = datetime.now(timezone.utc) - timedelta(minutes=30) |
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation