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

test: Internal APIs #698

Merged
merged 1 commit into from
Nov 26, 2024
Merged

test: Internal APIs #698

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
Empty file added tests/test_internal/__init__.py
View file
Open in desktop
Empty file.
31 changes: 31 additions & 0 deletions tests/test_internal/test_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from apps.internal.actions import get_accounting_fields, get_exported_entry
from tests.test_quickbooks_online.fixtures import data


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
Comment on lines +5 to +16
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage and assertions

While the basic test structure is good, consider these improvements:

  1. Add assertions to validate the structure and content of returned fields
  2. Add test cases for error scenarios (invalid org_id, API errors)
  3. Consider parameterizing the test for different resource types
  4. 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 None

Committable suggestion skipped: line range outside the PR's diff.



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'}
)

entry = get_exported_entry(query_params)
assert entry is not None
50 changes: 50 additions & 0 deletions tests/test_internal/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import pytest
from unittest.mock import patch
from django.urls import reverse

from apps.workspaces.permissions import IsAuthenticatedForInternalAPI

from tests.test_quickbooks_online.fixtures import data


@pytest.mark.django_db(databases=['default'])
@patch.object(IsAuthenticatedForInternalAPI, 'has_permission', return_value=True)
def test_qbo_fields_view(db, api_client, mocker):
url = reverse('accounting-fields')

response = api_client.get(url)
assert response.status_code == 400

response = api_client.get(url, {'org_id': 'or79Cob97KSh'})
assert response.status_code == 400

mocker.patch(
'qbosdk.apis.Employees.get_all_generator',
return_value=[data['employee_response']]
)

response = api_client.get(url, {'org_id': 'or79Cob97KSh', 'resource_type': 'employees'})
assert response.status_code == 200


@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
Comment on lines +30 to +50
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

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:

  1. The view wraps the response in a data field: {'data': response}
  2. Error responses include a message field
  3. 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:

  1. The mocked response {'summa': 'hehe'} is not representative of real data
  2. Missing assertions for response content
  3. 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

8 changes: 4 additions & 4 deletions tests/test_workspaces/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def test_connect_qbo_view_exceptions(api_client, test_connection):
def test_prepare_e2e_test_view(mock_db, mocker, api_client, test_connection):
url = reverse('setup-e2e-test', kwargs={'workspace_id': 3})

api_client.credentials(HTTP_X_E2E_Tests_Client_ID='dummy_id')
api_client.credentials(HTTP_X_Internal_API_Client_ID='dummy_id')
response = api_client.post(url)
assert response.status_code == 403

Expand All @@ -175,7 +175,7 @@ def test_prepare_e2e_test_view(mock_db, mocker, api_client, test_connection):
mocker.patch('fyle_integrations_platform_connector.fyle_integrations_platform_connector.PlatformConnector.import_fyle_dimensions', return_value=[])
mocker.patch('apps.workspaces.models.QBOCredential.objects.create', return_value=None)

api_client.credentials(HTTP_X_E2E_Tests_Client_ID='dummy_id')
api_client.credentials(HTTP_X_Internal_API_Client_ID='dummy_id')
response = api_client.post(url)
assert response.status_code == 400

Expand All @@ -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==')
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. Moving them to environment variables
  2. Using test fixtures
  3. 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)

response = api_client.post(url)
assert response.status_code == 200

url = reverse('setup-e2e-test', kwargs={'workspace_id': 6})
api_client.credentials(HTTP_X_E2E_Tests_Client_ID='gAAAAABi8oWVoonxF0K_g2TQnFdlpOJvGsBYa9rPtwfgM-puStki_qYbi0PdipWHqIBIMip94MDoaTP4MXOfERDeEGrbARCxPw==')
api_client.credentials(HTTP_X_Internal_API_Client_ID='gAAAAABi8oWVoonxF0K_g2TQnFdlpOJvGsBYa9rPtwfgM-puStki_qYbi0PdipWHqIBIMip94MDoaTP4MXOfERDeEGrbARCxPw==')
response = api_client.post(url)
assert response.status_code == 400

Expand Down
Loading