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

Feature/issue 6 submission function #7

Merged
merged 6 commits into from
Apr 8, 2022

Conversation

Chen2x
Copy link
Contributor

@Chen2x Chen2x commented Apr 4, 2022

Created a submission function for making post requests to the testbed api

@Chen2x Chen2x requested a review from jb-adams April 4, 2022 17:30
@Chen2x Chen2x linked an issue Apr 4, 2022 that may be closed by this pull request
Copy link
Member

@jb-adams jb-adams left a comment

Choose a reason for hiding this comment

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

Looks good so far. I have the following suggestions:

  • To make this even simpler, you could remove the __init__ constructor and make submit_report a static method that accepts a url, series id, series token, and report
  • make url a keyword argument, with a default value of the test endpoint, e.g.
def submit_report(..., url="http://localhost:4500/reports")

There's also no test code for this module. When you add your unit tests, assume that there is a running testbed api at http://localhost:4500, and that it has the two test report series we discussed

ga4gh/testbed/submit/submit_to_api.py Outdated Show resolved Hide resolved
ga4gh/testbed/submit/submit_to_api.py Outdated Show resolved Hide resolved
@Chen2x Chen2x requested a review from jb-adams April 5, 2022 14:45
Copy link
Member

@jb-adams jb-adams left a comment

Choose a reason for hiding this comment

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

In the last review, I mentioned:

There's also no test code for this module. When you add your unit tests, assume that there is a running testbed api at http://localhost:4500, and that it has the two test report series we discussed

I'm still not seeing any test code, which means there's no way to know if the function works correctly or breaks in the future. Can you please add tests?

@Chen2x
Copy link
Contributor Author

Chen2x commented Apr 6, 2022

Whoops I missed that part. Adding the tests in today.

Copy link
Member

@jb-adams jb-adams left a comment

Choose a reason for hiding this comment

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

in addition to the inline comments, the failing tests should also be addressed

'''
header = {"GA4GH-TestbedReportSeriesId": series_id, "GA4GH-TestbedReportSeriesToken": series_token}
submit_request = requests.post(url, headers=header ,json=report)
return submit_request.status_code
Copy link
Member

Choose a reason for hiding this comment

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

Returning the status code from this method is insufficient, especially if there's been an error we want to know why (i.e. the message). If the submission has been successful, then we want the id of the new report that was created on the server. So,

  • Always return status code
  • Return error message if submission wasn't successful, None if submission was successful
  • Return new report id if submission was successful, None if submission wasn't successful

Should be pretty easy to do in a Python dictionary

@jb-adams jb-adams merged commit c0bbada into develop Apr 8, 2022
@jb-adams jb-adams deleted the feature/issue-6-submission-function branch April 8, 2022 13:43
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.

Implement submission function
2 participants