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.

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

LITE-28258: list and export streams #220

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

gab832
Copy link
Collaborator

@gab832 gab832 commented Aug 3, 2023

No description provided.

@gab832 gab832 requested a review from ffaraone August 3, 2023 13:21
@gab832 gab832 force-pushed the LITE-28258-list-and-export-stream branch 6 times, most recently from 355d0bd to b5ba906 Compare August 7, 2023 15:28

stream_type = 'Computed' if 'sources' in response and response['sources'] else 'Simple'
input = response['samples'].get('input', {})
input_file_name = input['name'].split('/')[-1]
Copy link
Contributor

Choose a reason for hiding this comment

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

The input file is not mandatory. If missed, a keyerror will be raised here

raise ClickException(f'Error obtaining file {file_id} -> {file_destination}')
with open(file_destination, 'wb') as f:
f.write(response)
console.secho(
Copy link
Contributor

Choose a reason for hiding this comment

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

This secho looks weird. Better to have a separate progress for files.

'Stream Name': response['name'],
'Stream Description': response['description'],
'Stream Type': stream_type,
'Stream Category': 'Inbound'
Copy link
Contributor

Choose a reason for hiding this comment

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

Inbound shouldn't be hardcoded.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not hardcoded, the if is nested. If the active account id is the owner of the stream then it is Inbound.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 but check when it should be Inbound and when Outbound

origin,
resource['name'],
'Computed' if ('sources' in resource and resource['sources']) else 'Simple',
'Inbound' if resource['owner']['id'] == active_account_id else 'Outbound',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct.

  • In case of vendors, all streams are Outbound and the owner is the active account.
  • In case of distributors, it will be Outbound if the owner is the same as the current active account otherwise Inbound.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. But if it is a Vendor, he will be the owner of all the streams so it will work. Am I right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but I should be Outbound.

'Computed Stream Source ID': response['sources'][0]['id']
if stream_type == 'Computed'
else '',
'Computed Stream Source Name': response['sources'][0]['type']
Copy link
Contributor

Choose a reason for hiding this comment

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

Not type probably name should be taken here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. So I will add also the name. We will export id, name and type.

horizontal='left',
vertical='top',
)
task = progress.add_task('Filling transformations', total=len(transformations))
Copy link
Contributor

Choose a reason for hiding this comment

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

Before adding a progress, if the len of transformations is 0 you should return from this funciton

ws,
('ID', 'Name'),
)
task = progress.add_task('Filling and downloading attachments', total=len(attachments))
Copy link
Contributor

Choose a reason for hiding this comment

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

Before adding a progress, if the len of attachments is 0 you should return from this funciton

'Partner Name': response['context'].get('account', {}).get('name', ''),
'Marketplace ID': response['context'].get('marketplace', {}).get('id', ''),
'Marketplace Name': response['context'].get('marketplace', {}).get('name', ''),
'Pricelist ID': response['context'].get('price_list', {}).get('id', ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

This field is not filled when the computed stream is attached to a pricelist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the serializer they have the pricelist, so it should work.

'Marketplace ID': response['context'].get('marketplace', {}).get('id', ''),
'Marketplace Name': response['context'].get('marketplace', {}).get('name', ''),
'Pricelist ID': response['context'].get('price_list', {}).get('id', ''),
'Pricelist Name': response['context'].get('price_list', {}).get('name', ''),
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this.

@gab832 gab832 force-pushed the LITE-28258-list-and-export-stream branch from b5ba906 to 3666e59 Compare August 17, 2023 12:03
@gab832 gab832 requested a review from ffaraone August 17, 2023 13:06
@gab832 gab832 force-pushed the LITE-28258-list-and-export-stream branch from 3666e59 to 2f728da Compare August 17, 2023 13:21
@gab832 gab832 force-pushed the LITE-28258-list-and-export-stream branch from 2f728da to f7924ad Compare August 17, 2023 14:03
@sonarcloud
Copy link

sonarcloud bot commented Aug 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

97.7% 97.7% Coverage
0.0% 0.0% Duplication

origin,
resource['name'],
'Computed' if ('sources' in resource and resource['sources']) else 'Simple',
'Inbound' if resource['owner']['id'] == active_account_id else 'Outbound',
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but I should be Outbound.

'Stream Name': response['name'],
'Stream Description': response['description'],
'Stream Type': stream_type,
'Stream Category': 'Inbound'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 but check when it should be Inbound and when Outbound

@ffaraone ffaraone merged commit 6667aa8 into master Aug 17, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants