-
Notifications
You must be signed in to change notification settings - Fork 23
TDL-14567: Check linkedin ads accounts in discover mode #35
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
TDL-14567: Check linkedin ads accounts in discover mode #35
Conversation
tap_linkedin_ads/__init__.py
Outdated
|
||
LOGGER.info('Starting discover') | ||
check_accounts_list(config) | ||
check_accounts_list(config) # Check that accounts are valid numbers. | ||
client.check_accounts(config) # Check that accounts are valid LinkedIn Ads account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@savan-chovatiya There should be a single function for checking the accounts list. Can you make changes to the check_accounts_list() function to also check if the account is valid or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karanpanchal-crest I have removed check_accounts_list() function and handled that scenario in check_accounts() function itself.
tap_linkedin_ads/client.py
Outdated
elif response.status_code != 200: | ||
raise_for_error(response) | ||
if invalid_account: | ||
error_message = 'These accounts are invalid LinkedIn Ads accounts from provided accounts:{}'.format(invalid_account) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@savan-chovatiya The error message should be: "Invalid Linked Ads accounts provided during the configuration: [a1, a2. a3]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karanpanchal-crest Updated error message as per suggestion.
int(account) | ||
except ValueError as e: | ||
message = "The account '{}' provided in the configuration is having non-numeric value.".format(account) | ||
raise ValueError(message) from None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this function as we have added API calls for validating accounts. Those calls will return 400 code for accounts with an invalid number format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix conflicts
Description of change
TDL-14567:The account_number should be validated in the discovery mode
Manual QA steps
Risks
Rollback steps