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

Improved subscribers list fetching #23

Merged
merged 7 commits into from
May 31, 2024
Merged

Improved subscribers list fetching #23

merged 7 commits into from
May 31, 2024

Conversation

andreibaca
Copy link
Contributor

Optimise the subscribers method to use less queries to get all the contacts

…o the api

When $maxRange is set to null, the method will first query the number of contacts and get all contacts in one query. Overwise it requires 50 queries for 1000 contacts which is very slow.
…ers of subscribers would be exactly divizible by range.
@andreibaca
Copy link
Contributor Author

So there was a bug when the subscribers number was exactly the same as range it would try to make another request that resulted in the exception: "Yet,There are no contacts in this list."
I could not find a solution but to change the way the function works. I don't understand why the unit test is failing - the method works live with zoho.

@cappuc cappuc changed the title Main Improved subscribers list fetching May 31, 2024
@cappuc
Copy link
Contributor

cappuc commented May 31, 2024

Hi @andreibaca

I've made some changes to your code:

  • I've removed the call to get the subscribers count because it's not necessary (we can't get all subscribers at once because it's limited to 650)
  • I've added a try-catch to handle the "Yet,There are no contacts in this list." exception and just return in this case

Fix #24

@cappuc cappuc merged commit 552a0f3 into keepsuit:main May 31, 2024
9 checks passed
@andreibaca
Copy link
Contributor Author

I did not know about the 650 limit.
Then if the $range param was null it could have been set to 650 instead of unlimited in this case.

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