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

✨Study file resource #197

Merged
merged 5 commits into from
Apr 2, 2018
Merged

✨Study file resource #197

merged 5 commits into from
Apr 2, 2018

Conversation

parimalak
Copy link
Contributor

@parimalak parimalak commented Mar 23, 2018

Added resources for study_file endpoint
Added tests for study_file endpoint

Resolves #197

@parimalak parimalak added the feature New functionality label Mar 23, 2018
@parimalak parimalak self-assigned this Mar 23, 2018
@parimalak parimalak added this to the CHOP Sprint 4 milestone Mar 23, 2018
_links = ma.Hyperlinks({
'self': ma.URLFor(Meta.resource_url, kf_id='<kf_id>'),
'collection': ma.URLFor(Meta.collection_url),
'study_files': ma.URLFor('api.studies', kf_id='<study_id>')
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should just be study?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont know the usecase for having 'study' information in _links but, it is easier to get all the study_files from study as each study_file belong to one study. i am saying this because we can remove this line altogether from _links similar to what we did for investigator and study resources.



class StudyFileTest(FlaskTestCase):
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit, but we should stay consistent with the use of double quotes for docstrings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep i used study tests as reference i am gonna change for other resources as well👍

_links = ma.Hyperlinks({
'self': ma.URLFor(Meta.resource_url, kf_id='<kf_id>'),
'collection': ma.URLFor(Meta.collection_url),
'study_files': ma.URLFor('api.studies', kf_id='<study_id>')
Copy link
Member

Choose a reason for hiding this comment

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

Should be singular:

'study_file': ma.URLFor('api.studies', kf_id='<study_id>')

self.assertEqual(study_file['kf_id'], kf_id)
self.assertEqual(study_file['file_name'], body['file_name'])

def test_patch_study_file_no_required_field(self):
Copy link
Member

Choose a reason for hiding this comment

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

Could probably remove this test since it's essentially the same as test_patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -31,6 +32,12 @@ def participants(client):
db.session.add(s)
db.session.commit()

# Add study files for studies
for _ in range(102):
Copy link
Member

Choose a reason for hiding this comment

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

Could probably create the study files and link the files to a study in the loop where you're creating each study

@dankolbman dankolbman force-pushed the study-file-resource branch from c92b896 to a214931 Compare March 27, 2018 12:13
@baileyckelly baileyckelly removed this from the CHOP Sprint 4 milestone Mar 29, 2018
Copy link
Contributor

@dankolbman dankolbman left a comment

Choose a reason for hiding this comment

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

💯

@parimalak parimalak force-pushed the study-file-resource branch from a214931 to e6c6f24 Compare March 30, 2018 20:51
Copy link
Member

@znatty22 znatty22 left a comment

Choose a reason for hiding this comment

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

👍

@parimalak parimalak force-pushed the study-file-resource branch from e6c6f24 to ffcfca5 Compare April 2, 2018 16:37
@parimalak parimalak merged commit 0733160 into master Apr 2, 2018
@parimalak parimalak deleted the study-file-resource branch April 2, 2018 16:43
@dankolbman dankolbman mentioned this pull request Apr 2, 2018
@dankolbman 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants