-
Notifications
You must be signed in to change notification settings - Fork 9
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
fix: Incorrect subscribers param filtering #20
base: main
Are you sure you want to change the base?
Conversation
@@ -13,7 +13,7 @@ interactions: | |||
User-Agent: | |||
- MailerLite-Python-SDK-Client | |||
method: GET | |||
uri: https://connect.mailerlite.com/api/subscribers?limit=10&page=1 | |||
uri: https://connect.mailerlite.com/api/subscribers?limit=10&cursor=abc123 |
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.
You are leaking some email addresses in your cassettes, which I didn't want to do so I just manually updated the cassettes to get tests passing
@@ -95,13 +95,19 @@ def test_api_url_is_properly_set(self): | |||
"tests/vcr_cassettes/subscribers-list.yml", filter_headers=["Authorization"] | |||
) | |||
def test_list_of_all_subscribers_should_be_returned(self, subscriber_keys): | |||
response = self.client.subscribers.list(limit=10, page=1) | |||
response = self.client.subscribers.list(limit=10, cursor="abc123") |
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.
Perhaps a better test case would also assert that results are different when passing in different cursors, but as mentioned in another comment I didn't want to leak my actual email addresses in the cassettes
@@ -13,7 +13,7 @@ interactions: | |||
User-Agent: | |||
- MailerLite-Python-SDK-Client | |||
method: GET | |||
uri: https://connect.mailerlite.com/api/subscribers?limit=10&page=1 | |||
uri: https://connect.mailerlite.com/api/subscribers?limit=10&cursor=abc123 |
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.
Meta - perhaps your API endpoints should have a version number in the URL, so that your SDKs can be versioned to the API endpoints. Otherwise every time you change your API contract I dunno how you'll keep SDKs updated / backward-compatible
I couldn't get this api endpoint to actually paginate without using the
cursor
param described in your API docs