feat: POST to integrations settings API on onboarding completion #127
Conversation
(cherry picked from commit 8f9fe42)
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
WalkthroughThis pull request introduces changes across multiple files to enhance integration settings and request handling. The modifications include expanding HTTP status code handling in a helper function, adding a new function to post integration settings for workspaces, and introducing new environment variable configurations. The changes appear to be part of a broader effort to improve integration workflows and configuration management. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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: 2
🧹 Nitpick comments (2)
quickbooks_desktop_api/tests/settings.py (1)
249-249: Consider adding a default value for the environment variable.To prevent potential
Nonevalues in case the environment variable is not set, consider providing a default value.-INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API') +INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API', 'http://localhost:8000/api/v1')apps/workspaces/tasks.py (1)
186-186: Use ISO format method instead of strftime.Replace the hardcoded datetime format string with Python's built-in ISO format method.
- 'connected_at': datetime.now().strftime('%Y-%m-%dT%H:%M:%S.%fZ') + 'connected_at': datetime.now(timezone.utc).isoformat()
📜 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(2 hunks)apps/workspaces/tasks.py(1 hunks)quickbooks_desktop_api/settings.py(1 hunks)quickbooks_desktop_api/tests/settings.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (3)
apps/fyle/helpers.py (1)
31-31: LGTM! Enhanced status code handling.The addition of HTTP 201 status code handling aligns with REST API standards for resource creation responses.
apps/workspaces/serializers.py (1)
229-229: LGTM! Well-placed integration call.The integration settings update is correctly placed after the workspace onboarding is complete.
quickbooks_desktop_api/settings.py (1)
271-271: Verify environment variable configuration.The new environment variable
INTEGRATIONS_SETTINGS_APIis added without a default value. This could cause issues if the variable is not set in the environment.Run the following script to check if this variable is properly configured in deployment files and documentation:
Consider adding a default value or documenting that this is a required environment variable:
-INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API') +# Required for integration settings functionality +INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API', None) +if not INTEGRATIONS_SETTINGS_API: + raise ValueError('INTEGRATIONS_SETTINGS_API environment variable is required')
apps/workspaces/tasks.py
Outdated
| try: | ||
| post_request(url, json.dumps(payload), refresh_token) | ||
| org_id = Workspace.objects.get(id=workspace_id).org_id | ||
| logger.info(f'New integration record: Fyle Quickbooks Desktop (IIF) Integration (ACCOUNTING) | {workspace_id = } | {org_id = }') | ||
|
|
||
| except Exception as error: | ||
| logger.error(error) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with specific exceptions.
The current error handling is too broad. Consider catching specific exceptions and providing more context in the error message.
try:
post_request(url, json.dumps(payload), refresh_token)
org_id = Workspace.objects.get(id=workspace_id).org_id
logger.info(f'New integration record: Fyle Quickbooks Desktop (IIF) Integration (ACCOUNTING) | {workspace_id = } | {org_id = }')
- except Exception as error:
- logger.error(error)
+ except requests.RequestException as error:
+ logger.error(f"Failed to post to integration settings API: {error}")
+ raise
+ except Workspace.DoesNotExist:
+ logger.error(f"Workspace not found with id: {workspace_id}")
+ raise
+ except Exception as error:
+ logger.error(f"Unexpected error while posting to integration settings: {error}")
+ raise📝 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.
| try: | |
| post_request(url, json.dumps(payload), refresh_token) | |
| org_id = Workspace.objects.get(id=workspace_id).org_id | |
| logger.info(f'New integration record: Fyle Quickbooks Desktop (IIF) Integration (ACCOUNTING) | {workspace_id = } | {org_id = }') | |
| except Exception as error: | |
| logger.error(error) | |
| try: | |
| post_request(url, json.dumps(payload), refresh_token) | |
| org_id = Workspace.objects.get(id=workspace_id).org_id | |
| logger.info(f'New integration record: Fyle Quickbooks Desktop (IIF) Integration (ACCOUNTING) | {workspace_id = } | {org_id = }') | |
| except requests.RequestException as error: | |
| logger.error(f"Failed to post to integration settings API: {error}") | |
| raise | |
| except Workspace.DoesNotExist: | |
| logger.error(f"Workspace not found with id: {workspace_id}") | |
| raise | |
| except Exception as error: | |
| logger.error(f"Unexpected error while posting to integration settings: {error}") | |
| raise |
| def post_to_integration_settings(workspace_id: int, active: bool): | ||
| """ | ||
| Post to integration settings | ||
| """ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for workspace_id.
The function should validate that the workspace exists before proceeding.
def post_to_integration_settings(workspace_id: int, active: bool):
"""
Post to integration settings
"""
+ workspace = Workspace.objects.get(id=workspace_id)
+ if not workspace:
+ raise ValueError(f"Workspace not found with id: {workspace_id}")Committable suggestion skipped: line range outside the PR's diff.
JustARatherRidiculouslyLongUsername
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
scripts/python/003_create_integration_records.py (3)
1-6: Enhance logging configuration and add documentation.Consider improving the script's setup:
- Add a docstring explaining the script's purpose and usage
- Use proper logging configuration with file handler
+""" +Backfill script to process workspaces with completed onboarding state +and post their integration settings. + +Usage: + python scripts/python/003_create_integration_records.py +""" import logging from apps.workspaces.models import Workspace from apps.workspaces.tasks import post_to_integration_settings -logger = logging.getLogger(__name__) -logger.level = logging.INFO +logging.basicConfig( + level=logging.INFO, + format='%(asctime)s - %(name)s - %(levelname)s - %(message)s', + handlers=[ + logging.FileHandler('integration_records_backfill.log'), + logging.StreamHandler() + ] +) +logger = logging.getLogger(__name__)
8-8: LGTM! Consider adding a comment for clarity.The counter initialization is correct, but could benefit from a brief comment explaining their purpose.
+# Counters to track processing results processed = failed = 0
23-28: Enhance summary logging with timing information.Consider adding execution duration and structuring the summary for better readability.
+import time + +start_time = time.time() # ... processing ... +duration = time.time() - start_time logger.info( -f""" -Completed backfill. Total: {workspaces.count()} -Processed: {processed}, Failed: {failed} -""" + "Backfill Summary:\n" + f"Total workspaces: {total_count}\n" + f"Successfully processed: {processed}\n" + f"Failed: {failed}\n" + f"Duration: {duration:.2f} seconds" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/workspaces/tasks.py(1 hunks)scripts/python/003_create_integration_records.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/workspaces/tasks.py
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (1)
scripts/python/003_create_integration_records.py (1)
3-3: Verify integration settings API endpoint configuration.Ensure that the integration settings API endpoint is properly configured and accessible.
✅ Verification successful
Integration Settings API Endpoint Verified.
- The function
post_to_integration_settingsis defined inapps/workspaces/tasks.pyand correctly utilizessettings.INTEGRATIONS_SETTINGS_API.- The
INTEGRATIONS_SETTINGS_APIvariable is set inquickbooks_desktop_api/settings.py(and in tests), ensuring that the API endpoint is configured via an environment variable.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration settings API endpoint configuration # Search for the API endpoint definition rg -l "post_to_integration_settings" # Check for any environment variables or configuration related to the integration settings API rg -l "INTEGRATION_SETTINGS.*URL|INTEGRATION.*API.*URL" --type ini --type envLength of output: 249
Script:
#!/bin/bash # Verify the integration settings API endpoint configuration by checking its definition and related settings. # 1. Check for the function definition in apps/workspaces/tasks.py (with some context lines) echo "Searching for the 'post_to_integration_settings' function definition in apps/workspaces/tasks.py:" rg -A 10 "def post_to_integration_settings" apps/workspaces/tasks.py echo "" # 2. Search the entire repository for integration configuration environment variables without file-type restrictions echo "Searching for integration settings configuration (environment variables or URLs):" rg "INTEGRATION_SETTINGS.*URL|INTEGRATION.*API.*URL" .Length of output: 981
Script:
#!/bin/bash # Description: Further verify the integration settings API endpoint configuration by checking # for the "INTEGRATIONS_SETTINGS_API" variable referenced in the tasks file within the settings. echo "Searching for the 'INTEGRATIONS_SETTINGS_API' setting in the repository:" rg "INTEGRATIONS_SETTINGS_API" .Length of output: 516
| workspaces = Workspace.objects.filter(onboarding_state='COMPLETE') | ||
| for workspace in workspaces: | ||
| try: | ||
| logger.info(f"Processing workspace: {workspace.id} | {workspace.name}") | ||
| 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 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance robustness and performance of workspace processing.
The current implementation could benefit from several improvements:
- Batch processing to handle large datasets efficiently
- Progress tracking for long-running operations
- Retry mechanism for transient failures
-workspaces = Workspace.objects.filter(onboarding_state='COMPLETE')
-for workspace in workspaces:
+BATCH_SIZE = 100
+total_count = Workspace.objects.filter(onboarding_state='COMPLETE').count()
+for offset in range(0, total_count, BATCH_SIZE):
+ workspaces = Workspace.objects.filter(
+ onboarding_state='COMPLETE'
+ )[offset:offset + BATCH_SIZE]
+ logger.info(f"Processing batch {offset//BATCH_SIZE + 1} of {(total_count + BATCH_SIZE - 1)//BATCH_SIZE}")
+ for workspace in workspaces:
+ retries = 3
+ while retries > 0:
try:
logger.info(f"Processing workspace: {workspace.id} | {workspace.name}")
post_to_integration_settings(workspace.id, True)
processed += 1
+ break
except Exception as e:
- failed += 1
- logger.error(
- f"Failed to process workspace {workspace.id}: {str(e)}",
- exc_info=True
- )
+ retries -= 1
+ if retries == 0:
+ failed += 1
+ logger.error(
+ f"Failed to process workspace {workspace.id} after 3 attempts: {str(e)}",
+ exc_info=True
+ )
+ else:
+ logger.warning(
+ f"Retry {3-retries}/3 for workspace {workspace.id}: {str(e)}"
+ )📝 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.
| workspaces = Workspace.objects.filter(onboarding_state='COMPLETE') | |
| for workspace in workspaces: | |
| try: | |
| logger.info(f"Processing workspace: {workspace.id} | {workspace.name}") | |
| 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 | |
| ) | |
| BATCH_SIZE = 100 | |
| total_count = Workspace.objects.filter(onboarding_state='COMPLETE').count() | |
| for offset in range(0, total_count, BATCH_SIZE): | |
| workspaces = Workspace.objects.filter( | |
| onboarding_state='COMPLETE' | |
| )[offset:offset + BATCH_SIZE] | |
| logger.info(f"Processing batch {offset//BATCH_SIZE + 1} of {(total_count + BATCH_SIZE - 1)//BATCH_SIZE}") | |
| for workspace in workspaces: | |
| retries = 3 | |
| while retries > 0: | |
| try: | |
| logger.info(f"Processing workspace: {workspace.id} | {workspace.name}") | |
| post_to_integration_settings(workspace.id, True) | |
| processed += 1 | |
| break | |
| except Exception as e: | |
| retries -= 1 | |
| if retries == 0: | |
| failed += 1 | |
| logger.error( | |
| f"Failed to process workspace {workspace.id} after 3 attempts: {str(e)}", | |
| exc_info=True | |
| ) | |
| else: | |
| logger.warning( | |
| f"Retry {3-retries}/3 for workspace {workspace.id}: {str(e)}" | |
| ) |
(cherry picked from commit 8f9fe42)
Clickup
https://app.clickup.com/t/86cxm0f86
Summary by CodeRabbit
Release Notes
New Features
Configuration Updates
Improvements
These changes improve API interaction robustness and extend workspace configuration capabilities.