-
Notifications
You must be signed in to change notification settings - Fork 28
Adds the pathways for v3 handling around fhir_ids (BB-4166) #1419
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
Changes from 51 commits
df80391
cfeb712
ef64ba6
cf33395
ab29772
433d5c5
157d988
2707686
6cae063
a58570a
6bf69b0
cb698da
4838058
8614644
c65b63c
90c7921
672c806
87c39e7
717462b
ebf5125
8525ea3
8ea6bd7
6aec825
e6bc40b
aa7dc60
f4c043d
69e9d8d
dd315f8
98c523b
b873afb
c180fd1
f280493
2da17de
d97fd9b
5b43959
173ec43
c411169
56928af
1c1732c
3982daa
0bdd43d
b95785b
f6d4d7d
1ea136e
85fd19c
675f69d
72f0819
a31aeba
f19d8a4
4cf4355
4e41897
63e5245
80307e3
6021985
91c3a37
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| from django.test import TestCase | ||
|
|
||
| from apps.versions import VersionNotMatched | ||
| from ..utils import get_api_version_number_from_url | ||
|
|
||
| SUPPORTED_VERSION_TEST_CASES = [ | ||
| {'url_path': '/v2/fhir/Patient/', 'expected': 2}, | ||
| # return 0 because v2 does not have a leading / | ||
| {'url_path': 'v2/fhir/Patient/', 'expected': 0}, | ||
| {'url_path': '/v3/fhir/Coverage/', 'expected': 3}, | ||
| {'url_path': '/v3/fhir/Coverage/v2/', 'expected': 3}, | ||
| ] | ||
|
|
||
|
|
||
| class TestDOTUtils(TestCase): | ||
| def test_get_api_version_number(self): | ||
| for test in SUPPORTED_VERSION_TEST_CASES: | ||
| result = get_api_version_number_from_url(test['url_path']) | ||
| assert result == test['expected'] | ||
|
|
||
| def test_get_api_version_number_unsupported_version(self): | ||
| # unsupported version will raise an exception | ||
| with self.assertRaises(VersionNotMatched, msg='4 extracted from /v4/fhir/Coverage/'): | ||
| get_api_version_number_from_url('/v4/fhir/Coverage/') |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |
| from django.core.validators import MinLengthValidator | ||
| from apps.accounts.models import get_user_id_salt | ||
|
|
||
| from apps.versions import Versions, VersionNotMatched | ||
|
|
||
|
|
||
| class BBFhirBluebuttonModelException(APIException): | ||
| # BB2-237 custom exception | ||
|
|
@@ -164,29 +166,25 @@ class Meta: | |
| ) | ||
| ] | ||
|
|
||
| def fhir_id(self, version: int = 2) -> str: | ||
| def fhir_id(self, version: int = Versions.V2) -> str: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible we want to remove the default here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or we might do We'll discuss during our walkthrough. Good thought.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would definitely be ideal but changing this default led to a lot of errors within the apps/logging/serializer and that is possibly tied in with the fact that we would no longer have a fhir_id column. We'll create a ticket to address this more properly and will be part of a deprecate v2 workflow.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://jira.cms.gov/browse/BB2-4247 has been created |
||
| """Helper method to return fhir_id based on BFD version, preferred over direct access""" | ||
| if version in (1, 2): | ||
| if version in [Versions.V1, Versions.V2]: | ||
| if self.fhir_id_v2 is not None and self.fhir_id_v2 != '': | ||
| # TODO - This is legacy code, to be removed before migration bluebutton 0010 | ||
| # If fhir_id is empty, try to populate it from fhir_id_v2 to support old code | ||
| self._fhir_id = self.fhir_id_v2 | ||
| self.save() | ||
| return str(self.fhir_id_v2) | ||
| return self.fhir_id_v2 | ||
| # TODO - This is legacy code, to be removed before migration bluebutton 0010 | ||
| # If fhir_id_v2 is empty, try to populate it from _fhir_id to support new code | ||
| if self._fhir_id is not None and self._fhir_id != '': | ||
| self.fhir_id_v2 = self._fhir_id | ||
| self.save() | ||
| return str(self._fhir_id) | ||
| return self._fhir_id | ||
| return '' | ||
| elif version == 3: | ||
| # TODO BB2-4166: This will want to change. In order to make | ||
| # BB2-4181 work, the V3 value needed to be found in the V2 column. | ||
| # 4166 should flip this to _v3, and we should be able to find | ||
| # values there when using (say) the test client. | ||
| if self.fhir_id_v2 is not None and self.fhir_id_v2 != '': | ||
| return str(self.fhir_id_v2) | ||
| elif version == Versions.V3: | ||
| if self.fhir_id_v3 is not None and self.fhir_id_v3 != '': | ||
| return self.fhir_id_v3 | ||
| return '' | ||
| else: | ||
| raise ValidationError(f'{version} is not a valid BFD version') | ||
|
|
@@ -203,7 +201,7 @@ def set_fhir_id(self, value, version: int = 2) -> None: | |
| elif version == 3: | ||
| self.fhir_id_v3 = value | ||
| else: | ||
| raise ValidationError(f'{version} is not a valid BFD version') | ||
| raise VersionNotMatched(f'{version} is not a valid BFD version') | ||
|
|
||
| @property | ||
| def user_hicn_hash(self): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,29 +42,23 @@ def test_not_require_user_mbi(self): | |
| cw = Crosswalk.objects.get(user=user) | ||
| self.assertEqual(cw.user_mbi, None) | ||
|
|
||
| # TODO BB2-4166: This was commented out as part of | ||
| # BB2-4181, but after the work of 4166 is done, this test should | ||
| # pass. However, in 4181, we are storing _v3 values in _v2 (at the model level), and | ||
| # therefore the logic of this test does not work in 4181's branch. | ||
| # Once 4166 is fully implemented, uncomment this test, and it should pass and | ||
| # navigating the test client using the v3 pathway should work. | ||
| # def test_mutable_fhir_id(self): | ||
| # user = self._create_user( | ||
| # "john", | ||
| # "password", | ||
| # first_name="John", | ||
| # last_name="Smith", | ||
| # email="[email protected]", | ||
| # fhir_id_v2="-20000000000001", | ||
| # fhir_id_v3="-30000000000001", | ||
| # ) | ||
| # cw = Crosswalk.objects.get(user=user) | ||
| # self.assertEqual(cw.fhir_id(2), "-20000000000001") | ||
| # self.assertEqual(cw.fhir_id(3), "-30000000000001") | ||
| # cw.set_fhir_id("-20000000000002", 2) | ||
| # cw.set_fhir_id("-30000000000002", 3) | ||
| # self.assertEqual(cw.fhir_id(2), "-20000000000002") | ||
| # self.assertEqual(cw.fhir_id(3), "-30000000000002") | ||
| def test_mutable_fhir_id(self): | ||
| user = self._create_user( | ||
| "john", | ||
| "password", | ||
| first_name="John", | ||
| last_name="Smith", | ||
| email="[email protected]", | ||
| fhir_id_v2="-20000000000001", | ||
| fhir_id_v3="-30000000000001", | ||
| ) | ||
| cw = Crosswalk.objects.get(user=user) | ||
| self.assertEqual(cw.fhir_id(2), "-20000000000001") | ||
| self.assertEqual(cw.fhir_id(3), "-30000000000001") | ||
| cw.set_fhir_id("-20000000000002", 2) | ||
| cw.set_fhir_id("-30000000000002", 3) | ||
| self.assertEqual(cw.fhir_id(2), "-20000000000002") | ||
| self.assertEqual(cw.fhir_id(3), "-30000000000002") | ||
|
|
||
| def test_mutable_user_hicn_hash(self): | ||
| user = self._create_user( | ||
|
|
||
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.
Do we have
fhir_id_v1or both v1 and v2 sharefhir_id_v2, if so could you add comment in here to avoid confusion?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.
Sure thing. I can add a comment here, but for most areas we are definitely treating v1/v2 as the same thing since their pathways are fairly similar.
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.
@stiwarisemanticbits , for reference (and so we have it here):
There used to be only
fhir_id. We have now introduced_v2and_v3, and the references tofhir_idbecame a function on the model that now takes aversionparameter. So, everywhere we used to say:(which was, at that time, implicitly already v1 and v2)
we now say:
So, commenting that V1 now shares V2 is something we would have to comment throughout the entire codebase. @bwang-icf , before we start commenting this in the code, we may want to decide if/where we need to document this kind of design work. I personally think it would be the TRD.
Uh oh!
There was an error while loading. Please reload this 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 think commenting on the model is sufficient, otherwise we run into issues you described.
I believe we can also use
|in case statements in Python 3.11case Version.V1 | Version.V2This has the code read that they are treated the same, and if someone is curious as to why, they can reference the model comment. Just my two cents.