Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

exact name search #543

Merged
merged 3 commits into from
May 23, 2018
Merged

exact name search #543

merged 3 commits into from
May 23, 2018

Conversation

cianfoley-nearform
Copy link
Contributor

@cianfoley-nearform cianfoley-nearform commented May 18, 2018

rather than add a new endpoint, I added an optional type parameter to search, it defaults to our normal search mechanism, but when it is set to type=exact it will perform an exact name search.

we could put the same strategy in place for name search for users endpoint and policy endpoints if approved

@coveralls
Copy link

coveralls commented May 18, 2018

Coverage Status

Coverage increased (+0.03%) to 93.131% when pulling c8c7337 on exact-name-search into 765669c on master.

Copy link
Contributor

@mihaidma mihaidma left a comment

Choose a reason for hiding this comment

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

added a comment on the parameter naming, other than that LGTM

@@ -255,7 +255,8 @@ const teams = {
},
searchTeam: {
organizationId: validationRules.organizationId,
query: requiredString
query: requiredString,
type: Joi.string().optional().allow('default', 'exact')
Copy link
Contributor

Choose a reason for hiding this comment

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

the name default for the partial search doesn't sound well to me, maybe pattern?

Copy link
Contributor

@dberesford dberesford left a comment

Choose a reason for hiding this comment

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

Code looks good to me, should there be documentation changes also though?

@cianfoley-nearform
Copy link
Contributor Author

@dberesford I'll fix conflicts and add docs in a commit and then merge... swagger was updated, but we can put in explicit section i usage?

Copy link
Contributor

@dberesford dberesford left a comment

Choose a reason for hiding this comment

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

LGTM

@cianfoley-nearform cianfoley-nearform merged commit 04380c5 into master May 23, 2018
@cianfoley-nearform cianfoley-nearform deleted the exact-name-search branch May 23, 2018 08:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants