Max retry exports #391
Max retry exports #391
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes introduce enhancements to the export functionality and error handling within a financial application. The Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant Export
participant Queue
participant ErrorHandler
Workspace->>Export: Request Export with interval_hours
Export->>Queue: Schedule Bills Creation
Queue->>Queue: Validate Failing Export
Queue-->>ErrorHandler: Check for Errors
alt No Errors
Queue-->>Export: Complete Export
else Errors Detected
Queue-->>ErrorHandler: Log Error and Skip Export
end
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/workspaces/actions.py (2 hunks)
- apps/xero/queue.py (6 hunks)
- tests/test_xero/test_tasks.py (4 hunks)
Additional comments not posted (11)
apps/workspaces/actions.py (2)
208-209: LGTM!The changes enhance the function by allowing more granular control over the export process.
The code changes are approved.
225-226: LGTM!The changes enhance the function by allowing more granular control over the export process.
The code changes are approved.
apps/xero/queue.py (5)
20-28: LGTM!The function enhances the control flow by allowing the system to conditionally skip exporting expense groups that have encountered too many errors within a specified timeframe.
The code changes are approved.
123-123: LGTM!The changes enhance the error handling and export scheduling logic, making the system more robust in managing failed exports.
The code changes are approved.
140-150: LGTM!The changes enhance the error handling and export scheduling logic, making the system more robust in managing failed exports.
The code changes are approved.
199-199: LGTM!The changes enhance the error handling and export scheduling logic, making the system more robust in managing failed exports.
The code changes are approved.
217-229: LGTM!The changes enhance the error handling and export scheduling logic, making the system more robust in managing failed exports.
The code changes are approved.
tests/test_xero/test_tasks.py (4)
419-419: LGTM!The changes ensure that the test covers the new parameter
interval_hours.The code changes are approved.
517-517: LGTM!The changes ensure that the test covers the new parameter
interval_hours.The code changes are approved.
1024-1064: LGTM!The new test enhances the testing coverage by ensuring that the scheduling function behaves as expected in scenarios where errors are encountered.
The code changes are approved.
1067-1107: LGTM!The new test enhances the testing coverage by ensuring that the scheduling function behaves as expected in scenarios where errors are encountered.
The code changes are approved.
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (10)
apps/xero/queue.py (6)
21-29: Improve docstring clarity and simplify condition.The docstring can be improved for clarity. Additionally, the condition can be simplified for readability.
Apply this diff to improve the docstring and simplify the condition:
def validate_failing_export(is_auto_export: bool, interval_hours: int, error: Error): """ Validate if an export should be skipped based on conditions. :param is_auto_export: Indicates if auto export is enabled. :param interval_hours: The interval in hours for export scheduling. :param error: The error object containing error details. :return: True if the export should be skipped, False otherwise. """ - return is_auto_export and interval_hours and error and error.repetition_count > 100 and datetime.now().replace(tzinfo=timezone.utc) - error.updated_at <= timedelta(hours=24) + return ( + is_auto_export + and interval_hours + and error + and error.repetition_count > 100 + and datetime.now(timezone.utc) - error.updated_at <= timedelta(hours=24) + )
Line range hint
124-128: Update docstring to include new parameter.The docstring is missing the new
interval_hoursparameter.Apply this diff to update the docstring:
def schedule_bills_creation(workspace_id: int, expense_group_ids: List[str], is_auto_export: bool, fund_source: str, interval_hours: int) -> list: """ Schedule bills creation :param expense_group_ids: List of expense group ids :param workspace_id: workspace id + :param is_auto_export: Indicates if auto export is enabled. + :param fund_source: The source of funds. + :param interval_hours: The interval in hours for export scheduling. :return: List of chaining attributes """
150-150: Improve logging message for more details.The logging message can include more details about the error.
Apply this diff to improve the logging message:
logger.info('Skipping expense group %s as it has %s errors', expense_group.id, error.repetition_count) + logger.info('Skipping expense group %s due to error: %s (repetition count: %s)', expense_group.id, error.error_detail, error.repetition_count)
Line range hint
200-204: Update docstring to include new parameter.The docstring is missing the new
interval_hoursparameter.Apply this diff to update the docstring:
def schedule_bank_transaction_creation( workspace_id: int, expense_group_ids: List[str], is_auto_export: bool, fund_source: str, interval_hours: int ) -> list: """ Schedule bank transaction creation :param expense_group_ids: List of expense group ids :param workspace_id: workspace id + :param is_auto_export: Indicates if auto export is enabled. + :param fund_source: The source of funds. + :param interval_hours: The interval in hours for export scheduling. :return: List of chaining attributes """
229-229: Improve logging message for more details.The logging message can include more details about the error.
Apply this diff to improve the logging message:
logger.info('Skipping expense group %s as it has %s errors', expense_group.id, error.repetition_count) + logger.info('Skipping expense group %s due to error: %s (repetition count: %s)', expense_group.id, error.error_detail, error.repetition_count)
218-218: Replace print statements with proper logging.The print statements should be replaced with proper logging for consistency and better debugging.
Apply this diff to replace print statements with proper logging:
- print(len(expense_groups)) + logger.debug('Number of expense groups: %d', len(expense_groups)) - print('task_log', task_log.__dict__) + logger.debug('Task log details: %s', task_log.__dict__) - print('task_log', task_log.__dict__) + logger.debug('Task log details: %s', task_log.__dict__)Also applies to: 237-237, 243-243
tests/test_xero/test_tasks.py (4)
1024-1024: Add docstring to the test function.The test function is missing a docstring.
Apply this diff to add a docstring:
def test_skipping_schedule_bills_creation(db): + """ + Test the behavior of schedule_bills_creation function when encountering errors. + :param db: Database fixture. + :return: None + """
1054-1055: Add more assertions to validate the behavior.The test can include more assertions to validate the behavior.
Apply this diff to add more assertions:
task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first() assert task_log.type == 'FETCHING_EXPENSES' + assert task_log.status == 'READY' task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first() assert task_log.type == 'CREATING_BILL' + assert task_log.status == 'ENQUEUED'Also applies to: 1063-1064
1067-1067: Add docstring to the test function.The test function is missing a docstring.
Apply this diff to add a docstring:
def test_skipping_schedule_bank_transaction_creation(db): + """ + Test the behavior of schedule_bank_transaction_creation function when encountering errors. + :param db: Database fixture. + :return: None + """
1097-1098: Add more assertions to validate the behavior.The test can include more assertions to validate the behavior.
Apply this diff to add more assertions:
task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first() assert task_log.type == 'FETCHING_EXPENSES' + assert task_log.status == 'READY' task_log = TaskLog.objects.filter(expense_group_id=expense_group.id).first() assert task_log.type == 'CREATING_BANK_TRANSACTION' + assert task_log.status == 'ENQUEUED'Also applies to: 1106-1107
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | ||
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | ||
| if skip_export: |
There was a problem hiding this comment.
Handle potential exceptions in validate_failing_export function call.
The validate_failing_export function call should handle potential exceptions to avoid breaking the loop.
Apply this diff to handle potential exceptions:
error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first()
- skip_export = validate_failing_export(is_auto_export, interval_hours, error)
+ try:
+ skip_export = validate_failing_export(is_auto_export, interval_hours, error)
+ except Exception as e:
+ logger.error('Error validating export for expense group %s: %s', expense_group.id, str(e))
+ skip_export = FalseCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | |
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | |
| if skip_export: | |
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | |
| try: | |
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | |
| except Exception as e: | |
| logger.error('Error validating export for expense group %s: %s', expense_group.id, str(e)) | |
| skip_export = False | |
| if skip_export: |
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | ||
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | ||
| if skip_export: |
There was a problem hiding this comment.
Handle potential exceptions in validate_failing_export function call.
The validate_failing_export function call should handle potential exceptions to avoid breaking the loop.
Apply this diff to handle potential exceptions:
error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first()
- skip_export = validate_failing_export(is_auto_export, interval_hours, error)
+ try:
+ skip_export = validate_failing_export(is_auto_export, interval_hours, error)
+ except Exception as e:
+ logger.error('Error validating export for expense group %s: %s', expense_group.id, str(e))
+ skip_export = FalseCommittable 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | |
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | |
| if skip_export: | |
| error = errors.filter(workspace_id=workspace_id, expense_group=expense_group, is_resolved=False).first() | |
| try: | |
| skip_export = validate_failing_export(is_auto_export, interval_hours, error) | |
| except Exception as e: | |
| logger.error('Error validating export for expense group %s: %s', expense_group.id, str(e)) | |
| skip_export = False | |
| if skip_export: |
Ashutosh619-sudo
added
the
deploy
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
Staging
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/workspaces/actions.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/workspaces/actions.py
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
added
deploy
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
| task_log, _ = TaskLog.objects.get_or_create( | ||
| workspace_id=expense_group.workspace_id, | ||
| expense_group=expense_group, | ||
| defaults={"status": TaskLogStatusEnum.ENQUEUED, "type": TaskLogTypeEnum.CREATING_BILL}, | ||
| ) | ||
| if task_log.status not in [TaskLogStatusEnum.IN_PROGRESS, TaskLogStatusEnum.ENQUEUED]: | ||
| task_log.type = TaskLogTypeEnum.CREATING_BILL |
| task_log, _ = TaskLog.objects.get_or_create( | ||
| workspace_id=expense_group.workspace_id, | ||
| expense_group=expense_group, | ||
| defaults={"status": TaskLogStatusEnum.ENQUEUED, "type": TaskLogTypeEnum.CREATING_BANK_TRANSACTION}, | ||
| ) | ||
| if task_log.status not in [TaskLogStatusEnum.IN_PROGRESS, TaskLogStatusEnum.ENQUEUED]: | ||
| task_log.type = TaskLogTypeEnum.CREATING_BANK_TRANSACTION |
* Max retry export with test cases * added interval hours * fixed flake * change log level * cahnge log level * flake resolved * remove loggers
No description provided.