-
Notifications
You must be signed in to change notification settings - Fork 121
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
fix(searchGroupUsers func): searchOptions is now an optional parameter #616
Conversation
): Promise<ISearchGroupUsersResult> { | ||
const url = `${getPortalUrl(searchOptions)}/community/groups/${id}/userlist`; | ||
const options = appendCustomParams<ISearchGroupUsersOptions>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current tests don't communicate it, but the point of calling appendCustomParams(options)
was to allow this.
searchGroupUsers("5bc", {
name: "jupe",
sortField: "fullname",
sortOrder: "asc",
num: 2,
start: 2,
joined: [null, 123456],
memberType: "member"
authentication
})
(instead of requiring they be nested as a params
object.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @drewctate!
Just need to bring back appendCustomParams()
and this should be GTG.
httpMethod: "GET" | ||
} | ||
); | ||
const options: IRequestOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need to use appendCustomParams()
b/c the searchOptions
needs to go into options.params
right? Also we want to the only the white-listed searchOptions
properties.
expect(options.method).toBe("GET"); | ||
done(); | ||
searchGroupUsers("5bc", { | ||
params: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see why this works w/o appendCustomParams()
- these props should not be nested under params
when passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯
bbfdd9c
to
b445c62
Compare
b445c62
to
d54bddb
Compare
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 99 99
Lines 1483 1484 +1
Branches 256 257 +1
=====================================
+ Hits 1483 1484 +1
Continue to review full report at Codecov.
|
ISSUES CLOSED: #615