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

feat(Search samples) Modify samples endpoint to allow for searching. #67

Merged
merged 6 commits into from
Sep 20, 2022

Conversation

Vince-janv
Copy link
Contributor

@Vince-janv Vince-janv commented Sep 15, 2022

Description

Adds a sample search functionality.

Added

  • Function for querying samples based on substring

Changed

  • Added a enquiry parameter to the samples endpoint to allow for sample searches

Steps to consider while deploying

  • Configuration changes: N/A
  • Documentation updates: N/A
  • Inform users by email: N/A

Review:

  • Code approved by
  • Tests executed on stage by: (Document the test done with screen shots and description.)
  • "Merge and deploy" approved by

This version is a:

  • MAJOR - when you make incompatible API changes
  • MINOR - when you add functionality in a backwards compatible manner
  • PATCH - when you make backwards compatible bug fixes or documentation/instructions

@Vince-janv Vince-janv force-pushed the add-search-sample-endpoint branch from d99142e to 6bcb65e Compare September 15, 2022 14:55
@clingen-sthlm clingen-sthlm temporarily deployed to stage September 15, 2022 15:00 Inactive
@clingen-sthlm clingen-sthlm temporarily deployed to stage September 15, 2022 15:02 Inactive
@clingen-sthlm clingen-sthlm temporarily deployed to stage September 15, 2022 15:12 Inactive
@Vince-janv Vince-janv marked this pull request as ready for review September 16, 2022 08:18
@Vince-janv Vince-janv requested a review from Mropat as a code owner September 16, 2022 08:18
@Vince-janv
Copy link
Contributor Author

Test on the genotype stage swagger:

Screenshot 2022-09-16 at 11 03 53

@moedarrah moedarrah requested review from henrikstranneheim and removed request for Mropat September 19, 2022 11:40
Copy link

@elevu elevu left a comment

Choose a reason for hiding this comment

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

Approving the PR, just some tiny comments :)

  • The endpoints returns 500 when making some calls like https://genotype-stage.scilifelab.se/api/v2/samples/?skip=0&limit=2&enquiry=ACC&plate_id=ID127&incomplete=true&commented=true&status_missing=true but I guess it is not relevant to this PR.
  • The search by plate id seems also not to work, but maybe I am wrong and just searching for non existing combination (commented true and so on).
  • No tests? :D

@Vince-janv
Copy link
Contributor Author

Approving the PR, just some tiny comments :)

  • The endpoints returns 500 when making some calls like https://genotype-stage.scilifelab.se/api/v2/samples/?skip=0&limit=2&enquiry=ACC&plate_id=ID127&incomplete=true&commented=true&status_missing=true but I guess it is not relevant to this PR.
  • The search by plate id seems also not to work, but maybe I am wrong and just searching for non existing combination (commented true and so on).
  • No tests? :D

Thanks for the review @elevu! 😊

  • I played around a bit with the swagger UI and it seems that incomplete=true in combination with some other options returns 500. Probably not related to anything I changed, but I can take a look.
  • hmm, plate id works for me 🤔
  • That's why I wanted your review. How would you suggest to test these kind of PRs except via the swagger UI. There are no pytests in the genotype-api repo 😨

Copy link
Contributor

@henrikstranneheim henrikstranneheim left a comment

Choose a reason for hiding this comment

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

👍

@henrikstranneheim
Copy link
Contributor

@Vince-janv For API endpoint testing there is a framework in fastAPI to use. See https://github.com/Clinical-Genomics/atlas/tree/master/tests/api for inspiration

@Vince-janv
Copy link
Contributor Author

@henrikstranneheim Should I spend time starting to write tests here or should I merge this and prioritise something else?

@henrikstranneheim
Copy link
Contributor

@Vince-janv I think it is no worse then when you arrived. I think we should not spend time on this technical debt now, as this code seem to be of poor standards and would require some work. We should make a project out of it and work on it in a team in a more focused way.

@Vince-janv Vince-janv merged commit 9175050 into main Sep 20, 2022
@Vince-janv Vince-janv deleted the add-search-sample-endpoint branch September 20, 2022 13:21
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