test: Internal APIs #698
test: Internal APIs #698
Conversation
WalkthroughThis pull request introduces new test functions to validate the functionality of specific methods in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ashwin1111
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## internal-apis #698 +/- ##
================================================
Coverage ? 94.54%
================================================
Files ? 63
Lines ? 4989
Branches ? 0
================================================
Hits ? 4717
Misses ? 272
Partials ? 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/test_internal/test_actions.py (1)
1-31: Consider adding a test configuration fileTo improve test maintainability and reusability, consider:
- Creating a
conftest.pyfor shared test fixtures- Moving test data to separate fixture files
- Adding typing information to improve code clarity
- Documenting test scenarios in docstrings
Example structure:
# tests/test_internal/conftest.py import pytest @pytest.fixture def mock_qbo_response(): return { 'employee_response': {...}, 'bill_response': {...} } # tests/test_internal/fixtures/responses.py MOCK_RESPONSES = { 'accounting_fields': [...], 'exported_entry': {...} }tests/test_internal/test_views.py (2)
10-28: Enhance test coverage and assertionsWhile the basic test flow is good, consider these improvements:
- Add assertions for response content in the successful case
- Add negative test cases for invalid resource_type
- Validate that the correct parameters are being passed to the mocked function
- Consider parameterizing the test for different resource types
Here's a suggested enhancement:
@pytest.mark.django_db(databases=['default']) @patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True) -def test_qbo_fields_view(db, api_client, mocker): +@pytest.mark.parametrize('resource_type,mock_target', [ + ('employees', 'qbosdk.apis.Employees.get_all_generator'), + ('vendors', 'qbosdk.apis.Vendors.get_all_generator'), +]) +def test_qbo_fields_view(db, api_client, mocker, resource_type, mock_target): url = reverse('accounting-fields') response = api_client.get(url) assert response.status_code == 400 + assert 'message' in response.data response = api_client.get(url, {'org_id': 'or79Cob97KSh'}) assert response.status_code == 400 + assert 'message' in response.data + + # Test invalid resource_type + response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'invalid'}) + assert response.status_code == 400 mocker.patch( - 'qbosdk.apis.Employees.get_all_generator', + mock_target, return_value=[data['employee_response']] ) - response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'employees'}) + response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': resource_type}) assert response.status_code == 200 + assert isinstance(response.data, list) + mock = mocker.patch(mock_target) + mock.assert_called_once()
1-50: Consider creating shared test utilitiesThe test functions follow similar patterns for parameter validation. Consider extracting common test logic into shared utilities:
- Create a utility function for testing missing/invalid parameters
- Create fixtures for common test data
- Consider using pytest's parametrize for testing different resource types and scenarios
Example structure:
@pytest.fixture def mock_qbo_responses(): return { 'bills': {...}, 'employees': {...} } def assert_missing_params(client, url, params_list): """Utility to test missing parameters""" for params in params_list: response = client.get(url, params) assert response.status_code == 400 assert 'message' in response.datatests/test_workspaces/test_views.py (1)
Line range hint
167-193: Enhance test readability with pytest.mark.parametrizeWhile the test coverage is comprehensive, consider using
pytest.mark.parametrizeto make the test cases more maintainable and readable:@pytest.mark.parametrize('client_id,expected_status,test_case', [ ('dummy_id', 403, 'invalid_client_id'), (settings.TEST_INTERNAL_API_CLIENT_ID, 200, 'valid_credentials'), (settings.TEST_INTERNAL_API_CLIENT_ID_INVALID, 400, 'invalid_credentials') ]) def test_prepare_e2e_test_view(client_id, expected_status, test_case, mock_db, mocker, api_client, test_connection): url = reverse('setup-e2e-test', kwargs={'workspace_id': 3}) api_client.credentials(HTTP_X_Internal_API_Client_ID=client_id) # Setup mocks based on test_case if test_case == 'valid_credentials': mocker.patch('cryptography.fernet.Fernet.decrypt', return_value=settings.E2E_TESTS_CLIENT_SECRET.encode('utf-8')) mocker.patch('apps.quickbooks_online.utils.QBOConnector.sync_dimensions', return_value=None) # ... other mocks response = api_client.post(url) assert response.status_code == expected_status🧰 Tools
🪛 Gitleaks (8.21.1)
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
192-192: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
tests/test_internal/test_actions.py(1 hunks)tests/test_internal/test_views.py(1 hunks)tests/test_workspaces/test_views.py(3 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.1)
tests/test_workspaces/test_views.py
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
192-192: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (4)
tests/test_internal/test_actions.py (2)
1-2: LGTM! Imports are clean and appropriate.
The imports are specific to the test requirements, importing only the necessary functions and fixtures.
19-31: 🛠️ Refactor suggestion
Improve test data quality and coverage
The current test has several areas for improvement:
- The mock response
{'summa': 'hehe'}doesn't represent realistic QuickBooks data - The assertion is too basic
- Missing error scenario coverage
Suggested improvements:
+from tests.test_quickbooks_online.fixtures import mock_bill_response # Add realistic mock data
+
def test_get_exported_entry(db, mocker):
query_params = {
'org_id': 'or79Cob97KSh',
'resource_type': 'bills',
'internal_id': '1'
}
mocker.patch(
'qbosdk.apis.Bills.get_by_id',
- return_value={'summa': 'hehe'}
+ return_value=mock_bill_response
)
entry = get_exported_entry(query_params)
- assert entry is not None
+ assert isinstance(entry, dict), "Should return a dictionary"
+ assert 'Id' in entry, "Should contain QuickBooks Id"
+ assert 'Balance' in entry, "Should contain Balance field"
+
+def test_get_exported_entry_error_cases(db, mocker):
+ # Test missing internal_id
+ query_params = {
+ 'org_id': 'or79Cob97KSh',
+ 'resource_type': 'bills'
+ }
+ with pytest.raises(ValueError):
+ get_exported_entry(query_params)
+
+ # Test API error handling
+ query_params['internal_id'] = '1'
+ mocker.patch(
+ 'qbosdk.apis.Bills.get_by_id',
+ side_effect=Exception('API Error')
+ )
+ with pytest.raises(Exception):
+ get_exported_entry(query_params)Let's verify the actual QuickBooks response structure:
tests/test_internal/test_views.py (1)
1-8: LGTM: Imports are well-organized and appropriate
The imports are clean, well-organized, and include all necessary components for testing Django API endpoints with pytest.
tests/test_workspaces/test_views.py (1)
168-168: LGTM: Authentication header changes are well-structured
The change from HTTP_X_E2E_Tests_Client_ID to HTTP_X_Internal_API_Client_ID is consistent across all test cases and properly validates different response scenarios.
Also applies to: 178-178, 187-187, 192-192
| def test_get_accounting_fields(db, mocker): | ||
| query_params = { | ||
| 'org_id': 'or79Cob97KSh', | ||
| 'resource_type': 'employees', | ||
| } | ||
| mocker.patch( | ||
| 'qbosdk.apis.Employees.get_all_generator', | ||
| return_value=[data['employee_response']] | ||
| ) | ||
|
|
||
| fields = get_accounting_fields(query_params) | ||
| assert fields is not None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and assertions
While the basic test structure is good, consider these improvements:
- Add assertions to validate the structure and content of returned fields
- Add test cases for error scenarios (invalid org_id, API errors)
- Consider parameterizing the test for different resource types
- Document the purpose of the hardcoded org_id
Example enhancement:
def test_get_accounting_fields(db, mocker):
query_params = {
'org_id': 'or79Cob97KSh',
'resource_type': 'employees',
}
mocker.patch(
'qbosdk.apis.Employees.get_all_generator',
return_value=[data['employee_response']]
)
fields = get_accounting_fields(query_params)
- assert fields is not None
+ # Validate field structure and content
+ assert isinstance(fields, list), "Should return a list of fields"
+ assert len(fields) > 0, "Should return at least one field"
+ for field in fields:
+ assert 'attribute_type' in field, "Each field should have attribute_type"
+ assert 'display_name' in field, "Each field should have display_name"
@pytest.mark.parametrize('resource_type,expected_error', [
('invalid_type', ValueError),
('employees', None)
])
def test_get_accounting_fields_errors(db, mocker, resource_type, expected_error):
query_params = {
'org_id': 'or79Cob97KSh',
'resource_type': resource_type,
}
if expected_error:
with pytest.raises(expected_error):
get_accounting_fields(query_params)
else:
fields = get_accounting_fields(query_params)
assert fields is not NoneCommittable suggestion skipped: line range outside the PR's diff.
| @pytest.mark.django_db(databases=['default']) | ||
| @patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True) | ||
| def test_exported_entry_view(db, api_client, mocker): | ||
| url = reverse('exported-entry') | ||
|
|
||
| response = api_client.get(url) | ||
| assert response.status_code == 400 | ||
|
|
||
| response = api_client.get(url, {'org_id': 'or79Cob97KSh'}) | ||
| assert response.status_code == 400 | ||
|
|
||
| response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'bills'}) | ||
| assert response.status_code == 400 | ||
|
|
||
| mocker.patch( | ||
| 'qbosdk.apis.Bills.get_by_id', | ||
| return_value={'summa': 'hehe'} | ||
| ) | ||
|
|
||
| response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'bills', 'internal_id': '1'}) | ||
| assert response.status_code == 200 |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Improve test data and assertions
The test structure is good, but needs improvements in the mocked response and assertions to match the actual view implementation:
- The view wraps the response in a
datafield:{'data': response} - Error responses include a
messagefield - The view uses
get_exported_entry()function which should be properly mocked
@pytest.mark.django_db(databases=['default'])
@patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True)
def test_exported_entry_view(db, api_client, mocker):
url = reverse('exported-entry')
response = api_client.get(url)
assert response.status_code == 400
+ assert 'error' in response.data
response = api_client.get(url, {'org_id': 'or79Cob97KSh'})
assert response.status_code == 400
+ assert 'error' in response.data
response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'bills'})
assert response.status_code == 400
+ assert 'error' in response.data
+ mock_bill_response = {
+ 'Id': '123',
+ 'DocNumber': 'BILL-123',
+ 'TotalAmt': 100.00,
+ 'DueDate': '2024-01-01',
+ 'Balance': 100.00
+ }
+
mocker.patch(
- 'qbosdk.apis.Bills.get_by_id',
- return_value={'summa': 'hehe'}
+ 'apps.internal.utils.get_exported_entry',
+ return_value=mock_bill_response
)
params = {
'org_id': 'or79Cob97KSh',
'resource_type': 'bills',
'internal_id': '123'
}
response = api_client.get(url, params)
assert response.status_code == 200
- assert response.data == mock_bill_response
+ assert response.data == {'data': mock_bill_response}🔗 Analysis chain
Improve test data and assertions
The test structure is good, but there are several areas for improvement:
- The mocked response
{'summa': 'hehe'}is not representative of real data - Missing assertions for response content
- No validation that the correct parameters are passed to the mocked function
Here's a suggested improvement:
@pytest.mark.django_db(databases=['default'])
@patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True)
def test_exported_entry_view(db, api_client, mocker):
url = reverse('exported-entry')
response = api_client.get(url)
assert response.status_code == 400
+ assert 'message' in response.data
response = api_client.get(url, {'org_id': 'or79Cob97KSh'})
assert response.status_code == 400
+ assert 'message' in response.data
response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'bills'})
assert response.status_code == 400
+ assert 'message' in response.data
+ mock_bill_response = {
+ 'Id': '123',
+ 'DocNumber': 'BILL-123',
+ 'TotalAmt': 100.00,
+ 'Balance': 100.00,
+ 'DueDate': '2024-01-01'
+ }
+
mocker.patch(
'qbosdk.apis.Bills.get_by_id',
- return_value={'summa': 'hehe'}
+ return_value=mock_bill_response
)
- response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'bills', 'internal_id': '1'})
+ params = {
+ 'org_id': 'or79Cob97KSh',
+ 'resource_type': 'bills',
+ 'internal_id': '123'
+ }
+ response = api_client.get(url, params)
assert response.status_code == 200
+ assert response.data == mock_bill_response
+
+ # Verify the mock was called with correct parameters
+ mock = mocker.patch('qbosdk.apis.Bills.get_by_id')
+ mock.assert_called_once_with('123')Let's verify the actual response structure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find the actual bill response structure in the codebase
rg -A 10 "class Bills\(" .
Length of output: 28
Script:
#!/bin/bash
# Let's try different approaches to find bill-related code
# Search for bill-related files
fd -t f -i bill
# Search for bill-related imports or usage
rg -i "from.*bill.*import|import.*bill" .
# Search for bill-related classes or functions
ast-grep --pattern 'class $_ {
$$$
get_by_id($_) {
$$$
}
}'
Length of output: 1214
Script:
#!/bin/bash
# Let's look at the Bill model and related code
# Check Bill model definition
rg -A 10 "class Bill\(" .
# Look for bill-related test data or fixtures
rg -A 5 "mock.*bill" .
# Check the view being tested
rg -A 10 "class ExportedEntryView" .
Length of output: 9303
Script:
#!/bin/bash
# Let's check the test data structure used in other tests
# Look for bill response data in test files
rg -A 15 "bill_response.*=" tests/
rg -A 15 "'bill_response':" tests/
# Check the actual view implementation to understand expected response
rg -A 20 "def get\(self, request" apps/internal/views.py
Length of output: 3245
| @@ -184,12 +184,12 @@ def test_prepare_e2e_test_view(mock_db, mocker, api_client, test_connection): | |||
| healthy_token.is_expired = False | |||
| healthy_token.save() | |||
|
|
|||
| api_client.credentials(HTTP_X_E2E_Tests_Client_ID='gAAAAABi8oXHBll3lEUPGpMDXnZDhVgSl_LMOkIF0ilfmSCL3wFxZnoTIbpdzwPoOFzS0vFO4qaX51JtAqCG2RBHZaf1e98hug==') | |||
| api_client.credentials(HTTP_X_Internal_API_Client_ID='gAAAAABi8oXHBll3lEUPGpMDXnZDhVgSl_LMOkIF0ilfmSCL3wFxZnoTIbpdzwPoOFzS0vFO4qaX51JtAqCG2RBHZaf1e98hug==') | |||
There was a problem hiding this comment.
Security: Avoid hardcoding API credentials in test files
While these appear to be test credentials, it's best practice to avoid hardcoding them in the source code. Consider:
- Moving them to environment variables
- Using test fixtures
- Generating mock credentials dynamically
This helps prevent accidental credential exposure and makes credential rotation easier.
Example refactor:
- api_client.credentials(HTTP_X_Internal_API_Client_ID='gAAAAABi8oXHBll3lEUPGpMDXnZDhVgSl_LMOkIF0ilfmSCL3wFxZnoTIbpdzwPoOFzS0vFO4qaX51JtAqCG2RBHZaf1e98hug==')
+ api_client.credentials(HTTP_X_Internal_API_Client_ID=settings.TEST_INTERNAL_API_CLIENT_ID)
- api_client.credentials(HTTP_X_Internal_API_Client_ID='gAAAAABi8oWVoonxF0K_g2TQnFdlpOJvGsBYa9rPtwfgM-puStki_qYbi0PdipWHqIBIMip94MDoaTP4MXOfERDeEGrbARCxPw==')
+ api_client.credentials(HTTP_X_Internal_API_Client_ID=settings.TEST_INTERNAL_API_CLIENT_ID_INVALID)Also applies to: 192-192
🧰 Tools
🪛 Gitleaks (8.21.1)
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
* feat: Add support for internal APIs * test: Internal APIs (#698) * update path
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
Summary by CodeRabbit
accounting-fieldsandexported-entryendpoints to ensure correct status codes and responses.