-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🐛 Source Amazon Seller Partner: Fix check command to check access to correct streams #35062
🐛 Source Amazon Seller Partner: Fix check command to check access to correct streams #35062
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
orders_stream = Orders(**stream_kwargs) | ||
next(orders_stream.read_records(sync_mode=SyncMode.full_refresh)) | ||
|
||
if config.get("account_type", "Seller") == "Seller": |
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.
According to this, all existing users forced to Seller 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.
No, the check command will check access to the Vendor report stream for the Vendor account type. The default value was added to avoid errors for the config without account type, although account type is the required property with the default value of Seller
.
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.
So, if all users who set up connections before these changes will be forced to be Sellers because they do not have an account_type
specified in their credentials.
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.
No, actually, all the users should already have account_type
in their config as it is used for the Oauth, also this parameter is set during the migration from the previous version.
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.
lgtm
…correct streams (#35062)
…correct streams (#35062)
What
Fix the check command for the
Vendor
account type.How
Fix the check command to check access to the correct streams. In the new implementation, the check will pass for an empty report for the
Vendor
account.