Skip to content

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Dec 14, 2020

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

Stefan:
This PR adds a new endpoint /teams/:tid/search to brig that allows admins to search for team members. A reindex is required for this endpoint to work. If reindexing is skipped all existing endpoints that make use of ES are still fully functional.

@fisx fisx changed the title Search contacts by email inside team. [WIP] Search contacts by email inside team. Dec 22, 2020
@smatting smatting marked this pull request as ready for review January 21, 2021 17:23
@smatting
Copy link
Contributor

@fisx Feel free to "review" :)

@smatting smatting changed the title [WIP] Search contacts by email inside team. Search contacts by email inside team. Jan 21, 2021
Copy link
Contributor Author

@fisx fisx 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, but of course I have comments. :)

@Yserz at least the path has changed (a couple of times :)), and possibly other details. @smatting please make sure that you two agree this is all good for the client as well.

otherwise approved!

@@ -172,9 +173,10 @@ updateSearchIndex orig e = case e of
or
[ isJust eupName,
isJust eupAccentId,
isJust eupHandle
isJust eupHandle,
isJust eupManagedBy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about idp, role?

Copy link

Choose a reason for hiding this comment

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

@smatting Wanted to split this into two PRs I think. One for the general search endpoint and another one for sorting and filtering for these additional properties.

Copy link
Contributor

@smatting smatting Jan 22, 2021

Choose a reason for hiding this comment

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

idp: I've added a commit for that
role: not possible (easily) as this change to role originates in galley and I couldn't find an existing mechanism for creating Events (Brig.User.Event) externally

Doc.response 200 "The search result." Doc.end

get "/teams/:tid/search" (continue browseTeamH) $
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this route, should it be moved to the module where the other routes under /teams/:tid/ are stored?

Donno, our routing table tree is a bit out of sorts anyway, so I'm not sure how to keep this from getting worse. Leaving it here makes sense because it's also about searching.

Copy link

@Yserz Yserz Jan 21, 2021

Choose a reason for hiding this comment

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

https://github.com/wireapp/wire-web-packages/pull/3359/files#diff-011d7779c23ee1ccc3e3cfbda90c1e55091483907d4732773fd5c53bc5236ed5R49

Generally /teams/{tid}/... makes sense when you need the teamId. That would match the pattern for most of the team endpoints looking from the outside.

Copy link
Contributor

@smatting smatting Jan 22, 2021

Choose a reason for hiding this comment

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

I'd also prefer keeping it closer to search and the cost to lookup a path in the code is quite low

Comment on lines 455 to 459
withSettingsOverrides newOpts f <* deleteIndex opts indexName
withSettingsOverrides newOpts f
Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's this about?

Copy link
Contributor

@smatting smatting Jan 22, 2021

Choose a reason for hiding this comment

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

left-over from debugging and accidentally commited. I reverted it

@smatting smatting self-requested a review January 22, 2021 15:25
@smatting smatting merged commit 0fe0add into develop Jan 22, 2021
@smatting smatting deleted the fisx/es-updates branch January 22, 2021 15:33
) ->
Handler Response
teamUserSearchH (_ ::: uid ::: tid ::: mQuery ::: mRoleFilter ::: mSortBy ::: mSortOrder ::: size) = do
json <$> teamUserSearch uid tid mQuery (undefined mRoleFilter) mSortBy mSortOrder size
Copy link
Member

Choose a reason for hiding this comment

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

What is this undefined doing here?

Copy link
Contributor

@smatting smatting Jan 25, 2021

Choose a reason for hiding this comment

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

It was probably a reminder to not use this argument. mbRoleFilter has the wrong type for the function call. A quick and dirty way to make the type checker happy. It wouldn't cause problems since mRoleFilter, mSortB, mSortOrder are not used, but I should have caught this in my review, and it shouldn't get released like this.

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.

4 participants