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

Show file tree
Hide file tree
Changes from 2 commits
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
122 changes: 63 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,20 @@ 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 currency in currencies_generator:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test this once?

I think this not how generator works
You will have to two for loops

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 @Ashutosh619-sudo

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 +519,33 @@ 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:
Comment on lines +522 to +541
Copy link

Choose a reason for hiding this comment

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

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:

  1. It's initialized before the inner loop but incremented inside it.
  2. It's not reset for each new batch of locations from the generator.
  3. 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 subsidiaryList

This approach ensures that all subsidiaries are checked for each location, and the location is added if any subsidiary matches.

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 +556,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 +580,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 +926,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
4 changes: 2 additions & 2 deletions tests/test_netsuite/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6554,7 +6554,7 @@
"externalId": "report 1057 - [email protected]",
"internalId": "76115"
},
'get_all_locations': [
'get_all_locations': [[
{
'nullFieldList': None,
'name': '02: Boston Gaon',
Expand Down Expand Up @@ -6757,7 +6757,7 @@
'customFieldList': None,
'internalId': '5',
'externalId': None
}],
}]],
'get_all_currencies': [
[OrderedDict([('nullFieldList', None),
('name', 'USA PEW S A'),
Expand Down
6 changes: 3 additions & 3 deletions tests/test_netsuite/test_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
Copy link
Contributor

Choose a reason for hiding this comment

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

response structure would change

)
netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=49)
Expand Down Expand Up @@ -360,7 +360,7 @@ def test_sync_departments(mocker, db):
def test_sync_customers(mocker, db):
mocker.patch(
'netsuitesdk.api.customers.Customers.get_all_generator',
return_value=data['get_all_projects']
return_value=data['get_all_projects']
)

mocker.patch(
Expand Down Expand Up @@ -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]
Copy link
Contributor

Choose a reason for hiding this comment

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

response structure would change

)
netsuite_credentials = NetSuiteCredentials.objects.get(workspace_id=49)
Expand Down
Loading