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

feat: Add generators in syncing dimensions #638

Merged
merged 6 commits into from
Oct 1, 2024
Merged

feat: Add generators in syncing dimensions #638

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77dc2bd
Add generators in syncing dimensions
Ashutosh619-sudo Sep 6, 2024
0b82f13
resolved tests
Ashutosh619-sudo Sep 6, 2024
3193f81
Merge branch 'master' into add-generators
Ashutosh619-sudo Sep 23, 2024
cf013fe
Merge branch 'master' into add-generators
Ashutosh619-sudo Sep 27, 2024
8c705cb
Merge branch 'master' into add-generators
Ashutosh619-sudo Sep 27, 2024
a55e222
comment resolved
Ashutosh619-sudo Sep 30, 2024
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
124 changes: 65 additions & 59 deletions apps/netsuite/connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,20 +493,21 @@ def sync_currencies(self):
"""
Sync Currencies
"""
currencies = self.connection.currencies.get_all()
currencies_generator = self.connection.currencies.get_all_generator()

currency_attributes = []

for currency in currencies:
currency_attributes.append(
{
'attribute_type': 'CURRENCY',
'display_name': 'Currency',
'value': currency['symbol'],
'destination_id': currency['internalId'],
'active': True
}
)
for currencies in currencies_generator:
for currency in currencies:
currency_attributes.append(
{
'attribute_type': 'CURRENCY',
'display_name': 'Currency',
'value': currency['symbol'],
'destination_id': currency['internalId'],
'active': True
}
)

DestinationAttribute.bulk_create_or_update_destination_attributes(
Copy link
Contributor

Choose a reason for hiding this comment

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

upserting to DB should also be per page

currency_attributes, 'CURRENCY', self.workspace_id, True)
Expand All @@ -519,32 +520,34 @@ def sync_locations(self):
"""
subsidiary_mapping = SubsidiaryMapping.objects.get(workspace_id=self.workspace_id)

locations = self.connection.locations.get_all()
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:
Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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:

location_attributes.append({
'attribute_type': 'LOCATION',
'display_name': 'Location',
'value': location['name'],
'destination_id': location['internalId'],
'active': not location['isInactive']
})
else:
location_attributes.append({
'attribute_type': 'LOCATION',
'display_name': 'Location',
'value': location['name'],
'destination_id': location['internalId'],
'active': not location['isInactive']
})

DestinationAttribute.bulk_create_or_update_destination_attributes(
Copy link
Contributor

Choose a reason for hiding this comment

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

upserting to DB should also be per page

location_attributes, 'LOCATION', self.workspace_id, True)
Expand All @@ -555,19 +558,20 @@ def sync_classifications(self):
"""
Sync classification
"""
classifications = self.connection.classifications.get_all()
classification_generator = self.connection.classifications.get_all_generator()

classification_attributes = []

for classification in classifications:
if not classification['isInactive']:
classification_attributes.append({
'attribute_type': 'CLASS',
'display_name': 'Class',
'value': classification['name'],
'destination_id': classification['internalId'],
'active': not classification['isInactive']
})
for classifications in classification_generator:
for classification in classifications:
if not classification['isInactive']:
classification_attributes.append({
'attribute_type': 'CLASS',
'display_name': 'Class',
'value': classification['name'],
'destination_id': classification['internalId'],
'active': not classification['isInactive']
})

DestinationAttribute.bulk_create_or_update_destination_attributes(
classification_attributes, 'CLASS', self.workspace_id, True)
Expand All @@ -578,19 +582,20 @@ def sync_departments(self):
"""
Sync departments
"""
departments = self.connection.departments.get_all()
department_generator = self.connection.departments.get_all_generator()

department_attributes = []

for department in departments:
if not department['isInactive']:
department_attributes.append({
'attribute_type': 'DEPARTMENT',
'display_name': 'Department',
'value': department['name'],
'destination_id': department['internalId'],
'active': not department['isInactive']
})
for departments in department_generator:
for department in departments:
if not department['isInactive']:
department_attributes.append({
'attribute_type': 'DEPARTMENT',
'display_name': 'Department',
'value': department['name'],
'destination_id': department['internalId'],
'active': not department['isInactive']
})

DestinationAttribute.bulk_create_or_update_destination_attributes(
Copy link
Contributor

Choose a reason for hiding this comment

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

take care everywhere

department_attributes, 'DEPARTMENT', self.workspace_id, True)
Expand Down Expand Up @@ -923,20 +928,21 @@ def sync_subsidiaries(self):
"""
Sync subsidiaries
"""
subsidiaries = self.connection.subsidiaries.get_all()
subsidiary_generator = self.connection.subsidiaries.get_all_generator()
subsidiary_attributes = []

for subsidiary in subsidiaries:
subsidiary_attributes.append({
'attribute_type': 'SUBSIDIARY',
'display_name': 'Subsidiary',
'value': subsidiary['name'],
'destination_id': subsidiary['internalId'],
'detail': {
'country': subsidiary['country']
},
'active': True
})
for subsidiaries in subsidiary_generator:
for subsidiary in subsidiaries:
subsidiary_attributes.append({
'attribute_type': 'SUBSIDIARY',
'display_name': 'Subsidiary',
'value': subsidiary['name'],
'destination_id': subsidiary['internalId'],
'detail': {
'country': subsidiary['country']
},
'active': True
})

DestinationAttribute.bulk_create_or_update_destination_attributes(
subsidiary_attributes, 'SUBSIDIARY', self.workspace_id, True)
Expand Down
Loading