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

Addressing Feedback from Production Testing #290

Merged
merged 8 commits into from
Sep 13, 2023

Conversation

cassidysymons
Copy link
Collaborator

@cassidysymons cassidysymons commented Sep 11, 2023

  • Add consent decline warning to default Profile page
  • Add ability for legacy sources to cancel re-consent without selecting an age range
  • Add message to Consent Documents page for users who don't reconsent
  • Hide next/previous survey info for users who didn't reconsent
  • Disable Skip/Display functionality for users who haven't reconsented
  • Disallow participants who did not reconsent from continuing an existing FFQ
  • Fix View Results button alignment on My Kits tab
  • Fix disable_link() on Reports tab
  • Change top-left logo to go to /home rather than https://microsetta.ucsd.edu/
  • Change "Not Completed" to "Needs Review" on survey cards for existing users who have answered at least one question in a newly reorganized survey template
  • Add last taken/time estimates to mobile survey cards
  • Tweak tooltips for surveys/skipping questions
  • Change placeholder on FFQ registration code to match format of actual codes
  • Start sample collection time at midnight
  • Disable Polyphenols survey
  • Disable animal & environmental sources
  • Add Account Details link to admin_mode view of Dashboard
  • Fix JavaScript issue with Register Kit/Register FFQ button
  • Add admin-only Account Details link to Dashboard
  • Sort My Kits tab by sample_datetime descending, with null values moved to the top

@cassidysymons cassidysymons marked this pull request as ready for review September 12, 2023 22:26
if need_reconsent:
# If the user has not agreed to the current consent, we
# disable all of the fields.
field['disabled'] = True
Copy link
Member

Choose a reason for hiding this comment

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

Out of paranoia, and the fun you can have in a javascript console, do we assert consent prior to storing in the backend? I think yes but just checking...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -2480,12 +2522,22 @@ def admin_barcode_search_query_qiita(body):


def get_ajax_check_kit_valid(kit_name):
if re.match(
r"[dD][mM][kK][234689ACDEFHJKMNPRTVWXYacdefhjkmnprtvwxy]{6}",
Copy link
Member

Choose a reason for hiding this comment

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

why this and not re.match(r"DMK[234689ACDEFHJKMNPRTVWXY]{6}", kit_name.upper())? I think its the same in the end but maybe im missing something. If it is, benefit is easier to read regex imo...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, adjusting.

kit, error, _ = _get_kit(kit_name)
result = True if error is None else error
return flask.jsonify(result)


def get_ajax_list_kit_samples(kit_name):
if re.match(
r"[dD][mM][kK][234689ACDEFHJKMNPRTVWXYacdefhjkmnprtvwxy]{6}",
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to define the regex once, maybe at global level, eg:

DAKLAPACK_KIT_RE = re.compile(r"...")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adjusting.

@@ -149,13 +149,15 @@
}
}

function updateButtonState(kit_id_value) {
if(kit_id_value != "") {
/*
Copy link
Member

Choose a reason for hiding this comment

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

should this code be deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to revisit making the button responsive to user input, so I'd prefer not to delete the code entirely.

@@ -72,13 +72,15 @@
});
});

/*
Copy link
Member

Choose a reason for hiding this comment

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

should this function be deleted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to revisit making the button responsive to user input, so I'd prefer not to delete the code entirely.

@cassidysymons
Copy link
Collaborator Author

@wasade Ready for re-review when you have a moment.

@wasade
Copy link
Member

wasade commented Sep 12, 2023

Responses seem good, thanks!

@cassidysymons cassidysymons merged commit 58ffead into master Sep 13, 2023
3 checks passed
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.

2 participants