✨ Demographic resource and tests #62

znatty22 · 2018-01-22T15:00:28Z

Add a crud api for the demographic entity (get, post, put, delete methods)
Add testing crud api

znatty22 · 2018-01-22T15:01:53Z

dataservice/api/common/error_handler.py

+"""
+API level error handling/message formatting
+
+Includes helpers to parse the generic database integrity error


There doesn't seem to be a better way to distinguish specific types of integrity errors. If someone else knows of anything better please let me know.

znatty22 · 2018-01-22T15:06:53Z

dataservice/api/demographic/resources.py

+            context = {'method': 'create', 'entity': 'demographic',
+                       'ref_entity': 'participant', 'exception': e,
+                       'payload': body}
+            # raise BadInputError(context=context)


Left this comment in here to show that error handling could also be handled with custom exceptions. With this approach, we would throw a custom exception and then handle it at the API level using the api.errorhandler decorator. Decided to avoid this approach for now due to some related known bugs with flask_restplus errorhandling. See https://github.com/noirbizarre/flask-restplus/issues

znatty22 · 2018-01-22T15:20:38Z

dataservice/api/__init__.py

+
+
+# TODO
+"""


As a work around we may need to change our kf_response format a little bit to match the default restplus format. So instead of having

{ "_links": { ... }, "_status": { "code": 200, "message": "Success" }, "results": [ ... ] } ``` We might need to change to something like:

{ "_links": { ... }, "code": 200, "message": "Success" "results": [ ... ] } ```

Yeah, kf_response is a little restricted in what it actually applies to. I'm trying to move it down into the Resource but it may have to go deeper. For now, extending the Api with a modified error_handler may be easiest.

znatty22 · 2018-01-22T15:25:44Z

dataservice/api/demographic/resources.py

+        # Catch error and rollback db
+        except IntegrityError as e:
+            db.session.rollback()
+            context = {'method': 'create', 'entity': 'demographic',


Might be worth considering the crud mixin or something like it :). Reason being, if we had something to encapsulate our crud methods we could handle the db errors in that class and then avoid passing context parameters to another module (error_handler). The crud class would still call out to the error_handler to determine the type of error but it could take care of forming the actual error message with the context (crud method used, the entity, the reference entity, etc). Would also avoid duplicate code in every entity resource class

I think that exception handling from werkzeug and flask already encapsulate these things pretty well. It seems like the functionality here could be achieved by coercing to the standard error handling format, or by extending it as needed.

dankolbman

I think a lot of the abstractions your suggesting could be taken care of by extending the response handling in flask and flask-restplus to fit our needs. This approach seems more natural to me as it would still use the same familiar interface of a flask application rather than a high level library that ultimately deals with low level functionality.

I've been looking at how the resource responses could be handled here but there's some nuances with restplus' marshaling that make it difficult. It may be better to look more deeply into flask for how that may be achieved.

dankolbman · 2018-01-22T15:52:51Z

dataservice/api/__init__.py

+
+
+# TODO
+"""


Yeah, kf_response is a little restricted in what it actually applies to. I'm trying to move it down into the Resource but it may have to go deeper. For now, extending the Api with a modified error_handler may be easiest.

dankolbman · 2018-01-22T16:01:52Z

dataservice/api/common/error_handler.py

+NOT_FOUND_MSG_TEMPLATE = 'Client error: {} not found'
+
+
+def handle_invalid_input(context=None):


What is this function trying to accomplish? It looks like it's for formatting the error response from SQLAlchemy? In that case, does this really need to happen, and if so, could it happen by extending the SQLAlchemy exceptions?

Also, maybe the context should be kwargs? That way there's some sort of 'type' checking on the handle.

dankolbman · 2018-01-22T16:11:21Z

dataservice/api/demographic/resources.py

+        # Catch error and rollback db
+        except IntegrityError as e:
+            db.session.rollback()
+            context = {'method': 'create', 'entity': 'demographic',


I think that exception handling from werkzeug and flask already encapsulate these things pretty well. It seems like the functionality here could be achieved by coercing to the standard error handling format, or by extending it as needed.

dankolbman · 2018-01-22T16:19:01Z

dataservice/api/demographic/resources.py

+            kf_id=kf_id).one_or_none()
+
+        if not demographic:
+            return self._not_found(kf_id)


This should probably be refactored in the future. It seems the main reason we're doing it this way is to make our responses fit our desired format. If we can figure out how to extend the handling to use our response, this line could be as easy as raise HTTPException

dankolbman · 2018-01-22T16:20:49Z

dataservice/api/demographic/resources.py

+        if not demographic:
+            return self._not_found(kf_id)
+
+        demographic.external_id = body.get('external_id')


I've been thinking of moving the parsing functionality here to a from_json or from_dict on the ORM class. It Thoughts?

Yep, sounds like a good idea

dankolbman · 2018-01-22T16:29:08Z

dataservice/api/demographic/serializers.py

+    'sex': fields.String(
+        example='F',
+        description='The sex of the person this demographic belongs to'),
+    'person_id': fields.String(


participant_id?

Will update this PR now that #60 has been merged

dankolbman · 2018-01-22T16:30:17Z

tests/demographic/test_demographic_models.py

+        # Check that demographic was created correctly
+        self.assertEqual(Demographic.query.count(), 1)
+        new_demographic = Demographic.query.first()
+        self.assertGreater(new_demographic.created_at, dt)


We could probably draw out all these created_at and modified_at checks to a parametrized test.

znatty22 · 2018-01-22T17:12:44Z

Thanks for the comments/suggestions. Let's discuss more on error handling and response formatting

znatty22 · 2018-01-22T19:39:29Z

After discussing, we agreed that flask_restplus has too many issues with error handling and response marshaling. We've decided to go with regular flask instead of flask_restplus which will solve the error handling issues. And we are going with marshmallow to solve the request/response validation/formatting. See #63 for details.

znatty22 · 2018-01-28T21:33:59Z

dataservice/api/demographic/resources.py

+        try:
+            # Check if demographic exists
+            d1 = Demographic.query.filter_by(kf_id=kf_id).one()
+            # Deserialize


This can be done more dynamically/better in the future. Tried using sqlalchemy session.merge with the loaded obj from marshmallow but this achieves patch/partial update behavior. To do a full replace, would first have to iterate over the d1's columns and relationships and set them back to defaults and then perform merge with the deserialized object from marshmallow. This isn't trivial so will look into this in the near future.

dankolbman

It looks like a lot of the test functions could easily be replaced with 1-2 lines added to the parametrized tests here

dankolbman · 2018-01-29T15:21:00Z

tests/demographic/test_demographic_resources.py

+        message = 'Cannot create demographic, participant already'
+        self.assertIn(message, response['_status']['message'])
+
+        # Check database


No database checking?

Good catch, will add 👍

parimalak

👍

dankolbman

looks good!

✨ Demographic resource and tests

znatty22 requested review from dankolbman and parimalak January 22, 2018 15:00

znatty22 changed the title ~~✨ Add the demographic entity to the Kids First Dataservice~~ ✨ Add the demographic entity to the Kids First Dataservice Jan 22, 2018

znatty22 commented Jan 22, 2018

View reviewed changes

znatty22 added feature New functionality data model Changes to the underlying data representation labels Jan 22, 2018

znatty22 commented Jan 22, 2018

View reviewed changes

znatty22 self-assigned this Jan 22, 2018

dankolbman reviewed Jan 22, 2018

View reviewed changes

znatty22 mentioned this pull request Jan 22, 2018

✨ Add Demographic db model #64

Merged

znatty22 commented Jan 28, 2018

View reviewed changes

dankolbman reviewed Jan 29, 2018

View reviewed changes

znatty22 force-pushed the demographic-entity branch from 1cf797e to 006c71f Compare January 31, 2018 21:48

znatty22 changed the title ~~✨ Add the demographic entity to the Kids First Dataservice~~ ✨ Demographic resource and tests Jan 31, 2018

znatty22 force-pushed the demographic-entity branch 2 times, most recently from b68a3ca to c3862e3 Compare January 31, 2018 22:54

parimalak approved these changes Jan 31, 2018

View reviewed changes

dankolbman approved these changes Feb 1, 2018

View reviewed changes

✨ ✅ Add demographic resource and unit tests

78bc4b9

znatty22 force-pushed the demographic-entity branch from 10e3831 to 78bc4b9 Compare February 1, 2018 17:09

znatty22 merged commit 9d35767 into master Feb 1, 2018

znatty22 deleted the demographic-entity branch February 2, 2018 00:04

dankolbman mentioned this pull request Apr 2, 2018

🏷 Release 0.2.0 #217

Merged

alubneuski pushed a commit that referenced this pull request Oct 11, 2024

Merge pull request #62 from kids-first/demographic-entity

1d44918

✨ Demographic resource and tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Demographic resource and tests #62

✨ Demographic resource and tests #62

znatty22 commented Jan 22, 2018 •

edited

Loading

znatty22 Jan 22, 2018

znatty22 Jan 22, 2018

znatty22 Jan 22, 2018

dankolbman Jan 22, 2018

znatty22 Jan 22, 2018 •

edited

Loading

dankolbman Jan 22, 2018

dankolbman left a comment

dankolbman Jan 22, 2018

dankolbman Jan 22, 2018

dankolbman Jan 22, 2018

dankolbman Jan 22, 2018

dankolbman Jan 22, 2018

dankolbman Jan 22, 2018

znatty22 Jan 22, 2018

dankolbman Jan 22, 2018

znatty22 Jan 22, 2018

dankolbman Jan 22, 2018

znatty22 commented Jan 22, 2018 •

edited

Loading

znatty22 commented Jan 22, 2018 •

edited

Loading

znatty22 Jan 28, 2018

dankolbman left a comment

dankolbman Jan 29, 2018

znatty22 Jan 29, 2018

parimalak left a comment

dankolbman left a comment

		NOT_FOUND_MSG_TEMPLATE = 'Client error: {} not found'


		def handle_invalid_input(context=None):



		# TODO
		"""



		# TODO
		"""

✨ Demographic resource and tests #62

✨ Demographic resource and tests #62

Conversation

znatty22 commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znatty22 Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dankolbman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znatty22 commented Jan 22, 2018 • edited Loading

znatty22 commented Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

dankolbman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parimalak left a comment

Choose a reason for hiding this comment

dankolbman left a comment

Choose a reason for hiding this comment

znatty22 commented Jan 22, 2018 •

edited

Loading

znatty22 Jan 22, 2018 •

edited

Loading

znatty22 commented Jan 22, 2018 •

edited

Loading

znatty22 commented Jan 22, 2018 •

edited

Loading