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

(WIP) St dh port vioscreen #125

Merged
merged 33 commits into from
May 5, 2020
Merged

(WIP) St dh port vioscreen #125

merged 33 commits into from
May 5, 2020

Conversation

dhakim87
Copy link
Collaborator

A work in progress for including vioscreen surveys. Needs testing with real credentials to see if urls generated can access the vioscreen surveys.

Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Very good progress!!!

microsetta_private_api/api/implementation.py Outdated Show resolved Hide resolved
microsetta_private_api/api/implementation.py Show resolved Hide resolved
int(vio_info["status"]))
t.commit()

# TODO: Any reason to respond with anything?
Copy link
Member

Choose a reason for hiding this comment

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

open question: should we extract the FFQ survey results now, or later?

microsetta_private_api/repo/vioscreen_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/util/vioscreen.py Outdated Show resolved Hide resolved
microsetta_private_api/util/vioscreen.py Outdated Show resolved Hide resolved
microsetta_private_api/util/vioscreen.py Outdated Show resolved Hide resolved

regcode = SERVER_CONFIG["vioscreen_regcode"]

# TODO: Specifying a callback url that goes back to our api server is, once
Copy link
Member

Choose a reason for hiding this comment

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

Can we just put microsetta-rest.ucsd.edu?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This requires more thought. It's possible we could have the get return back to the rest api rather than the client, but it would then be up to the rest api to redirect back to the client. It's a slight change from what I had (Vioscreen redirects to client, which then sends a message to the api), but would probably be cleaner for developer implementing the api. Unfortunately, it requires us to hold state of how to redirect to the client on the api side, or alternately to encode a redirect url to the client inside of our encoding of a redirect to the api endpoint. It's probably possible, but something I'd prefer to design out more

Copy link
Member

Choose a reason for hiding this comment

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

Okay makes sense

microsetta_private_api/util/vioscreen.py Outdated Show resolved Hide resolved
…t_dh_port_vioscreen

# Conflicts:
#	microsetta_private_api/example/client_impl.py
#	repo_test_scratch.py
… the minimalInterface. Users can now take additional surveys. No idea if vioscreen actually works because I can't get past their crypto without real credentials
Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

The value 10001 is defined in 2 places, and specified outside of a variable in a third location. It would be great if this was in a single place (code or database)

@@ -1250,7 +1251,14 @@ components:
description: Client url to which the customized consent form should be posted.
schema:
type: string
example: "http://www.microsetta.org/workflow_create_human_source"
example: "https://www.microsetta.org/workflow_create_human_source"
Copy link
Member

Choose a reason for hiding this comment

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

what is microsetta.org?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an example url string? 🤷‍♂️ All I did was make it https

microsetta_private_api/example/client.yaml Outdated Show resolved Hide resolved
microsetta_private_api/example/client_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/example/client_impl.py Show resolved Hide resolved
microsetta_private_api/templates/source.jinja2 Outdated Show resolved Hide resolved
microsetta_private_api/util/vioscreen.py Outdated Show resolved Hide resolved
wasade and others added 4 commits April 29, 2020 21:51
Survey template id is not survey id
Need account and source to write ag_login_surveys
Need sample to associate a survey with a sample
I needed location headers out of ApiRequests
survey ids with no associated answers now appear in ag_login_surveys (these are the vio screen surveys), which complicates lookup of survey_template_id
Modifying of vioscreen status changed from update to upsert (insert/update).  This bypasses the need to track generation of vioscreen urls - we don’t add a row until a survey is completed.
Fixed ancient potential bug in pkcs7 unpadding.  Updated vioscreen code for python 3
vioscreen survey_ids now generated as secrets.token_hex(8)
@wasade
Copy link
Member

wasade commented Apr 30, 2020

Thanks @dhakim87! It looks like one of the failures is real. The other is a dependency issue on py38 -- is pycrypto actually needed? It doesn't look like it's used here but maybe I'm not seeing it?

@wasade wasade mentioned this pull request May 1, 2020
…t_dh_port_vioscreen

# Conflicts:
#	microsetta_private_api/api/implementation.py
#	microsetta_private_api/example/client.yaml
#	microsetta_private_api/example/client_impl.py
#	microsetta_private_api/repo/survey_template_repo.py
#	microsetta_private_api/templates/source.jinja2
@wasade wasade merged commit 6ae9cd5 into minimalInterface May 5, 2020
@dhakim87 dhakim87 deleted the st_dh_port_vioscreen branch June 8, 2020 19:09
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.

None yet

2 participants