Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: POST to integrations settings API on onboarding completion #123

Merged
merged 1 commit into from
Jan 29, 2025
+28 −2
Merged

feat: POST to integrations settings API on onboarding completion #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/fyle/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def post_request(url, body, refresh_token=None):
data=body
)

if response.status_code == 200:
if response.status_code in (200, 201):
return json.loads(response.text)
else:
raise Exception(response.text)
Expand Down
3 changes: 2 additions & 1 deletion apps/workspaces/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
from typing import Dict
from rest_framework import serializers
from apps.workspaces.tasks import post_to_integration_settings
from django_q.tasks import async_task
from django.core.cache import cache
from apps.mappings.models import QBDMapping
Expand Down Expand Up @@ -225,5 +226,5 @@ def create(self, validated_data):
workspace.onboarding_state = 'COMPLETE'
workspace.save()
async_task('apps.workspaces.tasks.async_create_admin_subcriptions', workspace_id)

post_to_integration_settings(workspace_id, True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Add error handling for the integration settings API call

The race condition concern can be dismissed as the integration settings update is independent of admin subscriptions. However, the error handling concern remains valid:

  • Consider wrapping post_to_integration_settings call in a try-except block to handle potential API failures gracefully
  • Log any errors to help with debugging integration issues
  • Optionally, notify the user if the integration settings update fails
🔗 Analysis chain

Consider error handling and execution order.

Two potential issues to consider:

  1. The function call could fail silently as exceptions are caught in the function. Consider handling errors at this level.
  2. The function is called after an async task which could lead to race conditions if the integration settings depend on the admin subscriptions.

Run this script to check if there are any dependencies between admin subscriptions and integration settings:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any code that shows dependencies between admin subscriptions and integration settings
rg -l "admin.*subscriptions.*integration.*settings|integration.*settings.*admin.*subscriptions" --type py

Length of output: 107

Script:

#!/bin/bash
# Find implementation of post_to_integration_settings
ast-grep --pattern 'def post_to_integration_settings'

# Find the async task call context
rg -B 5 -A 5 "post_to_integration_settings\(workspace_id, True\)" --type py

# Find any integration settings related code
rg "integration.*settings" --type py

Length of output: 1153

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for integration settings post.

Consider wrapping the call in a try-except block to prevent onboarding completion from failing if the integration settings post fails.

-            post_to_integration_settings(workspace_id, True)
+            try:
+                post_to_integration_settings(workspace_id, True)
+            except Exception as error:
+                logger.error(
+                    'Failed to post integration settings during onboarding completion for workspace %d: %s',
+                    workspace_id,
+                    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.

Suggested change
post_to_integration_settings(workspace_id, True)
try:
post_to_integration_settings(workspace_id, True)
except Exception as error:
logger.error(
'Failed to post integration settings during onboarding completion for workspace %d: %s',
workspace_id,
error
)

return advanced_setting
23 changes: 23 additions & 0 deletions apps/workspaces/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,3 +170,26 @@ def async_update_timestamp_in_qbd_direct(workspace_id: int) -> None:
raise Exception('Auth Token not present for workspace id {}'.format(workspace.id))
except Exception as e:
logger.error("Failed to sync timestamp to QBD Connector: {}".format(e))


def post_to_integration_settings(workspace_id: int, active: bool):
"""
Post to integration settings
"""
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 Quickbooks Desktop (IIF) Integration',
'type': 'ACCOUNTING',
'is_active': active,
'connected_at': datetime.now().strftime('%Y-%m-%dT%H:%M:%S.%fZ')
}

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)
Comment on lines +189 to +195
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and logging.

The error handling could be improved in several ways:

  1. The except clause is too broad - consider catching specific exceptions
  2. The error log should include the workspace_id for better debugging
     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 integration settings for workspace {workspace_id}: {error}')
+    except Workspace.DoesNotExist:
+        logger.error(f'Workspace {workspace_id} not found while posting integration settings')
+    except Exception as error:
+        logger.error(f'Unexpected error while posting integration settings for workspace {workspace_id}: {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.

Suggested change
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 integration settings for workspace {workspace_id}: {error}')
except Workspace.DoesNotExist:
logger.error(f'Workspace {workspace_id} not found while posting integration settings')
except Exception as error:
logger.error(f'Unexpected error while posting integration settings for workspace {workspace_id}: {error}')

1 change: 1 addition & 0 deletions quickbooks_desktop_api/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@
FYLE_TOKEN_URI = os.environ.get('FYLE_TOKEN_URI')
FYLE_CLIENT_ID = os.environ.get('FYLE_CLIENT_ID')
FYLE_CLIENT_SECRET = os.environ.get('FYLE_CLIENT_SECRET')
INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API')
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Environment variable INTEGRATIONS_SETTINGS_API needs proper documentation and default handling

The variable is used in production code but lacks proper setup guidance:

  • Add the variable to environment example files (.env.example)
  • Consider adding a default value or explicit error handling in os.environ.get('INTEGRATIONS_SETTINGS_API')
  • Document the expected value format since it's used to construct API URLs in apps/workspaces/tasks.py
🔗 Analysis chain

Verify environment variable setup across environments.

Ensure the new environment variable is properly configured.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if INTEGRATIONS_SETTINGS_API is documented and configured

# Check if variable is documented in README or similar files
echo "Checking documentation for INTEGRATIONS_SETTINGS_API..."
fd -t f -e md -e env -e env.example | xargs rg -i "INTEGRATIONS_SETTINGS_API"

# Check if variable is used in the codebase
echo "Checking usage in codebase..."
rg "INTEGRATIONS_SETTINGS_API" -t python

Length of output: 339

Script:

#!/bin/bash
# Search for the environment variable in Python files
echo "Checking Python files..."
rg "INTEGRATIONS_SETTINGS_API" -g "*.py"

# Search in environment and configuration files
echo -e "\nChecking configuration files..."
fd -e env -e env.example -e env.sample -e yaml -e yml | xargs rg -i "INTEGRATIONS_SETTINGS_API"

# Search for related settings patterns
echo -e "\nChecking for related patterns..."
rg "SETTINGS_API|INTEGRATION.*API" -g "*.py" -g "*.env*" -g "*.yml" -g "*.yaml"

Length of output: 1071


QBD_DIRECT_API_URL = os.environ.get('QBD_DIRECT_API_URL')

Expand Down
1 change: 1 addition & 0 deletions quickbooks_desktop_api/tests/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@
FYLE_TOKEN_URI = os.environ.get('FYLE_TOKEN_URI')
FYLE_CLIENT_ID = os.environ.get('FYLE_CLIENT_ID')
FYLE_CLIENT_SECRET = os.environ.get('FYLE_CLIENT_SECRET')
INTEGRATIONS_SETTINGS_API = os.environ.get('INTEGRATIONS_SETTINGS_API')

QBD_DIRECT_API_URL = os.environ.get('QBD_DIRECT_API_URL')

Expand Down
Loading