feat: Enhance Sage Intacct credential handling #699
Conversation
- Introduced `invalidate_sage_intacct_credentials` utility to manage expired credentials. - Updated exception handling in various modules to utilize the new utility for `InvalidTokenError`. - Added `is_expired` field to `SageIntacctCredential` model to track credential status. - Adjusted logging and error messages for clarity and consistency across the application. - Added migration for the new `is_expired` field in the database.
WalkthroughThe changes introduce explicit handling and invalidation of expired or invalid Sage Intacct credentials across exception handlers in multiple modules. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant View/Task
participant SageIntacctCredential
participant Utils
participant IntegrationSettings
User->>View/Task: Initiate Sage Intacct operation
View/Task->>SageIntacctCredential: Retrieve credentials
alt Credentials missing
View/Task->>User: Return error (credentials missing)
else Credentials found
View/Task->>Sage Intacct: Perform operation
alt InvalidTokenError raised
View/Task->>Utils: invalidate_sage_intacct_credentials(workspace_id, credentials)
Utils->>SageIntacctCredential: Set is_expired = True
Utils->>IntegrationSettings: patch_integration_settings(is_token_expired=True)
View/Task->>User: Return error (invalid token)
else NoPrivilegeError raised
View/Task->>User: Return error (no privilege)
else Success
View/Task->>User: Return success
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (1)
fyle_intacct_api/utils.py (1)
30-38: Enhance error handling and add input validation.The function logic is correct, but consider improving robustness:
- Add workspace_id validation
- Handle potential database errors
- Consider returning success status for verification
def invalidate_sage_intacct_credentials(workspace_id, sage_intacct_credentials=None): + if not workspace_id: + raise ValueError("workspace_id is required") + if not sage_intacct_credentials: sage_intacct_credentials = SageIntacctCredential.objects.filter(workspace_id=workspace_id, is_expired=False).first() if sage_intacct_credentials: - if not sage_intacct_credentials.is_expired: - patch_integration_settings(workspace_id, is_token_expired=True) - sage_intacct_credentials.is_expired = True - sage_intacct_credentials.save() + try: + if not sage_intacct_credentials.is_expired: + patch_integration_settings(workspace_id, is_token_expired=True) + sage_intacct_credentials.is_expired = True + sage_intacct_credentials.save() + return True + except Exception as e: + logger.error(f"Failed to invalidate credentials for workspace {workspace_id}: {e}") + return False + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/mappings/exceptions.py(2 hunks)apps/mappings/tasks.py(2 hunks)apps/sage_intacct/dependent_fields.py(2 hunks)apps/sage_intacct/helpers.py(1 hunks)apps/sage_intacct/tasks.py(8 hunks)apps/sage_intacct/views.py(4 hunks)apps/workspaces/migrations/0045_sageintacctcredential_is_expired.py(1 hunks)apps/workspaces/models.py(1 hunks)fyle_intacct_api/utils.py(2 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(5 hunks)tests/test_mappings/test_exceptions.py(2 hunks)tests/test_sageintacct/test_tasks.py(1 hunks)tests/test_sageintacct/test_views.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/sage_intacct/tasks.py (2)
fyle_intacct_api/utils.py (1)
invalidate_sage_intacct_credentials(30-38)apps/workspaces/models.py (1)
SageIntacctCredential(166-181)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (32)
apps/workspaces/migrations/0045_sageintacctcredential_is_expired.py (1)
1-19: LGTM! Well-structured Django migration.The migration correctly adds the
is_expiredBooleanField to theSageIntacctCredentialmodel with appropriate default value and descriptive help text. The migration follows Django conventions and dependencies are properly set.apps/workspaces/models.py (1)
175-175: LGTM! Appropriate field addition for credential expiry tracking.The
is_expiredBooleanField is well-designed with a sensible default value ofFalseand clear help text. This field will enable proper tracking of credential expiration status as part of the enhanced credential management system.tests/test_sageintacct/test_views.py (2)
195-195: Improved error messaging specificity.The updated error message now includes the specific workspace_id, making it easier to identify which workspace has missing credentials. This change aligns with the enhanced error handling improvements.
219-219: Consistent error messaging pattern.The error message follows the same improved pattern as the sync_dimensions test, providing specific workspace identification for better debugging and user experience.
tests/test_mappings/test_exceptions.py (2)
79-79: Enhanced error specificity for InvalidTokenError.The updated error message provides clear distinction between invalid token errors and missing credentials, improving debugging capabilities. The workspace_id inclusion helps identify the specific workspace affected.
91-91: Clear separation of credential existence vs validity errors.The specific error message for missing credentials helps distinguish between scenarios where credentials don't exist versus when they're invalid/expired. This granular error handling supports better user experience and debugging.
fyle_intacct_api/utils.py (1)
3-4: LGTM! Clean import additions.The imports are appropriately added for the new utility function.
apps/mappings/tasks.py (2)
13-13: LGTM! Import addition for credential invalidation utility.The import is correctly added to support the new credential invalidation logic.
112-117:✅ Verification successful
Excellent separation of exception handling logic.
The split exception handling provides better granularity:
SageIntacctCredential.DoesNotExist: Logs missing credentialsInvalidTokenError: Properly invalidates credentials before loggingThis ensures proper cleanup of invalid tokens while maintaining clear error messaging.
Let me verify the consistency of this pattern across the codebase:
🏁 Script executed:
#!/bin/bash # Description: Verify that invalidate_sage_intacct_credentials is called consistently for InvalidTokenError across the codebase # Expected: All InvalidTokenError handlers should call the invalidation function rg -A 2 -B 2 "except InvalidTokenError" --type pyLength of output: 5387
InvalidTokenError Cleanup Verified Across Sage Intacct Integration
Allexcept InvalidTokenErrorblocks in the Sage Intacct code paths invokeinvalidate_sage_intacct_credentials(...)before logging, ensuring consistent cleanup of invalid tokens:
- apps/mappings/tasks.py
- apps/mappings/exceptions.py
- apps/sage_intacct/views.py
- apps/sage_intacct/dependent_fields.py
- apps/sage_intacct/tasks.py
Excellent work maintaining this pattern!
tests/test_sageintacct/test_tasks.py (1)
672-681: Great test coverage for enhanced exception scenarios.The test properly covers the new exception handling patterns:
SageIntacctCredential.DoesNotExist: Verifies FAILED statusNoPrivilegeError: Verifies FAILED statusThe test setup with
updated_attimestamp manipulation is appropriate for simulating the exception scenarios.apps/mappings/exceptions.py (2)
18-18: LGTM! Consistent import for credential invalidation.The import follows the same pattern used across other modules in this PR.
52-61: Excellent consistency with refined exception handling.The split exception handling matches the pattern established across the codebase:
SageIntacctCredential.DoesNotExist: Clear error messaging for missing credentialsInvalidTokenError: Proper credential invalidation followed by error loggingBoth maintain appropriate alert levels (False) and status (FAILED) for these scenarios.
apps/sage_intacct/helpers.py (2)
117-117: Good improvement to function signature design.Changing the default value of
errorsfrom0toNoneimproves the function's design by making it clearer when no error count should be included in the payload. This aligns well with the conditional payload construction logic.
124-132: LGTM! Clean conditional payload construction.The payload construction logic is well-designed:
- The
tpa_nameis always included as a constanterrors_countandis_token_expiredare only included when their values are notNoneThis approach prevents sending unnecessary or default values in the API payload, making the integration more efficient and cleaner.
apps/sage_intacct/dependent_fields.py (2)
12-12: Import addition supports the enhanced credential handling.The import of
invalidate_sage_intacct_credentialsis correctly placed and necessary for the improved exception handling logic below.
385-391: Excellent refinement of exception handling logic.The separation of exception handling is a significant improvement:
SageIntacctCredential.DoesNotExist: Appropriately handles missing credentials with a specific error messageInvalidTokenError: Correctly callsinvalidate_sage_intacct_credentials(workspace_id)to mark credentials as expired, then logs the specific errorThis change ensures that invalid tokens trigger the proper credential invalidation workflow, which aligns with the broader enhancement described in the PR objectives.
tests/sql_fixtures/reset_db_fixtures/reset_db.sql (4)
5-7: The version bump frompg_dump version 15.12to15.13is purely informational and has no impact on the fixture’s behavior.
5963-5963: Confirm migration registration
The new record0045_sageintacctcredential_is_expiredis correctly appended todjango_migrations. Ensure this lines up with your filesystem migration numbering.
9840-9841: Ensure COPY column alignment
TheCOPYstatement now includesworkspace_idandis_expired, matching the updated table schema. Verify the column order is correct to avoid data misalignment.
10003-10003: Update sequence to reflect new migration
Thedjango_migrations_id_seqis properly set to235after adding the new migration entry.apps/sage_intacct/views.py (4)
16-16: LGTM: Proper import addition.The import for
invalidate_sage_intacct_credentialsis correctly added to support the new credential invalidation functionality.
223-223: LGTM: Credential assignment for exception handling.The
sage_intacct_credentialsvariable is properly assigned to be available for theInvalidTokenErrorexception handler. This ensures the variable is in scope when needed for credential invalidation.
234-250: LGTM: Improved granular exception handling.The separation of
SageIntacctCredential.DoesNotExistandInvalidTokenErrorinto distinct exception blocks is an excellent improvement. This allows for:
- Specific error messages for each scenario
- Proper credential invalidation when tokens are invalid
- Better logging and user feedback
The call to
invalidate_sage_intacct_credentialsis correctly placed and properly parameterized.
281-297: LGTM: Consistent exception handling pattern.The exception handling pattern is consistently applied in the second POST method, maintaining the same structure and logic as the first method. This ensures uniform behavior across the API endpoints.
apps/sage_intacct/tasks.py (8)
14-14: LGTM: Consistent import addition.The import for
invalidate_sage_intacct_credentialsis correctly added, maintaining consistency with the views module.
725-737: LGTM: Proper exception separation in journal entry creation.The separation of
NoPrivilegeErrorandInvalidTokenErroris well-implemented:
NoPrivilegeErrormaintains existing error handling behaviorInvalidTokenErrorproperly invalidates credentials before handling the error- Variable scope is correct as
sage_intacct_credentialsis available from line 622
900-911: LGTM: Consistent pattern in expense report creation.The exception handling follows the same pattern as journal entry creation, ensuring consistent behavior across different export types.
1059-1070: LGTM: Consistent pattern in bill creation.The exception handling maintains the same structure and logic, providing uniform credential invalidation behavior.
1219-1230: LGTM: Consistent pattern in charge card transaction creation.The exception handling follows the established pattern, ensuring comprehensive coverage across all export types.
1423-1428: LGTM: Proper credential invalidation in AP payment creation.The
InvalidTokenErrorhandling correctly invalidates credentials using the proper workspace ID fromtask_log.workspace_idand the credentials retrieved earlier in the function.
1554-1560: LGTM: Proper credential invalidation in reimbursement creation.The exception handling follows the same pattern as AP payment creation, maintaining consistency in error handling behavior.
1623-1629: LGTM: Excellent exception handling restructure.The updated exception handling in
check_sage_intacct_object_statusis well-structured:
- Combines
SageIntacctCredential.DoesNotExistandNoPrivilegeErroras they both result in early return- Separates
InvalidTokenErrorfor proper credential invalidation- Variable scope is correct since
InvalidTokenErrorcan only occur after successful credential retrieval
| workspace_id integer NOT NULL, | ||
| is_expired boolean NOT NULL | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a default value for is_expired
To ensure new inserts without an explicit is_expired value default to false (matching the Django migration), update the column definition as follows:
- is_expired boolean NOT NULL
+ is_expired boolean NOT NULL DEFAULT false📝 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.
| workspace_id integer NOT NULL, | |
| is_expired boolean NOT NULL | |
| ); | |
| workspace_id integer NOT NULL, | |
| is_expired boolean NOT NULL DEFAULT false | |
| ); |
🤖 Prompt for AI Agents
In tests/sql_fixtures/reset_db_fixtures/reset_db.sql around lines 3609 to 3611,
the column is_expired lacks a default value, which can cause issues when
inserting rows without specifying this field. Modify the is_expired column
definition to include a default value of false by adding "DEFAULT false" to its
declaration, ensuring consistency with the Django migration and preventing null
or unexpected values.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #699 +/- ##
==========================================
- Coverage 93.04% 91.99% -1.06%
==========================================
Files 66 66
Lines 6515 6805 +290
==========================================
+ Hits 6062 6260 +198
- Misses 453 545 +92
🚀 New features to boost your workflow:
|
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_sageintacct/test_utils.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_sageintacct/test_utils.py
1531-1531: Avoid equality comparisons to True; use if kwargs['is_token_expired']: for truth checks
Replace with kwargs['is_token_expired']
(E712)
1535-1535: Avoid equality comparisons to True; use if sage_intacct_credentials.is_expired: for truth checks
Replace with sage_intacct_credentials.is_expired
(E712)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (1)
tests/test_sageintacct/test_utils.py (1)
16-16: LGTM: Import statement correctly added.The import for
invalidate_sage_intacct_credentialsis properly structured and necessary for the new test function.
| def test_invalidate_sage_intacct_credentials(mocker, db): | ||
| """ | ||
| Test invalidate sage intacct credentials | ||
| """ | ||
| workspace_id = 1 | ||
| sage_intacct_credentials = SageIntacctCredential.objects.filter(workspace_id=workspace_id, is_expired=False).first() | ||
|
|
||
| mocked_patch = mocker.MagicMock() | ||
| mocker.patch('fyle_intacct_api.utils.patch_integration_settings', side_effect=mocked_patch) | ||
|
|
||
| # Should not fail if sage_intacct_credentials was not found | ||
| sage_intacct_credentials.delete() | ||
| invalidate_sage_intacct_credentials(workspace_id) | ||
| assert not mocked_patch.called | ||
|
|
||
| # Should not call patch_integration_settings if sage_intacct_credentials.is_expired is True | ||
| sage_intacct_credentials.is_expired = True | ||
| sage_intacct_credentials.save() | ||
| invalidate_sage_intacct_credentials(workspace_id) | ||
| assert not mocked_patch.called | ||
|
|
||
| # Should call patch_integration_settings with the correct arguments if sage_intacct_credentials.is_expired is False | ||
| sage_intacct_credentials.is_expired = False | ||
| sage_intacct_credentials.save() | ||
|
|
||
| invalidate_sage_intacct_credentials(workspace_id) | ||
|
|
||
| args, kwargs = mocked_patch.call_args | ||
| assert args[0] == workspace_id | ||
| assert kwargs['is_token_expired'] == True | ||
|
|
||
| # Verify the credentials were marked as expired | ||
| sage_intacct_credentials.refresh_from_db() | ||
| assert sage_intacct_credentials.is_expired == True |
There was a problem hiding this comment.
Fix logical error and improve code style.
The test has a critical logical error and some style issues:
-
Critical Issue: After deleting the credential object on line 1513, the test tries to access it again on line 1517, which will cause an
AttributeError. -
Style Issues: Lines 1531 and 1535 use explicit
== Truecomparisons, which is not Pythonic.
Apply this diff to fix the logical error and style issues:
def test_invalidate_sage_intacct_credentials(mocker, db):
"""
Test invalidate sage intacct credentials
"""
workspace_id = 1
- sage_intacct_credentials = SageIntacctCredential.objects.filter(workspace_id=workspace_id, is_expired=False).first()
mocked_patch = mocker.MagicMock()
mocker.patch('fyle_intacct_api.utils.patch_integration_settings', side_effect=mocked_patch)
# Should not fail if sage_intacct_credentials was not found
+ sage_intacct_credentials = SageIntacctCredential.objects.filter(workspace_id=workspace_id, is_expired=False).first()
sage_intacct_credentials.delete()
invalidate_sage_intacct_credentials(workspace_id)
assert not mocked_patch.called
# Should not call patch_integration_settings if sage_intacct_credentials.is_expired is True
+ sage_intacct_credentials = SageIntacctCredential.objects.create(
+ workspace_id=workspace_id,
+ si_user_id='test_user',
+ si_company_id='test_company',
+ si_user_password='test_password',
+ is_expired=True
+ )
- sage_intacct_credentials.is_expired = True
- sage_intacct_credentials.save()
invalidate_sage_intacct_credentials(workspace_id)
assert not mocked_patch.called
# Should call patch_integration_settings with the correct arguments if sage_intacct_credentials.is_expired is False
+ sage_intacct_credentials.delete()
+ sage_intacct_credentials = SageIntacctCredential.objects.create(
+ workspace_id=workspace_id,
+ si_user_id='test_user',
+ si_company_id='test_company',
+ si_user_password='test_password',
+ is_expired=False
+ )
- sage_intacct_credentials.is_expired = False
- sage_intacct_credentials.save()
invalidate_sage_intacct_credentials(workspace_id)
args, kwargs = mocked_patch.call_args
assert args[0] == workspace_id
- assert kwargs['is_token_expired'] == True
+ assert kwargs['is_token_expired']
# Verify the credentials were marked as expired
sage_intacct_credentials.refresh_from_db()
- assert sage_intacct_credentials.is_expired == True
+ assert sage_intacct_credentials.is_expiredCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.11.9)
1531-1531: Avoid equality comparisons to True; use if kwargs['is_token_expired']: for truth checks
Replace with kwargs['is_token_expired']
(E712)
1535-1535: Avoid equality comparisons to True; use if sage_intacct_credentials.is_expired: for truth checks
Replace with sage_intacct_credentials.is_expired
(E712)
🤖 Prompt for AI Agents
In tests/test_sageintacct/test_utils.py around lines 1502 to 1535, fix the
logical error by re-fetching or recreating the sage_intacct_credentials object
after it is deleted before accessing it again to avoid AttributeError. Also,
improve code style by removing explicit '== True' comparisons in assertions on
lines 1531 and 1535, using direct boolean checks instead.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_workspaces/test_utils.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_workspaces/test_utils.py (1)
apps/workspaces/utils.py (1)
send_email(6-24)
🪛 GitHub Actions: Continuous Integration
tests/test_workspaces/test_utils.py
[error] 8-8: flake8: D103 Missing docstring in public function
[error] 18-18: flake8: D103 Missing docstring in public function
[error] 20-43: flake8: W293 blank line contains whitespace (lines 20, 26, 29, 32, 35, 43)
[error] 45-45: flake8: W291 trailing whitespace and W292 no newline at end of file
tests/test_workspaces/test_utils.py
Outdated
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
| from django.conf import settings | ||
| from apps.workspaces.utils import send_email | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing import for proper test coverage.
The test is missing an import for the From class which is used in the send_email implementation. This could cause the test assertion on line 42 to fail.
Add the missing import:
import pytest
from unittest.mock import patch, MagicMock
from django.conf import settings
+from sendgrid.helpers.mail import From
from apps.workspaces.utils import send_email📝 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.
| import pytest | |
| from unittest.mock import patch, MagicMock | |
| from django.conf import settings | |
| from apps.workspaces.utils import send_email | |
| import pytest | |
| from unittest.mock import patch, MagicMock | |
| from django.conf import settings | |
| from sendgrid.helpers.mail import From | |
| from apps.workspaces.utils import send_email |
🤖 Prompt for AI Agents
In tests/test_workspaces/test_utils.py at the beginning of the file (lines 1 to
5), add an import statement for the From class used in the send_email
implementation. This will ensure the test has proper coverage and prevent
assertion failures related to the missing From class reference.
tests/test_workspaces/test_utils.py
Outdated
| @pytest.fixture | ||
| def mock_sendgrid(): | ||
| with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \ | ||
| patch('apps.workspaces.utils.Mail') as mock_mail: | ||
| mock_instance = MagicMock() | ||
| mock_sg.return_value = mock_instance | ||
| mock_mail_instance = MagicMock() | ||
| mock_mail.return_value = mock_mail_instance | ||
| yield mock_sg, mock_mail |
There was a problem hiding this comment.
Add docstring and improve mock setup.
The fixture is missing a docstring as indicated by the pipeline failure. Additionally, consider mocking the From class for complete test isolation.
Apply this diff to address the issues:
@pytest.fixture
def mock_sendgrid():
+ """
+ Fixture to mock SendGrid components for testing email functionality.
+ """
- with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \
- patch('apps.workspaces.utils.Mail') as mock_mail:
+ with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \
+ patch('apps.workspaces.utils.Mail') as mock_mail, \
+ patch('apps.workspaces.utils.From') as mock_from:
mock_instance = MagicMock()
mock_sg.return_value = mock_instance
mock_mail_instance = MagicMock()
mock_mail.return_value = mock_mail_instance
- yield mock_sg, mock_mail
+ mock_from_instance = MagicMock()
+ mock_from.return_value = mock_from_instance
+ yield mock_sg, mock_mail, mock_from📝 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.
| @pytest.fixture | |
| def mock_sendgrid(): | |
| with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \ | |
| patch('apps.workspaces.utils.Mail') as mock_mail: | |
| mock_instance = MagicMock() | |
| mock_sg.return_value = mock_instance | |
| mock_mail_instance = MagicMock() | |
| mock_mail.return_value = mock_mail_instance | |
| yield mock_sg, mock_mail | |
| @pytest.fixture | |
| def mock_sendgrid(): | |
| """ | |
| Fixture to mock SendGrid components for testing email functionality. | |
| """ | |
| with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \ | |
| patch('apps.workspaces.utils.Mail') as mock_mail, \ | |
| patch('apps.workspaces.utils.From') as mock_from: | |
| mock_instance = MagicMock() | |
| mock_sg.return_value = mock_instance | |
| mock_mail_instance = MagicMock() | |
| mock_mail.return_value = mock_mail_instance | |
| mock_from_instance = MagicMock() | |
| mock_from.return_value = mock_from_instance | |
| yield mock_sg, mock_mail, mock_from |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 8-8: flake8: D103 Missing docstring in public function
🤖 Prompt for AI Agents
In tests/test_workspaces/test_utils.py around lines 7 to 15, add a descriptive
docstring to the mock_sendgrid fixture explaining its purpose. Also, extend the
mock setup by patching the 'apps.workspaces.utils.From' class alongside
SendGridAPIClient and Mail to ensure complete test isolation. Update the fixture
to yield all three mocks accordingly.
tests/test_workspaces/test_utils.py
Outdated
| def test_send_email(mock_sendgrid): | ||
| mock_sg, mock_mail = mock_sendgrid | ||
|
|
||
| # Test data | ||
| recipient_email = ['[email protected]'] | ||
| subject = 'Test Subject' | ||
| message = '<p>Test Message</p>' | ||
| sender_email = '[email protected]' | ||
|
|
||
| # Mock settings | ||
| settings.SENDGRID_API_KEY = 'test_api_key' | ||
|
|
||
| # Call the function | ||
| send_email(recipient_email, subject, message, sender_email) | ||
|
|
||
| # Verify SendGridAPIClient was initialized with correct API key | ||
| mock_sg.assert_called_once_with(api_key='test_api_key') | ||
|
|
||
| # Verify Mail was created with correct parameters | ||
| mock_mail.assert_called_once() | ||
| mail_args = mock_mail.call_args[1] | ||
| assert mail_args['to_emails'] == recipient_email | ||
| assert mail_args['subject'] == subject | ||
| assert mail_args['html_content'] == message | ||
| assert mail_args['from_email'].email == sender_email | ||
|
|
||
| # Verify mail was sent | ||
| mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value) |
There was a problem hiding this comment.
Fix formatting issues and improve test assertions.
Multiple flake8 issues need to be addressed including missing docstring, whitespace issues, and missing newline at end of file. Also, update the test to handle the mocked From class.
Apply this diff to fix all formatting and logic issues:
def test_send_email(mock_sendgrid):
+ """
+ Test that send_email correctly integrates with SendGrid API.
+ """
- mock_sg, mock_mail = mock_sendgrid
-
+ mock_sg, mock_mail, mock_from = mock_sendgrid
+
# Test data
recipient_email = ['[email protected]']
subject = 'Test Subject'
message = '<p>Test Message</p>'
sender_email = '[email protected]'
-
+
# Mock settings
settings.SENDGRID_API_KEY = 'test_api_key'
-
+
# Call the function
send_email(recipient_email, subject, message, sender_email)
-
+
# Verify SendGridAPIClient was initialized with correct API key
mock_sg.assert_called_once_with(api_key='test_api_key')
-
+
+ # Verify From object was created with correct sender email
+ mock_from.assert_called_once_with(email=sender_email)
+
# Verify Mail was created with correct parameters
mock_mail.assert_called_once()
mail_args = mock_mail.call_args[1]
assert mail_args['to_emails'] == recipient_email
assert mail_args['subject'] == subject
assert mail_args['html_content'] == message
- assert mail_args['from_email'].email == sender_email
-
+ assert mail_args['from_email'] == mock_from.return_value
+
# Verify mail was sent
- mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value)
+ mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value)📝 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 test_send_email(mock_sendgrid): | |
| mock_sg, mock_mail = mock_sendgrid | |
| # Test data | |
| recipient_email = ['[email protected]'] | |
| subject = 'Test Subject' | |
| message = '<p>Test Message</p>' | |
| sender_email = '[email protected]' | |
| # Mock settings | |
| settings.SENDGRID_API_KEY = 'test_api_key' | |
| # Call the function | |
| send_email(recipient_email, subject, message, sender_email) | |
| # Verify SendGridAPIClient was initialized with correct API key | |
| mock_sg.assert_called_once_with(api_key='test_api_key') | |
| # Verify Mail was created with correct parameters | |
| mock_mail.assert_called_once() | |
| mail_args = mock_mail.call_args[1] | |
| assert mail_args['to_emails'] == recipient_email | |
| assert mail_args['subject'] == subject | |
| assert mail_args['html_content'] == message | |
| assert mail_args['from_email'].email == sender_email | |
| # Verify mail was sent | |
| mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value) | |
| def test_send_email(mock_sendgrid): | |
| """ | |
| Test that send_email correctly integrates with SendGrid API. | |
| """ | |
| mock_sg, mock_mail, mock_from = mock_sendgrid | |
| # Test data | |
| recipient_email = ['[email protected]'] | |
| subject = 'Test Subject' | |
| message = '<p>Test Message</p>' | |
| sender_email = '[email protected]' | |
| # Mock settings | |
| settings.SENDGRID_API_KEY = 'test_api_key' | |
| # Call the function | |
| send_email(recipient_email, subject, message, sender_email) | |
| # Verify SendGridAPIClient was initialized with correct API key | |
| mock_sg.assert_called_once_with(api_key='test_api_key') | |
| # Verify From object was created with correct sender email | |
| mock_from.assert_called_once_with(email=sender_email) | |
| # Verify Mail was created with correct parameters | |
| mock_mail.assert_called_once() | |
| mail_args = mock_mail.call_args[1] | |
| assert mail_args['to_emails'] == recipient_email | |
| assert mail_args['subject'] == subject | |
| assert mail_args['html_content'] == message | |
| assert mail_args['from_email'] == mock_from.return_value | |
| # Verify mail was sent | |
| mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value) | |
| # (Ensure there is a trailing newline at end of file) |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 18-18: flake8: D103 Missing docstring in public function
[error] 20-43: flake8: W293 blank line contains whitespace (lines 20, 26, 29, 32, 35, 43)
[error] 45-45: flake8: W291 trailing whitespace and W292 no newline at end of file
🤖 Prompt for AI Agents
In tests/test_workspaces/test_utils.py around lines 18 to 45, fix flake8
formatting issues by adding a proper docstring to the test function, correcting
any whitespace inconsistencies, and ensuring there is a newline at the end of
the file. Additionally, update the test assertions to correctly handle the
mocked From class by verifying the 'from_email' attribute appropriately,
ensuring the test logic aligns with the mock structure.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/sage_intacct/tasks.py (1)
729-735: Improved exception handling structure.Splitting the exception blocks improves code readability and maintainability.
Note: There's an empty line at line 730 that was previously flagged for removal.
🧹 Nitpick comments (1)
apps/internal/actions.py (1)
15-19: Good credential handling improvement with minor enhancement opportunity.The changes correctly implement the new credential retrieval pattern and provide clear error handling. The try-except block ensures graceful handling when credentials are missing.
Consider improving exception chaining for better debugging:
except SageIntacctCredential.DoesNotExist: - raise Exception('Sage Intacct credentials not found') + raise Exception('Sage Intacct credentials not found') from NoneThis follows the static analysis recommendation and makes it clear that the original exception context is intentionally omitted.
🧰 Tools
🪛 Ruff (0.11.9)
19-19: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/internal/actions.py(1 hunks)apps/mappings/imports/modules/base.py(1 hunks)apps/mappings/tasks.py(4 hunks)apps/sage_intacct/helpers.py(2 hunks)apps/sage_intacct/tasks.py(17 hunks)apps/sage_intacct/views.py(5 hunks)apps/workspaces/models.py(1 hunks)apps/workspaces/tasks.py(1 hunks)apps/workspaces/views.py(3 hunks)tests/test_workspaces/test_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_workspaces/test_utils.py
- apps/sage_intacct/helpers.py
- apps/workspaces/models.py
- apps/mappings/tasks.py
- apps/sage_intacct/views.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/workspaces/tasks.py (1)
apps/workspaces/models.py (2)
SageIntacctCredential(166-185)get_active_sage_intacct_credentials(184-185)
apps/workspaces/views.py (1)
apps/workspaces/models.py (2)
SageIntacctCredential(166-185)get_active_sage_intacct_credentials(184-185)
apps/internal/actions.py (2)
apps/workspaces/models.py (2)
SageIntacctCredential(166-185)get_active_sage_intacct_credentials(184-185)apps/sage_intacct/utils.py (1)
SageIntacctConnector(76-2096)
apps/sage_intacct/tasks.py (3)
fyle_intacct_api/utils.py (1)
invalidate_sage_intacct_credentials(30-38)apps/workspaces/models.py (2)
SageIntacctCredential(166-185)get_active_sage_intacct_credentials(184-185)apps/sage_intacct/utils.py (2)
SageIntacctConnector(76-2096)search_and_create_vendors(2057-2096)
🪛 Ruff (0.11.9)
apps/internal/actions.py
19-19: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
apps/sage_intacct/tasks.py
1862-1863: SyntaxError: Expected ':', found newline
🪛 GitHub Actions: Continuous Integration
apps/sage_intacct/tasks.py
[error] 1862-1862: SyntaxError: expected ':' at line 1862, column 17.
🔇 Additional comments (13)
apps/workspaces/tasks.py (1)
160-160: LGTM! Credential retrieval pattern improved.The replacement of direct ORM query with
SageIntacctCredential.get_active_sage_intacct_credentials(workspace_id)correctly centralizes credential retrieval logic and ensures only active (non-expired) credentials are used. The existing exception handling appropriately logs and returns early when credentials are missing.apps/workspaces/views.py (3)
282-282: LGTM! Consistent credential retrieval pattern.The replacement with
SageIntacctCredential.get_active_sage_intacct_credentials(workspace_id)ensures only active credentials are retrieved, aligning with the broader credential management improvements.
329-335: Excellent addition of explicit exception handling.This explicit exception handling for
SageIntacctCredential.DoesNotExistin thepostmethod is a valuable improvement. It provides a clear, actionable error message and consistent error handling that was previously missing. The error message format matches the pattern used elsewhere in the codebase.
383-383: LGTM! Consistent with credential management refactoring.The use of
get_active_sage_intacct_credentialsensures only non-expired credentials are retrieved, maintaining consistency with the updated credential handling approach.apps/mappings/imports/modules/base.py (1)
211-233: Excellent defensive programming approach.The wrapping of credential retrieval and synchronization logic in a try-except block is well-implemented. The method gracefully handles missing credentials by logging an informational message and returning early, preventing unnecessary processing. The use of
get_active_sage_intacct_credentialsensures only active credentials are used for synchronization operations.The error handling pattern is consistent with other files in this refactoring effort and maintains the method's existing functionality when credentials are available.
apps/sage_intacct/tasks.py (8)
14-14: LGTM!Good addition of the utility function import to support credential invalidation throughout the file.
278-283: Excellent improvement to credential handling.The standardized credential retrieval with proper exception handling and graceful failure is a good improvement over direct ORM queries.
626-626: Good standardization of credential retrieval.Consistent use of the new class method improves maintainability.
736-742: Excellent addition of token invalidation handling.The specific handling for
InvalidTokenErrorwith credential invalidation improves the robustness of token management.
799-799: Good credential retrieval standardization.Consistent use of the new class method.
904-909: Consistent exception handling improvement.The split exception blocks follow the established pattern and improve code structure.
910-916: Consistent token invalidation handling.Proper implementation following the established pattern for handling expired tokens.
970-970: Consistent implementation across all functions.All these changes follow the established patterns for:
- Standardized credential retrieval using the new class method
- Proper exception handling with separated blocks for
NoPrivilegeErrorandInvalidTokenError- Token invalidation when credentials expire
These improvements enhance code maintainability and robustness.
Also applies to: 1063-1075, 1130-1130, 1223-1235, 1367-1367, 1427-1427, 1497-1497, 1559-1559, 1625-1625, 1627-1633
| @@ -810,7 +823,7 @@ | |||
| expense_group, configuration | |||
| ) | |||
|
|
|||
| sage_intacct_credentials = SageIntacctCredential.objects.get(workspace_id=expense_group.workspace_id) | |||
| sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(expense_group.workspace_id) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant credential retrieval.
Credentials are already retrieved at line 799 in the same function. This duplicate retrieval is unnecessary and should be removed.
- sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(expense_group.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.
| sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(expense_group.workspace_id) |
🤖 Prompt for AI Agents
In apps/sage_intacct/tasks.py at line 826, the code redundantly retrieves Sage
Intacct credentials which were already fetched at line 799 in the same function.
Remove the line at 826 that calls
SageIntacctCredential.get_active_sage_intacct_credentials to avoid duplicate
retrieval and rely on the credentials obtained earlier.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_workspaces/test_utils.py (3)
1-6: Missing import for the From class used in send_email function.The test is missing an import for the
Fromclass fromsendgrid.helpers.mailwhich is used in thesend_emailimplementation. This will cause the test assertion on line 50 to fail because thefrom_emailattribute references an unmockedFrominstance.Add the missing import:
import pytest from unittest.mock import patch, MagicMock from django.conf import settings +from sendgrid.helpers.mail import From from apps.workspaces.utils import send_email from typing import Generator from apps.workspaces.serializers import ConfigurationSerializer
9-20: Mock the From class for complete test isolation.The fixture is missing a mock for the
Fromclass which is instantiated in thesend_emailfunction. Without mocking this class, the test assertion on line 50 will not work as expected.Apply this diff to add the missing From class mock:
@pytest.fixture def mock_sendgrid() -> Generator[tuple[MagicMock, MagicMock], None, None]: """ Mock SendGridAPIClient and Mail """ with patch('apps.workspaces.utils.SendGridAPIClient') as mock_sg, \ - patch('apps.workspaces.utils.Mail') as mock_mail: + patch('apps.workspaces.utils.Mail') as mock_mail, \ + patch('apps.workspaces.utils.From') as mock_from: mock_instance = MagicMock() mock_sg.return_value = mock_instance mock_mail_instance = MagicMock() mock_mail.return_value = mock_mail_instance - yield mock_sg, mock_mail + mock_from_instance = MagicMock() + mock_from.return_value = mock_from_instance + yield mock_sg, mock_mail, mock_fromAlso update the type annotation:
-def mock_sendgrid() -> Generator[tuple[MagicMock, MagicMock], None, None]: +def mock_sendgrid() -> Generator[tuple[MagicMock, MagicMock, MagicMock], None, None]:
23-53: Update test to handle mocked From class correctly.The test needs to be updated to unpack the additional mock from the fixture and verify the
Fromclass interaction properly.Apply this diff to fix the test logic:
def test_send_email(mock_sendgrid) -> None: """ Test send_email """ - mock_sg, mock_mail = mock_sendgrid + mock_sg, mock_mail, mock_from = mock_sendgrid # Test data recipient_email = ['[email protected]'] subject = 'Test Subject' message = '<p>Test Message</p>' sender_email = '[email protected]' # Mock settings settings.SENDGRID_API_KEY = 'test_api_key' # Call the function send_email(recipient_email, subject, message, sender_email) # Verify SendGridAPIClient was initialized with correct API key mock_sg.assert_called_once_with(api_key='test_api_key') + # Verify From object was created with correct sender email + mock_from.assert_called_once_with(email=sender_email) + # Verify Mail was created with correct parameters mock_mail.assert_called_once() mail_args = mock_mail.call_args[1] assert mail_args['to_emails'] == recipient_email assert mail_args['subject'] == subject assert mail_args['html_content'] == message - assert mail_args['from_email'].email == sender_email + assert mail_args['from_email'] == mock_from.return_value # Verify mail was sent mock_sg.return_value.send.assert_called_once_with(mock_mail.return_value)
🧹 Nitpick comments (7)
tests/test_workspaces/test_signal.py (1)
18-18: Fix typo in function name.There's a typo in the function name: "configration" should be "configuration".
-def test_run_post_configration_triggers_auto_map_employees_update(db): +def test_run_post_configuration_triggers_auto_map_employees_update(db):tests/test_mappings/test_imports/test_modules/test_base.py (1)
500-521: Remove unused dummy classesThese dummy classes
DummyErrorandDummyImportLogare defined but never used in any of the test functions. This is dead code that should be removed to maintain code cleanliness.-class DummyError: - """ - Dummy Error class - """ - def __init__(self, id): - self.id = id - - -class DummyImportLog: - """ - Dummy Import Log class - """ - def __init__(self): - self.status = 'IN_PROGRESS' - self.last_successful_run_at = None - self.error_log = [] - self.total_batches_count = 0 - self.processed_batches_count = 0 - - def save(self): - self.saved = Truetests/test_workspaces/test_utils.py (1)
56-63: Consider adding more comprehensive test coverage for ConfigurationSerializer.The current test only validates that data is returned unchanged when
partial=True. Consider adding tests for various validation scenarios.Consider adding additional test cases:
def test_configuration_serializer_full_validate(): """ Test ConfigurationSerializer.validate with partial=False """ data = {'required_field': 'value'} serializer = ConfigurationSerializer(partial=False) result = serializer.validate(data) assert result == data def test_configuration_serializer_empty_data(): """ Test ConfigurationSerializer.validate with empty data """ data = {} serializer = ConfigurationSerializer(partial=True) result = serializer.validate(data) assert result == datatests/test_mappings/test_helpers.py (1)
81-88: LGTM! Correct test for recent sync prevention.The test correctly verifies that sync is prevented when the last successful run was very recent (current time), which should return
Falseas the last run is within the 30-minute threshold.Note: Line 88 appears to be missing the
~annotation marker, but this doesn't affect the test functionality.tests/test_fyle/test_helpers_api.py (3)
67-73: Improve test isolation by resetting all mocks.While the test resets
mock_syncandmock_workspace.save, it doesn't resetmock_get. For better test isolation, consider resetting all relevant mocks between test scenarios.# With source_synced_at, but > 0 days mock_sync.reset_mock() mock_workspace.save.reset_mock() +mock_get.reset_mock() mock_workspace.source_synced_at = datetime.now(timezone.utc) - timedelta(days=1)
99-103: Reset mocks between test scenarios for clearer assertions.The second test scenario reuses the same mock objects without resetting them, making the assertions less clear about what calls were made in each scenario.
# No dependent_field_settings +mock_platform_instance.import_fyle_dimensions.reset_mock() +mock_update.reset_mock() mock_filter.return_value.first.return_value = None helpers.sync_dimensions(1) mock_platform_instance.import_fyle_dimensions.assert_called() mock_update.assert_called()
106-136: Enhance test coverage for conditional branches in handle_refresh_dimensions.The test sets up configuration flags like
import_vendors_as_merchants=Trueandimport_categories=Truebut doesn't verify that the correspondingchain.append()calls are made for these specific scenarios. Consider adding assertions to verify the expected number of chain operations.helpers.handle_refresh_dimensions(1) +# Verify that chain.append was called the expected number of times +# (1 for mapping, 1 for vendors, 1 for categories = 3 total) +assert mock_chain_instance.append.call_count == 3 mock_chain_instance.run.assert_called_once() mock_sync.assert_called_once_with(1) mock_workspace.save.assert_called()Additionally, consider testing edge cases where configuration is None or different expense object types to ensure full branch coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/test_fyle/test_helpers_api.py(1 hunks)tests/test_mappings/test_helpers.py(1 hunks)tests/test_mappings/test_imports/test_modules/test_base.py(2 hunks)tests/test_sageintacct/test_models.py(2 hunks)tests/test_workspaces/test_signal.py(1 hunks)tests/test_workspaces/test_utils.py(1 hunks)tests/workers/export/test_worker.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
tests/test_workspaces/test_utils.py (1)
apps/workspaces/utils.py (1)
send_email(6-24)
tests/test_sageintacct/test_models.py (1)
apps/workspaces/models.py (3)
Workspace(48-70)SageIntacctCredential(166-190)get_active_sage_intacct_credentials(184-190)
tests/test_fyle/test_helpers_api.py (1)
apps/fyle/helpers.py (4)
get_request(69-104)patch_request(107-129)add_expense_id_to_expense_group_settings(171-182)handle_refresh_dimensions(248-308)
tests/test_mappings/test_helpers.py (1)
apps/mappings/helpers.py (3)
schedule_or_delete_auto_mapping_tasks(13-23)prepend_code_to_name(26-36)is_project_sync_allowed(39-55)
tests/test_mappings/test_imports/test_modules/test_base.py (2)
apps/mappings/imports/modules/base.py (5)
Base(28-379)resolve_expense_attribute_errors(130-144)create_ccc_category_mappings(146-157)update_import_log_post_import(337-351)check_import_log_and_start_import(353-379)apps/mappings/tasks.py (1)
resolve_expense_attribute_errors(55-77)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: pytest
🔇 Additional comments (19)
tests/test_workspaces/test_signal.py (1)
18-31: LGTM! Test correctly validates signal behavior.The test properly validates that the post-save signal updates
auto_map_employeesfromFalseto'NAME'under the specified conditions. The test setup with specific configuration values and the assertion logic correctly verifies the expected signal-triggered behavior.tests/workers/export/test_worker.py (2)
7-7: LGTM!The import statement is correctly placed and necessary for the new test function.
111-118: LGTM! Well-structured test with clear intent.The test effectively verifies that
handle_exportsunpacks the input dictionary and passes it as keyword arguments toimport_and_export_expenses. The test follows good practices:
- Clear, descriptive test name
- Proper use of mocking to isolate the unit under test
- Focused assertion that validates the expected behavior
- Consistent with the existing test patterns in the file
tests/test_sageintacct/test_models.py (2)
15-15: LGTM: Import statement correctly added.The import for
SageIntacctCredentialis appropriately added to support the new test function. This aligns with the enhanced credential handling introduced in this PR.
1312-1320: LGTM: Well-structured test for the new credential method.The test correctly validates the
get_active_sage_intacct_credentialsstatic method by:
- Mocking the ORM
getmethod appropriately- Verifying the method calls the ORM with correct filters (
workspace_id=123, is_expired=False)- Asserting the expected return value
This test ensures that only active (non-expired) credentials are retrieved, which is essential for the enhanced credential management introduced in this PR.
tests/test_mappings/test_imports/test_modules/test_base.py (6)
22-24: LGTM: Clean import additionsThe new imports for
patch,MagicMock, andBaseare appropriate for the test functions being added and follow proper conventions.
523-540: Well-structured test for error resolution logicThis test properly validates the conditional logic in
resolve_expense_attribute_errors. It correctly tests both the case where no errored attributes exist and where errored attributes exist but no mapped attributes are found.
542-556: Good test for conditional category mapping logicThe test correctly validates that
bulk_create_ccc_category_mappingsis not called when the configuration conditions are not met. The mock setup appropriately simulates the configuration object.
558-570: Effective test for batch processing logicThis test properly validates the
update_import_log_post_importmethod behavior whenis_last_batchis False, ensuring the processed batch count increments and the log is saved.
572-586: Good coverage of import progress preventionThis test effectively validates that the import process doesn't start when an import log is already in progress and not newly created, which is important for preventing concurrent imports.
588-602: Proper test for time-based import preventionThis test correctly validates that the import process doesn't start when the
sync_aftertimestamp is in the future, which is crucial for scheduled import timing.tests/test_mappings/test_helpers.py (5)
1-8: LGTM! Clean imports and test setup.The imports are appropriate for the test functionality, including the necessary mocking utilities and the functions being tested.
10-17: LGTM! Simple and effective test helper class.The
DummyConfigclass provides a clean way to mock configuration objects with the required attributes for testing.
19-42: LGTM! Comprehensive test coverage for conditional logic.The test correctly verifies the conditional behavior of
schedule_or_delete_auto_mapping_tasks:
- When
auto_map_employees=True: charge card mapping is skipped- When
auto_map_employees=False: all three functions are called- Proper mock reset between test scenarios
The test logic aligns perfectly with the implementation in
apps/mappings/helpers.py.
44-54: LGTM! Thorough coverage of all logical branches.The test covers all scenarios for the
prepend_code_to_namefunction:
- Code prepending when flag is
Trueand code is provided- No prepending when flag is
False- No prepending when code is
None(edge case)All assertions match the expected behavior from the implementation.
56-79: LGTM! Good coverage of sync allowance conditions.These tests properly verify the conditions where project sync should be allowed:
- When import_log is
None- When
last_successful_run_atisNone- When the last run was over 30 minutes ago (1 hour in the test)
The test logic correctly reflects the implementation's time-based logic.
tests/test_fyle/test_helpers_api.py (3)
9-20: LGTM! Error handling test is correctly implemented.The test properly mocks a 400 status response and verifies that the
get_requestfunction raises an exception with the expected error message.
24-35: LGTM! Error handling test is correctly implemented.The test properly mocks a 400 status response and verifies that the
patch_requestfunction raises an exception with the expected error message.
39-49: LGTM! Test correctly verifies the expense ID addition logic.The test properly mocks the ExpenseGroupSettings object and verifies that
expense_idis added to the corporate credit card expense group fields and the save method is called.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/sage_intacct/views.py (1)
291-299:⚠️ Potential issueSame critical variable scoping issue exists here.
This has the same problem as the previous exception handler -
sage_intacct_credentialsmay not be defined if credential retrieval fails.Apply the same fix pattern as suggested for the previous handler.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 296-296: Formatting a regular string which could be an f-string
(C0209)
apps/sage_intacct/tasks.py (1)
819-819: 🛠️ Refactor suggestionRemove redundant credential retrieval.
This line retrieves credentials that were already fetched at line 792 in the same function scope. This is unnecessary and should be removed to avoid redundant database queries.
- sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(expense_group.workspace_id)🧰 Tools
🪛 Pylint (3.3.7)
[convention] 819-819: Line too long (124/120)
(C0301)
🧹 Nitpick comments (4)
apps/workspaces/views.py (1)
331-331: Use f-string for better readability.Replace the old-style string formatting with an f-string as suggested by the static analysis tool.
- 'message': 'Sage Intacct credentials does not exist workspace_id - %s' % workspace_id + 'message': f'Sage Intacct credentials does not exist workspace_id - {workspace_id}'🧰 Tools
🪛 Pylint (3.3.7)
[convention] 331-331: Formatting a regular string which could be an f-string
(C0209)
apps/mappings/tasks.py (2)
112-117: Enhanced exception handling looks good with minor logging improvements needed.The separation of exception handling for
DoesNotExistandInvalidTokenErroris a good improvement. The credential invalidation on token errors is properly implemented.However, consider addressing the static analysis hints for better logging practices:
- logger.info('Sage Intacct credentials does not exist workspace_id - {0}'.format(workspace_id)) + logger.info('Sage Intacct credentials does not exist workspace_id - %s', workspace_id)- logger.info('Invalid Sage Intacct Token Error for workspace_id - {0}'.format(workspace_id)) + logger.info('Invalid Sage Intacct Token Error for workspace_id - %s', workspace_id)🧰 Tools
🪛 Pylint (3.3.7)
[warning] 113-113: Use lazy % formatting in logging functions
(W1202)
[convention] 113-113: Formatting a regular string which could be an f-string
(C0209)
[warning] 117-117: Use lazy % formatting in logging functions
(W1202)
[convention] 117-117: Formatting a regular string which could be an f-string
(C0209)
225-225: Credential retrieval standardization approved, but line is too long.The change to use
get_active_sage_intacct_credentialsis correct and consistent with the overall improvement.- sage_intacct_credentials: SageIntacctCredential = SageIntacctCredential.get_active_sage_intacct_credentials(workspace_id) + sage_intacct_credentials: SageIntacctCredential = ( + SageIntacctCredential.get_active_sage_intacct_credentials(workspace_id) + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 225-225: Line too long (125/120)
(C0301)
apps/sage_intacct/tasks.py (1)
1620-1626: Improved exception handling in status checking function.The enhanced exception handling in
check_sage_intacct_object_statusproperly separates different types of errors and includes credential invalidation for token errors. However, the line is quite long and could benefit from formatting.- except (SageIntacctCredential.DoesNotExist, NoPrivilegeError): - logger.info('SageIntacct credentials does not exist - %s or Insufficient permission to access the requested module', workspace_id) + except (SageIntacctCredential.DoesNotExist, NoPrivilegeError): + logger.info( + 'SageIntacct credentials does not exist - %s or Insufficient permission to access the requested module', + workspace_id + )🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1621-1621: Line too long (138/120)
(C0301)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/mappings/imports/modules/base.py(1 hunks)apps/mappings/tasks.py(4 hunks)apps/sage_intacct/tasks.py(17 hunks)apps/sage_intacct/views.py(5 hunks)apps/workspaces/models.py(1 hunks)apps/workspaces/tasks.py(1 hunks)apps/workspaces/views.py(2 hunks)tests/sql_fixtures/reset_db_fixtures/reset_db.sql(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/sql_fixtures/reset_db_fixtures/reset_db.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/workspaces/tasks.py
- apps/workspaces/models.py
- apps/mappings/imports/modules/base.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/sage_intacct/views.py (2)
fyle_intacct_api/utils.py (1)
invalidate_sage_intacct_credentials(30-38)apps/workspaces/models.py (2)
SageIntacctCredential(166-190)get_active_sage_intacct_credentials(184-190)
apps/mappings/tasks.py (2)
fyle_intacct_api/utils.py (1)
invalidate_sage_intacct_credentials(30-38)apps/workspaces/models.py (2)
SageIntacctCredential(166-190)get_active_sage_intacct_credentials(184-190)
apps/workspaces/views.py (1)
apps/workspaces/models.py (2)
SageIntacctCredential(166-190)get_active_sage_intacct_credentials(184-190)
🪛 Pylint (3.3.7)
apps/sage_intacct/views.py
[convention] 240-240: Formatting a regular string which could be an f-string
(C0209)
[error] 245-245: Using variable 'sage_intacct_credentials' before assignment
(E0601)
[convention] 249-249: Formatting a regular string which could be an f-string
(C0209)
[convention] 287-287: Formatting a regular string which could be an f-string
(C0209)
[convention] 296-296: Formatting a regular string which could be an f-string
(C0209)
apps/mappings/tasks.py
[warning] 113-113: Use lazy % formatting in logging functions
(W1202)
[convention] 113-113: Formatting a regular string which could be an f-string
(C0209)
[warning] 117-117: Use lazy % formatting in logging functions
(W1202)
[convention] 117-117: Formatting a regular string which could be an f-string
(C0209)
[convention] 225-225: Line too long (125/120)
(C0301)
apps/sage_intacct/tasks.py
[convention] 819-819: Line too long (124/120)
(C0301)
[convention] 1360-1360: Line too long (122/120)
(C0301)
[convention] 1621-1621: Line too long (138/120)
(C0301)
[convention] 1858-1858: Line too long (125/120)
(C0301)
apps/workspaces/views.py
[convention] 331-331: Formatting a regular string which could be an f-string
(C0209)
🔇 Additional comments (10)
apps/workspaces/views.py (1)
382-382: Excellent improvement for credential retrieval.The change to use
SageIntacctCredential.get_active_sage_intacct_credentials(workspace.id)is a great improvement as it properly filters out expired credentials, aligning with the new credential expiration tracking feature.This ensures that only active (non-expired) credentials are returned, which is consistent with the broader refactor to handle credential expiration explicitly across the codebase.
apps/mappings/tasks.py (2)
13-13: LGTM! Good addition of the credential invalidation utility.The import of
invalidate_sage_intacct_credentialssupports the enhanced credential management pattern being implemented across the codebase.
95-95: Excellent improvement to credential retrieval.Using
get_active_sage_intacct_credentialsensures only non-expired credentials are retrieved, which aligns with the enhanced credential management strategy.apps/sage_intacct/views.py (2)
16-16: LGTM! Consistent with the credential management improvements.The import aligns with the pattern being implemented across the codebase.
225-225: Good standardization of credential retrieval.Both instances correctly use the new
get_active_sage_intacct_credentialsmethod to ensure only active credentials are retrieved.Also applies to: 267-267
apps/sage_intacct/tasks.py (5)
14-14: LGTM! Consistent import for credential management.The import supports the enhanced credential invalidation pattern implemented throughout this file.
278-278: Excellent standardization of credential retrieval across all functions.All credential retrieval calls have been consistently updated to use
get_active_sage_intacct_credentials, which ensures only non-expired credentials are used. This is a significant improvement to the credential management strategy.Also applies to: 622-622, 792-792, 963-963, 1123-1123, 1360-1360, 1490-1490, 1618-1618, 1856-1856
721-735: Enhanced exception handling pattern looks excellent.The separation of
NoPrivilegeErrorandInvalidTokenErrorinto distinct exception blocks with proper credential invalidation is a significant improvement. This provides better error categorization and ensures credentials are properly invalidated when tokens become invalid.Also applies to: 897-909, 1056-1068, 1216-1228
1420-1420: Proper credential invalidation in payment functions.The addition of credential invalidation calls in the payment creation functions (
create_ap_paymentandcreate_sage_intacct_reimbursement) ensures consistent error handling when token errors occur during payment processing.Also applies to: 1552-1552
1855-1862: Good addition of exception handling for vendor search function.The try-except block properly handles missing credentials when searching for vendors. The exception handling is appropriate and follows the established pattern.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1858-1858: Line too long (125/120)
(C0301)
| except InvalidTokenError: | ||
| invalidate_sage_intacct_credentials(workspace.id, sage_intacct_credentials) | ||
| logger.info('Invalid Sage Intact Token for workspace_id - %s', kwargs['workspace_id']) | ||
| return Response( | ||
| data={ | ||
| 'message': 'Invalid Sage Intact Token for workspace_id - %s' % kwargs['workspace_id'] | ||
| }, | ||
| status=status.HTTP_400_BAD_REQUEST | ||
| ) |
There was a problem hiding this comment.
Critical issue: Variable used before assignment in InvalidTokenError handler.
The sage_intacct_credentials variable is referenced in the InvalidTokenError exception handler but may not be assigned if an exception occurs during credential retrieval.
try:
workspace = Workspace.objects.get(pk=kwargs['workspace_id'])
sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(workspace.id)
+ except SageIntacctCredential.DoesNotExist:
+ logger.info('Sage Intacct credentials not found workspace_id - %s', kwargs['workspace_id'])
+ return Response(
+ data={
+ 'message': 'Sage Intacct credentials not found workspace_id - %s' % kwargs['workspace_id']
+ },
+ status=status.HTTP_400_BAD_REQUEST
+ )
+
+ try:
async_task(
'apps.sage_intacct.helpers.check_interval_and_sync_dimension',
kwargs['workspace_id'],
)
return Response(
status=status.HTTP_200_OK
)
- except SageIntacctCredential.DoesNotExist:
- logger.info('Sage Intacct credentials not found workspace_id - %s', kwargs['workspace_id'])
- return Response(
- data={
- 'message': 'Sage Intacct credentials not found workspace_id - %s' % kwargs['workspace_id']
- },
- status=status.HTTP_400_BAD_REQUEST
- )
except InvalidTokenError:
invalidate_sage_intacct_credentials(workspace.id, sage_intacct_credentials)📝 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.
| except InvalidTokenError: | |
| invalidate_sage_intacct_credentials(workspace.id, sage_intacct_credentials) | |
| logger.info('Invalid Sage Intact Token for workspace_id - %s', kwargs['workspace_id']) | |
| return Response( | |
| data={ | |
| 'message': 'Invalid Sage Intact Token for workspace_id - %s' % kwargs['workspace_id'] | |
| }, | |
| status=status.HTTP_400_BAD_REQUEST | |
| ) | |
| try: | |
| workspace = Workspace.objects.get(pk=kwargs['workspace_id']) | |
| sage_intacct_credentials = SageIntacctCredential.get_active_sage_intacct_credentials(workspace.id) | |
| except SageIntacctCredential.DoesNotExist: | |
| logger.info('Sage Intacct credentials not found workspace_id - %s', kwargs['workspace_id']) | |
| return Response( | |
| data={ | |
| 'message': 'Sage Intacct credentials not found workspace_id - %s' % kwargs['workspace_id'] | |
| }, | |
| status=status.HTTP_400_BAD_REQUEST | |
| ) | |
| try: | |
| async_task( | |
| 'apps.sage_intacct.helpers.check_interval_and_sync_dimension', | |
| kwargs['workspace_id'], | |
| ) | |
| return Response( | |
| status=status.HTTP_200_OK | |
| ) | |
| except InvalidTokenError: | |
| invalidate_sage_intacct_credentials(workspace.id, sage_intacct_credentials) | |
| logger.info('Invalid Sage Intact Token for workspace_id - %s', kwargs['workspace_id']) | |
| return Response( | |
| data={ | |
| 'message': 'Invalid Sage Intact Token for workspace_id - %s' % kwargs['workspace_id'] | |
| }, | |
| status=status.HTTP_400_BAD_REQUEST | |
| ) |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 245-245: Using variable 'sage_intacct_credentials' before assignment
(E0601)
[convention] 249-249: Formatting a regular string which could be an f-string
(C0209)
🤖 Prompt for AI Agents
In apps/sage_intacct/views.py around lines 244 to 252, the variable
sage_intacct_credentials is used in the InvalidTokenError exception handler but
may not be assigned if an error occurs before its assignment. To fix this,
ensure sage_intacct_credentials is assigned before the try block or add a check
to confirm it is assigned before calling invalidate_sage_intacct_credentials.
Alternatively, restructure the code to retrieve and assign
sage_intacct_credentials before the try-except block so it is always defined
when the exception handler runs.
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
invalidate_sage_intacct_credentialsutility to manage expired credentials.InvalidTokenError.is_expiredfield toSageIntacctCredentialmodel to track credential status.is_expiredfield in the database.Clickup
https://app.clickup.com/
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores