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.

Sign up for GitHub

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

Jump to bottom

[SQSERVICES-1773] Allow pagination for team search endpoint #2895

Merged
merged 7 commits into from
Dec 5, 2022
Merged

[SQSERVICES-1773] Allow pagination for team search endpoint #2895

merged 7 commits into from
Dec 5, 2022

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Dec 1, 2022 •
edited
Loading

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

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix December 1, 2022 16:06 Inactive
@battermann battermann temporarily deployed to cachix December 1, 2022 16:06 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 10:30 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 10:30 Inactive
@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 temporarily deployed to cachix December 2, 2022 10:35 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 10:35 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 11:26 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 11:26 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 11:37 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 11:37 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 11:38 Inactive
@battermann battermann marked this pull request as ready for review December 2, 2022 11:38
@battermann battermann temporarily deployed to cachix December 2, 2022 11:38 Inactive
@battermann battermann requested a review from fisx December 2, 2022 11:40
@battermann battermann mentioned this pull request Dec 2, 2022
Merged
2 tasks
@mdimjasevic mdimjasevic self-requested a review December 2, 2022 14:36
mdimjasevic
mdimjasevic reviewed Dec 2, 2022
View reviewed changes
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.

Do you want to update JSON golden test files with an example where searchPagingState is a Just value? It's a bit weird to put Nothings in 20 existing cases and to none with a Just value.

@battermann battermann temporarily deployed to cachix December 2, 2022 15:34 Inactive
@battermann battermann temporarily deployed to cachix December 2, 2022 15:34 Inactive
@battermann
Copy link
Contributor Author

Do you want to update JSON golden test files with an example where searchPagingState is a Just value? It's a bit weird to put Nothings in 20 existing cases and to none with a Just value.

You are right. Done!

mdimjasevic
mdimjasevic reviewed Dec 5, 2022
View reviewed changes
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.

Are you aware of the Wire.API.Routes.MultiTablePaging module and the paging facilities provided by it? If yes, why rolling your own pagination system?

services/brig/src/Brig/User/API/Search.hs
@@ -136,7 +137,7 @@ searchLocally searcherId searchTerm maybeMaxResults = do
esResult <-
if esMaxResults > 0
then Q.searchIndex (Q.LocalSearch searcherId searcherTeamId teamSearchInfo) searchTerm esMaxResults
else pure $ SearchResult 0 0 0 [] FullSearch
else pure $ SearchResult 0 0 0 [] FullSearch Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the context here, but should maybe this be an input parameter to the searchLocally function instead of always being Nothing?

Copy link
Contributor Author

@battermann battermann Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This function is part of the ConnectionAPI and this API is not subject to change in this ticket/PR. If it is Nothing then the behavior doesn't change. The only good reason I can think of to pass as parameter would be to make it more visible. Do you think we should do this?

services/brig/src/Brig/User/API/Search.hs
@@ -113,7 +113,8 @@ searchRemotely domain searchTerm = do
searchFound = count,
searchReturned = count,
searchTook = 0,
searchPolicy = S.searchPolicy searchResponse
searchPolicy = S.searchPolicy searchResponse,
searchPagingState = Nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the context here, but should maybe this be an input parameter to the searchRemotely function instead of always being Nothing?

Copy link
Contributor Author

@battermann battermann Dec 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

See answer to previous comment.

@battermann
Copy link
Contributor Author

battermann commented Dec 5, 2022 •
edited
Loading

Are you aware of the Wire.API.Routes.MultiTablePaging module and the paging facilities provided by it? If yes, why rolling your own pagination system?

Wire.API.Routes.MultiTablePaging is designed for Cassandra. But this one is ElasticSearch. Even though some of the simple types could maybe be shared, I don't think it is good to couple the two things.

mdimjasevic
mdimjasevic approved these changes Dec 5, 2022
View reviewed changes
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.

Thanks for clarifying! I'd say this is good to go.

@battermann battermann merged commit 1518191 into develop Dec 5, 2022
@battermann battermann deleted the SQSERVICES-1773-be-tm-allow-pagination-for-team-search-endpoints branch December 5, 2022 10:29
@zebot zebot mentioned this pull request Jan 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdimjasevic mdimjasevic mdimjasevic approved these changes

@fisx fisx Awaiting requested review from fisx

Assignees
No one assigned
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@battermann @mdimjasevic @zebot