✨ Study resource #175

dankolbman · 2018-03-08T20:35:42Z

Adds endpoints for the study resource.

Addresses #161

znatty22 · 2018-03-09T16:33:11Z

dataservice/api/study/resources.py

+            abort(404, 'could not find {} `{}`'
+                  .format('Study', kf_id))
+
+        st = StudySchema(strict=True).load(body, instance=st).data


Couple requested changes here..

Add the partial=True kw arg to the load so that marshmallow allows you to deserialize without all required fields. With PATCH we want to allow someone to update one or more fields without having to include all required fields.

Surround the load with a try/except ValidationError.

This is what i did for participant:

# Partial update - validate but allow missing required fields try: p = ParticipantSchema(strict=True).load(body, instance=p, partial=True).data except ValidationError as err: abort(400, 'could not update participant: {}'.format(err.messages))

hmm I thought we had a general ValidationError handler, maybe we should add one?

znatty22 · 2018-03-09T16:34:24Z

dataservice/api/study/resources.py

+
+    def patch(self, kf_id):
+        """
+        Update an existing study


Can we update the comment to something like:

Update an existing study. Allows for partial update of resource.

znatty22 · 2018-03-09T17:49:44Z

tests/study/test_study_resources.py

+        study = resp['results']
+        self.assertEqual(kf_id, study['kf_id'])
+
+    def test_patch_study(self):


Probably a good idea to check the database as well. Make sure that only the fields in the body were modified and others remained the same.

znatty22 · 2018-03-09T17:53:38Z

dataservice/api/study/resources.py

+              Study
+        """
+        try:
+            st = Study.query.filter_by(kf_id=kf_id).one()


Should we change this to use .get now that kf_id is the primary key? If we decide to make that change, we'll also have to change the try/except NoResultFound to a if else statement to check if st is None. I think get() returns None if obj is not found

yeah, I'd like to use get_or_404(), but you're not able to pass a custom message to it. If we could do that, these 5 lines would be reduced down to one tidy line. We could consider hacking get_or_404() to take a message arg or not having a specific entity type returned in the not found response.

znatty22 · 2018-03-09T17:53:59Z

dataservice/api/study/resources.py

+        """
+        body = request.json
+        try:
+            st = Study.query.filter_by(kf_id=kf_id).one()


Similar to above comment, should we change this to use .get now that kf_id is the primary key?

znatty22 · 2018-03-09T17:54:09Z

dataservice/api/study/resources.py

+              Study
+        """
+        try:
+            st = Study.query.filter_by(kf_id=kf_id).one()


Similar to above comment, should we change this to use .get now that kf_id is the primary key?

znatty22 · 2018-03-09T18:22:13Z

tests/test_api.py

@@ -85,7 +94,8 @@ def test_read_only(self, client, entities, endpoint, field):

    @pytest.mark.parametrize('endpoint,field', [
        ('/participants', 'blah'),
-        ('/samples', 'blah')
+        ('/samples', 'blah'),
+        ('/studies', 'blah'),
    ])
    def test_unknown_field(self, client, entities, endpoint, field):


I changed this method in #173 to provide an HTTP method as input so that we could perform this test for both POST and PATCH (and later PUT). We may want to merge #173 before this one and then you can update this test for study

znatty22 · 2018-03-09T22:38:47Z

dataservice/api/study/resources.py

+        st = StudySchema(strict=True).load(body, instance=st).data
+        db.session.commit()
+
+        return StudySchema(


Should return 200 instead of 201 since we are not creating a new resource

znatty22 · 2018-03-09T22:40:14Z

tests/study/test_study_resources.py

+                                           kf_id=kf_id),
+                                   headers=self._api_headers(),
+                                   data=json.dumps(body))
+        self.assertEqual(response.status_code, 201)


Rember to change to check for 200 after updating patch method to return 200 instead of 201

znatty22 · 2018-03-12T20:16:49Z

dataservice/api/study/resources.py

+        st = Study.query.get(kf_id)
+        if st is None:
+            abort(404, 'could not find {} `{}`'
+                  .format('Study', kf_id))


Change 'Study' to 'study' to keep all error msgs standard

znatty22 · 2018-03-12T20:16:57Z

dataservice/api/study/resources.py

+        st = Study.query.get(kf_id)
+        if st is None:
+            abort(404, 'could not find {} `{}`'
+                  .format('Study', kf_id))


Change Study to study to keep all error msgs standard

znatty22 · 2018-03-12T20:18:00Z

dataservice/api/study/resources.py

+        """
+        st = Study.query.get(kf_id)
+        if st is None:
+            abort(404, 'could not find {} `{}`'.format('Study', kf_id))


Change Study to study to keep all error msgs standard

znatty22 · 2018-03-12T20:45:32Z

tests/test_api.py

-        ('/participants/123', 'DELETE', 'could not find Participant `123`')
+        ('/participants/123', 'DELETE', 'could not find Participant `123`'),
+        ('/studies', 'GET', 'success'),
+        ('/studies/123', 'GET', 'could not find Study `123`'),


Remember to change from Study to study after updating resource class

znatty22 · 2018-03-16T13:55:24Z

tests/study/test_study_resources.py

+                                    data=json.dumps(body))
+        return response
+
+    def _test_response_content(self, resp, status_code):


Remove method and calls to it

znatty22 · 2018-03-16T13:55:39Z

tests/study/test_study_resources.py

+
+from flask import url_for
+
+from dataservice.extensions import db


Remove unused import: dataservice.extensions.db

znatty22 · 2018-03-16T13:57:00Z

dataservice/api/study/resources.py

+            st = (StudySchema(strict=True).load(body, instance=st,
+                                                partial=True).data)
+        except ValidationError as err:
+            abort(400, 'could not update participant: {}'.format(err.messages))


Prob copy/paste error, change could not update participant to could not update study

znatty22 · 2018-03-16T13:59:31Z

dataservice/api/study/resources.py

+
+    def patch(self, kf_id):
+        """
+        Update fields in a study and preserve existing fields.


Nitpicking but can we be consistent w the docstrings for crud resources? All the other resources have patch docstring like: Update an existing demographic. Allows partial update of resource. If you don't like above, then lets at least create an issue to standardize docstrings for the resource crud methods

znatty22

👍

✨ Study resource

dankolbman added the feature New functionality label Mar 8, 2018

dankolbman self-assigned this Mar 8, 2018

dankolbman requested review from znatty22 and parimalak March 8, 2018 20:35

dankolbman force-pushed the study-resource branch 5 times, most recently from f76e73f to f1e7ed7 Compare March 8, 2018 22:06

znatty22 reviewed Mar 9, 2018

View reviewed changes

znatty22 mentioned this pull request Mar 9, 2018

✨ Investigator resource #176

Merged

znatty22 reviewed Mar 9, 2018

View reviewed changes

dankolbman force-pushed the study-resource branch from f1e7ed7 to cdb42a8 Compare March 12, 2018 14:56

znatty22 reviewed Mar 12, 2018

View reviewed changes

dankolbman force-pushed the study-resource branch 5 times, most recently from 868634b to c70cd06 Compare March 16, 2018 13:39

znatty22 reviewed Mar 16, 2018

View reviewed changes

dankolbman force-pushed the study-resource branch 2 times, most recently from 5a69450 to 3988371 Compare March 16, 2018 14:41

dankolbman added 2 commits March 16, 2018 10:41

✨ Add study resource

b740a77

✅ Add tests for study resource

d458a98

dankolbman force-pushed the study-resource branch from 3988371 to d458a98 Compare March 16, 2018 14:42

znatty22 approved these changes Mar 16, 2018

View reviewed changes

dankolbman merged commit 33550a0 into master Mar 16, 2018

dankolbman deleted the study-resource branch March 16, 2018 15:48

dankolbman mentioned this pull request Apr 2, 2018

🏷 Release 0.2.0 #217

Merged

dankolbman removed the ready-for-review This PR needs a review label May 7, 2018

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

Merge pull request #175 from kids-first/study-resource

ffcb636

✨ Study resource

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Study resource #175

✨ Study resource #175

dankolbman commented Mar 8, 2018

znatty22 Mar 9, 2018

dankolbman Mar 12, 2018

znatty22 Mar 9, 2018

znatty22 Mar 9, 2018 •

edited

Loading

znatty22 Mar 9, 2018 •

edited

Loading

dankolbman Mar 12, 2018

znatty22 Mar 9, 2018

znatty22 Mar 9, 2018

znatty22 Mar 9, 2018 •

edited

Loading

znatty22 Mar 9, 2018

znatty22 Mar 9, 2018

znatty22 Mar 12, 2018

znatty22 Mar 12, 2018

znatty22 Mar 12, 2018

znatty22 Mar 12, 2018

znatty22 Mar 16, 2018

znatty22 Mar 16, 2018 •

edited

Loading

znatty22 Mar 16, 2018

znatty22 Mar 16, 2018

znatty22 left a comment


		from flask import url_for

		from dataservice.extensions import db

✨ Study resource #175

✨ Study resource #175

Conversation

dankolbman commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znatty22 Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

znatty22 Mar 9, 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 Mar 9, 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

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 Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

znatty22 left a comment

Choose a reason for hiding this comment

znatty22 Mar 9, 2018 •

edited

Loading

znatty22 Mar 9, 2018 •

edited

Loading

znatty22 Mar 9, 2018 •

edited

Loading

znatty22 Mar 16, 2018 •

edited

Loading