Skip to content

v4 API: Forms and Landing Pages: Pagination and Total Count #91

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

Merged
merged 13 commits into from
Apr 5, 2024

Conversation

n7studios
Copy link
Contributor

@n7studios n7studios commented Apr 1, 2024

Summary

Adds total count and pagination support for get_forms and get_landing_pages methods.

Testing

Existing tests pass. Tests for get_resources method removed, as this method no longer exists, as the v4 API doesn't support the subscription_forms endpoint, and each method (get_forms, get_landing_pages, get_tags) makes its own call to get.

Checklist

@n7studios n7studios added this to the 2.0 milestone Apr 1, 2024
@n7studios n7studios self-assigned this Apr 1, 2024
@n7studios n7studios requested review from a team, noelherrick, jenessawhite and mercedesb and removed request for a team April 1, 2024 16:15
@n7studios
Copy link
Contributor Author

n7studios commented Apr 1, 2024

@noelherrick @mercedesb The PHP SDK has always had a distinction between Forms and Landing Pages (get_forms(), get_landing_pages()), with get_landing_pages() querying the forms endpoint and only returning results that are Landing Pages.

With the introduction of pagination support in the v4 API, get_landing_pages() becomes flaky, given that a per_page or cursor argument might not return any landing pages.

I'm unsure what the best route here is:

  1. Drop pagination support on get_landing_pages. This would require querying the forms endpoint with per_page set to the maximum, and hope that an account hasn't had over 1,000 forms and landing pages in its lifetime.
  2. Drop get_landing_pages as a supported method in the PHP SDK. Consumer / developers can filter results via get_forms if they only wanted to fetch landing pages. I like the convenience of having a get_landing_pages method though (we do something similar across all ConvertKit WordPress Plugins).
  3. Investigate whether it's possible to add a filter to the API to return forms, landing pages or both. I'm unsure of the technical workload needed for this, or whether this is even viable. It would be fantastic if this could be an option; as a use case example, the WordPress Plugin would definitely benefit from it when we come to use the v4 API, as we distinguish between forms and landing pages in some settings there.

Would love to have your input!

@n7studios n7studios marked this pull request as ready for review April 1, 2024 16:22
@mercedesb
Copy link

With the introduction of pagination support in the v4 API, get_landing_pages() becomes flaky, given that a per_page or cursor argument might not return any landing pages.

@n7studios Thanks for calling this out. I have an open PR to be able to filter /v4/forms by either ?type=embed or ?type=hosted.

This should allow you to continue supporting get_landing_pages by using ?type=hosted

It should be merged and deployed within the next day or so.

Copy link

@noelherrick noelherrick left a comment

Choose a reason for hiding this comment

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

Looks good - after Mercedes gets that PR merged and you start using those new params, we should be good to go.

@n7studios
Copy link
Contributor Author

With the introduction of pagination support in the v4 API, get_landing_pages() becomes flaky, given that a per_page or cursor argument might not return any landing pages.

@n7studios Thanks for calling this out. I have an open PR to be able to filter /v4/forms by either ?type=embed or ?type=hosted.

This should allow you to continue supporting get_landing_pages by using ?type=hosted

It should be merged and deployed within the next day or so.

That's awesome - thanks so much!

@mercedesb
Copy link

/v4/forms?type=hosted should work in prod now

@n7studios
Copy link
Contributor Author

/v4/forms?type=hosted should work in prod now

@mercedesb Thanks so much!

Would it be possible to add a boolean filter for archived? I've since realised pagination tests are still flaky as the default behaviour of the SDK and WordPress Plugins has always been to remove archived Forms and Landing Pages from the results returned, given we don't need to show them to the end user.

I'm happy with the default behaviour of archived forms being included in results - but any option to filter them out in the API request would be fantastic, so that the per_page and cursor pagination can be supported.

Sorry for not spotting this sooner.

@mercedesb
Copy link

Would it be possible to add a boolean filter for archived?

Whoops, we shouldn't have changed behavior. I'll add a status filter. We want to stay away from booleans :) But I'll update it so default behavior is active and then we can filter on the various statuses. The docs will have the full list. My plate is crazy full but I'll try to get to this later today but it might be tomorrow.

@mercedesb
Copy link

Ok, you can filter forms by status now. And it'll default to active. Docs have been updated with status options.

@n7studios n7studios requested a review from noelherrick April 4, 2024 13:18
@n7studios
Copy link
Contributor Author

Ok, you can filter forms by status now. And it'll default to active. Docs have been updated with status options.

Awesome, thanks!

@noelherrick have requested a re-review as a few changes since the initial PR, so would be great to have you check this over.

@n7studios n7studios merged commit 10f8442 into v4-api-oauth Apr 5, 2024
@n7studios n7studios deleted the v4-api-forms-pagination branch June 26, 2024 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants