feat: Added limits to attributes #564
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes introduce 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: 2
🧹 Outside diff range and nitpick comments (2)
apps/sage_intacct/utils.py (2)
117-117: Avoid hardcoding dates; consider making the date configurableHardcoding the date
datetime(2024, 10, 1)may lead to maintenance issues in the future. Consider defining the date as a constant or retrieving it from a configuration to improve flexibility.Apply this diff to define the date as a constant:
+ SYNC_START_DATE = datetime(2024, 10, 1) workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at - if workspace_created_at > datetime(2024, 10, 1) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]: + if workspace_created_at > SYNC_START_DATE and attribute_count > SYNC_UPPER_LIMIT[attribute_type]:Alternatively, define
SYNC_START_DATEat the class or module level for broader accessibility.🧰 Tools
🪛 Ruff
117-120: Return the negated condition directly
Inline condition
(SIM103)
150-151: Correct grammatical errors in log messagesThe log messages contain minor grammatical errors. Adjust them for clarity and correctness.
Apply this diff to correct the messages:
- logger.info('Skipping sync of accounts for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of accounts for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count) - 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 items which are over the limit', self.workspace_id, attribute_count) - logger.info('Skipping sync of expense_type for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of expense types for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count) - logger.info('Skipping sync of cost_types for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of cost types for workspace %s as it has %s items which are over the limit', self.workspace_id, 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) + logger.info('Skipping sync of projects for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count) - logger.info('Skipping sync of items for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of items for workspace %s as it has %s items which are over the limit', self.workspace_id, 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) + logger.info('Skipping sync of locations for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count) - logger.info('Skipping sync of classes for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of classes for workspace %s as it has %s items which are over the limit', self.workspace_id, 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) + logger.info('Skipping sync of customers for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count) - logger.info('Skipping sync of tax_details for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count) + logger.info('Skipping sync of tax details for workspace %s as it has %s items which are over the limit', self.workspace_id, attribute_count)Also applies to: 198-199, 235-236, 325-326, 356-357, 400-401, 433-434, 650-651, 678-679, 707-708
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- apps/sage_intacct/utils.py (8 hunks)
🧰 Additional context used
🪛 Ruff
apps/sage_intacct/utils.py
117-120: Return the negated condition directly
Inline condition
(SIM103)
🔇 Additional comments (1)
apps/sage_intacct/utils.py (1)
117-120: [Verify] Ensure theSYNC_UPPER_LIMITdictionary includes all necessary attribute typesVerify that all attribute types used in the
is_sync_allowedmethod are present in theSYNC_UPPER_LIMITdictionary to preventKeyErrorexceptions.Run the following script to list all attribute types checked and ensure they are keys in
SYNC_UPPER_LIMIT:🧰 Tools
🪛 Ruff
117-120: Return the negated condition directly
Inline condition
(SIM103)
| if not self.is_sync_allowed(attribute_type = 'accounts', attribute_count=attribute_count): | ||
| logger.info('Skipping sync of accounts 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
Refactor repetitive sync checks to a helper method
The pattern of obtaining attribute_count, checking is_sync_allowed, logging, and returning is repeated across multiple sync_* methods. Consider refactoring this logic into a helper function to reduce code duplication and enhance maintainability.
Here's an example of how you might refactor the repeated code:
def check_and_sync(self, attribute_name: str, attribute_type: str, sync_function):
attribute_count = getattr(self.connection, attribute_name).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 items which are over the limit', attribute_type, self.workspace_id, attribute_count)
return
sync_function()Then, update your sync_* methods accordingly:
- def sync_accounts(self):
+ def sync_accounts(self):
"""
Get accounts
"""
- attribute_count = self.connection.accounts.count()
- if not self.is_sync_allowed(attribute_type='accounts', attribute_count=attribute_count):
- logger.info('Skipping sync of accounts for workspace %s as it has %s counts which is over the limit', self.workspace_id, attribute_count)
- return
+ self.check_and_sync('accounts', 'accounts', self._sync_accounts_logic)
+ def _sync_accounts_logic(self):
+ # Existing logic for syncing accounts goes hereRepeat this pattern for the other sync_* methods by moving their existing logic into corresponding private methods (e.g., _sync_departments_logic) and calling them through check_and_sync.
Also applies to: 196-200, 233-237, 323-327, 354-358, 399-402, 431-434, 648-652, 676-680, 705-709
apps/sage_intacct/utils.py
Outdated
| if workspace_created_at > datetime(2024, 10, 1) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]: | ||
| return False | ||
|
|
||
| return True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the return statement by returning the condition directly
The is_sync_allowed method can be simplified by returning the negated condition directly, improving readability and conciseness.
Apply this diff to simplify the return statement:
workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at
- if workspace_created_at > datetime(2024, 10, 1) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]:
- return False
-
- return True
+ return not (workspace_created_at > datetime(2024, 10, 1) 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.
| if workspace_created_at > datetime(2024, 10, 1) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]: | |
| return False | |
| return True | |
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at | |
| return not (workspace_created_at > datetime(2024, 10, 1) and attribute_count > SYNC_UPPER_LIMIT[attribute_type]) |
🧰 Tools
🪛 Ruff
117-120: Return the negated condition directly
Inline condition
(SIM103)
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
tests/test_mappings/test_imports/test_modules/test_tax_groups.py (2)
Line range hint
10-10: Fix the typo in the function nameThe function name
test_sync_destination_atrributesis misspelled. It should betest_sync_destination_attributes.Apply this diff to correct the function name:
-def test_sync_destination_atrributes(mocker, db): +def test_sync_destination_attributes(mocker, db):
Line range hint
40-40: Fix the typo in the function nameThe function name
test_sync_expense_atrributesis misspelled. It should betest_sync_expense_attributes.Apply this diff to correct the function name:
-def test_sync_expense_atrributes(mocker, db): +def test_sync_expense_attributes(mocker, db):tests/test_mappings/test_tasks.py (2)
140-147: Consider adding assertions to validate synchronization with non-zero countsYou've appropriately mocked the
Locations.countandDepartments.countmethods to return1, simulating scenarios where these entities are present. To enhance test robustness, consider adding assertions that verify the synchronization logic handles these counts as expected.
173-177: Ensure test coverage for scenarios with zero cost typesMocking
CostTypes.countto return0effectively simulates the absence of cost types. Adding assertions to confirm that the synchronization logic correctly handles this scenario will strengthen the test and ensure that edge cases are properly validated.tests/test_sageintacct/test_utils.py (1)
Line range hint
510-542: Unusual attribute key'GLDIMWHAT_IS_NILESH_PANT'in test assertionIn the
test_sync_allocationsfunction, the assertion checks for'GLDIMWHAT_IS_NILESH_PANT'as a key inallocation_attribute.detail.keys(). This attribute name appears unconventional and might be a placeholder or temporary identifier. Please verify if this is intentional and consider renaming it to reflect its purpose more clearly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- apps/sage_intacct/utils.py (9 hunks)
- tests/test_mappings/test_imports/test_modules/test_categories.py (7 hunks)
- tests/test_mappings/test_imports/test_modules/test_expense_custom_fields.py (1 hunks)
- tests/test_mappings/test_imports/test_modules/test_tax_groups.py (2 hunks)
- tests/test_mappings/test_tasks.py (2 hunks)
- tests/test_sageintacct/test_utils.py (5 hunks)
🧰 Additional context used
🪛 Ruff
apps/sage_intacct/utils.py
118-121: Return the negated condition directly
Inline condition
(SIM103)
🔇 Additional comments (13)
apps/sage_intacct/utils.py (6)
149-153: LGTM: Sync allowance check addedThe addition of the sync allowance check at the beginning of the
sync_accountsmethod is a good improvement. It prevents unnecessary processing when the sync is not allowed and provides informative logging.
197-201: LGTM: Consistent sync allowance check addedThe addition of the sync allowance check in the
sync_departmentsmethod is consistent with the changes in thesync_accountsmethod. This ensures that department synchronization adheres to the defined limits and prevents unnecessary processing when sync is not allowed.
234-238: LGTM: Sync allowance check added for expense typesThe addition of the sync allowance check in the
sync_expense_typesmethod is consistent with the changes in other sync methods. This ensures that expense type synchronization adheres to the defined limits and prevents unnecessary processing when sync is not allowed.
355-391: LGTM: Sync allowance check and formatting improvementsThe changes in the
sync_projectsmethod include:
- Addition of a sync allowance check, consistent with other sync methods.
- Improved code formatting with better whitespace usage.
These changes enhance sync control and improve code readability.
399-423: LGTM: Sync allowance check and formatting improvements for itemsThe changes in the
sync_itemsmethod include:
- Addition of a sync allowance check, consistent with other sync methods.
- Improved code formatting with better whitespace usage.
These changes enhance sync control and improve code readability.
649-669: LGTM: Sync allowance check and formatting improvements for classesThe changes in the
sync_classesmethod include:
- Addition of a sync allowance check, consistent with other sync methods.
- Improved code formatting with better whitespace usage.
These changes enhance sync control and improve code readability.
tests/test_mappings/test_imports/test_modules/test_tax_groups.py (2)
15-18: LGTM!The mocking of
sageintacctsdk.apis.TaxDetails.countis set up correctly to return100, which aligns with the test requirements.
87-90: LGTM!The additional mock setup for
sageintacctsdk.apis.TaxDetails.countis appropriate and ensures consistent test behavior by returning100.tests/test_mappings/test_imports/test_modules/test_categories.py (2)
42-46: Refactor repeated mocks into fixtures or helper functionsSimilarly, the mock for
'sageintacctsdk.apis.ExpenseTypes.count'with a return value of5is repeated. Refactoring this into a shared fixture can improve code reusability.
238-241: Verify handling of zero accounts in testsThe mocked return value of
'sageintacctsdk.apis.Accounts.count'is set to0. Please ensure that the test correctly handles cases where there are no accounts and that all assertions reflect the expected behavior in this scenario.tests/test_sageintacct/test_utils.py (3)
79-82: Properly mocked'Departments.count'in the testThe addition of the mock for
'sageintacctsdk.apis.Departments.count'ensures that the test simulates department counts accurately, leading to more reliable test outcomes.
102-106: Properly mocked'ExpenseTypes.count'in the testIncluding the mock for
'sageintacctsdk.apis.ExpenseTypes.count'allows the test to correctly simulate the number of expense types, enhancing the test's reliability.
221-224: Properly mocked'Locations.count'in the testAdding the mock for
'sageintacctsdk.apis.Locations.count'ensures the test accurately reflects location counts, contributing to more dependable test results.
apps/sage_intacct/utils.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 the return statement and add error handling
The is_sync_allowed method can be improved in two ways:
- Simplify the return statement by directly returning the boolean expression.
- Add error handling for cases where the
attribute_typeis not in theSYNC_UPPER_LIMITdictionary.
Here's a suggested implementation:
def is_sync_allowed(self, attribute_type: str, attribute_count: int):
"""
Checks if the sync is allowed
Returns:
- bool: True
+ 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
+ if attribute_type not in SYNC_UPPER_LIMIT:
+ logger.warning(f"Unknown attribute type: {attribute_type}")
+ return False
+
+ return not (
+ workspace_created_at > timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone())
+ and attribute_count > SYNC_UPPER_LIMIT[attribute_type]
+ )This change simplifies the logic, improves readability, and adds error handling for unknown attribute types.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 Ruff
118-121: Return the negated condition directly
Inline condition
(SIM103)
| mocker.patch( | ||
| 'sageintacctsdk.apis.Locations.count', | ||
| return_value=21 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent use of mock.patch and mocker.patch
In this test function, both mock.patch (from the unittest.mock module) and mocker.patch (from the pytest-mock plugin) are used. Mixing these two can lead to confusion and potential issues in test behavior. It's recommended to use one mocking approach consistently throughout your tests to enhance readability and maintainability. Since mock.patch is used in your context managers elsewhere in the test, consider replacing mocker.patch with mock.patch.
Apply this diff to standardize the mocking framework:
- mocker.patch(
+ with mock.patch(
'sageintacctsdk.apis.Locations.count',
return_value=21
- )
+ ):
+ # Place the code that requires the patched method hereAlternatively, if you prefer using mocker.patch, replace all instances of mock.patch with mocker.patch.
Committable suggestion was skipped due to low confidence.
| mocker.patch( | ||
| 'sageintacctsdk.apis.Accounts.count', | ||
| return_value=2 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated mocks into fixtures or helper functions
The mock patching of 'sageintacctsdk.apis.Accounts.count' with a return value of 2 is duplicated across multiple tests. Consider refactoring this mock into a fixture or helper function to reduce code duplication and enhance maintainability.
| mocker.patch( | ||
| 'sageintacctsdk.apis.TaxDetails.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
Inconsistency between mocked 'TaxDetails.count' and test assertions
The mock for 'sageintacctsdk.apis.TaxDetails.count' returns 5, but both tax_code_count and new_tax_code_count are asserted to be 0. This inconsistency may lead to confusion or inaccurate test results. Please verify and adjust the return value or assertions accordingly to ensure the test reflects the intended scenario.
| mocker.patch( | ||
| 'sageintacctsdk.apis.Accounts.count', | ||
| return_value=5 | ||
| ) |
There was a problem hiding this comment.
Inconsistency between mocked 'Accounts.count' and test assertions
The mock for 'sageintacctsdk.apis.Accounts.count' returns 5, yet both account_count and new_account_count are asserted to be 170. This mismatch could cause the test to produce misleading results. Please ensure the mocked return value aligns with the expected counts to maintain test accuracy.
| mocker.patch( | ||
| 'sageintacctsdk.apis.Departments.count', | ||
| return_value=2 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated mock patches into a reusable fixture
The mock patches for '*.count' methods are repeated across multiple tests. Refactoring these into a common fixture or helper function can reduce code duplication and improve test maintainability.
Also applies to: 102-106, 221-224, 453-456, 476-479
|
apps/sage_intacct/utils.py
Outdated
| Returns: | ||
| bool: True | ||
| """ | ||
| workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at |
There was a problem hiding this comment.
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
output doesn't change but this queries db only when count is > limit
apps/sage_intacct/utils.py
Outdated
| 'detail': detail | ||
| }) | ||
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
There was a problem hiding this comment.
upsert should happen per page
apps/sage_intacct/utils.py
Outdated
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| item_attributes, 'ITEM', self.workspace_id, True) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
There was a problem hiding this comment.
upsert should happen per page
apps/sage_intacct/utils.py
Outdated
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| class_attributes, 'CLASS', self.workspace_id, True) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
apps/sage_intacct/utils.py
Outdated
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( | ||
| customer_attributes, 'CUSTOMER', self.workspace_id, True) | ||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
| @@ -459,6 +473,10 @@ def test_sync_tax_details(mocker, db): | |||
| def tests_sync_accounts(mocker, db): | |||
| workspace_id = 1 | |||
|
|
|||
| mocker.patch( | |||
There was a problem hiding this comment.
pls to add tests for skip case by mocking and returning high values
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
tests/test_sageintacct/test_utils.py (2)
936-969: LGTM: New skip sync test addedGreat addition of the
test_skip_sync_attributesfunction. This test covers the important case of skipping synchronization when the number of attributes is too high. It effectively addresses the previously requested skip case tests.A small suggestion for improvement:
Instead of usingdatetime.today(), consider using a fixed date orfreezegunlibrary to make the test deterministic. This will prevent potential issues if the test is run close to midnight or if the date affects the behavior in any way.
Line range hint
1-969: Overall improvements with some remaining issuesThe changes in this file generally improve the test coverage and structure:
- New mocks have been added for various API calls, enhancing the test isolation.
- A new test function
test_skip_sync_attributeshas been implemented, addressing the previously requested skip case scenarios.- Code readability has been improved with better spacing between functions.
However, there are still some areas that need attention:
- Some inconsistencies between mocked values and assertions remain, as noted in previous comments.
- Consider refactoring repeated mock patches into reusable fixtures, as suggested earlier.
- The new test function could be made more deterministic by using a fixed date instead of
datetime.today().Please address these remaining issues to further improve the quality and reliability of the test suite.
apps/sage_intacct/utils.py (1)
358-385: LGTM! Good improvements to the sync_projects method.The changes to the
sync_projectsmethod are well-implemented:
- The addition of the
is_sync_allowedcheck is consistent with other sync methods, preventing excessive syncing.- The updated method structure improves readability and potentially efficiency.
- The use of
construct_get_all_generator_paramsandis_import_enabledmethods suggests good code reuse.One minor suggestion for improvement:
Consider renaming the
paramsvariable to something more descriptive, such asgenerator_paramsorsync_params, to better reflect its purpose:generator_params = self.construct_get_all_generator_params(fields=fields, latest_updated_at=latest_updated_at) project_generator = self.connection.projects.get_all_generator(**generator_params)This small change could further enhance code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- apps/sage_intacct/utils.py (13 hunks)
- tests/test_sageintacct/test_utils.py (7 hunks)
🧰 Additional context used
🪛 Ruff
apps/sage_intacct/utils.py
119-122: Return the negated condition directly
Inline condition
(SIM103)
🔇 Additional comments (13)
tests/test_sageintacct/test_utils.py (5)
3-3: LGTM: Necessary import addedThe addition of the
datetimeimport is correct and will be used in the new test case added later in the file.
80-83: Consider using a more flexible mocking approachThe new mock for 'sageintacctsdk.apis.Departments.count' is correctly implemented. However, consider using a variable or parameterized value instead of a fixed return value of 2. This would allow for testing different scenarios more easily.
As previously suggested, consider refactoring repeated mock patches into a reusable fixture to improve test maintainability.
454-457: New mock added, but inconsistency remainsThe new mock for 'sageintacctsdk.apis.TaxDetails.count' is correctly implemented. However, the previously noted inconsistency between the mocked count (5) and the test assertions (0) still exists. Please review and adjust either the mock return value or the assertions to ensure the test accurately reflects the intended scenario.
477-480: New mock added, but issues remainThe new mock for 'sageintacctsdk.apis.Accounts.count' is correctly implemented. However:
The previously noted inconsistency between the mocked count (5) and the test assertions (170) still exists. Please review and adjust either the mock return value or the assertions to ensure the test accurately reflects the intended scenario.
The requested tests for the skip case have not been implemented yet. Consider adding these tests to improve coverage.
934-935: LGTM: Improved readabilityThe addition of blank lines improves the readability of the code by clearly separating test functions.
apps/sage_intacct/utils.py (8)
152-156: LGTM! Good addition of sync limit check.The addition of the
is_sync_allowedcheck is a good improvement to prevent excessive syncing. The log message is informative and includes relevant details. The early return prevents unnecessary processing if sync is not allowed.
200-204: LGTM! Consistent implementation of sync limit check.The addition of the
is_sync_allowedcheck in thesync_departmentsmethod is consistent with the improvements in thesync_accountsmethod. This ensures a uniform approach to preventing excessive syncing across different entity types.
237-241: LGTM! Consistent implementation of sync limit check for expense types.The addition of the
is_sync_allowedcheck in thesync_expense_typesmethod maintains consistency with other sync methods. This uniform approach to preventing excessive syncing across different entity types is commendable.
435-438: LGTM! Consistent implementation of sync limit check for locations.The addition of the
is_sync_allowedcheck in thesync_locationsmethod maintains consistency with other sync methods. This uniform approach to preventing excessive syncing across different entity types is commendable.
652-669: LGTM! Good improvements to the sync_classes method.The changes to the
sync_classesmethod are well-implemented:
- The addition of the
is_sync_allowedcheck is consistent with other sync methods, preventing excessive syncing.- The updated method structure improves readability.
- The use of a generator for fetching classes suggests efficient data handling.
These changes maintain consistency with other sync methods and improve the overall functionality of the method.
680-698: LGTM! Consistent improvements to the sync_customers method.The changes to the
sync_customersmethod are well-implemented and consistent with the improvements made to other sync methods:
- The addition of the
is_sync_allowedcheck prevents excessive syncing.- The updated method structure improves readability.
- The use of a generator for fetching customers suggests efficient data handling.
These changes maintain consistency across sync methods and improve the overall functionality of the method.
709-713: LGTM! Consistent implementation of sync limit check for tax details.The addition of the
is_sync_allowedcheck in thesync_tax_detailsmethod maintains consistency with other sync methods. This uniform approach to preventing excessive syncing across different entity types is commendable.
402-423: LGTM! Good improvements to the sync_items method with a query.The changes to the
sync_itemsmethod are well-implemented and consistent with other sync methods. The addition of theis_sync_allowedcheck prevents excessive syncing, which is good.I noticed an interesting condition in the code:
if item['ITEMTYPE'] == 'Non-Inventory': # ... add item to item_attributesCould you provide more context about why only 'Non-Inventory' items are being processed? This selective processing might have implications for the overall synchronization process. If this is intentional, consider adding a comment explaining the rationale. If not, we might need to review if other item types should be included.
To verify this, we can check if there are other item types in use:
This will help us understand if there are other item types that might need to be considered in this synchronization process.
| 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.
🛠️ Refactor suggestion
Improve the is_sync_allowed method for better readability and robustness.
The is_sync_allowed method can be improved in several ways:
- Simplify the return statement for better readability.
- Add error handling for unknown attribute types.
- Consider making the date (October 1, 2024) configurable.
- Add a docstring for better documentation.
Here's a suggested implementation:
def is_sync_allowed(self, attribute_type: str, attribute_count: int) -> bool:
"""
Checks if the sync is allowed based on attribute type and count.
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.
Raises:
ValueError: If the attribute_type is unknown.
"""
if attribute_type not in SYNC_UPPER_LIMIT:
raise ValueError(f"Unknown attribute type: {attribute_type}")
workspace_created_at = Workspace.objects.get(id=self.workspace_id).created_at
sync_limit_date = timezone.make_aware(datetime(2024, 10, 1), timezone.get_current_timezone())
return not (
workspace_created_at > sync_limit_date
and attribute_count > SYNC_UPPER_LIMIT[attribute_type]
)This implementation improves readability, adds error handling, and includes a docstring for better documentation. Consider making the sync_limit_date configurable if needed.
🧰 Tools
🪛 Ruff
119-122: Return the negated condition directly
Inline condition
(SIM103)
| workspace_id = 1 | ||
|
|
||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Classes.count', |
There was a problem hiding this comment.
same comment as netsuite
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
| mocker.patch( | ||
| 'sageintacctsdk.apis.Projects.count', | ||
| return_value=25001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Classes.count', | ||
| return_value=1001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Accounts.count', | ||
| return_value=2001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Locations.count', | ||
| return_value=1001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Departments.count', | ||
| return_value=1001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Customers.count', | ||
| return_value=10001 | ||
| ) | ||
| mocker.patch( | ||
| 'sageintacctsdk.apis.Vendors.count', | ||
| return_value=20001 | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| 'sageintacctsdk.apis.TaxDetails.count', | ||
| return_value=201 | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| 'sageintacctsdk.apis.CostTypes.count', | ||
| return_value=500001 | ||
| ) | ||
|
|
||
| mocker.patch( | ||
| 'sageintacctsdk.apis.ExpenseTypes.count', | ||
| return_value=1001 | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring repeated mock patches into a reusable fixture
In the test_skip_sync_attributes function, the mock patches for 'sageintacctsdk.apis.*.count' are repeated multiple times. Refactoring these mock patches into a common fixture or helper function can reduce code duplication and improve test maintainability.
* feat: Added limits to attributes * test fixed * resolved comments and added test * removed loggers * added 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
Documentation
Tests