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

Support source creation from consent, remove kit password #55

Merged
merged 5 commits into from
Jan 21, 2020
Merged

Conversation

AmandaBirmingham
Copy link
Collaborator

@AmandaBirmingham AmandaBirmingham commented Jan 18, 2020

Fixes #27, moving the consent endpoint under account and adding a post to enable creation of a human source from a submitted consent form.

Unfortunately, those who live by the shiny new supporting library also die by the shiny new supporting library: although the openapi3 specs support allowing different media types to be passed into a single endpoint (which would have allowed us to add creating a human source from consent form data as another aspect of the /accounts/{account_id}/sources POST that already can take in json data), connexion doesn't currently implement the "multiple media types per endpoint" aspect of openapi3 (spec-first/connexion#655, spec-first/connexion#805 ). Therefore it was necessary to kludge a little. The new /accounts/{account_id}/consent endpoint has a GET that gets consent form html as well as, now, a POST that creates a human source from a submitted consent form (using ONLY the form data media type). I am open to suggestions for better ways to do this.

Because @dhakim87 has ongoing work on source.py, I can't carry through the full implementation of creating a source from the consent form, but I have verified I can parse the incoming data and toss it over to the existing create_source call. Note that, for some reason I can't fathom, connexion sends all form field values to the back end as lists, regardless of the input element type ... e.g.,

{'age_range': ['7-12'], 'consent_child': ['Yes'], 'participant_name': ['a'], 'participant_email': ['b@c.com'], 'consent_witness': ['Yes'], 'obtainer_name': ['Me'], 'consent_parent': ['Yes'], 'parent_1_name': ['b'], 'parent_2_name': ['c'], 'deceased_parent': ['true']}

... Given this, the main thing I do with the form data is ensure each of these lists is really just a single value and then dig it out to make, e.g.,

{'age_range': '7-12', 'consent_child': 'Yes', 'participant_name': 'a', 'participant_email': 'b@c.com', 'consent_witness': 'Yes', 'obtainer_name': 'Me', 'consent_parent': 'Yes', 'parent_1_name': 'b', 'parent_2_name': 'c', 'deceased_parent': 'true'}

... which is what I thought we should get anyway ...

This PR also fixes #27, removing use of kit_password from the yaml api, implementation.py, and kit_repo.py.

Copy link
Collaborator

@dhakim87 dhakim87 left a comment

Choose a reason for hiding this comment

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

Change Request: Can you check if you can change the type from array to whatever in the yaml and fix everything coming in as lists in the body?
Comment: Pick either 18-plus or 18+ and let me know which one to use in the database
Comment: Let me know what to store for deceased_parent, needs schema change and migration heuristic most likely.

@@ -278,23 +278,44 @@ def dissociate_answered_survey(account_id, source_id, sample_id, survey_id):
return not_yet_implemented()


def read_kit(kit_name, kit_password):
def read_kit(kit_name):
with Transaction() as t:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... I know we aren't going to have kit_password in the future, but does this break migration for existing users? Are our existing kit_name's secure enough to allow everyone to login by kit_name alone until they first register an account with us? I wonder if we need a flag in the db for old_kit or new_kit to enable this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding was that the decision had been made to take this out, so if that is being rethought, I gotta kick this one to @wasade ....

microsetta_private_api/api/microsetta_private_api.yaml Outdated Show resolved Hide resolved
microsetta_private_api/api/implementation.py Outdated Show resolved Hide resolved
microsetta_private_api/api/microsetta_private_api.yaml Outdated Show resolved Hide resolved
@AmandaBirmingham
Copy link
Collaborator Author

@dhakim87 , think we need an update on what is required before this PR can be approved. AFAIK, the form field lists issue is fixed (thanks again :) and the 18+ vs 18-plus question has been resolved (in a way that doesn't require changes to this PR). The two remaining issues I see are:

  1. Are we really taking out kit_password or no? I think we are still waiting for feedback from @wasade on this?
  2. How should deceased_parent be represented? My current bid is to change it from a bool to an enum (so we can still load the existing freaky values but users can't make up new ones). Let me know if this works for you or if you want a different solution ...

@dhakim87 dhakim87 merged commit 4dfe9b9 into master Jan 21, 2020
@dhakim87 dhakim87 deleted the Issue27 branch June 8, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine and implement correct submission path for consent forms
2 participants