Refactor imports costcenter #312
Conversation
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe recent update enhances the data synchronization and error handling capabilities for Fyle integrations. It introduces new mappings, refines the conditions for auto-syncing, and streamlines tasks related to importing data into Fyle. Additionally, it simplifies the scheduling of Fyle import tasks and consolidates logic for handling source fields, making the system more efficient and robust against errors. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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: 2
Configuration used: CodeRabbit UI
Files selected for processing (9)
- apps/mappings/constants.py (1 hunks)
- apps/mappings/exceptions.py (2 hunks)
- apps/mappings/helpers.py (1 hunks)
- apps/mappings/queue.py (2 hunks)
- apps/mappings/schedules.py (1 hunks)
- apps/mappings/signals.py (2 hunks)
- apps/mappings/tasks.py (4 hunks)
- apps/workspaces/apis/import_settings/triggers.py (4 hunks)
- apps/xero/actions.py (3 hunks)
Additional comments: 26
apps/mappings/constants.py (1)
- 47-53: The addition of new mappings for 'ACCOUNT', 'ITEM', 'TAX_CODE', 'CONTACT', and 'CUSTOMER' in the
SYNC_METHODSdictionary is correctly implemented and aligns with the PR objectives. This enhancement supports the synchronization of various entities between the application and Fyle efficiently.apps/mappings/helpers.py (1)
- 45-54: The implementation of the
is_auto_sync_allowedfunction is correct and aligns with the PR objectives to introduce a new permission check for auto-sync. The function's logic is sound, and it is well-documented, enhancing maintainability.apps/mappings/schedules.py (1)
- 8-47: Aside from the minor typo, the implementation of the
new_schedule_or_delete_fyle_import_tasksfunction is correct and aligns with the PR objectives to optimize the scheduling and execution of import tasks. The logic is sound, and the function is well-documented.apps/mappings/tasks.py (23)
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-3]
The imports section remains unchanged, which is good as it indicates that the necessary dependencies for the remaining functions are still intact. However, it's crucial to ensure that any removed functions did not leave behind unused imports, which could clutter the codebase.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-3]
Given the removal of several functions related to cost centers and project mappings, it's important to verify that these changes align with the PR's objectives of streamlining the import process and that no critical functionality has been inadvertently removed. This verification should ideally be done through comprehensive testing, including unit and integration tests, to ensure that the remaining functionalities work as expected and that the removals do not negatively impact the application's behavior.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [5-7]
The constants and enums imported from various modules, such as
FyleAttributeEnumandFYLE_EXPENSE_SYSTEM_FIELDS, are crucial for the logic within this file. It's good practice to review their usage within the modified and remaining functions to ensure that any changes in their definitions or removals are accounted for and do not introduce bugs.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-32]
The
DEFAULT_FYLE_CATEGORIESlist remains unchanged, which is appropriate as it provides a set of default categories for mapping purposes. It's essential to ensure that this list is still relevant and used within the context of the modified functions and overall application logic, especially after the removal of specific functions related to cost centers and project mappings.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-52]
The
disable_expense_attributesfunction appears to be untouched by the changes in this PR. It's crucial to ensure that its logic is still valid and that it interacts correctly with the modified parts of the codebase, especially in the context of disabling expense attributes based on the new import and mapping logic. Additionally, reviewing the use of Django's ORM filtering and updating methods for performance and correctness is advisable.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [54-71]
The
resolve_expense_attribute_errorsfunction remains unchanged. Given the refactor, it's important to verify that this function's error resolution logic still aligns with the updated import and mapping processes. Ensuring that it correctly identifies and resolves errors related to expense attribute mappings is crucial for maintaining data integrity and user experience.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [73-91]
The
remove_duplicatesfunction is a utility function that ensures uniqueness among attributes. Its logic is straightforward and does not seem to be directly affected by the changes in this PR. However, it's essential to ensure that its usage across the file, especially in functions that have been modified or are new, correctly contributes to the deduplication of attributes as intended.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [93-123]
The
create_fyle_categories_payloadfunction constructs a payload for category import to Fyle. This function is critical for the import process and should be reviewed for correctness, especially in handling theupdated_categoriesparameter. Ensuring that the payload construction logic correctly handles both new and updated categories, and adheres to Fyle's API requirements, is essential for the successful import of categories.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [125-147]
The
get_all_categories_from_fylefunction fetches categories from Fyle and constructs a name map. It's important to ensure that this function correctly handles pagination and data transformation, especially after the refactor. Verifying that the API call to Fyle is made correctly and that the response is handled appropriately to construct the category name map is crucial for maintaining the integrity of category data.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [149-205]
The
upload_categories_to_fylefunction is a key part of the category import process. It's crucial to review this function for performance, especially considering the multiple API calls and database operations involved. Optimizing the database queries and ensuring efficient handling of API responses can significantly impact the performance of this function. Additionally, verifying that the logic aligns with the updated import process and correctly interacts with the Fyle and Xero APIs is essential.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [207-238]
The
auto_create_category_mappingsfunction triggers the category mapping process. It's important to ensure that this function correctly utilizes theupload_categories_to_fylefunction and handles the mappings appropriately. Verifying that the mappings are created correctly and that any errors are handled appropriately is crucial for maintaining the integrity of category mappings.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [240-268]
The
async_auto_map_employeesfunction handles the asynchronous mapping of employees. Given the refactor, it's essential to verify that this function's logic still aligns with the updated import and mapping processes. Ensuring that employees are correctly mapped and that any errors are resolved appropriately is crucial for maintaining the integrity of employee data.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [270-284]
The
sync_xero_attributesfunction is responsible for syncing attributes from Xero. It's crucial to ensure that this function correctly handles the syncing process for different attribute types and that it interacts correctly with the Xero API. Additionally, verifying that the logic aligns with the updated import process and that any errors are handled appropriately is essential for maintaining the integrity of synced data.
- 287-292: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [286-308]
The
disable_renamed_projectsfunction appears to be related to handling projects that have been renamed. Given the removal of functions related to project mappings, it's important to verify that this function is still relevant and correctly implemented. Ensuring that renamed projects are correctly identified and handled is crucial for maintaining the integrity of project data.
- 317-322: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [310-365]
The
create_fyle_expense_custom_fields_payloadfunction constructs a payload for custom field import to Fyle. This function is critical for the import process and should be reviewed for correctness, especially in handling the construction of the payload. Ensuring that the payload construction logic correctly handles custom fields and adheres to Fyle's API requirements is essential for the successful import of custom fields.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [367-396]
The
upload_attributes_to_fylefunction is a key part of the attribute import process. It's crucial to review this function for performance, especially considering the API calls and database operations involved. Optimizing the database queries and ensuring efficient handling of API responses can significantly impact the performance of this function. Additionally, verifying that the logic aligns with the updated import process and correctly interacts with the Fyle API is essential.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [398-423]
The
auto_create_expense_fields_mappingsfunction triggers the expense field mapping process. It's important to ensure that this function correctly utilizes theupload_attributes_to_fylefunction and handles the mappings appropriately. Verifying that the mappings are created correctly and that any errors are handled appropriately is crucial for maintaining the integrity of expense field mappings.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [425-448]
The
async_auto_create_custom_field_mappingsfunction handles the asynchronous creation of custom field mappings. Given the refactor, it's essential to verify that this function's logic still aligns with the updated import and mapping processes. Ensuring that custom field mappings are correctly created and that any errors are resolved appropriately is crucial for maintaining the integrity of custom field data.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [450-486]
The
upload_tax_groups_to_fylefunction is responsible for uploading tax groups to Fyle. It's crucial to ensure that this function correctly handles the upload process and that it interacts correctly with the Fyle API. Additionally, verifying that the logic aligns with the updated import process and that any errors are handled appropriately is essential for maintaining the integrity of tax group data.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [488-515]
The
create_fyle_tax_group_payloadfunction constructs a payload for tax group import to Fyle. This function is critical for the import process and should be reviewed for correctness, especially in handling the construction of the payload. Ensuring that the payload construction logic correctly handles tax groups and adheres to Fyle's API requirements is essential for the successful import of tax groups.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [517-541]
The
auto_create_tax_codes_mappingsfunction triggers the tax code mapping process. It's important to ensure that this function correctly utilizes theupload_tax_groups_to_fylefunction and handles the mappings appropriately. Verifying that the mappings are created correctly and that any errors are handled appropriately is crucial for maintaining the integrity of tax code mappings.
- 287-292: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [543-559]
The
auto_create_suppliers_as_merchantsfunction is responsible for creating suppliers as merchants in Fyle. Given the refactor, it's essential to verify that this function's logic still aligns with the updated import and mapping processes. Ensuring that suppliers are correctly created as merchants and that any errors are resolved appropriately is crucial for maintaining the integrity of supplier data.
- 570-575: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [561-590]
The
auto_import_and_map_fyle_fieldsfunction has been modified by removing logic related to project mappings. It's crucial to ensure that the remaining logic correctly handles the import and mapping of Fyle fields, especially in the context of the updated import process. Verifying that the import and mapping processes are executed correctly and that any errors are handled appropriately is essential for maintaining the integrity of the data.
apps/mappings/schedules.py
Outdated
| func='apps.mappings.queues.construct_tasks_and_chain_import_fields_to_fyle', | ||
| args={}.format(workspace_general_settings_instance.workspace_id) |
There was a problem hiding this comment.
There's a typo in the function name within the Schedule.objects.filter call. It should match the function name used in the update_or_create method to ensure the correct schedule is targeted for deletion.
- func='apps.mappings.queues.construct_tasks_and_chain_import_fields_to_fyle',
+ func='apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle',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.
| func='apps.mappings.queues.construct_tasks_and_chain_import_fields_to_fyle', | |
| args={}.format(workspace_general_settings_instance.workspace_id) | |
| func='apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle', | |
| args={}.format(workspace_general_settings_instance.workspace_id) |
apps/mappings/tasks.py
Outdated
| for offset in range(0, xero_attribute_count, page_size): | ||
| limit = offset + page_size | ||
| paginated_xero_attributes = DestinationAttribute.objects.filter( | ||
| attribute_type=xero_attribute_type, workspace_id=workspace_id | ||
| ).order_by("value", "id")[offset:limit] | ||
|
|
||
| paginated_xero_attributes = remove_duplicates(paginated_xero_attributes) | ||
|
|
||
| fyle_payload: List[Dict] = create_fyle_cost_centers_payload( | ||
| paginated_xero_attributes, existing_cost_center_names | ||
| ) | ||
|
|
||
| if fyle_payload: | ||
| platform.cost_centers.post_bulk(fyle_payload) | ||
| platform.cost_centers.sync() | ||
|
|
||
| Mapping.bulk_create_mappings( | ||
| paginated_xero_attributes, FyleAttributeEnum.COST_CENTER, xero_attribute_type, workspace_id | ||
| ) | ||
|
|
||
|
|
||
| @handle_import_exceptions(task_name="auto create cost center mappings") | ||
| def auto_create_cost_center_mappings(workspace_id: int): | ||
| """ | ||
| Create Cost Center Mappings | ||
| """ | ||
|
|
||
| fyle_credentials: FyleCredential = FyleCredential.objects.get( | ||
| workspace_id=workspace_id | ||
| ) | ||
|
|
||
| platform = PlatformConnector(fyle_credentials) | ||
|
|
||
| mapping_setting = MappingSetting.objects.get( | ||
| source_field=FyleAttributeEnum.COST_CENTER, import_to_fyle=True, workspace_id=workspace_id | ||
| ) | ||
|
|
||
| platform.cost_centers.sync() | ||
|
|
||
| sync_xero_attributes(mapping_setting.destination_field, workspace_id=workspace_id) | ||
|
|
||
| post_cost_centers_in_batches( | ||
| platform, workspace_id, mapping_setting.destination_field | ||
| ) | ||
|
|
||
|
|
||
| def create_fyle_projects_payload( | ||
| projects: List[DestinationAttribute], | ||
| existing_project_names: list, | ||
| updated_projects: List[ExpenseAttribute] = None, | ||
| ): | ||
| """ | ||
| Create Fyle Projects Payload from Xero Projects and Customers | ||
| :param projects: Xero Projects | ||
| :return: Fyle Projects Payload | ||
| """ | ||
| payload = [] | ||
| if updated_projects: | ||
| for project in updated_projects: | ||
| destination_id_of_project = ( | ||
| project.mapping.first().destination.destination_id | ||
| ) | ||
| payload.append( | ||
| { | ||
| "id": project.source_id, | ||
| "name": project.value, | ||
| "code": destination_id_of_project, | ||
| "description": "Project - {0}, Id - {1}".format( | ||
| project.value, destination_id_of_project | ||
| ), | ||
| "is_enabled": project.active, | ||
| } | ||
| ) | ||
| else: | ||
| existing_project_names = [ | ||
| project_name.lower() for project_name in existing_project_names | ||
| ] | ||
|
|
||
| for project in projects: | ||
| if project.value.lower() not in existing_project_names: | ||
| payload.append( | ||
| { | ||
| "name": project.value, | ||
| "code": project.destination_id, | ||
| "description": "Project - {0}, Id - {1}".format( | ||
| project.value, project.destination_id | ||
| ), | ||
| "is_enabled": True | ||
| if project.active is None | ||
| else project.active, | ||
| } | ||
| ) | ||
|
|
||
| logger.info("| Importing Projects to Fyle | Content: {{Fyle Payload count: {}}}".format(len(payload))) | ||
| return payload | ||
|
|
||
|
|
||
| def disable_renamed_projects(workspace_id, destination_field): | ||
| page_size = 200 | ||
| expense_attributes_count = ExpenseAttribute.objects.filter( |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [9-11]
The logger setup at the beginning of the file is standard and appropriate for logging information and errors. However, given the significant changes in this file, it might be beneficial to revisit the logging statements within the modified and remaining functions to ensure they provide enough context and detail for debugging and monitoring the import processes.
Consider enhancing logging statements throughout the file to include more context, especially around critical operations like API calls and data transformations. This will improve the ability to troubleshoot issues and understand the flow of data.
ashwin1111
changed the base branch from
imports-refactor-master
to
refactor-imports
* Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field
…roject, cost_center
Hrishabh17
changed the base branch from
refactor-imports
to
imports-refactor-master
Hrishabh17
changed the base branch from
imports-refactor-master
to
refactor-imports
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
* Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field * Refactor imports costcenter (#312) * Resource Cost_Center: refactored imports * Refactor imports for Project resource (#311) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field
* Added integrations_imports submodule, made changes in settings (#310) * Refactor imports for Project resource (#311) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field * Refactor imports for project and cost_center (#314) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field * Refactor imports costcenter (#312) * Resource Cost_Center: refactored imports * Refactor imports for Project resource (#311) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Project: added cluster import, fixed minor typo, added new supplier field * Refactor imports custom expense (#313) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Cost_Center: refactored imports * Resource Custom Expense Field: refactored imports * Resource Project: added cluster import, fixed minor typo, added new supplier field * Resource Tax Group: refactored imports (#315) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Cost_Center: refactored imports * Resource Custom Expense Field: refactored imports * Resource Project: added cluster import, fixed minor typo, added new supplier field * Resource Tax Group: refactored imports * Resource Tax Group: changed to is_auto_sync to False by default * Resource Category: refactored imports (#316) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Cost_Center: refactored imports * Resource Custom Expense Field: refactored imports * Resource Tax Group: refactored imports * Resource Category: refactored imports * Resource Category: changed is_auto_sync_enabled to True by default * Refactor imports supplier (#318) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Cost_Center: refactored imports * Resource Custom Expense Field: refactored imports * Resource Tax Group: refactored imports * Resource Category: refactored imports * Added RetryException handler and bumped the sdk versions (#306) * Set tasks limit to 1L (#317) * Set tasks limit to 1L * set timeout * Modified the import schedule creation condition --------- Co-authored-by: ruuushhh <[email protected]> * Refactor old flow (#321) * Added integrations_imports submodule, made changes in settings * Resource Project: Added new flow for the Project Import * Typo Fix * Resource Project: removed old import logic for Project * Resource Cost_Center: refactored imports * Resource Custom Expense Field: refactored imports * Resource Tax Group: refactored imports * Resource Category: refactored imports * Added RetryException handler and bumped the sdk versions (#306) * Set tasks limit to 1L (#317) * Set tasks limit to 1L * set timeout * Modified the import schedule creation condition * Removed dead code, add few test cases, added script for adding new schedule for different workspaces * Added post deploy sql script to dump old schedules and delete --------- Co-authored-by: ruuushhh <[email protected]> * Merged changes from master * Update delete workspace func (#319) * Update delete workspace func * Fix comments * upate workspace last synced fix (#320) * Modified the workflow for submodule * Decreased the coverage to 88% --------- Co-authored-by: ruuushhh <[email protected]>
Summary by CodeRabbit
New Features
SYNC_METHODSto support additional entities like 'ACCOUNT', 'ITEM', 'TAX CODE', 'CONTACT', and 'CUSTOMER'.Refactor
Chores