-
Notifications
You must be signed in to change notification settings - Fork 17
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
Overhaul Adjustments Based on Testing #284
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a ino
@@ -917,7 +932,7 @@ | |||
$("#n_archaea_background").text(result["Archaea"].toLocaleString()).addClass("microbe-count").removeClass("spinner-grow spinner-grow-sm"); | |||
}); | |||
retrieveNeighbors(state, 1) | |||
.then(queryNeighborAndSelfMetadata(state, ["age_years", "sugary_sweets_frequency"])) | |||
.then(queryNeighborAndSelfMetadata(state, ["age_cat", "sugary_sweets_frequency"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this looks good, then let's keep it, but the corresponding variable would I think be host_age
followign the metadata updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...same for below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
host_age doesn't work relative to the current results from microsetta-processing (just tried and reverted). We can poke at that separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from wgs? maybe lack of samples and okay
# We need to know if their consent is current if they're trying to take an | ||
# external survey. Profiles without current consent can view their surveys | ||
# but we'll block them from updating them on the POST action. | ||
need_reconsent = check_current_consent(account_id, source_id, "data") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be handled by a prerequisite
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...I'm not really sure from the flow so genuine question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't use prerequisites for the need_reconsent decision-making because in many of the cases, we're allowing them to access a given API path but with reduced functionality. There probably would be a way to use the prerequisite system, but I think it would end up more complex and prone to issues than the current code if/when it needs to be maintained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk
@@ -1315,6 +1464,13 @@ def post_generate_vioscreen_url(*, | |||
account_id=None, | |||
source_id=None, | |||
body=None): | |||
need_reconsent = check_current_consent(account_id, source_id, "data") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does seem boilerplate so it may make sense as a prereq
@@ -3135,6 +3318,19 @@ def session_locale(): | |||
return matched_locale | |||
|
|||
|
|||
def check_current_consent(account_id, source_id, consent_type): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely should have prerequisites
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a helper function, I don't think having prerequisites would be appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk
Notable changes:
NOTE: Some Spanish translations are missing. They will be updated in a separate PR once Alejandra is available to review.