healthcare API: update all FHIR v1beta1 samples to v1 #3384

noerog · 2020-04-13T20:59:09Z

Originally from #3300 but split up here

* healthcare API: update all v1beta1 samples to v1 * healthcare API: use v1 instead of v1beta1 endpoint * healthcare API: remove FHIR conditional methods from v1 because they are only in v1beta1 * add requirements-test.txt files * Change test response for searching for studies to 204 * fix missing param in execute_bundle.json causing test failures * fix HL7v2 store failure. notificationConfig was deprecated in favor of notificationConfigs * address review comments: use ADC and simplify credentials auth * delete new FHIR files, they are now in #3384 * delete new DICOM files, they are now in #3387 * delete new HL7v2 files, they are now in #3388 * clean up test_dataset fixture * remove try block from the sample * more robust test setup and cleanup Co-authored-by: Takashi Matsuo <[email protected]>

tmatsuo · 2020-04-20T01:01:38Z

healthcare/api-client/v1/fhir/fhir_resources.py

Nit: Usually, we format long docstring like the following:

def get_client(): """Returns an authorized API client. It assumes you set GOOGLE_APPLICATION_CREDENTIALS environment variable. """

Short summary, then long description.

tmatsuo · 2020-04-20T01:02:04Z

healthcare/api-client/v1/fhir/fhir_resources.py

Code removed.

tmatsuo · 2020-04-20T01:03:04Z

healthcare/api-client/v1/fhir/fhir_resources.py

Here and there, I'd remove these 2 empty lines.

A result of black as we discussed in the other PR. I'll try removing it when I remember to.

tmatsuo · 2020-04-20T01:06:02Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

Can you put pytest import in the bottom below?

I don't see the change. The import statements should be in 3 blocks; 1) standard libs 2) 3rd party libs 3) local libs

So they should be:

import os import sys import uuid import backoff from googleapiclient.errors import HttpError import pytest # Add datasets for bootstrapping datasets for testing sys.path.append(os.path.join(os.path.dirname(__file__), "..", "datasets")) # noqa import datasets import fhir_stores import fhir_resources

or something like that

tmatsuo · 2020-04-20T01:07:25Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

FYI, I replaced retrying with backoff in #3434.

It's up to you to follow.

tmatsuo · 2020-04-20T01:09:59Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

This code ignores the exception when the status is 401. Is this intended?

Code removed.

Sorry I still see it

tmatsuo · 2020-04-20T01:11:41Z

healthcare/api-client/v1/fhir/fhir_stores.py

Short summary and long description.

tmatsuo · 2020-04-20T01:15:42Z

healthcare/api-client/v1/fhir/fhir_stores_test.py

I think it is very easy to create a fixture for blob. WDYT?
With the current code, it is not guaranteed to be deleted.

Created a fixture.

tmatsuo

Looking pretty good modulo few more changes

tmatsuo · 2020-04-21T17:17:27Z

healthcare/api-client/v1/fhir/fhir_resources.py

Can you format the docstring like:

"""Deletes a FHIR resource. Regardless of whether the operation succeeds or....

tmatsuo · 2020-04-21T17:18:25Z

healthcare/api-client/v1/fhir/fhir_resources.py

ditto:

"""Searches resources in the given FHIR store. It uses the ...

tmatsuo · 2020-04-21T17:20:58Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

I don't see the change. The import statements should be in 3 blocks; 1) standard libs 2) 3rd party libs 3) local libs

So they should be:

import os import sys import uuid import backoff from googleapiclient.errors import HttpError import pytest # Add datasets for bootstrapping datasets for testing sys.path.append(os.path.join(os.path.dirname(__file__), "..", "datasets")) # noqa import datasets import fhir_stores import fhir_resources

or something like that

tmatsuo · 2020-04-21T17:23:12Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

Sorry I still see it

tmatsuo · 2020-04-21T17:23:44Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

Why do you want to print this?

tmatsuo · 2020-04-21T17:24:48Z

healthcare/api-client/v1/fhir/fhir_stores_test.py

Remove an empty line

tmatsuo · 2020-04-21T22:43:25Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

nit: you don't need this I guess.

Weird, I don't know where these statements came from.

I still see print(out)

tmatsuo · 2020-04-21T22:43:34Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

I still see this

tmatsuo · 2020-04-21T22:43:41Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

tmatsuo · 2020-04-21T22:43:50Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

tmatsuo

I still see print(out). Please confirm

tmatsuo · 2020-04-22T16:04:43Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

I still see print(out)

tmatsuo · 2020-04-22T16:04:55Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

I still see this

noerog · 2020-04-22T18:10:21Z

I still see print(out). Please confirm

Please check again?

kurtisvg · 2020-04-22T20:02:32Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

Can we expand this comment (similar to other PR)?

kurtisvg · 2020-04-22T20:02:43Z

healthcare/api-client/v1/fhir/fhir_resources_test.py

healthcare/api-client/v1/fhir/fhir_stores.py

healthcare/api-client/v1/fhir/fhir_stores_test.py

…ource operations

healthcare/api-client/v1/fhir/fhir_resources_test.py

noerog requested a review from a team as a code owner April 13, 2020 20:59

blunderbuss-gcf bot assigned leahecole Apr 13, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 13, 2020

noerog mentioned this pull request Apr 13, 2020

healthcare API: update v1beta1 datasets samples to v1 #3300

Merged

noerog assigned tmatsuo Apr 13, 2020

tmatsuo suggested changes Apr 20, 2020

View reviewed changes

tmatsuo suggested changes Apr 21, 2020

View reviewed changes

noerog requested a review from kurtisvg April 21, 2020 20:02

noerog unassigned leahecole Apr 21, 2020

tmatsuo approved these changes Apr 21, 2020

View reviewed changes

tmatsuo suggested changes Apr 22, 2020

View reviewed changes

tmatsuo approved these changes Apr 22, 2020

View reviewed changes

kurtisvg suggested changes Apr 22, 2020

View reviewed changes

noerog added 15 commits April 22, 2020 17:15

healthcare API: update all FHIR v1beta1 samples to v1

08cb6ce

update FHIR v1beta1 samples to v1

0e45af5

minor fixes, remove requests package from requirements.txt

ba1bcb8

try adding oauth2client to requirements.txt for tests

8aa3373

fix lint

0b035cd

additional methods/cleanup for FHIR v1

e27d7cc

use backoff for tests, go back to using requests session for FHIR res…

7feb1fc

…ource operations

fix lint

c6c101f

fix failing tests

7653a0d

address review comments

e8c71df

fix lint

a991465

fix failures

cef304b

address review comments

58ac998

remove some print lines

d1a315a

address review comments

458e212

kurtisvg approved these changes Apr 23, 2020

View reviewed changes

healthcare/api-client/v1/fhir/fhir_resources_test.py Show resolved Hide resolved

Merge branch 'master' into chc-api-fhir

8f564fb

noerog merged commit 079d59c into GoogleCloudPlatform:master Apr 23, 2020

healthcare API: update all FHIR v1beta1 samples to v1 #3384

healthcare API: update all FHIR v1beta1 samples to v1 #3384

Uh oh!

Conversation

noerog commented Apr 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment