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

[SQSERVICES-1773] Allow pagination for team search endpoints - has_more #2898

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Dec 2, 2022

This is an extension of #2895 and adds the has_more field to the search result.

https://wearezeta.atlassian.net/browse/SQSERVICES-1773

Checklist

  • No new changelog entry needed as this already exists in the base PR
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix December 2, 2022 13:14 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 13:14 Inactive
@battermann battermann changed the base branch from develop to SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints December 2, 2022 13:14
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Dec 2, 2022
@battermann battermann requested a review from fisx December 2, 2022 13:29
@battermann battermann force-pushed the SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints-hasmore branch from c088c65 to e795d32 Compare December 2, 2022 15:40
@battermann battermann temporarily deployed to cachix December 2, 2022 15:40 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 15:40 Inactive
Base automatically changed from SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints to develop December 5, 2022 10:29
@battermann battermann force-pushed the SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints-hasmore branch from e795d32 to 4e2e61d Compare December 5, 2022 10:33
@battermann battermann temporarily deployed to cachix December 5, 2022 10:33 Inactive
@battermann battermann temporarily deployed to cachix December 5, 2022 10:33 Inactive
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

Please add a changelog. You can extend the one from the previous PR by listing both PR numbers, which should look nice in the release notes.

The part I didn't get is +1; a comment is inlined.

Furthermore, is there a way for the API user to iterate over result pages?

let (IndexQuery q f sortSpecs) = teamUserSearchQuery tid mbSearchText mRoleFilter mSortBy mSortOrder
idx <- asks idxName
let search =
(ES.mkSearch (Just q) (Just f))
{ ES.size = ES.Size (fromIntegral s),
{ ES.size = ES.Size (fromIntegral size + 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it + 1?

@battermann
Copy link
Contributor Author

Please add a changelog. You can extend the one from the previous PR by listing both PR numbers, which should look nice in the release notes.

The part I didn't get is +1; a comment is inlined.

Good points, I'll add a comment about the +1 and update the changelog.

Furthermore, is there a way for the API user to iterate over result pages?

Not sure what you mean exactly. This PR adds the has_more field. If it is true, it means that there is a next page. They can use the paging_state and include it in a request, and this will give them the next page.

@battermann battermann temporarily deployed to cachix December 6, 2022 09:44 Inactive
@battermann battermann temporarily deployed to cachix December 6, 2022 09:44 Inactive
@mdimjasevic
Copy link
Contributor

Furthermore, is there a way for the API user to iterate over result pages?

Not sure what you mean exactly. This PR adds the has_more field. If it is true, it means that there is a next page. They can use the paging_state and include it in a request, and this will give them the next page.

Ok, that's what I was asking about. Thanks for clarifying.

Given the experience with the Cassandra paging endpoints, I expect client devs to be asking how to use this Es paging system. Perhaps it'd be good to include a thorough description in Swagger analogous to the way the Cassandra paging endpoints have it, by extending the endpoint description.

@battermann battermann temporarily deployed to cachix December 6, 2022 13:12 Inactive
@battermann battermann temporarily deployed to cachix December 6, 2022 13:12 Inactive
@battermann
Copy link
Contributor Author

Given the experience with the Cassandra paging endpoints, I expect client devs to be asking how to use this Es paging system. Perhaps it'd be good to include a thorough description in Swagger analogous to the way the Cassandra paging endpoints have it, by extending the endpoint description.

I updated the docs, and tried to be more clear. From the client perspective, ES's and C*'s paging mechanisms works exactly the same. In other words, the API is the same (only the implementation differs). So I assume they are already fairly familiar with it. However, it can't hurt to be more clear.

@battermann battermann merged commit fc9f1d2 into develop Dec 7, 2022
@battermann battermann deleted the SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints-hasmore branch December 7, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants