feat: Added limits to attributes #649
Conversation
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve the addition of a new method, Changes
Possibly related PRs
Suggested labels
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 (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
tests/test_netsuite/test_connector.py (1)
198-198: Remove Trailing WhitespaceThere's a trailing whitespace on line 198 after
return_value=5. Removing it will keep the code clean and adhere to styling guidelines.Apply this diff:
- return_value=5 + return_value=5apps/netsuite/connector.py (1)
620-620: Correct log message to use plural 'departments'In the
sync_departmentsmethod, the log message should use the plural form 'departments' for consistency.Apply this diff:
- logger.info('Skipping sync of department for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of departments for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- apps/netsuite/connector.py (7 hunks)
- requirements.txt (1 hunks)
- tests/test_netsuite/test_connector.py (4 hunks)
🧰 Additional context used
🪛 Ruff
apps/netsuite/connector.py
96-99: Return the negated condition directly
Inline condition
(SIM103)
🔇 Additional comments (4)
requirements.txt (1)
36-36: LGTM. Verify integration with updated netsuitesdk.The update of
netsuitesdkfrom 2.21.3 to 2.23.0 looks good. This minor version update should typically be backwards compatible.Please ensure the following:
- Verify that all functionality dependent on
netsuitesdkstill works as expected with the new version.- Update the PR description to explain the reason for this version bump, especially if it's related to the PR objective of "Adding limits to attributes".
To help verify the change, you can run the following script:
This will help identify any significant changes or new features that might be relevant to your PR objective.
tests/test_netsuite/test_connector.py (3)
196-199: Ensure Consistency in Mock Return ValuesThe
return_valueformocker.patchintest_sync_accountsis set to5. Verify that this value aligns with the expected count in your test assertions to ensure the test behaves as intended.
436-439: Consistent Mocking Across TestsSimilar to previous tests, ensure that the
return_value=5intest_sync_classificationsmatches the intended count for classifications in your test environment.
436-439: Duplicate Mocking Logic DetectedThe mocking logic in
test_sync_classificationsis duplicated from previous test functions. Refactoring this logic into a shared fixture can improve code maintainability.
| mocker.patch( | ||
| 'netsuitesdk.api.accounts.Accounts.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Repeated Mocking of Count Methods
The mocking of the count method for Accounts, Locations, Departments, and Classifications is repeated in multiple tests with similar structure. Consider refactoring this repeated code into a fixture or a helper function to adhere to the DRY (Don't Repeat Yourself) principle.
Here's how you might refactor using a fixture:
# In conftest.py or your fixtures file
import pytest
@pytest.fixture
def mock_count(mocker):
def _mock_count(module_path, return_value=5):
mocker.patch(
f'netsuitesdk.api.{module_path}.count',
return_value=return_value
)
return _mock_countAnd then, in your test functions:
def test_sync_accounts(mocker, db, mock_count):
mock_count('accounts.Accounts')
# rest of your test code
def test_sync_locations(mocker, db, mock_count):
mock_count('locations.Locations')
# rest of your test codeThis refactoring improves code maintainability and readability.
Also applies to: 331-334, 352-355, 436-439
| mocker.patch( | ||
| 'netsuitesdk.api.locations.Locations.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize Mock Patch Context Managers
Consider using a context manager for mocker.patch when patching methods used within a specific scope. This can enhance code clarity and ensure patches are only applied where necessary.
Example:
def test_sync_locations(mocker, db):
with mocker.patch('netsuitesdk.api.locations.Locations.count', return_value=5):
# rest of your test code| mocker.patch( | ||
| 'netsuitesdk.api.classifications.Classifications.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
Potential Typographical Error in Indentation
Check the indentation level on line 438. The alignment seems off compared to other mocker.patch calls. Correct indentation is crucial for code readability in Python.
Apply this diff:
- return_value=5
+ return_value=5📝 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.
| mocker.patch( | |
| 'netsuitesdk.api.classifications.Classifications.count', | |
| return_value=5 | |
| ) | |
| mocker.patch( | |
| 'netsuitesdk.api.classifications.Classifications.count', | |
| return_value=5 | |
| ) |
| mocker.patch( | ||
| 'netsuitesdk.api.departments.Departments.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent Mocked Return Value with Test Assertion
The mocked Departments.count is set to return 5, but the test assertion expects locations to be 12. This suggests a mismatch between the mock and the expected behavior. Please ensure that the mocked return value aligns with the test's expected outcome.
- File: tests/test_netsuite/test_connector.py
- Function:
test_sync_departments
🔗 Analysis chain
Verify Mocked Return Value Matches Expected Count
In test_sync_departments, the mocked return_value is set to 5. Ensure that this count aligns with your test's expected behavior, especially with assertions later in the test function.
Run the following command to check the usage of test_sync_departments:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the expected count matches the mocked return value.
# Search for assertions in the test function
rg --type python -A 5 'def test_sync_departments' tests/test_netsuite/test_connector.py
Length of output: 120
Script:
#!/bin/bash
# Description: Verify that the expected count matches the mocked return value by searching for assertions in the test function.
# Search for the test_sync_departments function and capture the next 5 lines for assertions
rg 'def test_sync_departments' --context 5 tests/test_netsuite/test_connector.py
Length of output: 377
Script:
#!/bin/bash
# Description: Search for all usages of Departments.count within the test_sync_departments function.
rg 'Departments\.count' --context 5 'def test_sync_departments' tests/test_netsuite/test_connector.py
Length of output: 895
apps/netsuite/connector.py
Outdated
| def is_sync_allowed(self, attribute_type: str, attribute_count: int): | ||
| """ | ||
| Checks if the sync is allowed | ||
|
|
||
| Returns: | ||
| bool: True | ||
| """ | ||
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at | ||
| if workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone()) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]: | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify is_sync_allowed by returning the negated condition directly
You can simplify the is_sync_allowed method by returning the negated condition directly, improving readability as suggested by the static analysis tool Ruff (SIM103).
Apply this diff:
def is_sync_allowed(self, attribute_type: str, attribute_count: int):
"""
Checks if the sync is allowed
- Returns:
- bool: True
+ Returns:
+ bool:
+ True if sync is allowed, False otherwise
"""
workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at
- if workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone()) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]:
- return False
-
- return True
+ return not (
+ workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone())
+ and attribute_count > SYNC_UPPER_LIMIT[attribute_type]
+ )📝 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 is_sync_allowed(self, attribute_type: str, attribute_count: int): | |
| """ | |
| Checks if the sync is allowed | |
| Returns: | |
| bool: True | |
| """ | |
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at | |
| if workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone()) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]: | |
| return False | |
| return True | |
| def is_sync_allowed(self, attribute_type: str, attribute_count: int): | |
| """ | |
| Checks if the sync is allowed | |
| Returns: | |
| bool: | |
| True if sync is allowed, False otherwise | |
| """ | |
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at | |
| return not ( | |
| workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone()) | |
| and attribute_count > SYNC_UPPER_LIMIT[attribute_type] | |
| ) |
🧰 Tools
🪛 Ruff
96-99: Return the negated condition directly
Inline condition
(SIM103)
apps/netsuite/connector.py
Outdated
| 'projects': 10000, | ||
| 'customers': 25000, | ||
| 'classes': 2000, | ||
| 'accounts': 2000, | ||
| 'expense_category': 2000, | ||
| 'locations': 2000, | ||
| 'departments': 2000, | ||
| 'vendors': 20000, | ||
| 'tax_items': 15000, |
There was a problem hiding this comment.
Add is_sync_allowed checks to missing sync methods
You have added 'expense_category' and 'vendors' to SYNC_UPPER_LIMIT, but the corresponding is_sync_allowed checks are missing in the sync_expense_categories and sync_vendors methods. To enforce these limits, please add the necessary checks.
Apply this diff to sync_expense_categories:
def sync_expense_categories(self):
"""
Sync Expense Categories
"""
+ attribute_count = self.connection.expense_categories.count()
+ if not self.is_sync_allowed(attribute_type='expense_category', attribute_count=attribute_count):
+ logger.info('Skipping sync of expense categories for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count)
+ return
categories_generator = self.connection.expense_categories.get_all_generator()
# rest of the codeApply this diff to sync_vendors:
def sync_vendors(self):
"""
Sync vendors
"""
+ attribute_count = self.connection.vendors.count()
+ if not self.is_sync_allowed(attribute_type='vendors', attribute_count=attribute_count):
+ logger.info('Skipping sync of vendors for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count)
+ return
# rest of the codeCommittable suggestion was skipped due to low confidence.
apps/netsuite/connector.py
Outdated
| Returns: | ||
| bool: True | ||
| """ | ||
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at |
| mocker.patch( | ||
| 'netsuitesdk.api.classifications.Classifications.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
write test for high limits
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
| ) | ||
|
|
||
| mocker.patch( | ||
| 'netsuitesdk.api.classifications.Classifications.count', |
There was a problem hiding this comment.
let's write test for all apis, also delete destination attributes and then do .sync()
mock return count for all apis to be limit + 1
destination_attributes.delete(workspace_id=asdasd)
sync_dimensions()
destination_attributes.count() -> assert
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/netsuite/connector.py (8 hunks)
- tests/test_netsuite/test_connector.py (7 hunks)
🧰 Additional context used
🪛 Ruff
apps/netsuite/connector.py
95-98: Return the negated condition directly
Inline condition
(SIM103)
🔇 Additional comments (8)
apps/netsuite/connector.py (3)
4-4: New import addedThe
timezonemodule fromdjango.utilshas been imported. This is likely used in the newis_sync_allowedmethod for datetime comparisons.
33-39: Update to SYNC_UPPER_LIMIT dictionaryNew limits have been added for various attribute types. This change seems to be related to the new synchronization limits feature.
1-1: Overall assessment of changesThe changes in this file implement a new feature for synchronization limits in the NetSuite connector. This feature adds checks to prevent excessive synchronization of various attributes (locations, classifications, departments, vendors, projects, and customers) based on count limits and the workspace creation date.
Key improvements:
- The new
is_sync_allowedmethod provides a centralized way to check sync limits.- Sync methods for various attributes have been updated to use this new limit check.
- Handling of active/inactive items has been improved in some methods.
Main areas for improvement:
- The
is_sync_allowedmethod can be simplified and its logic clarified.- Consider using a decorator to reduce code duplication in sync methods.
- Simplify the attribute creation logic in
sync_projectsandsync_customersmethods.- Ensure consistent error handling and logging across all sync methods.
These changes enhance the robustness of the NetSuite connector by preventing potential issues with over-synchronization. With the suggested improvements, the code will be more maintainable and consistent.
tests/test_netsuite/test_connector.py (5)
136-139: Repeated Mocking of Count MethodsThe mocking of the
countmethod forVendorsis repeated in multiple tests. This issue has been previously identified.
201-204: Repeated Mocking of Count MethodsThe repetitive mocking of
Accounts.counthas been noted before. Refactoring to reduce duplication is recommended.
336-339: Optimize Mock Patch Context ManagersThe suggestion to use context managers for
mocker.patchcalls has been made in prior reviews.
357-360: Inconsistent Mocked Return Value with Test AssertionThis concern has been previously raised regarding the mismatch between mocked return values and test assertions.
441-444: Potential Typographical Error in IndentationA prior comment has addressed the indentation issue on line 438.
| def is_sync_allowed(self, attribute_type: str, attribute_count: int): | ||
| """ | ||
| Checks if the sync is allowed | ||
|
|
||
| Returns: | ||
| bool: True | ||
| """ | ||
| if attribute_count > SYNC_UPPER_LIMIT[attribute_type]: | ||
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at | ||
| if workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone()): | ||
| return False | ||
| else: | ||
| return True | ||
|
|
||
| return True |
There was a problem hiding this comment.
New method: is_sync_allowed
This method implements the new synchronization limit feature. However, there are a few issues to address:
- The method always returns
Trueif theattribute_countis within the limit, regardless of the workspace creation date. - The date comparison logic can be simplified.
- The return value description in the docstring is incomplete.
Consider refactoring the method as follows:
def is_sync_allowed(self, attribute_type: str, attribute_count: int) -> bool:
"""
Checks if the sync is allowed based on attribute count and workspace creation date.
Args:
attribute_type (str): The type of attribute being synced.
attribute_count (int): The count of attributes.
Returns:
bool: True if sync is allowed, False otherwise.
"""
if attribute_count <= SYNC_UPPER_LIMIT[attribute_type]:
return True
workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at
cutoff_date = timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone())
return workspace_created_at <= cutoff_dateThis refactored version:
- Simplifies the logic
- Improves readability
- Correctly implements the intended behavior
- Provides a more complete docstring
🧰 Tools
🪛 Ruff
95-98: Return the negated condition directly
Inline condition
(SIM103)
| attribute_count = self.connection.locations.count() | ||
| if not self.is_sync_allowed(attribute_type = 'locations', attribute_count = attribute_count): | ||
| logger.info('Skipping sync of locations for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implementation of sync limits in multiple methods
The new is_sync_allowed method has been integrated into several sync methods (sync_locations, sync_classifications, sync_departments, sync_vendors). This is a good implementation of the new feature.
However, to improve code maintainability and reduce duplication, consider creating a decorator for this check.
Create a decorator to handle the sync limit check:
from functools import wraps
def check_sync_limit(attribute_type):
def decorator(func):
@wraps(func)
def wrapper(self, *args, **kwargs):
attribute_count = getattr(self.connection, attribute_type).count()
if not self.is_sync_allowed(attribute_type=attribute_type, attribute_count=attribute_count):
logger.info(
'Skipping sync of %s for workspace %s as it has %s counts which is over the limit',
attribute_type, self.workspace_id, attribute_count
)
return
return func(self, *args, **kwargs)
return wrapper
return decoratorThen use it in the sync methods:
@check_sync_limit('locations')
def sync_locations(self):
# Existing method body
@check_sync_limit('classifications')
def sync_classifications(self):
# Existing method body
@check_sync_limit('departments')
def sync_departments(self):
# Existing method body
@check_sync_limit('vendors')
def sync_vendors(self):
# Existing method bodyThis approach will centralize the sync limit logic and make it easier to maintain and modify in the future.
Also applies to: 590-594, 619-622, 647-651
| attribute_count = self.connection.projects.count() | ||
| if not self.is_sync_allowed(attribute_type = 'projects', attribute_count = attribute_count): | ||
| logger.info('Skipping sync of projects for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) | ||
| return | ||
|
|
||
| projects_generator = self.connection.projects.get_all_generator() | ||
| for projects in projects_generator: | ||
| attributes = [] | ||
| destination_ids = DestinationAttribute.objects.filter( | ||
| workspace_id=self.workspace_id, | ||
| attribute_type= 'PROJECT', | ||
| display_name='Project' | ||
| ).values_list('destination_id', flat=True) | ||
|
|
||
| for project in projects: | ||
| value = self.__decode_project_or_customer_name(project['entityId']) | ||
| for project in projects: | ||
| value = self.__decode_project_or_customer_name(project['entityId']) | ||
|
|
||
| if project['internalId'] in destination_ids : | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Project', | ||
| 'value': value, | ||
| 'destination_id': project['internalId'], | ||
| 'active': not project['isInactive'] | ||
| }) | ||
| elif not project['isInactive']: | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Project', | ||
| 'value': value, | ||
| 'destination_id': project['internalId'], | ||
| 'active': True | ||
| }) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| attributes, 'PROJECT', self.workspace_id, True) | ||
| if project['internalId'] in destination_ids : | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Project', | ||
| 'value': value, | ||
| 'destination_id': project['internalId'], | ||
| 'active': not project['isInactive'] | ||
| }) | ||
| elif not project['isInactive']: | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Project', | ||
| 'value': value, | ||
| 'destination_id': project['internalId'], | ||
| 'active': True | ||
| }) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| attributes, 'PROJECT', self.workspace_id, True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Changes in sync_projects method
The sync_projects method has been updated to include the sync limit check and improve the handling of active/inactive projects. These changes align with the new sync limit feature and improve the overall robustness of the method.
However, there's an opportunity to simplify the logic for adding attributes.
Consider refactoring the attribute creation logic as follows:
for project in projects:
value = self.__decode_project_or_customer_name(project['entityId'])
attributes.append({
'attribute_type': 'PROJECT',
'display_name': 'Project',
'value': value,
'destination_id': project['internalId'],
'active': not project['isInactive']
})
DestinationAttribute.bulk_create_or_update_destination_attributes(
attributes, 'PROJECT', self.workspace_id, True)This simplification removes the need for separate conditions based on destination_ids and isInactive, making the code more concise and easier to maintain.
| attribute_count = self.connection.customers.count() | ||
| if not self.is_sync_allowed(attribute_type = 'customers', attribute_count = attribute_count): | ||
| logger.info('Skipping sync of customers for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) | ||
| return | ||
|
|
||
| customers_generator = self.connection.customers.get_all_generator() | ||
|
|
||
| for customers in customers_generator: | ||
| attributes = [] | ||
| destination_ids = DestinationAttribute.objects.filter(workspace_id=self.workspace_id,\ | ||
| attribute_type= 'PROJECT', display_name='Customer').values_list('destination_id', flat=True) | ||
| for customer in customers: | ||
| value = self.__decode_project_or_customer_name(customer['entityId']) | ||
| if customer['internalId'] in destination_ids : | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Customer', | ||
| 'value': value, | ||
| 'destination_id': customer['internalId'], | ||
| 'active': not customer['isInactive'] | ||
| }) | ||
| elif not customer['isInactive']: | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Customer', | ||
| 'value': value, | ||
| 'destination_id': customer['internalId'], | ||
| 'active': True | ||
| }) | ||
| for customers in customers_generator: | ||
| attributes = [] | ||
| destination_ids = DestinationAttribute.objects.filter(workspace_id=self.workspace_id,\ | ||
| attribute_type= 'PROJECT', display_name='Customer').values_list('destination_id', flat=True) | ||
| for customer in customers: | ||
| value = self.__decode_project_or_customer_name(customer['entityId']) | ||
| if customer['internalId'] in destination_ids : | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Customer', | ||
| 'value': value, | ||
| 'destination_id': customer['internalId'], | ||
| 'active': not customer['isInactive'] | ||
| }) | ||
| elif not customer['isInactive']: | ||
| attributes.append({ | ||
| 'attribute_type': 'PROJECT', | ||
| 'display_name': 'Customer', | ||
| 'value': value, | ||
| 'destination_id': customer['internalId'], | ||
| 'active': True | ||
| }) | ||
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| attributes, 'PROJECT', self.workspace_id, True) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| attributes, 'PROJECT', self.workspace_id, True) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Changes in sync_customers method
Similar to the sync_projects method, sync_customers has been updated with the sync limit check and improved handling of active/inactive customers. The same simplification can be applied here as well.
Refactor the attribute creation logic in sync_customers as follows:
for customer in customers:
value = self.__decode_project_or_customer_name(customer['entityId'])
attributes.append({
'attribute_type': 'PROJECT',
'display_name': 'Customer',
'value': value,
'destination_id': customer['internalId'],
'active': not customer['isInactive']
})
DestinationAttribute.bulk_create_or_update_destination_attributes(
attributes, 'PROJECT', self.workspace_id, True)This simplification will make the code more consistent with the suggested changes in the sync_projects method.
| def test_sync_limit(mocker, db): | ||
| mocker.patch( | ||
| 'netsuitesdk.api.projects.Projects.count', | ||
| return_value=10001 | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| 'netsuitesdk.api.classifications.Classifications.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.accounts.Accounts.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.locations.Locations.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.departments.Departments.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.customers.Customers.count', | ||
| return_value=25001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.vendors.Vendors.count', | ||
| return_value=20001 | ||
| ) | ||
|
|
||
| today = datetime.today() | ||
| Workspace.objects.filter(id=1).update(created_at=today) | ||
| netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=1) | ||
| netsuite_connection = NetSuiteConnector(netsuite_credentials=netsuite_credentials, workspace_id=1) | ||
|
|
||
| Mapping.objects.filter(workspace_id=1).delete() | ||
| CategoryMapping.objects.filter(workspace_id=1).delete() | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='PROJECT').delete() | ||
|
|
||
| netsuite_connection.sync_projects() | ||
|
|
||
| new_project_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type='PROJECT').count() | ||
| assert new_project_count == 0 | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='CLASS').delete() | ||
|
|
||
| netsuite_connection.sync_classifications() | ||
|
|
||
| classifications = DestinationAttribute.objects.filter(attribute_type='CLASS', workspace_id=1).count() | ||
| assert classifications == 0 | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='ACCOUNT').delete() | ||
|
|
||
| netsuite_connection.sync_accounts() | ||
|
|
||
| new_project_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type='ACCOUNT').count() | ||
| assert new_project_count == 0 | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='LOCATION').delete() | ||
|
|
||
| netsuite_connection.sync_locations() | ||
|
|
||
| new_project_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type='LOCATION').count() | ||
| assert new_project_count == 0 | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='DEPARTMENT').delete() | ||
|
|
||
| netsuite_connection.sync_departments() | ||
|
|
||
| new_project_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type='DEPARTMENT').count() | ||
| assert new_project_count == 0 | ||
|
|
||
| DestinationAttribute.objects.filter(workspace_id=1, attribute_type='CUSTOMER').delete() | ||
|
|
||
| netsuite_connection.sync_customers() | ||
|
|
||
| new_project_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type='CUSTOMER').count() | ||
| assert new_project_count == 0 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor test_sync_limit for Better Maintainability
The test_sync_limit function is lengthy and tests multiple attributes sequentially. Consider refactoring it to improve readability and maintainability.
Suggestion: Parameterize the Test Function
You can use pytest.mark.parametrize to parameterize the test cases for different attribute types and their counts. This will reduce code duplication and make the test more scalable.
Here's how you might refactor the test:
import pytest
@pytest.mark.parametrize('attribute_type, mock_count_return_value', [
('PROJECT', 10001),
('CLASS', 2001),
('ACCOUNT', 2001),
('LOCATION', 2001),
('DEPARTMENT', 2001),
('CUSTOMER', 25001),
('VENDOR', 20001),
])
def test_sync_limit(mocker, db, attribute_type, mock_count_return_value):
mocker.patch(
f'netsuitesdk.api.{attribute_type.lower()}s.{attribute_type.capitalize()}s.count',
return_value=mock_count_return_value
)
Workspace.objects.filter(id=1).update(created_at=datetime.today())
netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=1)
netsuite_connection = NetSuiteConnector(netsuite_credentials=netsuite_credentials, workspace_id=1)
DestinationAttribute.objects.filter(workspace_id=1, attribute_type=attribute_type).delete()
sync_method = getattr(netsuite_connection, f'sync_{attribute_type.lower()}s', None)
if sync_method:
sync_method()
new_attribute_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type=attribute_type).count()
assert new_attribute_count == 0This refactoring:
- Utilizes
pytest.mark.parametrizeto run the test for each attribute type. - Dynamically patches the
countmethod for each attribute. - Calls the corresponding
syncmethod for each attribute. - Reduces code repetition and enhances clarity.
| 'netsuitesdk.api.classifications.Classifications.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.accounts.Accounts.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.locations.Locations.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.departments.Departments.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.customers.Customers.count', | ||
| return_value=25001 | ||
| ) | ||
| mocker.patch( | ||
| 'netsuitesdk.api.vendors.Vendors.count', | ||
| return_value=20001 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate Mocking of Multiple Attributes' count Methods
Currently, you are individually mocking the count methods for multiple attributes. This can be streamlined.
Suggestion: Use a Helper Function to Mock Counts
Create a helper function or fixture to mock the count methods for multiple attributes at once. This adheres to the DRY principle and simplifies the test setup.
Example using a fixture:
# In your fixtures file (e.g., conftest.py)
import pytest
@pytest.fixture
def mock_counts(mocker):
def _mock_counts(attribute_counts):
for attribute, count in attribute_counts.items():
api_module = f'netsuitesdk.api.{attribute.lower()}s.{attribute.capitalize()}s.count'
mocker.patch(api_module, return_value=count)
return _mock_countsThen, modify your test:
def test_sync_limit(mocker, db, mock_counts):
attribute_counts = {
'Projects': 10001,
'Classifications': 2001,
'Accounts': 2001,
'Locations': 2001,
'Departments': 2001,
'Customers': 25001,
'Vendors': 20001,
}
mock_counts(attribute_counts)
Workspace.objects.filter(id=1).update(created_at=datetime.today())
netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=1)
netsuite_connection = NetSuiteConnector(netsuite_credentials=netsuite_credentials, workspace_id=1)
for attribute in attribute_counts.keys():
attribute_type = attribute.upper()
DestinationAttribute.objects.filter(workspace_id=1, attribute_type=attribute_type).delete()
sync_method_name = f'sync_{attribute.lower()}'
sync_method = getattr(netsuite_connection, sync_method_name, None)
if sync_method:
sync_method()
new_count = DestinationAttribute.objects.filter(workspace_id=1, attribute_type=attribute_type).count()
assert new_count == 0This approach:
- Centralizes the mocking logic.
- Makes it easier to adjust mock return values.
- Enhances test readability.
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
* feat: Added limits to attributes * resolved comments and added test * added test * renamed function * fixed test
Description
Please add PR description here, add screenshots if needed
Clickup
Please add link here
https://app.clickup.com/1864988/v/l/6-901603904304-1
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
netsuitesdkdependency to the latest version for better performance and features.