Added webhook callback #86
Conversation
Hrishabh17
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
WalkthroughThe update introduces a decorator for handling exceptions, a new function for asynchronously handling webhook callbacks, and a new view for managing these callbacks. It also adds functionality for creating admin subscriptions during workspace serialization and invokes these tasks asynchronously. API URL configurations and test settings have also been updated. Additionally, coverage thresholds in GitHub workflows have been slightly reduced. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebhookCallbackView
participant FylePlatform
participant TaskQueue
User->>+WebhookCallbackView: Sends webhook callback
WebhookCallbackView->>+TaskQueue: async_handle_webhook_callback(body)
TaskQueue->>FylePlatform: Import and export expenses
FylePlatform-->>TaskQueue: Response
TaskQueue-->>-WebhookCallbackView: Completion status
WebhookCallbackView-->>User: Callback processed response
Note right of WebhookCallbackView: New flow with decorators
sequenceDiagram
participant Serializer
participant TaskQueue
participant FylePlatform
Serializer->>+TaskQueue: async_create_admin_subscriptions(workspace_id)
TaskQueue->>+FylePlatform: Create admin subscriptions
FylePlatform-->>-TaskQueue: Response
TaskQueue-->>-Serializer: Subscriptions created
Note right of Serializer: Updated with async task calls
Poem
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.
Actionable comments posted: 6
Outside diff range and nitpick comments (3)
quickbooks_desktop_api/tests/settings.py (1)
Line range hint
29-29: Simplify the assignment ofDEBUGto remove unnecessary conditional.- DEBUG = True if os.environ.get('DEBUG') == 'True' else False + DEBUG = os.environ.get('DEBUG') == 'True'quickbooks_desktop_api/settings.py (2)
Line range hint
33-33: Simplify the assignment ofDEBUGto remove unnecessary conditional.- DEBUG = True if os.environ.get('DEBUG') == 'True' else False + DEBUG = os.environ.get('DEBUG') == 'True'
Line range hint
150-150: Update logging format specifiers to use modern Python formatting.- 'format': '{levelname} %s {asctime} {module} {message} ' % 'QBD_API', + 'format': f'{{levelname}} QBD_API {{asctime}} {{module}} {{message}}', - 'format': 'request {levelname} %s {asctime} {message}' % 'QBD_API', + 'format': f'request {{levelname}} QBD_API {{asctime}} {{message}}',Also applies to: 154-154
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- apps/fyle/exceptions.py (1 hunks)
- apps/fyle/queue.py (2 hunks)
- apps/fyle/urls.py (1 hunks)
- apps/fyle/views.py (2 hunks)
- apps/workspaces/serializers.py (3 hunks)
- apps/workspaces/tasks.py (3 hunks)
- quickbooks_desktop_api/settings.py (1 hunks)
- quickbooks_desktop_api/tests/settings.py (1 hunks)
- scripts/python/001_add_admin_subscriptions.py (1 hunks)
- tests/test_workspaces/test_tasks.py (4 hunks)
Additional context used
Ruff
scripts/python/001_add_admin_subscriptions.py
12-12: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/workspaces/tasks.py
100-100: Use f-string instead of
formatcall (UP032)Convert to f-string
tests/test_workspaces/test_tasks.py
95-95: Use f-string instead of
formatcall (UP032)Convert to f-string
122-122: Use f-string instead of
formatcall (UP032)Convert to f-string
apps/workspaces/serializers.py
163-164: Use a single
ifstatement instead of nestedifstatements (SIM102)quickbooks_desktop_api/tests/settings.py
29-29: Remove unnecessary
True if ... else False(SIM210)Remove unnecessary
True if ... else False
132-132: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
136-136: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
quickbooks_desktop_api/settings.py
33-33: Remove unnecessary
True if ... else False(SIM210)Remove unnecessary
True if ... else False
150-150: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
154-154: Use format specifiers instead of percent format (UP031)
Replace with format specifiers
Additional comments not posted (6)
apps/fyle/urls.py (1)
3-3: The URL path forWebhookCallbackViewis correctly defined and follows Django's URL naming conventions.Also applies to: 8-8
apps/fyle/views.py (1)
25-36: The implementation ofWebhookCallbackViewis appropriate for handling webhook callbacks. It uses the correct decorator and function calls for asynchronous processing.apps/fyle/queue.py (1)
63-73: The implementation ofasync_handle_webhook_callbackis well-designed to handle webhook callbacks based on the action specified. It correctly retrieves and triggers tasks for the appropriate workspace.apps/workspaces/serializers.py (1)
Line range hint
163-164: InAdvancedSettingSerializer.create, consider refactoring the nestedifstatements into a single condition to simplify the logic and improve readability.
[REFACTOR_SUGGESTion]- if not advanced_setting: - if 'expense_memo_structure' not in validated_data: + if not advanced_setting and 'expense_memo_structure' not in validated_data:quickbooks_desktop_api/tests/settings.py (1)
244-244: Ensure the newAPI_URLvariable is securely used and properly documented.quickbooks_desktop_api/settings.py (1)
266-266: The introduction ofAPI_URLand other Fyle-related environment variables is well-handled. Ensure that these variables are used securely across the application.
| def handle_view_exceptions(): | ||
| def decorator(func): | ||
| def new_fn(*args, **kwargs): | ||
| try: | ||
| return func(*args, **kwargs) | ||
| except AccountingExport.DoesNotExist: | ||
| return Response(data={'message': 'AccountingExport not found'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except FyleCredential.DoesNotExist: | ||
| return Response(data={'message': 'Fyle credentials not found in workspace'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except FyleInvalidTokenError as exception: | ||
| logger.info('Fyle token expired workspace_id - %s %s', kwargs['workspace_id'], {'error': exception.response}) | ||
| return Response(data={'message': 'Fyle token expired workspace_id'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except NoPrivilegeError as exception: | ||
| logger.info('Invalid Fyle Credentials / Admin is disabled for workspace_id%s %s', kwargs['workspace_id'], {'error': exception.response}) | ||
| return Response(data={'message': 'Invalid Fyle Credentials / Admin is disabled'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except RetryException: | ||
| logger.info('Fyle Retry Exception for workspace_id %s', kwargs['workspace_id']) | ||
| return Response(data={'message': 'Fyle API rate limit exceeded'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except Workspace.DoesNotExist: | ||
| return Response(data={'message': 'Workspace with this id does not exist'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except AdvancedSetting.DoesNotExist: | ||
| return Response(data={'message': 'Advanced Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except ExportSettings.DoesNotExist: | ||
| return Response({'message': 'Export Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| except Exception as exception: | ||
| logger.exception(exception) | ||
| return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| return new_fn | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
The handle_view_exceptions decorator function is comprehensive and covers a wide range of exceptions. However, consider adding specific HTTP status codes for different exceptions to differentiate the errors more clearly.
- return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_400_BAD_REQUEST)
+ return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_500_INTERNAL_SERVER_ERROR)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def handle_view_exceptions(): | |
| def decorator(func): | |
| def new_fn(*args, **kwargs): | |
| try: | |
| return func(*args, **kwargs) | |
| except AccountingExport.DoesNotExist: | |
| return Response(data={'message': 'AccountingExport not found'}, status=status.HTTP_400_BAD_REQUEST) | |
| except FyleCredential.DoesNotExist: | |
| return Response(data={'message': 'Fyle credentials not found in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except FyleInvalidTokenError as exception: | |
| logger.info('Fyle token expired workspace_id - %s %s', kwargs['workspace_id'], {'error': exception.response}) | |
| return Response(data={'message': 'Fyle token expired workspace_id'}, status=status.HTTP_400_BAD_REQUEST) | |
| except NoPrivilegeError as exception: | |
| logger.info('Invalid Fyle Credentials / Admin is disabled for workspace_id%s %s', kwargs['workspace_id'], {'error': exception.response}) | |
| return Response(data={'message': 'Invalid Fyle Credentials / Admin is disabled'}, status=status.HTTP_400_BAD_REQUEST) | |
| except RetryException: | |
| logger.info('Fyle Retry Exception for workspace_id %s', kwargs['workspace_id']) | |
| return Response(data={'message': 'Fyle API rate limit exceeded'}, status=status.HTTP_400_BAD_REQUEST) | |
| except Workspace.DoesNotExist: | |
| return Response(data={'message': 'Workspace with this id does not exist'}, status=status.HTTP_400_BAD_REQUEST) | |
| except AdvancedSetting.DoesNotExist: | |
| return Response(data={'message': 'Advanced Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except ExportSettings.DoesNotExist: | |
| return Response({'message': 'Export Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except Exception as exception: | |
| logger.exception(exception) | |
| return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_400_BAD_REQUEST) | |
| return new_fn | |
| return decorator | |
| def handle_view_exceptions(): | |
| def decorator(func): | |
| def new_fn(*args, **kwargs): | |
| try: | |
| return func(*args, **kwargs) | |
| except AccountingExport.DoesNotExist: | |
| return Response(data={'message': 'AccountingExport not found'}, status=status.HTTP_400_BAD_REQUEST) | |
| except FyleCredential.DoesNotExist: | |
| return Response(data={'message': 'Fyle credentials not found in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except FyleInvalidTokenError as exception: | |
| logger.info('Fyle token expired workspace_id - %s %s', kwargs['workspace_id'], {'error': exception.response}) | |
| return Response(data={'message': 'Fyle token expired workspace_id'}, status=status.HTTP_400_BAD_REQUEST) | |
| except NoPrivilegeError as exception: | |
| logger.info('Invalid Fyle Credentials / Admin is disabled for workspace_id%s %s', kwargs['workspace_id'], {'error': exception.response}) | |
| return Response(data={'message': 'Invalid Fyle Credentials / Admin is disabled'}, status=status.HTTP_400_BAD_REQUEST) | |
| except RetryException: | |
| logger.info('Fyle Retry Exception for workspace_id %s', kwargs['workspace_id']) | |
| return Response(data={'message': 'Fyle API rate limit exceeded'}, status=status.HTTP_400_BAD_REQUEST) | |
| except Workspace.DoesNotExist: | |
| return Response(data={'message': 'Workspace with this id does not exist'}, status=status.HTTP_400_BAD_REQUEST) | |
| except AdvancedSetting.DoesNotExist: | |
| return Response(data={'message': 'Advanced Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except ExportSettings.DoesNotExist: | |
| return Response({'message': 'Export Settings does not exist in workspace'}, status=status.HTTP_400_BAD_REQUEST) | |
| except Exception as exception: | |
| logger.exception(exception) | |
| return Response(data={'message': 'An unhandled error has occurred, please re-try later'}, status=status.HTTP_500_INTERNAL_SERVER_ERROR) | |
| return new_fn | |
| return decorator |
| def async_create_admin_subcriptions(workspace_id: int) -> None: | ||
| """ | ||
| Create admin subscriptions | ||
| :param workspace_id: workspace id | ||
| :return: None | ||
| """ | ||
| fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | ||
| platform = PlatformConnector(fyle_credentials) | ||
| payload = { | ||
| 'is_enabled': True, | ||
| 'webhook_url': '{}/workspaces/{}/fyle/webhook_callback/'.format(settings.API_URL, workspace_id) | ||
| } | ||
| platform.subscriptions.post(payload) |
There was a problem hiding this comment.
The function async_create_admin_subcriptions (note the typo in 'subscriptions') is well-implemented for creating admin subscriptions. However, consider using f-strings for better performance and readability in line 100.
- 'webhook_url': '{}/workspaces/{}/fyle/webhook_callback/'.format(settings.API_URL, workspace_id)
+ 'webhook_url': f'{settings.API_URL}/workspaces/{workspace_id}/fyle/webhook_callback/'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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def async_create_admin_subcriptions(workspace_id: int) -> None: | |
| """ | |
| Create admin subscriptions | |
| :param workspace_id: workspace id | |
| :return: None | |
| """ | |
| fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | |
| platform = PlatformConnector(fyle_credentials) | |
| payload = { | |
| 'is_enabled': True, | |
| 'webhook_url': '{}/workspaces/{}/fyle/webhook_callback/'.format(settings.API_URL, workspace_id) | |
| } | |
| platform.subscriptions.post(payload) | |
| def async_create_admin_subcriptions(workspace_id: int) -> None: | |
| """ | |
| Create admin subscriptions | |
| :param workspace_id: workspace id | |
| :return: None | |
| """ | |
| fyle_credentials = FyleCredential.objects.get(workspace_id=workspace_id) | |
| platform = PlatformConnector(fyle_credentials) | |
| payload = { | |
| 'is_enabled': True, | |
| 'webhook_url': f'{settings.API_URL}/workspaces/{workspace_id}/fyle/webhook_callback/' | |
| } | |
| platform.subscriptions.post(payload) |
Tools
Ruff
100-100: Use f-string instead of
formatcall (UP032)Convert to f-string
|
|
||
| payload = { | ||
| 'is_enabled': True, | ||
| 'webhook_url': '{}/workspaces/{}/fyle/webhook_callback/'.format(settings.API_URL, workspace_id) |
There was a problem hiding this comment.
In the test cases for async_create_admin_subscriptions, replace the format method with f-strings for string formatting to enhance readability and performance.
- 'webhook_url': '{}/workspaces/{}/fyle/webhook_callback/'.format(settings.API_URL, workspace_id)
+ 'webhook_url': f'{settings.API_URL}/workspaces/{workspace_id}/fyle/webhook_callback/'Also applies to: 122-122
Tools
Ruff
95-95: Use f-string instead of
formatcall (UP032)Convert to f-string
| @@ -184,5 +183,6 @@ def create(self, validated_data): | |||
| if workspace.onboarding_state == 'ADVANCED_SETTINGS': | |||
| workspace.onboarding_state = 'COMPLETE' | |||
| workspace.save() | |||
| async_task('apps.workspaces.tasks.async_create_admin_subcriptions', workspace_id) | |||
There was a problem hiding this comment.
The call to async_create_admin_subcriptions within AdvancedSettingSerializer is correctly placed to trigger after settings update. However, ensure the function name typo is corrected across the codebase.
- async_task('apps.workspaces.tasks.async_create_admin_subcriptions', workspace_id)
+ async_task('apps.workspaces.tasks.async_create_admin_subscriptions', workspace_id)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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async_task('apps.workspaces.tasks.async_create_admin_subcriptions', workspace_id) | |
| async_task('apps.workspaces.tasks.async_create_admin_subscriptions', workspace_id) |
| @@ -0,0 +1,13 @@ | |||
| # Create admin subscriptions for existing workspaces | |||
|
|
|||
| from apps.workspaces.tasks import async_create_admin_subcriptions | |||
There was a problem hiding this comment.
Correct the typo in the import statement for async_create_admin_subscriptions.
- from apps.workspaces.tasks import async_create_admin_subcriptions
+ from apps.workspaces.tasks import async_create_admin_subscriptionsCommittable 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.
| from apps.workspaces.tasks import async_create_admin_subcriptions | |
| from apps.workspaces.tasks import async_create_admin_subscriptions |
| try: | ||
| async_create_admin_subcriptions(workspace.id) | ||
| except Exception as e: | ||
| print('Error while creating admin subscriptions for workspace - {} with ID - {}'.format(workspace.name, workspace.id)) |
There was a problem hiding this comment.
Replace format with an f-string for better performance and readability.
- print('Error while creating admin subscriptions for workspace - {} with ID - {}'.format(workspace.name, workspace.id))
+ print(f'Error while creating admin subscriptions for workspace - {workspace.name} with ID - {workspace.id}')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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print('Error while creating admin subscriptions for workspace - {} with ID - {}'.format(workspace.name, workspace.id)) | |
| print(f'Error while creating admin subscriptions for workspace - {workspace.name} with ID - {workspace.id}') |
Tools
Ruff
12-12: Use f-string instead of
formatcall (UP032)Convert to f-string
Hrishabh17
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/codecov.yml (1 hunks)
- .github/workflows/tests.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/codecov.yml
Additional comments not posted (1)
.github/workflows/tests.yml (1)
19-19: Lowering the coverage threshold to 97% is noted. Ensure this aligns with the intended quality standards of the project.
Summary by CodeRabbit
New Features
Improvements
Configuration
API_URLconfiguration settings.Tests
Chores