feat: POST to integrations settings API on onboarding completion #243
Conversation
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThe pull request introduces enhancements to the workspace integration settings and HTTP request handling. A modification in the Fyle helpers expands successful HTTP response criteria, while the workspaces module adds a new trigger for posting integration settings. The changes include updating the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
🧹 Nitpick comments (3)
apps/workspaces/triggers.py (2)
16-17: Consider using getLogger with nameThe logger configuration could be improved by using
__name__instead of hardcoding the logger name.-logger = logging.getLogger(__name__) -logger.level = logging.INFO +logger = logging.getLogger(__name__)Note: Let the logging configuration in settings.py handle the log level.
80-101: Add error details to log messageThe error handling could be improved by including more details in the error log.
except Exception as error: - logger.error(error) + logger.error('Failed to post integration settings for workspace %s - %s', workspace_id, str(error))Also, consider adding type hints for better code maintainability:
@staticmethod - def post_to_integration_settings(workspace_id: int, active: bool): + def post_to_integration_settings(workspace_id: int, active: bool) -> None:apps/workspaces/serializers.py (1)
429-429: Consider handling potential exceptionsThe call to
post_to_integration_settingscould fail, but the exception handling is delegated to the trigger method. Consider wrapping this call in a try-except block to ensure workspace completion isn't affected by integration settings failures.workspace.save() async_task('apps.workspaces.tasks.async_create_admin_subcriptions', workspace.id) - AdvancedSettingsTriggers.post_to_integration_settings(workspace_id, True) + try: + AdvancedSettingsTriggers.post_to_integration_settings(workspace_id, True) + except Exception as error: + logger.error('Failed to post integration settings during workspace completion: %s', str(error))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/fyle/helpers.py(1 hunks)apps/workspaces/serializers.py(1 hunks)apps/workspaces/triggers.py(2 hunks)sage_desktop_api/settings.py(1 hunks)sage_desktop_api/tests/settings.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (3)
sage_desktop_api/tests/settings.py (1)
181-181: LGTM!The addition of the INTEGRATIONS_SETTINGS_API environment variable is appropriate and follows the existing pattern in the settings file.
sage_desktop_api/settings.py (1)
259-259: LGTM!The addition of the INTEGRATIONS_SETTINGS_API environment variable is consistent with the test settings and follows the project's environment variable pattern.
apps/fyle/helpers.py (1)
129-129: LGTM! The status code check enhancement is appropriate.The change to accept both 200 (OK) and 201 (Created) status codes is a good improvement, as it properly handles the standard HTTP response codes for successful POST requests. This aligns well with the PR's objective of posting to the integrations settings API.
apps/workspaces/triggers.py
Outdated
| } | ||
|
|
||
| try: | ||
| post_request(url, json.dumps(payload), refresh_token) |
There was a problem hiding this comment.
we have update post_request behaviour
Remove json.dumps it is handled inside post_request
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
* feat: backfill integration records * fix: add error handling and fail count to output
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
This behaviour was changed in #245
JustARatherRidiculouslyLongUsername
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
scripts/python/002_backfill_integrations.py (3)
1-7: Add docstring and type hints for better code documentation.Consider adding a module-level docstring explaining the purpose of this script and type hints for better code maintainability.
+""" +Backfill script to post integration settings for workspaces with completed onboarding. + +This script processes all workspaces with 'COMPLETE' onboarding state and posts their +integration settings to the integration settings API. +""" import logging from apps.workspaces.models import Workspace from apps.workspaces.triggers import AdvancedSettingsTriggers
10-21: Consider implementing batch processing for better performance.For large datasets, processing all workspaces at once could be memory-intensive. Consider implementing batch processing.
-workspaces = Workspace.objects.filter(onboarding_state='COMPLETE') -for workspace in workspaces: +BATCH_SIZE = 100 +offset = 0 + +while True: + workspaces = Workspace.objects.filter( + onboarding_state='COMPLETE' + ).order_by('id')[offset:offset + BATCH_SIZE] + + if not workspaces: + break + + for workspace in workspaces: try: logger.info(f"Processing workspace: {workspace.id} | {workspace.name}") AdvancedSettingsTriggers.post_to_integration_settings(workspace.id, True) processed += 1 except Exception as e: failed += 1 logger.error( f"Failed to process workspace {workspace.id}: {str(e)}", exc_info=True ) + + offset += BATCH_SIZE
23-28: Add more detailed logging for better observability.The summary could include more details about the failures and processing time.
+start_time = datetime.now() logger.info( f""" Completed backfill. Total: {workspaces.count()} Processed: {processed}, Failed: {failed} +Time taken: {datetime.now() - start_time} +Success rate: {(processed / workspaces.count() * 100):.2f}% """ )apps/workspaces/triggers.py (1)
80-102: Enhance error handling and documentation.The method could benefit from:
- More specific error handling
- Type hints in docstring
- Constants for hardcoded values
@staticmethod def post_to_integration_settings(workspace_id: int, active: bool): """ Post to integration settings + + Args: + workspace_id (int): The ID of the workspace + active (bool): Flag indicating if the integration is active + + Raises: + FyleCredential.DoesNotExist: If credentials are not found + requests.RequestException: If the API request fails """ + INTEGRATION_TYPE = 'ACCOUNTING' + INTEGRATION_NAME = 'Fyle Sage 300 Integration' + refresh_token = FyleCredential.objects.get(workspace_id=workspace_id).refresh_token url = '{}/integrations/'.format(settings.INTEGRATIONS_SETTINGS_API) payload = { 'tpa_id': settings.FYLE_CLIENT_ID, - 'tpa_name': 'Fyle Sage 300 Integration', - 'type': 'ACCOUNTING', + 'tpa_name': INTEGRATION_NAME, + 'type': INTEGRATION_TYPE, 'is_active': active, 'connected_at': datetime.now().strftime('%Y-%m-%dT%H:%M:%S.%fZ') } try: post_request(url, payload, refresh_token) org_id = Workspace.objects.get(id=workspace_id).org_id logger.info(f'New integration record: {INTEGRATION_NAME} ({INTEGRATION_TYPE}) | {workspace_id = } | {org_id = }') - except Exception as error: + except FyleCredential.DoesNotExist as error: + logger.error(f'Credentials not found for workspace {workspace_id}: {error}') + raise + except requests.RequestException as error: logger.error(error) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/workspaces/triggers.py(2 hunks)scripts/python/002_backfill_integrations.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
apps/workspaces/triggers.py
2-2: json imported but unused
Remove unused import: json
(F401)
apps/workspaces/triggers.py
Outdated
| from datetime import datetime | ||
| import json |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused import.
The json import is not being used in this file.
from datetime import datetime
-import json
import logging📝 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.
| from datetime import datetime | |
| import json | |
| from datetime import datetime | |
| import logging |
🧰 Tools
🪛 Ruff (0.8.2)
2-2: json imported but unused
Remove unused import: json
(F401)
JustARatherRidiculouslyLongUsername
deployed
to
CI Environment
GitHub Actions
Active
|
Clickup
https://app.clickup.com/t/86cxm0f86
Summary by CodeRabbit
Release Notes
New Features
Configuration
INTEGRATIONS_SETTINGS_APIfor integration settings configuration.Backend Improvements
These changes enhance system flexibility and integration capabilities with minimal user-facing impact.