-
Notifications
You must be signed in to change notification settings - Fork 23
[ENG-9638] add search by guid for contributors and moderators add modal #743
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
[ENG-9638] add search by guid for contributors and moderators add modal #743
Conversation
| const baseUrl = `${this.apiUrl}/users/?filter[full_name]=${value}&page=${page}`; | ||
| searchUsers(value: string, page = 1, findById = false): Observable<PaginatedData<ContributorAddModel[]>> { | ||
| const filterParamName = findById ? 'id' : 'full_name'; | ||
| const baseUrl = `${this.apiUrl}/users/?filter[${filterParamName}]=${value}&page=${page}`; |
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 believe we should be using the search api endpoint for this. For example, https://api.osf.io/v2/search/users/?q=typ46 or https://api.osf.io/v2/search/users/?q=brian%20geiger.
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.
@brianjgeiger it was/is used with the following api call
https://api.staging3.osf.io/v2/users/?filter[full_name]=pavlo&page=1 to get users for the modal ( from my point of view it works as expected or do you see something wrong)
and we may reuse that call for guid by using 'id' as filter when it is needed
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.
Filtering on the user column is slower and more processor intensive than using the search endpoint, which uses elastic to do the searching. Also, how do you know which filter parameter to use? If you were going the filtering route, I believe it would save some trouble to filter[id,full_name]=<query string>. But I think the search endpoint is the better way to do that.
I just tested, and the search endpoint is much faster.
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.
Also, how do you know which filter parameter to use?
I have made an attempt to filter by id and it was succeeded (otherwise might look to some alternative approach)
I just tested, and the search endpoint is much faster.
on ?search it is needed to have full match when typing so it may be a reason (if we change existed approach with ?search, i think it breaks UX and user expectations, when they type something not to end)
170b-2d6b-4d73-90bb-5b63a89f3a08.mp4
1ee5-db6c-44f1-b32b-6305720b51a8.mp4
also not see much difference when use search in comparison with current appoach for guid ( but if needed we may apply search for this case, because we need full match)
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.
Try your time experiments on Production rather than localhost to see the difference in speed.
Also, definitely don't make two requests, one for fullname and one for id. Use the combined filter[fullname,id]=<query> approach instead.
You can get search to substring words by using wildcard characters (bri* will match on brian, for example).
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 have tried to compare for staging3 back end (
if it is ok when you tell for experiments on Production
have CORS error to test local front end with PROD
)
const baseUrl = ${this.apiUrl}/users/?filter[full_name,id]=${value}&page=${page};
const baseUrl = ${this.apiUrl}/search/users/?q=${value}*&page=${page};
from my point of view ?filter[full_name,id] is even faster (and we may search by id only if searchString.length === 5 and it may be even more faster),
but if you see it is faster with search? with more data for prod we may replace it with search?
613c-ef81-436e-befc-7dea6d79a99b.mp4
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.
You can just visit the URLs in the browser to test on Prod, you don't have to use the front end to test the speed.
Searching by length==5 is a potential optimization, but if we go with filtering, for now I think just keeping it simple would be fine.
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.
@brianjgeiger thanks
I have tested for PROD both for some requests users/?filter and /search/users/? and /search looks to be faster when we have many records
https://api.osf.io/v2/users/?filter[full_name,id]=epcad&page=1
https://api.osf.io/v2/search/users/?q=epcad*&page=1
have updated code to be it faster when we have many records
https://github.com/CenterForOpenScience/angular-osf/pull/743/files
…er when there are many records
40e63f3
into
CenterForOpenScience:feature/pbs-25.04
Purpose
Some user workflows rely on using the OSF profile identifiers to locate users reliably. This was possible previously (at least in most contributor searches), but is not any longer.
Summary of Changes
When adding contributors, searching by user GUID should call up that user. This can be implemented across all instances of this widget/modal. Requires API change
Screenshot(s)
Side Effects
QA Notes