Imports refactor master #322
Conversation
* 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 * 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 * 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
* 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
* 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
* 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]>
* 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]>
* Update delete workspace func * Fix comments
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe recent updates introduce a series of enhancements and optimizations across the project, focusing on improving the integration and synchronization of data with Fyle. Key changes include the introduction of a submodule, the consolidation of task scheduling and exception handling, and the refactoring of various functions to streamline operations. These modifications aim to simplify the codebase, improve maintainability, and enhance the efficiency of data import and mapping processes. 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: 16
Configuration used: CodeRabbit UI
Files selected for processing (28)
- .gitmodules (1 hunks)
- 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 (3 hunks)
- apps/mappings/tasks.py (4 hunks)
- apps/workspaces/apis/import_settings/serializers.py (1 hunks)
- apps/workspaces/apis/import_settings/triggers.py (3 hunks)
- apps/workspaces/utils.py (2 hunks)
- apps/xero/actions.py (3 hunks)
- fyle_integrations_imports (1 hunks)
- fyle_xero_api/settings.py (2 hunks)
- fyle_xero_api/tests/settings.py (2 hunks)
- scripts/python/add_new_import_schedule.py (1 hunks)
- sql/scripts/024-dump-schedules-to-csv-and-delete.sql (1 hunks)
- tests/test_fyle_integrations_imports/conftest.py (1 hunks)
- tests/test_fyle_integrations_imports/helpers.py (1 hunks)
- tests/test_fyle_integrations_imports/test_modules/test_base.py (1 hunks)
- tests/test_fyle_integrations_imports/test_queue.py (1 hunks)
- tests/test_mappings/conftest.py (2 hunks)
- tests/test_mappings/test_exceptions.py (1 hunks)
- tests/test_mappings/test_helpers.py (2 hunks)
- tests/test_mappings/test_queues.py (1 hunks)
- tests/test_mappings/test_signals.py (4 hunks)
- tests/test_mappings/test_tasks.py (3 hunks)
- tests/test_xero/test_views.py (1 hunks)
Files skipped from review due to trivial changes (1)
- fyle_integrations_imports
Additional comments: 24
.gitmodules (1)
- 1-3: The addition of the
fyle_integrations_importssubmodule is correctly implemented. Ensure that the submodule repository is accessible and consider the implications of this integration on the project's build and deployment processes.apps/mappings/helpers.py (1)
- 5-14: The
is_auto_sync_allowedfunction is implemented correctly and efficiently uses short-circuit evaluation. Consider adding type checks or using Python's type annotations for the function parameters to ensure they are always of the expected types (WorkspaceGeneralSettingsandMappingSetting). This can enhance maintainability and reduce the risk of runtime errors.sql/scripts/024-dump-schedules-to-csv-and-delete.sql (1)
- 16-22: Ensure the deletion of schedules does not adversely affect the application's functionality. Review the impact of removing these specific schedules on the system's behavior and document any necessary precautions or adjustments.
apps/mappings/constants.py (1)
- 47-53: The addition of the
SYNC_METHODSdictionary centralizes sync method definitions, improving maintainability. Ensure that the keys and values accurately reflect the application's requirements and sync methods.tests/test_fyle_integrations_imports/helpers.py (1)
- 8-37: The helper functions
get_base_class_instanceandget_platform_connectionare well-implemented, enhancing test setup reusability. Consider implementing teardown procedures or using context managers in tests to ensure proper cleanup of resources created by these functions.scripts/python/add_new_import_schedule.py (1)
- 16-39: The script correctly handles the addition and verification of import schedules using Django's ORM. Consider using logging instead of printing errors directly to the console to improve error visibility and management in production environments.
tests/test_mappings/conftest.py (1)
- 32-65: The new fixtures for creating a temporary workspace and adding Xero and Fyle credentials are well-implemented, enhancing test setup reusability. Consider implementing teardown procedures or using context managers in tests to ensure proper cleanup of resources created by these fixtures.
apps/mappings/schedules.py (1)
- 8-50: The
new_schedule_or_delete_fyle_import_tasksfunction is correctly implemented, using conditions to determine whether to schedule or delete tasks. Consider parameterizing the scheduling interval and using timezone-aware datetime objects fornext_runto enhance flexibility and correctness in different environments.apps/workspaces/apis/import_settings/triggers.py (1)
- 28-44: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [7-75]
The refactoring in
triggers.pyto usenew_schedule_or_delete_fyle_import_tasksfor handling tax groups creation and mapping settings is a positive change towards simplifying the code structure and improving maintainability. However, there are a few considerations:
Ensure that
new_schedule_or_delete_fyle_import_tasksadequately handles all scenarios previously covered by individual scheduling functions. This includes edge cases and error handling.Verify that the new function is thoroughly tested, especially since it centralizes important logic. Consider adding or updating tests in the test suite to cover the new functionality and any edge cases that may arise from the consolidation of logic.
Review the documentation and comments to ensure they are updated to reflect the new logic. Clear documentation is crucial for maintainability, especially when refactoring critical parts of the system.
Verify that all scenarios previously covered by individual functions are adequately handled by
new_schedule_or_delete_fyle_import_tasks.Ensure that the new function is thoroughly tested, including edge cases.
Update documentation and comments to reflect the new logic and improve maintainability.
tests/test_xero/test_views.py (1)
- 121-122: The addition of a mock for
ExpenseCustomFieldand its methodsync_expense_attributesin thetest_post_refresh_dimensionsfunction is a good practice for isolating the test environment from external dependencies. However, ensure that the mock accurately reflects the behavior of the realExpenseCustomFieldto maintain the validity of the test.apps/mappings/signals.py (3)
- 5-25: The addition of imports and the reorganization of function calls and imports are generally positive changes that can enhance the readability and maintainability of the code. However, ensure that all newly added imports are used within the file to avoid unnecessary dependencies.
- 58-64: The logic within
run_post_mapping_settings_triggersfor determining when to callnew_schedule_or_delete_fyle_import_tasksis clear and concise. However, consider adding a comment explaining the significance ofALLOWED_SOURCE_FIELDSand the conditioninstance.is_customfor future maintainability.- 86-142: The error handling and logging within
run_pre_mapping_settings_triggersare commendable for robustness. However, ensure that theValidationErrorraised in case of aWrongParamsErroris caught and handled appropriately upstream to prevent unintended interruptions in the application flow.apps/workspaces/apis/import_settings/serializers.py (1)
- 154-159: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the call to
trigger.pre_save_mapping_settings()in theupdatemethod of the serializer could have implications on the application's behavior, particularly in how mapping settings are processed before saving. Ensure that this change is intentional and that any necessary logic has been relocated or accounted for elsewhere in the application to maintain the desired functionality.fyle_xero_api/tests/settings.py (2)
- 47-47: Adding
fyle_integrations_importsto the installed apps is a necessary step to ensure that its models and functionalities are available during testing. This aligns with the PR's objective of enhancing modular integration capabilities.- 189-192: The configuration of logging for
fyle_integrations_importsis appropriate and follows the established pattern for other components. Ensure that the log level and propagation settings are consistent with the application's logging strategy to maintain a coherent log output.fyle_xero_api/settings.py (2)
- 49-49: The inclusion of
fyle_integrations_importsin the list of installed apps is crucial for enabling its functionalities within the application. This change supports the PR's objective of enhancing modular integration capabilities.- 199-202: The logging configuration for
fyle_integrations_importsis set up correctly, following the established logging pattern. This ensures that logs from this module are captured consistently with the rest of the application.apps/workspaces/utils.py (1)
- 225-230: The addition of
new_schedule_or_delete_fyle_import_tasksintroduces a more flexible approach to handling Fyle import tasks based on workspace settings. However, ensure that the removal ofschedule_tax_groups_creation(not shown in the provided code but mentioned in the summary) does not impact any dependent functionalities or leave orphaned tasks that were previously scheduled by it.
- Consider verifying the impact of removing
schedule_tax_groups_creationon the overall task scheduling logic and ensure that any tasks previously scheduled by it are either migrated or appropriately handled.tests/test_fyle_integrations_imports/test_modules/test_base.py (5)
- 26-46: The test
test_remove_duplicateseffectively checks the removal of duplicateDestinationAttributeentries. However, it's important to ensure that the logic for removing duplicates is robust and handles all edge cases, such as case sensitivity and trimming whitespace.
- Consider adding more test cases to cover edge cases like case sensitivity and whitespace in attribute values to ensure the duplicate removal logic is comprehensive.
- 62-79: The test
test_construct_attributes_filterverifies the construction of attribute filters based on various conditions. It's well-structured and covers different scenarios, including filtering by attribute type, workspace ID, and value inclusion.
- Good coverage of different filtering scenarios ensures that the attribute filter construction logic is thoroughly tested.
- 82-104: The test
test_expense_attributes_sync_afterchecks the functionality of syncing expense attributes based on a specific timestamp (sync_after). This test ensures that only attributes updated after the specified timestamp are considered.
- This test is crucial for verifying that the syncing logic respects the
sync_aftertimestamp, ensuring that only relevant updates are processed.
- 107-173: The test
test_auto_create_destination_attributescovers the auto-creation of destination attributes based on data from Fyle and Xero. It mocks external API calls and verifies that the import logic behaves as expected under various conditions.
- Comprehensive testing of the auto-creation logic with mocked external dependencies ensures that the import functionality is robust and behaves correctly under different scenarios.
- 228-273: The test
test_resolve_expense_attribute_errorsverifies the resolution of expense attribute errors upon successful mapping creation. It ensures that errors are marked as resolved when the underlying issue is addressed.
- This test is important for ensuring that the system correctly identifies and resolves mapping errors, improving data integrity and user experience.
|
|
||
| from apps.fyle.enums import FyleAttributeEnum | ||
| from apps.fyle.models import ExpenseGroupSettings | ||
| from apps.mappings.queue import schedule_auto_map_employees, schedule_tax_groups_creation | ||
| from apps.mappings.queue import schedule_auto_map_employees | ||
| from apps.workspaces.models import Workspace, WorkspaceGeneralSettings | ||
| from apps.xero.queue import schedule_payment_creation, schedule_reimbursements_sync, schedule_xero_objects_status_sync | ||
| from fyle_xero_api.utils import assert_valid | ||
| from apps.mappings.schedules import new_schedule_or_delete_fyle_import_tasks | ||
|
|
||
|
|
||
| def generate_token(authorization_code: str, redirect_uri: str = None) -> str: |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [245-245]
Decoding a JWT token with verify=False bypasses integrity checks, which could potentially allow tampered tokens to be accepted. This is a security risk as it makes the application vulnerable to token tampering attacks.
Consider verifying the JWT token to ensure its integrity. If the token's signature cannot be verified in this context due to missing keys or other reasons, document the rationale and assess the risk. Implement additional security measures if necessary to mitigate potential threats.
- decoded_jwt = jwt.decode(successful_response["id_token"], options={"verify_signature": False})
+ decoded_jwt = jwt.decode(successful_response["id_token"], options={"verify_signature": True})| def test_construct_tasks_and_chain_import_fields_to_fyle( | ||
| db, | ||
| mocker, | ||
| create_temp_workspace, | ||
| add_xero_credentials, | ||
| create_mapping_setting | ||
| ): | ||
| workspace_id = 3 | ||
| mocker.patch('apps.mappings.queue.chain_import_fields_to_fyle') | ||
|
|
||
| WorkspaceGeneralSettings.objects.create( | ||
| workspace_id=workspace_id, | ||
| import_suppliers_as_merchants=True, | ||
| import_categories=True, | ||
| import_tax_codes=True, | ||
| charts_of_accounts=['Income'], | ||
| import_customers=True | ||
| ) | ||
|
|
||
| construct_tasks_and_chain_import_fields_to_fyle(workspace_id) |
There was a problem hiding this comment.
The test setup for construct_tasks_and_chain_import_fields_to_fyle is well-implemented, but the test lacks assertions to verify the outcome of the function call. Consider adding assertions to ensure the test effectively validates the function's behavior under the given conditions.
| @@ -0,0 +1,22 @@ | |||
| rollback; | |||
There was a problem hiding this comment.
The initial rollback; statement might be unnecessary or a mistake unless it serves a specific, undocumented purpose. Consider removing it or documenting its necessity.
| ) | ||
| ); | ||
|
|
||
| \copy (select * from old_schedules) to '/tmp/django_q_schedule.csv' with csv header; |
There was a problem hiding this comment.
The file path /tmp/django_q_schedule.csv is hard-coded, which could lead to issues in different environments or if the script is run by multiple instances simultaneously. Consider parameterizing the file path or implementing a mechanism to handle potential conflicts.
| def test_schedule_or_delete_fyle_import_tasks_with_no_configuration(configuration, mapping_settings): | ||
| new_schedule_or_delete_fyle_import_tasks(configuration, mapping_settings) | ||
|
|
||
|
|
||
| def test_schedule_or_delete_fyle_import_tasks_with_import_categories(configuration, db): | ||
| def test_schedule_or_delete_fyle_import_tasks_with_import_categories(configuration, mapping_settings): | ||
| configuration.import_categories = True | ||
| schedule_or_delete_fyle_import_tasks(configuration) | ||
| new_schedule_or_delete_fyle_import_tasks(configuration, mapping_settings) | ||
|
|
||
|
|
||
| def test_schedule_or_delete_fyle_import_tasks_with_project_mapping(configuration, db): | ||
| def test_schedule_or_delete_fyle_import_tasks_with_project_mapping(configuration, mapping_settings): | ||
| project_mapping = MappingSetting.objects.create( | ||
| source_field="PROJECT", workspace_id=1, import_to_fyle=True | ||
| ) | ||
| configuration.import_categories = False | ||
| schedule_or_delete_fyle_import_tasks(configuration) | ||
| new_schedule_or_delete_fyle_import_tasks(configuration, mapping_settings) |
There was a problem hiding this comment.
The tests and the new mapping_settings fixture are well-implemented, enhancing test flexibility and readability. Ensure that assertions are added to verify the expected outcomes of new_schedule_or_delete_fyle_import_tasks based on the configurations and mapping settings.
| workspace_id, | ||
| mapping_setting.destination_field, | ||
| mapping_setting.source_field, | ||
| mapping_setting.source_placeholder, | ||
| ) | ||
|
|
||
|
|
||
| def upload_tax_groups_to_fyle( | ||
| platform_connection: PlatformConnector, workspace_id: int | ||
| ): | ||
| existing_tax_codes_name = ExpenseAttribute.objects.filter( | ||
| attribute_type=FyleAttributeEnum.TAX_GROUP, workspace_id=workspace_id | ||
| ).values_list("value", flat=True) | ||
|
|
||
| xero_attributes = DestinationAttribute.objects.filter( | ||
| attribute_type="TAX_CODE", workspace_id=workspace_id | ||
| ).order_by("value", "id") | ||
|
|
||
| xero_attributes = remove_duplicates(xero_attributes) | ||
|
|
||
| fyle_payload: List[Dict] = create_fyle_tax_group_payload( | ||
| xero_attributes, existing_tax_codes_name | ||
| attribute_type="MERCHANT", | ||
| defaults={ | ||
| 'status': 'IN_PROGRESS' | ||
| } | ||
| ) | ||
|
|
||
| if fyle_payload: | ||
| platform_connection.tax_groups.post_bulk(fyle_payload) | ||
|
|
||
| platform_connection.tax_groups.sync() | ||
| Mapping.bulk_create_mappings(xero_attributes, FyleAttributeEnum.TAX_GROUP, "TAX_CODE", workspace_id) | ||
| sync_after = None | ||
|
|
||
| if not is_created: | ||
| sync_after = import_log.last_successful_run_at if import_log.last_successful_run_at else None | ||
|
|
||
| def create_fyle_tax_group_payload( | ||
| xero_attributes: List[DestinationAttribute], existing_fyle_tax_groups: list | ||
| ): | ||
| """ | ||
| Create Fyle tax Group Payload from Xero Objects | ||
| :param existing_fyle_tax_groups: Existing tax groups names | ||
| :param xero_attributes: Xero Objects | ||
| :return: Fyle tax Group Payload | ||
| """ | ||
|
|
||
| existing_fyle_tax_groups = [ | ||
| tax_group.lower() for tax_group in existing_fyle_tax_groups | ||
| ] | ||
|
|
||
| fyle_tax_group_payload = [] | ||
| for xero_attribute in xero_attributes: | ||
| if xero_attribute.value.lower() not in existing_fyle_tax_groups: | ||
| fyle_tax_group_payload.append( | ||
| { | ||
| "name": xero_attribute.value, | ||
| "is_enabled": True, | ||
| "percentage": round((xero_attribute.detail["tax_rate"] / 100), 2), | ||
| } | ||
| ) | ||
|
|
||
| logger.info("| Importing Tax Groups to Fyle | Content: {{Fyle Payload count: {}}}".format(len(fyle_tax_group_payload))) | ||
| return fyle_tax_group_payload | ||
|
|
||
| time_difference = datetime.now() - timedelta(minutes=30) | ||
| offset_aware_time_difference = time_difference.replace(tzinfo=timezone.utc) | ||
|
|
||
| @handle_import_exceptions(task_name="auto create tax codes_mappings") | ||
| def auto_create_tax_codes_mappings(workspace_id: int): | ||
| """ | ||
| Create Tax Codes Mappings | ||
| :return: None | ||
| """ | ||
|
|
||
| fyle_credentials: FyleCredential = FyleCredential.objects.get( | ||
| workspace_id=workspace_id | ||
| ) | ||
|
|
||
| platform = PlatformConnector(fyle_credentials=fyle_credentials) | ||
| platform.tax_groups.sync() | ||
|
|
||
| sync_xero_attributes("TAX_CODE", workspace_id) | ||
|
|
||
| upload_tax_groups_to_fyle(platform, workspace_id) | ||
| if (import_log.status == 'IN_PROGRESS' and not is_created) \ | ||
| or (sync_after and (sync_after > offset_aware_time_difference)): | ||
| return | ||
|
|
||
| else: | ||
| import_log.status = 'IN_PROGRESS' | ||
| import_log.processed_batches_count = 0 | ||
| import_log.total_batches_count = 0 | ||
| import_log.save() | ||
|
|
||
| def auto_create_suppliers_as_merchants(workspace_id): | ||
| fyle_credentials: FyleCredential = FyleCredential.objects.get( | ||
| workspace_id=workspace_id | ||
| ) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [67-114]
The refactoring in apps/mappings/tasks.py, including the introduction of auto_create_suppliers_as_merchants, is a significant improvement towards simplifying the codebase. However, there are a few points to consider:
-
Ensure that the removal of functions related to managing expense attributes, categories, projects, etc., does not remove any critical functionality or leave parts of the system unable to handle certain scenarios.
-
The new
auto_create_suppliers_as_merchantsfunction should be thoroughly tested, especially since it interacts with external systems (Fyle and Xero). Consider adding tests that cover successful cases, error handling, and edge cases. -
The use of
ImportLogto track the import status and log errors is a good practice. Ensure that the error logging is comprehensive and useful for debugging issues. -
The handling of time comparisons and status checks in
auto_create_suppliers_as_merchants(lines 83-88) is crucial for avoiding unnecessary imports. Verify that the logic correctly handles time zones and edge cases related to timing. -
Verify that no critical functionality is lost due to the removal of functions.
-
Thoroughly test
auto_create_suppliers_as_merchants, including error handling and edge cases. -
Ensure comprehensive and useful error logging in
ImportLog. -
Verify the correctness of time comparisons and status checks to avoid unnecessary imports.
| Mapping.objects.create( | ||
| destination_type="ACCOUNT", | ||
| destination=da_instance, | ||
| workspace_id=workspace_id, | ||
| source=ea_instance, | ||
| ) | ||
|
|
||
| ea_instance.save() | ||
|
|
||
| xero_attributes = upload_categories_to_fyle(workspace_id=workspace_id) | ||
| assert len(xero_attributes) == 29 | ||
|
|
||
| xero_credentials = XeroCredentials.objects.get(workspace_id=workspace_id) | ||
| xero_credentials.delete() | ||
|
|
||
| # Expect XeroCredentials.DoesNotExist exception since we've deleted the credentials | ||
| with pytest.raises(XeroCredentials.DoesNotExist): | ||
| xero_attributes = upload_categories_to_fyle(workspace_id=workspace_id) | ||
|
|
||
|
|
||
| def test_create_fyle_category_payload(mocker, db): | ||
| workspace_id = 1 | ||
| qbo_attributes = DestinationAttribute.objects.filter( | ||
| workspace_id=1, attribute_type="ACCOUNT" | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| "fyle.platform.apis.v1beta.admin.Categories.list_all", | ||
| return_value=fyle_data["get_all_categories"], | ||
| ) | ||
|
|
||
| qbo_attributes = remove_duplicates(qbo_attributes) | ||
|
|
||
| fyle_credentials: FyleCredential = FyleCredential.objects.get( | ||
| workspace_id=workspace_id | ||
| ) | ||
| platform = PlatformConnector(fyle_credentials) | ||
|
|
||
| category_map = get_all_categories_from_fyle(platform=platform) | ||
| fyle_category_payload = create_fyle_categories_payload( | ||
| qbo_attributes, 2, category_map | ||
| ) | ||
|
|
||
| assert ( | ||
| dict_compare_keys(fyle_category_payload[0], data["fyle_category_payload"][0]) | ||
| == [] | ||
| ), "category upload api return diffs in keys" | ||
|
|
||
|
|
||
| def test_auto_create_category_mappings(db, mocker): | ||
| workspace_id = 1 | ||
| mocker.patch( | ||
| "fyle_integrations_platform_connector.apis.Categories.post_bulk", | ||
| return_value=[], | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| "xerosdk.apis.Accounts.get_all", return_value=xero_data["get_all_accounts"] | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| "fyle.platform.apis.v1beta.admin.Categories.list_all", | ||
| return_value=fyle_data["get_all_categories"], | ||
| ) | ||
|
|
||
| response = auto_create_category_mappings(workspace_id=workspace_id) | ||
| assert response == [] | ||
|
|
||
| mappings = CategoryMapping.objects.filter(workspace_id=workspace_id) | ||
|
|
||
| assert len(mappings) == 0 | ||
|
|
||
| with mock.patch("apps.mappings.tasks.upload_categories_to_fyle") as mock_call: | ||
| mock_call.side_effect = WrongParamsError( | ||
| msg="wrong parameter error", response="wrong parameter error" | ||
| ) | ||
| response = auto_create_category_mappings(workspace_id=workspace_id) | ||
|
|
||
| mock_call.side_effect = FyleInvalidTokenError( | ||
| msg="Invalid Token for Fyle", response="Invalid Token for Fyle" | ||
| ) | ||
| response = auto_create_category_mappings(workspace_id=workspace_id) | ||
|
|
||
| mock_call.side_effect = InternalServerError( | ||
| msg="Internal server error while importing to Fyle", | ||
| response="Internal server error while importing to Fyle", | ||
| ) | ||
| response = auto_create_category_mappings(workspace_id=workspace_id) | ||
|
|
||
| fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | ||
| fyle_credentials.delete() | ||
|
|
||
| response = auto_create_category_mappings(workspace_id=workspace_id) | ||
|
|
||
| assert response == None | ||
|
|
||
|
|
||
| def test_async_auto_map_employees(mocker, db): | ||
| workspace_id = 1 | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-111]
The changes in tests/test_mappings/test_tasks.py reflect the refactoring in the tasks module. However, to ensure the tests are effective and maintainable, consider the following:
-
Verify that all new functions introduced in the tasks module, such as
auto_create_suppliers_as_merchants, are covered by tests. This includes testing for success scenarios, error handling, and edge cases. -
For removed functions, ensure that their corresponding tests are either removed or updated to reflect the new logic in the tasks module.
-
The use of mocks and patching should be carefully reviewed to ensure that they accurately represent the dependencies and interactions of the functions under test. This is particularly important for functions that interact with external systems.
-
Consider adding comments or documentation to the tests to explain the purpose and expected behavior of each test case. This can improve maintainability and make it easier for future contributors to understand the test suite.
-
Ensure coverage for all new functions in the tasks module.
-
Update or remove tests corresponding to removed functions.
-
Review the use of mocks and patching for accuracy.
-
Add comments or documentation to improve the understandability of the test suite.
| ) | ||
|
|
||
| # 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, | ||
| '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) |
There was a problem hiding this comment.
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_settingsis 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_fylefunction. -
Update the function's documentation to reflect its expanded responsibilities.
-
Consider refactoring the construction of
task_settingsto improve readability. -
Review and possibly externalize hard-coded values and conditions for better flexibility.
-
Ensure comprehensive testing of the function's expanded functionality.
| 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() | ||
|
|
There was a problem hiding this comment.
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
ImportLogstatus 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 whicherror['alert']is set toTrueorFalseto ensure consistency across different types of errors. -
The function updates the
ImportLogwith error details, which is crucial for debugging. Ensure that theImportLogmodel 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_v2to 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
ImportLogmodel adequately supports error logging. -
Add unit tests for
handle_import_exceptions_v2.
| mapping_setting.save() | ||
|
|
||
| schedule = Schedule.objects.filter( | ||
| func="apps.mappings.tasks.async_auto_create_custom_field_mappings", | ||
| func="apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle", | ||
| args="{}".format(workspace_id), | ||
| ).first() | ||
|
|
||
| assert ( | ||
| schedule.func == "apps.mappings.tasks.async_auto_create_custom_field_mappings" | ||
| schedule.func == "apps.mappings.queue.construct_tasks_and_chain_import_fields_to_fyle" | ||
| ) | ||
| assert schedule.args == "1" | ||
|
|
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-102]
The updates in tests/test_mappings/test_signals.py to use construct_tasks_and_chain_import_fields_to_fyle are appropriate given the changes in the queue module. However, to ensure the tests are effective:
-
Verify that the test cases adequately cover the scenarios that
construct_tasks_and_chain_import_fields_to_fyleis expected to handle. This includes different configurations of workspace settings and mapping settings. -
Ensure that the assertions in the test cases are meaningful and accurately reflect the expected outcomes of calling
construct_tasks_and_chain_import_fields_to_fyle. For example, verifying that the correct tasks are constructed and chained based on the provided settings. -
Consider adding more detailed comments or documentation within the test cases to explain the setup and expected outcomes. This can improve maintainability and make it easier for future contributors to understand the purpose of each test case.
-
Ensure comprehensive coverage of scenarios for
construct_tasks_and_chain_import_fields_to_fyle. -
Review and enhance assertions to accurately reflect expected outcomes.
-
Add detailed comments or documentation within the test cases for better understandability.
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
added
deploy
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Hrishabh17
added
deploy
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- .github/workflows/codecov.yml (1 hunks)
- .github/workflows/production_deployment.yml (1 hunks)
- .github/workflows/pytest.yml (1 hunks)
- .github/workflows/staging_deployment.yml (1 hunks)
- scripts/python/add_new_import_schedule.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- scripts/python/add_new_import_schedule.py
Additional comments: 4
.github/workflows/codecov.yml (1)
- 16-17: The addition of
submodules: recursiveunder theactions/checkout@v2step is a good practice for ensuring that all submodules are checked out before proceeding with the CI steps. This is particularly important for projects that rely on submodules for critical functionality or dependencies. This change aligns with the PR objectives of enhancing modularity and improving maintainability by ensuring that all necessary code, including that in submodules, is available for testing and coverage analysis..github/workflows/pytest.yml (1)
- 18-19: The addition of
submodules: recursiveunder theactions/checkout@v2step in the CI workflow for running pytest and testing for token health is essential. This ensures that all submodules are properly checked out before the services are brought up and tests are run. Given the project's focus on improving integration capabilities and streamlining task scheduling, having access to the complete codebase, including submodules, is crucial for accurate testing and coverage reporting..github/workflows/staging_deployment.yml (1)
- 17-18: Adding
submodules: recursiveunder theactions/checkout@v2step in the staging deployment workflow is a prudent decision. This ensures that all submodules, which might contain necessary configurations or dependencies, are available during the deployment process. This change supports the PR's goal of optimizing code structure and improving error handling by ensuring that the deployment process has access to the full, intended codebase, including any submodules..github/workflows/production_deployment.yml (1)
- 13-14: Incorporating
submodules: recursiveunder theactions/checkout@v2step for the production deployment workflow is a critical improvement. This ensures that all necessary submodules are checked out and available during the deployment process, which is vital for a successful and error-free deployment to production. This adjustment aligns with the PR's objectives of enhancing modularity and ensuring that the deployment process is robust and reliable by including all necessary code components.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #322 +/- ##
==========================================
- Coverage 94.54% 93.14% -1.40%
==========================================
Files 60 61 +1
Lines 3627 3470 -157
==========================================
- Hits 3429 3232 -197
- Misses 198 238 +40 ☔ View full report in Codecov by Sentry. |
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Summary by CodeRabbit