feat: Add generators in syncing dimensions #638
Conversation
Ashutosh619-sudo
had a problem deploying
to
CI Environment
GitHub Actions
Failure
WalkthroughThe changes involve a transition from synchronous data retrieval to a generator-based approach in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connector
participant Generator
Client->>Connector: Request data sync
Connector->>Generator: Call get_all_generator()
loop For each batch
Generator-->>Connector: Yield batch of items
loop For each item in batch
Connector->>Connector: Process item
end
end
Connector-->>Client: Sync complete
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/netsuite/connector.py (5 hunks)
Additional comments not posted (4)
apps/netsuite/connector.py (4)
561-574: Review: Implementation of generator insync_classificationsmethod.The method
sync_classificationsuses a generator (classification_generator) effectively to handle potentially large sets of classification data. The structure and logic of the method are consistent with the changes in other syncing methods, which is good for maintaining uniformity across the module.Ensure that the
bulk_create_or_update_destination_attributesfunction is capable of handling the data efficiently without performance issues, especially when dealing with large datasets.
496-510: Review: Implementation of generator insync_currenciesmethod.The transition to using a generator (
currencies_generator) for fetching currencies is implemented correctly. The method iterates over batches of currencies, processing each currency individually within the nested loop. This approach should indeed help in handling large datasets more efficiently by not loading all data into memory at once.However, ensure that the
bulk_create_or_update_destination_attributesfunction is optimized to handle potentially large batches of data without performance degradation.
585-598: Review: Implementation of generator insync_departmentsmethod.The implementation of
department_generatorin thesync_departmentsmethod follows the established pattern of using generators for efficient data handling. The method processes departments in batches, which should help in managing memory usage effectively.As with other methods, verify that the
bulk_create_or_update_destination_attributesfunction is optimized for handling large volumes of data efficiently.
931-945: Review: Implementation of generator insync_subsidiariesmethod.The
sync_subsidiariesmethod correctly uses a generator (subsidiary_generator) to fetch subsidiaries in batches. This approach is consistent with the other changes in the module and is effective for handling large datasets without loading all data into memory.The method processes each subsidiary individually within the nested loop, which is good for memory efficiency. Ensure that the
bulk_create_or_update_destination_attributesfunction is capable of handling these operations efficiently.
apps/netsuite/connector.py
Outdated
| location_generator = self.connection.locations.get_all_generator() | ||
|
|
||
|
|
||
| location_attributes = [] | ||
|
|
||
| for location in locations: | ||
| if not location['isInactive']: | ||
| if 'subsidiaryList' in location and location['subsidiaryList']: | ||
| subsidiaries = location['subsidiaryList']['recordRef'] | ||
| counter = 0 | ||
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | ||
| counter += 1 | ||
| for locations in location_generator: | ||
| for location in locations: | ||
| if not location['isInactive']: | ||
| if 'subsidiaryList' in location and location['subsidiaryList']: | ||
| subsidiaries = location['subsidiaryList']['recordRef'] | ||
| counter = 0 | ||
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | ||
| counter += 1 | ||
| location_attributes.append({ | ||
| 'attribute_type': 'LOCATION', | ||
| 'display_name': 'Location', | ||
| 'value': location['name'], | ||
| 'destination_id': location['internalId'], | ||
| 'active': not location['isInactive'] | ||
| }) | ||
| else: |
There was a problem hiding this comment.
Review: Implementation of generator in sync_locations method.
The use of location_generator to fetch locations in batches is correctly implemented. The nested loop structure allows for incremental processing of each location, which is beneficial for memory management. The additional logic to check for active locations and subsidiary matching is preserved, which is good for maintaining the existing functionality.
One potential issue is the use of a counter (counter) which seems to be intended for iterating over subsidiaries but is not incremented in a loop. This might be a bug or a misunderstanding in the code logic.
Consider revising the logic around the counter variable to ensure it functions as intended, or remove it if it's not necessary.
- counter = 0
- if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id:
- counter += 1
+ for subsidiary in subsidiaries:
+ if subsidiary['internalId'] == subsidiary_mapping.internal_id:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| location_generator = self.connection.locations.get_all_generator() | |
| location_attributes = [] | |
| for location in locations: | |
| if not location['isInactive']: | |
| if 'subsidiaryList' in location and location['subsidiaryList']: | |
| subsidiaries = location['subsidiaryList']['recordRef'] | |
| counter = 0 | |
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | |
| counter += 1 | |
| for locations in location_generator: | |
| for location in locations: | |
| if not location['isInactive']: | |
| if 'subsidiaryList' in location and location['subsidiaryList']: | |
| subsidiaries = location['subsidiaryList']['recordRef'] | |
| counter = 0 | |
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | |
| counter += 1 | |
| location_attributes.append({ | |
| 'attribute_type': 'LOCATION', | |
| 'display_name': 'Location', | |
| 'value': location['name'], | |
| 'destination_id': location['internalId'], | |
| 'active': not location['isInactive'] | |
| }) | |
| else: | |
| location_generator = self.connection.locations.get_all_generator() | |
| location_attributes = [] | |
| for locations in location_generator: | |
| for location in locations: | |
| if not location['isInactive']: | |
| if 'subsidiaryList' in location and location['subsidiaryList']: | |
| subsidiaries = location['subsidiaryList']['recordRef'] | |
| for subsidiary in subsidiaries: | |
| if subsidiary['internalId'] == subsidiary_mapping.internal_id: | |
| location_attributes.append({ | |
| 'attribute_type': 'LOCATION', | |
| 'display_name': 'Location', | |
| 'value': location['name'], | |
| 'destination_id': location['internalId'], | |
| 'active': not location['isInactive'] | |
| }) | |
| else: |
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apps/netsuite/connector.py (5 hunks)
- tests/test_netsuite/fixtures.py (2 hunks)
- tests/test_netsuite/test_connector.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- apps/netsuite/connector.py
Additional comments not posted (4)
tests/test_netsuite/test_connector.py (3)
408-408: Approved: Transition to generator-based method.The change from
get_alltoget_all_generatorintest_sync_currenciesaligns with the PR's objectives to use generators for syncing dimensions. Ensure that the integration with the rest of the system is seamless and that all dependent functionalities are updated accordingly.Run the following script to verify the integration:
Verification successful
Verified: Transition to
get_all_generatorintest_sync_currencies.The
test_sync_currenciesfunction intests/test_netsuite/test_connector.pycorrectly mocks theget_all_generatormethod, confirming the transition to a generator-based approach. This change aligns with the PR's objectives, and no issues were found with this specific update. Ensure that any other dependencies on the old method are also updated if necessary.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in `test_sync_currencies`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'test_sync_currencies'Length of output: 75
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in `test_sync_currencies`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'test_sync_currencies'Length of output: 524
328-328: Approved: Transition to generator-based method.The change from
get_alltoget_all_generatorintest_sync_locationsaligns with the PR's objectives to use generators for syncing dimensions. Ensure that the integration with the rest of the system is seamless and that all dependent functionalities are updated accordingly.Run the following script to verify the integration:
Verification successful
Verified: Successful integration of
get_all_generator.The transition to using
get_all_generatoris well-integrated across the codebase, including in test files such astests/test_netsuite/test_connector.py. This confirms that the generator-based approach is being thoroughly tested and utilized. No further issues were found regarding this integration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in `test_sync_locations`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'test_sync_locations'Length of output: 74
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in the codebase. # Corrected Test: Search for the usage of `get_all_generator` in Python files. rg 'get_all_generator' --type py -A 5Length of output: 19655
363-363: Approved: Transition to generator-based method.The change from
get_alltoget_all_generatorintest_sync_customersaligns with the PR's objectives to use generators for syncing dimensions. Ensure that the integration with the rest of the system is seamless and that all dependent functionalities are updated accordingly.Run the following script to verify the integration:
Verification successful
Verified: Integration of
get_all_generatorintest_sync_customers.The
test_sync_customersfunction intests/test_netsuite/test_connector.pycorrectly uses theget_all_generatormethod, confirming the transition to a generator-based approach. This aligns with the PR's objectives, and no issues were found with the integration.
- Location:
tests/test_netsuite/test_connector.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in `test_sync_customers`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'test_sync_customers'Length of output: 74
Script:
#!/bin/bash # Description: Verify the integration of `get_all_generator` in `test_sync_customers`. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'test_sync_customers'Length of output: 442
tests/test_netsuite/fixtures.py (1)
Line range hint
6557-6760: Ensure proper handling of nested list structure inget_all_locations.The function
get_all_locationshas been modified to return a nested list structure. It is crucial to ensure that:
- The new data structure is correctly implemented.
- All downstream processes that consume this data are updated to handle the nested list structure.
- Adequate error handling and data validation are in place to manage the nested lists effectively.
Please verify that all integrations and dependent modules have been updated to accommodate this change.
| 'active': True | ||
| } | ||
| ) | ||
| for currency in currencies_generator: |
There was a problem hiding this comment.
Can you test this once?
I think this not how generator works
You will have to two for loops
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
Ashutosh619-sudo
added
the
deploy
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
Ashutosh619-sudo
temporarily deployed
to
Staging
GitHub Actions
Inactive
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apps/netsuite/connector.py (5 hunks)
Additional comments not posted (5)
apps/netsuite/connector.py (5)
496-509: Improved efficiency in sync_currencies methodThe modification to use a generator (
get_all_generator()) instead of retrieving all currencies at once is a good improvement. This change can significantly reduce memory usage, especially when dealing with large datasets, as it allows for processing currencies in batches rather than loading all of them into memory at once.The core logic for creating currency attributes remains unchanged, ensuring that the functionality is preserved while improving efficiency.
559-572: Improved efficiency in sync_classifications methodThe modification to use a generator (
get_all_generator()) for retrieving classifications is a good improvement. This change enhances memory efficiency, especially when dealing with large datasets, as it allows for processing classifications in batches rather than loading all of them into memory at once.The core logic for creating classification attributes remains unchanged, ensuring that the functionality is preserved while improving efficiency. The nested loop structure correctly handles the batches of classifications provided by the generator.
583-596: Improved efficiency in sync_departments methodThe change to use a generator (
get_all_generator()) for retrieving departments is a positive improvement. This modification enhances memory efficiency, particularly when dealing with large datasets, as it allows for processing departments in batches rather than loading all of them into memory at once.The core logic for creating department attributes remains unchanged, ensuring that the functionality is preserved while improving efficiency. The nested loop structure correctly handles the batches of departments provided by the generator.
929-943: Improved efficiency in sync_subsidiaries methodThe transition to using a generator (
get_all_generator()) for retrieving subsidiaries is a commendable improvement. This change enhances memory efficiency, particularly when dealing with large datasets, as it allows for processing subsidiaries in batches rather than loading all of them into memory at once.The core logic for creating subsidiary attributes remains unchanged, ensuring that the functionality is preserved while improving efficiency. The nested loop structure correctly handles the batches of subsidiaries provided by the generator.
Line range hint
1-2155: Overall improvement in sync methods, with one minor issueThe changes made to the sync methods (sync_currencies, sync_locations, sync_classifications, sync_departments, and sync_subsidiaries) in this file consistently improve memory efficiency by transitioning from retrieving all records at once to using generators. This approach is particularly beneficial when dealing with large datasets, as it allows for processing data in batches.
The core logic for creating attributes in each method remains largely unchanged, which helps maintain the existing functionality while improving performance.
However, there is a minor issue in the sync_locations method regarding the usage of a counter variable, which should be addressed to ensure correct functionality.
Overall, these changes represent a positive step towards more efficient data synchronization with NetSuite.
| location_generator = self.connection.locations.get_all_generator() | ||
|
|
||
| location_attributes = [] | ||
|
|
||
| for location in locations: | ||
| if not location['isInactive']: | ||
| if 'subsidiaryList' in location and location['subsidiaryList']: | ||
| subsidiaries = location['subsidiaryList']['recordRef'] | ||
| counter = 0 | ||
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | ||
| counter += 1 | ||
| for locations in location_generator: | ||
| for location in locations: | ||
| if not location['isInactive']: | ||
| if 'subsidiaryList' in location and location['subsidiaryList']: | ||
| subsidiaries = location['subsidiaryList']['recordRef'] | ||
| counter = 0 | ||
| if subsidiaries[counter]['internalId'] == subsidiary_mapping.internal_id: | ||
| counter += 1 | ||
| location_attributes.append({ | ||
| 'attribute_type': 'LOCATION', | ||
| 'display_name': 'Location', | ||
| 'value': location['name'], | ||
| 'destination_id': location['internalId'], | ||
| 'active': not location['isInactive'] | ||
| }) | ||
| else: |
There was a problem hiding this comment.
Improved sync_locations method, but check counter logic
The transition to using a generator (get_all_generator()) for retrieving locations is a good improvement for memory efficiency, especially when dealing with large datasets. The nested loop structure allows for incremental processing of each location, which is beneficial.
However, there's an issue with the counter variable:
- It's initialized before the inner loop but incremented inside it.
- It's not reset for each new batch of locations from the generator.
- It's only incremented when a condition is met, which may lead to incorrect indexing.
Consider revising the logic around the counter variable. If it's meant to iterate over subsidiaries, you might want to replace it with a loop over the subsidiaries list. For example:
for location in locations:
if not location['isInactive']:
if 'subsidiaryList' in location and location['subsidiaryList']:
subsidiaries = location['subsidiaryList']['recordRef']
for subsidiary in subsidiaries:
if subsidiary['internalId'] == subsidiary_mapping.internal_id:
location_attributes.append({
'attribute_type': 'LOCATION',
'display_name': 'Location',
'value': location['name'],
'destination_id': location['internalId'],
'active': not location['isInactive']
})
break # Assuming you only want to add the location once if any subsidiary matches
else:
# Existing code for locations without subsidiaryListThis approach ensures that all subsidiaries are checked for each location, and the location is added if any subsidiary matches.
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
Ashutosh619-sudo
changed the title
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
Ashutosh619-sudo
changed the title
Ashutosh619-sudo
changed the title
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
|
PR description must contain a link to a ClickUp (case-insensitive) |
|
PR title must start with "fix:", "feat:", "chore:", "refactor", or "test:" (case-insensitive) |
Ashutosh619-sudo
changed the title
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
| 'active': True | ||
| } | ||
| ) | ||
| for currency in currencies_generator: |
| 'destination_id': currency['internalId'], | ||
| 'active': True | ||
| } | ||
| ) | ||
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
There was a problem hiding this comment.
upserting to DB should also be per page
apps/netsuite/connector.py
Outdated
| 'value': location['name'], | ||
| 'destination_id': location['internalId'], | ||
| 'active': not location['isInactive'] | ||
| }) | ||
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
There was a problem hiding this comment.
upserting to DB should also be per page
apps/netsuite/connector.py
Outdated
| 'value': department['name'], | ||
| 'destination_id': department['internalId'], | ||
| 'active': not department['isInactive'] | ||
| }) | ||
|
|
||
| DestinationAttribute.bulk_create_or_update_destination_attributes( |
| @@ -405,7 +405,7 @@ def test_sync_tax_items(mocker, db): | |||
|
|
|||
| def test_sync_currencies(mocker, db): | |||
| mocker.patch( | |||
| 'netsuitesdk.api.currencies.Currencies.get_all', | |||
| 'netsuitesdk.api.currencies.Currencies.get_all_generator', | |||
| return_value=data['get_all_currencies'][0] | |||
There was a problem hiding this comment.
response structure would change
| @@ -325,7 +325,7 @@ def test_sync_subsidiaries(mocker, db): | |||
|
|
|||
| def test_sync_locations(mocker, db): | |||
| mocker.patch( | |||
| 'netsuitesdk.api.locations.Locations.get_all', | |||
| 'netsuitesdk.api.locations.Locations.get_all_generator', | |||
| return_value=data['get_all_locations'] | |||
There was a problem hiding this comment.
response structure would change
Ashutosh619-sudo
temporarily deployed
to
CI Environment
GitHub Actions
Inactive
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #638 +/- ##
==========================================
- Coverage 89.31% 88.70% -0.61%
==========================================
Files 59 59
Lines 5193 5382 +189
==========================================
+ Hits 4638 4774 +136
- Misses 555 608 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* Add generators in syncing dimensions * resolved tests * comment resolved
https://app.clickup.com/1864988/v/l/6-901603904304-1
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation