Import Logs for Deps Import #186
Conversation
…check for current_state
|
| @@ -103,10 +106,14 @@ def post_dependent_cost_code(dependent_field_setting: DependentFieldSetting, pla | |||
| logger.error(f'Exception while posting dependent cost code | Error: {exception} | Payload: {payload}') | |||
| raise | |||
|
|
|||
| import_log.status = 'COMPLETE' | |||
There was a problem hiding this comment.
can we also store batches count while posting?
There was a problem hiding this comment.
store batch count in import log?
| post_dependent_cost_type(dependent_field_setting, platform, filters) | ||
| post_dependent_cost_type(cost_category_import_log, dependent_field_setting, platform, filters) | ||
|
|
||
| if cost_code_import_log.status in ['FAILED', 'FATAL']: |
There was a problem hiding this comment.
wouldn't there be exception incase of errors?
There was a problem hiding this comment.
Both post_cost_code and post_cost_type method are decoarated with handle_import_exception
- If post of cost code fails, we handle in exception handler decorator and set the import log status to Failed.
- When the call returns we check the status, if its in Complete, we call the post of cost type (here if error occurs the status will be Failed/Fatal). Otherwise we mark complete.
There was a problem hiding this comment.
also fixed the call ordering, it was wrong earlier. can check code again for this.
| dependent_field_setting=dependent_field_settings, | ||
| platform=platform.return_value | ||
| ) | ||
| assert cost_code_import_log.status == 'FATAL' |
There was a problem hiding this comment.
how it this going fatal?
There was a problem hiding this comment.
Passed a less param (removed filters). To explicitly raise an error.
|
|
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Hrishabh17
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (12)
apps/mappings/tasks.py (1)
[!TIP]
Codebase VerificationThe search results indicate multiple instances of
Sage300Credential.objects.get(workspace_id=workspace_id)across various files, but there is no evidence of exception handling for potential errors that might arise from this database query.To ensure robust error handling, it is crucial to wrap the
getcall in a try-except block to catch exceptions such asSage300Credential.DoesNotExistandSage300Credential.MultipleObjectsReturned.
- apps/mappings/tasks.py: Add exception handling around
Sage300Credential.objects.get(workspace_id=workspace_id).Category:
Analysis chain
Line range hint
6-17: Ensure proper error handling for potential exceptions fromSage300Credential.objects.get.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper exception handling for Sage300Credential retrieval. # Test: Search for exception handling related to Sage300Credential in the entire codebase. rg --type python 'Sage300Credential.*get' --context 5Length of output: 86
Script:
#!/bin/bash # Description: Verify proper exception handling for Sage300Credential retrieval. # Test: Search for exception handling related to Sage300Credential in the entire codebase. rg --type py 'Sage300Credential.*get' --context 5Length of output: 13382
apps/mappings/exceptions.py (1)
Line range hint
20-35: Refactor the exception handling to use f-strings for better performance and readability.- error['message'] = 'Invalid Token or Sage 300 credentials does not exist workspace_id - {0}'.format(workspace_id) + error['message'] = f'Invalid Token or Sage 300 credentials does not exist workspace_id - {workspace_id}'apps/fyle/serializers.py (1)
Line range hint
59-59: Improve exception handling by linking the cause of the exceptions.- raise APIException("Internal Server Error", code='server_error') + raise APIException("Internal Server Error", code='server_error') from exceptionAlso applies to: 63-63
tests/test_fyle/test_views.py (1)
Line range hint
17-17: Convert to f-strings for consistency and improved readability.- 'Bearer {}'.format(test_connection.access_token) + f'Bearer {test_connection.access_token}'Repeat this pattern for all occurrences.
Also applies to: 52-52, 82-82, 99-99, 112-112, 130-130
apps/sage300/helpers.py (1)
Line range hint
66-66: Convert to f-strings for consistency and improved readability.- 'Sage 300 Project - {0}, Id - {1}'.format(expense_attribute.value, code) + f'Sage 300 Project - {expense_attribute.value}, Id - {code}'Also applies to: 107-110
apps/sage300/dependent_fields.py (3)
Line range hint
26-26: Refactor to use dictionary'sgetmethod for cleaner code.- placeholder = existing_attribute['placeholder'] if 'placeholder' in existing_attribute else None + placeholder = existing_attribute.get('placeholder', None)
Line range hint
35-35: Convert to use f-string for more modern, readable string formatting.- new_placeholder = 'Select {0}'.format(fyle_attribute) + new_placeholder = f'Select {fyle_attribute}'
Line range hint
73-111: The methodpost_dependent_cost_codeis well-structured with clear exception handling and logging. However, consider adding a batch count as suggested in previous comments.Would you like me to add functionality to store batch counts in the import log?
apps/sage300/utils.py (1)
Line range hint
80-83: Simplify the assignment usinggetmethod with a default value.- if vendor_type_mapping: - if item.type_id in vendor_type_mapping: - detail['type'] = vendor_type_mapping[item.type_id] - else: - detail['type'] = None + detail['type'] = vendor_type_mapping.get(item.type_id, None)apps/mappings/imports/modules/base.py (3)
Line range hint
65-65: Avoid using mutable default arguments in function definitions to prevent potential bugs.- def construct_attributes_filter(self, attribute_type: str, paginated_destination_attribute_values: List[str] = []): + def construct_attributes_filter(self, attribute_type: str, paginated_destination_attribute_values: List[str] = None): + if paginated_destination_attribute_values is None: + paginated_destination_attribute_values = []
Line range hint
132-132: Convert to use f-string for more modern, readable string formatting.- logger.info("| Importing {} to Fyle | Content: {{WORKSPACE_ID: {} Fyle Payload count: {} is_last_batch: {}}}".format(self.destination_field, self.workspace_id, len(fyle_payload), is_last_batch)) + logger.info(f"| Importing {self.destination_field} to Fyle | Content: {{WORKSPACE_ID: {self.workspace_id} Fyle Payload count: {len(fyle_payload)} is_last_batch: {is_last_batch}}}")Also applies to: 307-307
Line range hint
268-268: Remove unnecessary conditional expression.- is_auto_sync_status_allowed = True if (self.destination_field == 'PROJECT' and self.source_field == 'PROJECT') or self.source_field == 'CATEGORY' else False + is_auto_sync_status_allowed = (self.destination_field == 'PROJECT' and self.source_field == 'PROJECT') or self.source_field == 'CATEGORY'
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (13)
- apps/fyle/serializers.py (2 hunks)
- apps/mappings/exceptions.py (2 hunks)
- apps/mappings/imports/modules/base.py (3 hunks)
- apps/mappings/imports/tasks.py (1 hunks)
- apps/mappings/models.py (1 hunks)
- apps/mappings/tasks.py (2 hunks)
- apps/sage300/dependent_fields.py (5 hunks)
- apps/sage300/helpers.py (2 hunks)
- apps/sage300/utils.py (3 hunks)
- requirements.txt (1 hunks)
- tests/test_fyle/test_views.py (1 hunks)
- tests/test_sage300/test_dependent_fields.py (5 hunks)
- tests/test_sage300/test_utils.py (5 hunks)
Files skipped from review due to trivial changes (1)
- requirements.txt
Additional context used
Ruff
apps/mappings/exceptions.py
29-29: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
29-29: Use f-string instead of
formatcall (UP032)Convert to f-string
43-43: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
43-43: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/fyle/serializers.py
59-59: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)
63-63: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling (B904)tests/test_fyle/test_views.py
17-17: Use f-string instead of
formatcall (UP032)Convert to f-string
52-52: Use f-string instead of
formatcall (UP032)Convert to f-string
82-82: Use f-string instead of
formatcall (UP032)Convert to f-string
99-99: Use f-string instead of
formatcall (UP032)Convert to f-string
112-112: Use f-string instead of
formatcall (UP032)Convert to f-string
130-130: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/sage300/helpers.py
66-66: Use f-string instead of
formatcall (UP032)Convert to f-string
107-110: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
107-110: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/sage300/dependent_fields.py
26-26: Use
existing_attribute.get('placeholder', None)instead of anifblock (SIM401)Replace with
existing_attribute.get('placeholder', None)
35-35: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
35-35: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/sage300/utils.py
80-83: Use
detail['type'] = vendor_type_mapping.get(item.type_id, None)instead of anifblock (SIM401)Replace with
detail['type'] = vendor_type_mapping.get(item.type_id, None)apps/mappings/imports/modules/base.py
65-65: Do not use mutable data structures for argument defaults (B006)
Replace with
None; initialize within function
132-132: Use f-string instead of
formatcall (UP032)Convert to f-string
268-268: Remove unnecessary
True if ... else False(SIM210)Remove unnecessary
True if ... else False
307-307: Use f-string instead of
formatcall (UP032)Convert to f-string
Additional comments not posted (17)
apps/mappings/tasks.py (1)
16-17: The use of lambdas for sync functions is acceptable, but consider defining separate functions if the logic becomes more complex in the future.apps/mappings/models.py (1)
38-51: The methodcreate_import_logcorrectly sets the import log status to 'IN_PROGRESS'. Ensure that all necessary fields are initialized properly.apps/mappings/imports/tasks.py (1)
52-57: Creation and usage ofimport_logfor 'COST_CODE' and 'COST_CATEGORY' are implemented correctly. Ensure that these logs are handled appropriately in error scenarios.Verification successful
The search results indicate that error handling for
import_logis indeed implemented in various parts of the codebase. Theimport_logstatus and error messages are being set appropriately in different scenarios, including exceptions and test cases.
- Error Handling Locations:
tests/test_mappings/test_exceptions.py: Multiple instances whereimport_logstatus and error messages are asserted.apps/sage300/dependent_fields.py: Handling exceptions and settingimport_logstatus and error logs.apps/mappings/exceptions.py: Settingimport_logerror logs.apps/mappings/imports/tasks.py: Loggingimport_logstatus and error logs.apps/mappings/imports/modules/expense_custom_fields.py: Settingimport_logstatus and error logs.apps/mappings/imports/modules/base.py: Settingimport_logstatus and error logs.Given this comprehensive error handling, the concern about ensuring appropriate handling of
import_login error scenarios is addressed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling for import logs. # Test: Search for error handling related to import logs in the entire codebase. rg --type python 'import_log.*error' --context 5Length of output: 81
Script:
#!/bin/bash # Description: Verify error handling for import logs. # Test: Search for error handling related to import logs in the entire codebase. rg --type py 'import_log.*error' --context 5Length of output: 10491
apps/mappings/exceptions.py (1)
Line range hint
20-35: Ensure thatImportLoginstances are correctly identified and handled. The current implementation seems robust.apps/fyle/serializers.py (2)
18-18: Ensure that the new importchain_import_fields_to_fyleis used appropriately within the file.
42-42: The integration ofchain_import_fields_to_fylewithin thecreatemethod is correctly placed to handle therefreshcondition. This should enhance the import functionality as intended.tests/test_fyle/test_views.py (1)
13-13: The addition of the mocker patch forchain_import_fields_to_fyleis appropriate for testing the import functionality. Ensure that the mocked return value aligns with expected outcomes in real scenarios.tests/test_sage300/test_dependent_fields.py (2)
9-9: The addition ofImportLogimport is crucial for the new logging functionality in tests. Ensure thatImportLogis used consistently across all test functions.
68-68: The creation of import logs in test functions is correctly implemented. This setup is essential for testing the new import logging functionality.Also applies to: 110-110, 149-149, 178-178
apps/sage300/helpers.py (1)
15-15: The addition ofImportLogimport is essential for the new functionality inupdate_and_disable_cost_code. This aligns with the overall enhancement of import logging.apps/sage300/dependent_fields.py (3)
14-15: Added imports forImportLogandhandle_import_exceptionsare appropriate for the new functionality related to import logging and exception handling.
Line range hint
115-141: The methodpost_dependent_cost_typefollows similar structure and error handling aspost_dependent_cost_code. Ensure that the exception handling decorates properly manage all potential exceptions.
155-168: Proper handling of import log statuses to manage the flow of dependent field posting. This ensures that if cost codes fail, cost categories are not processed, which is a logical flow control.apps/sage300/utils.py (2)
8-8: Import forhandle_import_exceptionsis correctly added to handle exceptions in sync methods.
266-267: The addition of exception handling decorators tosync_cost_codesandsync_cost_categoriesmethods is a good practice to ensure robust error management.Also applies to: 277-278
tests/test_sage300/test_utils.py (2)
5-5: Import ofImportLogis correctly added to support the new testing scenarios involving import logs.
Line range hint
457-510: Tests forsync_cost_categoriesandsync_cost_codescorrectly create import logs and pass them to the sync methods, aligning with the changes in the main codebase.
[APROVED]
| @@ -158,17 +158,14 @@ def post_dependent_expense_field_values(workspace_id: int, dependent_field_setti | |||
| posted_cost_types = post_dependent_cost_code(cost_code_import_log, dependent_field_setting, platform, filters) | |||
| if posted_cost_types: | |||
| filters['cost_code_name__in'] = posted_cost_types | |||
| post_dependent_cost_type(cost_category_import_log, dependent_field_setting, platform, filters) | |||
|
|
|||
| if cost_code_import_log.status in ['FAILED', 'FATAL']: | |||
| cost_category_import_log.status = 'FAILED' | |||
| cost_category_import_log.error_log = {'message': 'Importing COST_CODE failed'} | |||
There was a problem hiding this comment.
we're storing all details in the exception, why are we overriding?
| cost_category_import_log.status = 'COMPLETE' | ||
| cost_category_import_log.error_log = [] | ||
| cost_category_import_log.save() | ||
| post_dependent_cost_type(cost_category_import_log, dependent_field_setting, platform, filters) | ||
|
|
||
| DependentFieldSetting.objects.filter(workspace_id=workspace_id).update(last_successful_import_at=datetime.now()) |
There was a problem hiding this comment.
we should bring this inside else block, else we might mark timestamp even for failed runs
| @@ -35,6 +35,20 @@ class Meta: | |||
| db_table = 'import_logs' | |||
| unique_together = ('workspace', 'attribute_type') | |||
|
|
|||
| @classmethod | |||
| def create_import_log(self, attribute_type, workspace_id): | |||
| @@ -103,10 +106,14 @@ def post_dependent_cost_code(dependent_field_setting: DependentFieldSetting, pla | |||
| logger.error(f'Exception while posting dependent cost code | Error: {exception} | Payload: {payload}') | |||
| raise | |||
|
|
|||
| import_log.status = 'COMPLETE' | |||
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
apps/sage300/helpers.py (2)
Line range hint
62-62: Convert theformatcall to an f-string for improved readability and performance.- 'Sage 300 Project - {0}, Id - {1}'.format(expense_attribute.value, code) + f'Sage 300 Project - {expense_attribute.value}, Id - {code}'
Line range hint
103-106: Use f-string for consistency and enhanced performance. Also, remove explicit positional indices which are redundant.- 'Sage 300 Project - {0}, Id - {1}'.format(expense_attribute.value, code) + f'Sage 300 Project - {expense_attribute.value}, Id - {code}'
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/sage300/helpers.py (2 hunks)
Additional context used
Ruff
apps/sage300/helpers.py
62-62: Use f-string instead of
formatcall (UP032)Convert to f-string
103-106: Use implicit references for positional format fields (UP030)
Remove explicit positional indices
103-106: Use f-string instead of
formatcall (UP032)Convert to f-string
Additional comments not posted (1)
apps/sage300/helpers.py (1)
14-14: Added import forImportLogis correctly placed and necessary for the new functionality.
|
|
||
| # This call will disable the cost codes in Fyle that has old project name | ||
| posted_cost_codes = post_dependent_cost_code(dependent_field_setting, platform, filters, is_enabled=False) | ||
| posted_cost_codes = post_dependent_cost_code(cost_code_import_log, dependent_field_setting, platform, filters, is_enabled=False) |
There was a problem hiding this comment.
Tip
Codebase Verification
The post_dependent_cost_code function lacks any associated documentation comments or docstrings to describe its parameters and functionality. This needs to be addressed to ensure the changes are well-documented.
- Location:
apps/sage300/dependent_fields.py, around thepost_dependent_cost_codefunction definition.
Please add appropriate docstrings or comments to document the new import_log parameter and the overall functionality of the function.
Analysis chain
Correct usage of the post_dependent_cost_code function with the new import_log parameter. Ensure that the function's modifications are well-documented to reflect these changes.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify documentation for `post_dependent_cost_code`.
# Test: Search for updated documentation for `post_dependent_cost_code`. Expect: Updated documentation reflecting new parameters.
rg --type python $'def post_dependent_cost_code($$$) { $$$ }' apps/sage300/dependent_fields.py
Length of output: 255
Script:
#!/bin/bash
# Description: Verify documentation for `post_dependent_cost_code`.
# Test: Search for the function definition and its associated documentation.
rg --type python 'def post_dependent_cost_code' -A 10 apps/sage300/dependent_fields.py
Length of output: 119
Script:
#!/bin/bash
# Description: Verify documentation for `post_dependent_cost_code`.
# Test: Search for the function definition and its associated documentation.
rg --type py 'def post_dependent_cost_code' -A 10 apps/sage300/dependent_fields.py
Length of output: 748
Script:
#!/bin/bash
# Description: Verify documentation for `post_dependent_cost_code`.
# Test: Search for documentation comments or docstrings around the function definition.
rg --type py 'def post_dependent_cost_code' -A 20 -B 5 apps/sage300/dependent_fields.py
Length of output: 1329
| @@ -134,9 +135,10 @@ | |||
| 'job_id__in':list(cost_codes_to_disable.keys()), | |||
| 'workspace_id': workspace_id | |||
| } | |||
| cost_code_import_log = ImportLog.create_import_log('COST_CODE', workspace_id) | |||
There was a problem hiding this comment.
Tip
Codebase Verification
The create_import_log method in apps/mappings/models.py does not contain any try-except blocks to handle exceptions. This could lead to unhandled exceptions during import operations.
- Location:
apps/mappings/models.py,create_import_logmethod
Analysis chain
Ensure that the create_import_log method correctly handles all possible exceptions and edge cases since it's critical for tracking import operations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify exception handling in `create_import_log` method.
# Test: Search for exception handling in `create_import_log` method. Expect: Proper exception handling.
rg --type python $'def create_import_log($$$) { try { $$$ } catch ($$$) { $$$ } }' apps/mappings/models.py
Length of output: 281
Script:
#!/bin/bash
# Description: Verify exception handling in `create_import_log` method.
# Find the definition of the `create_import_log` method in the codebase.
ast-grep --lang python --pattern 'def create_import_log($_) { $$$ }'
# Check for try-except blocks within the `create_import_log` method.
ast-grep --lang python --pattern 'def create_import_log($_) { try: $$$ except: $$$ }'
Length of output: 959
Script:
#!/bin/bash
# Description: Verify exception handling in `create_import_log` method.
# Find the definition of the `create_import_log` method in the codebase.
rg --type python --pcre2 'def create_import_log\('
# Check for try-except blocks within the `create_import_log` method.
rg --type python --pcre2 'def create_import_log\([^\)]*\):(\n|.)*?try:(\n|.)*?except:'
Length of output: 203
Script:
#!/bin/bash
# Description: Verify exception handling in `create_import_log` method.
# Find the definition of the `create_import_log` method in the codebase.
rg --type py --pcre2 'def create_import_log\('
# Check for try-except blocks within the `create_import_log` method.
rg --type py --pcre2 'def create_import_log\([^\)]*\):(\n|.)*?try:(\n|.)*?except:'
Length of output: 220
Summary by CodeRabbit
New Features
Tests