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.

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

372 search by orcid #393

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

OrigamiBurger
Copy link
Contributor

Allow users to search by ORCID #372
I tested this by directly inserting a row into provider_authorizations and was able to get a successful return when I provided the entire orcid (provider_uid) to the search endpoint
Added integrations tests as well

@kueda kueda requested a review from pleary August 15, 2023 21:25
Copy link
Member

@kueda kueda left a comment

Choose a reason for hiding this comment

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

This looks like it should work, but I'm not sure we want to support ORCID search via the q param. An orcid param seems clearer to me. Thoughts on that, @pleary?

One thing that definitely needs to happen here is v2 implementation.

@OrigamiBurger
Copy link
Contributor Author

OrigamiBurger commented Aug 16, 2023

This looks like it should work, but I'm not sure we want to support ORCID search via the q param. An orcid param seems clearer to me. Thoughts on that, @pleary?

One thing that definitely needs to happen here is v2 implementation.

For the implementation, I was going off the GET request example posted in the linked community post here https://forum.inaturalist.org/t/search-users-by-orcid/37362
I can see how a new parameter of orcid will make the usage/implementation more clear but would require updates in the website and/or mobile apps to implement. With this implementation I was able to do a search with an orcid on the website without an update since the search query is sent in the q parameter.
@kueda @pleary let know what you think. If you would like it to be a separate parameter, I can make that update by adding the orcid parameter and conditionally add the orcid elastic filter on whether it's present or not.

Screenshot from 2023-08-16 19-25-20

@pleary
Copy link
Member

pleary commented Aug 17, 2023

I'm OK with having the /search endpoint query against orcid with q (orcid is a keyword field so the full orcid is needed so searches like q=0 wont start returning a lot of extra results), but I agree it would be good to have it as its own parameter, probably in the V2 /users endpoint. I think that would make the most sense for people going to the API documentation looking to search for users by orcid - they may go to the users endpoint and look for an orcid parameter. Would it be possible to add this as a separate parameter for the V2 /users endpoint in addition to what you have here?

Let me know if you're having trouble navigating the V2 configuration which involves more code than V1, but also generates its own documentation along the way. The endpoint is described in the V2 path, implemented in the V2 controller, and parameters defined in the openapi request schema. Thanks!

@OrigamiBurger
Copy link
Contributor Author

@pleary Thanks for the review! Yeah I can update the v2 users endpoint as well

@OrigamiBurger
Copy link
Contributor Author

@pleary @kueda updated the V2 users endpoint to accept in orcid as a separate GET parameter and added a corresponding test

@pleary
Copy link
Member

pleary commented Aug 21, 2023

This looks good to me. Thanks!

@pleary pleary merged commit 4472b89 into inaturalist:main Aug 21, 2023
2 checks passed
@pleary
Copy link
Member

pleary commented Aug 21, 2023

For future reference, we have an .eslintrc file in the project root, and I'd recommend getting your text editor working with an automatic linter so you can see if any code formatting violated any of the rules in the config. There were a few spacing issues here that I fixed in a follow-up commit. I also added a pattern validator for the V2 orcid parameter since we know that the orcid will have to have a very particular format to work.

@OrigamiBurger OrigamiBurger deleted the 372-search-by-orcid branch August 21, 2023 16:26
@OrigamiBurger
Copy link
Contributor Author

@pleary that is good to know and will add the ESLint validation as a set to my future development. TBH I code in vim and don't have any ESLint plugins installed.
That's a good idea on the regex piece. I didn't know if the orcid was in the format of just numbers or was allowed to have letters present
Thanks for taking a look at my update!

@dshorthouse
Copy link

Comment on @pleary's commit should it be of use here: ed4f0c8#commitcomment-125768601

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants