Return previous survey results #466
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Unittest not modified for a modified read_survey_template() method; it doesn't appear to currently have one. |
Removed source_id as a field for consideration when testing. It appears to be dynamically generated.
Test changed to confirm full results will NOT match.
charles-cowart
changed the title
|
@cassidysymons Sorry for the wait. This is now ready for review. |
cassidysymons
requested changes
Oct 18, 2022
wasade
reviewed
Oct 28, 2022
Most issues addressed. Currently looking into the model and adding percentages to it.
wasade
requested changes
Nov 16, 2022
Added 'percentage_completed' as an attribute to SurveyTemplateLinkInfo. Like most of the other fields, it will initially be set to None and be populated by get_survey_responses().
|
Project conventions
…On Tue, Nov 15, 2022, 17:50 Charles Cowart ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In microsetta_private_api/repo/survey_template_repo.py
<#466 (comment)>
:
> + ag.group_questions b where
+ a.survey_group = b.survey_group and
+ survey_id = {survey_template_id} group by
+ survey_template_id"""
+
+ total_count = None
+
+ with self._transaction.cursor() as cur:
+ cur.execute(sql)
+ total_count = cur.fetchone()[1]
+
+ # adding indexes to these individual columns may improve performance.
+ # ag.survey_answers.survey_question_id
+ # ag.surveys.survey_id
+
+ sql = f"""select a.survey_id, a.survey_question_id, a.response,
Is there a technical reason to prefer JOIN over WHERE? I don't believe
there is a performance difference, given that this is an inner join, not a
left or other outer join.
—
Reply to this email directly, view it on GitHub
<#466 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADTZMV4DK56247PXKLG4O3WIQ4YFANCNFSM6AAAAAARCVWOZA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Will pull in the month fix from Cassidy's branch, reformat the query to use JOINs over WHEREs, and I'll review and enhance the test in question later tonight or earlier tomorrow. |
|
Thanks!
…On Tue, Nov 15, 2022, 18:06 Charles Cowart ***@***.***> wrote:
Will pull in the month fix from Cassidy's branch, reformat the query to
use JOINs over WHEREs, and I'll review and enhance the test in question
later tonight or earlier tomorrow.
—
Reply to this email directly, view it on GitHub
<#466 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADTZMQDLFZZGV3WT4PZPTDWIQ6TTANCNFSM6AAAAAARCVWOZA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@cassidysymons @wasade All feedback should be incorporated. All tests passing. Please feel free to review. |
|
Per @wasade's request, we want some additional testing to confirm push/pull when multiple surveys of template n are available, are the results passed to and from Qiita intelligently. |
|
This PR should also be against master-overhaul, not master. |
charles-cowart
added a commit
to charles-cowart/microsetta-private-api
that referenced
this pull request
Dec 5, 2022
migrate_templates() functionality moved to SurveyTemplateRepo, along w/associated functions. Now used by read_survey_template() to populate results w/legacy responses before handing them to the UI. migrate_templates() no longer generates new surveys from old surveys and saves them. migrate_templates() was a better version of get_survey_responses() found in PR biocore#466. Hence I incorporated the other component of biocore#466 (percentage completed value) and folded it into the PR. PR biocore#466 can be considered superseeded.
|
Closing. Superseded by #489 |
cassidysymons
added a commit
that referenced
this pull request
Dec 21, 2022
* (WIP) migrating survey to updated structure Work in progress. Triggers need to be added. Some remapped questions need to be remade into new questions due to constraints and new/changed/deleted options. Migraines category needs to be merged and removed. Basic testing w/UI needed. * (WIP) Updated questions 15 new questions created w/shortnames ending in '_v2' to address questions with many changed response options. New questions linked and existing questions unlinked and retired. All triggers added. Migraines section needs merging. Testing against current unittests needed. * (WIP) patch should successfully load. * First unittest fixed Traced entire execution and reviewed all SQL statements. This test appears like it can be resolved with changing some constants. * One test updated * Fix test_edit_sample_locked() Fixed test to use new existing survey. relativedelta(month=2) appeared to have issues w/rollover for both month and year when the current month is 11 (November). Replaced w/standard datetime.timedelta() using 61 days, the median number of days in a given two month span (from beginning of the month to the end of the next month). * Fix test_fetch_survey_template() Added survey information for the new surveys. Did not delete references to the removed surveys for the moment. * Fixed test_delete_source() * Fixed test_survey_localization() Fixed localization using new Basic Information (10) survey. Possible todos in the future would be to replace the magic number 10 w/a new const name and change localization test to es_MX where the localizations are more different. * Fixed test_associate_sample_and_survey() * Fixed test_scrub* tests (3) Also pushing some partial changes for test_bobo_takes_all_local_surveys() from last night. Not sure why they're not in there. * Fixed test_dissociate_sample_from_source_success() * Fixed test_survey_create_success_empty() * Fixed test_fetch_observed_survey_templates() * Fixed test_surveys() * Fixed test_associate_sample_locked() * Fixed test_dissociate_sample_from_source_locked() * Fixed test_dissociate_sample_from_source_success() * Fixed test_update_sample_association_locked() * Fixed test_associate_answered_survey_to_sample_success() * Fixed test_bobo_takes_all_local_surveys() * Fixed last non-test_metadata_qiita_compatible_* test Removed entries from survey_groups that were retired. Added survey_template_ids2() to this commit for possible use fixing test_metadata_qiita_compatible_* tests. * Updated code and tests Updated code and tests to resolve errors after previous adjustments to 0103.sql (now 0104.sql) broke two tests. Broken tests currently stand at four once more. * Resolved conflict * Restored four missing questions in survey groups Four questions were present in the system but missing entries in group_questions, which prevented them from being shown. This has been fixed. * (WIP) Data Migration for Surveys Minor bug in creating new surveys Localization is not implemented Removing old survey not implemented * (WIP) Data Migration Complete Data Migration code complete. Undergoing additional testing. 4 qiita-related tests + 1 additional test is now breaking. Will review and fix along with review of the database changes. * Bugfix * All retired questions migrated to group 10 All retired questions were migrated to group 10 (Basic Information). They previously made up the group number 9 (Retired Questions). They shoudn't appear in querys, because they are retired. However all questions need to be a member of a group, due to the mechanics of the system. * Fixed test_metadata_qiita_compatible_valid() and 1 other error * Fixed test_metadata_qiita_compatible_invalid() * All tests passing * New magic numbers replaced with const names * flake8 * Revised patch sql for new approach. * Updated query to exclude retired survey template ids * Change 0105.sql to 0107.sql * Fixed test_metadata_qiita_compatible_valid() Notes for debug of test_metadata_qiita_compatible_valid_private() included. 3 broken tests outstanding. * All tests passing It appears that the issue was not specifically with template 5, but with some needed reviewing and fixing of _to_pandas_dataframe() and _to_pandas_series(). * flake8 pass * Comments added. Non-ASCII characters replaced. All but one Non-ASCII characters replaced with ASCII equivalents. * sql fix * SQL-injection vectors closed. * lists converted to tuples for sql stmts * ag_login_survey.survey_template_ids now fully populate * Prepended tables w/schema names as may be needed * Inserts updated to handle new constraint * Minor fixes * Resolved name collision * Removed unnecessary updates to text questions. After earlier and more recent changes, including replacing non-ASCII characters with their equivalents and removing encapsulating quotes, there are now only 67 questions which required text changes, down from the original 169. * Minor updates Capitalized a few responses. Removed duplicate entries. Converted magic numbers to consts. * Modified function for clarity. Modified _remote_survey_template_id_from_survey_id() for clarity. * Migrated logic from v1 migration into testable functions. * Added support in migration for text and string values * Confirm free-text is generated dynamically Tests pass locally. Display obs and exp in CI for review * Removed migration function We will not be copying survey data into new surveys with new ids and new template ids. * Migrating code to proper module Migrating new and associated code to survey_template_repo, where it seems more appropriate. * read_survey_template() now uses migrate_templates() migrate_templates() functionality moved to SurveyTemplateRepo, along w/associated functions. Now used by read_survey_template() to populate results w/legacy responses before handing them to the UI. migrate_templates() no longer generates new surveys from old surveys and saves them. migrate_templates() was a better version of get_survey_responses() found in PR #466. Hence I incorporated the other component of #466 (percentage completed value) and folded it into the PR. PR #466 can be considered superseeded. * Possible bugfix tests running on CI appear to think we are constructing a HumanInfo object with 10 parameters, not the nine that are obviously there. Reformatting for clarity and possible fix. * possible bugfix * changing parameter passing to by name * moving back to positional arguments * manually constructing * confirm * clean up and remove comments * Added code + tests to address push to/pull from Qiita. * Fix test to use queried account_id, etc. Test uses an account_id, sample_id, and source_id that are tied to a fixed barcode id, but are dynamically generated on db init. * Fixed private/no private tests Sensitive questions have shortnames that begin with 'PM_'. Unfortunately, all of the 'PM_' questions were retired. Of concern is some of the '_v2' questions (RACE_v2, GENDER_v2, and more) are continuations of retired questions (RACE, GENDER, etc.) that also had 'PM_' variations (PM_ETHNCITIY, PM_GENDER). I've taken the liberty of changing GENDER_v2 to PM_GENDER_v2 as it seems an obvious choice and rebuilt the two tests around that. The others I will leave for discussion. * Updated migrate_responses() methods to use source ids Other minor issues from feedback addressed. * flake8 * (WIP) migrate_responses_by_barcode() updated. migrate_responses_by_barcode() now best fits the responses it returns according to the values closest to a particular timestamp. This timestamp can be defined separately in a helper method. Currently the helper looks for a timestamp from a survey with the same template as the one requested. If a survey doesn't exist, the method will return the creation timestamp of the source associated w/the barcode. * Private data tests now rely on EBI_REMOVE. * Tests fixed * flake8. adjust HumanInfo() for CI * Removed vestigial functionality * HumanInfo fix for CI * flake8 * Update microsetta_private_api/admin/tests/test_admin_api.py Co-authored-by: Daniel McDonald <d3mcdonald@eng.ucsd.edu> * Update microsetta_private_api/admin/tests/test_admin_api.py Co-authored-by: Daniel McDonald <d3mcdonald@eng.ucsd.edu> * Update microsetta_private_api/api/tests/test_api.py Co-authored-by: Daniel McDonald <d3mcdonald@eng.ucsd.edu> * Update microsetta_private_api/repo/tests/test_survey_template_repo.py Co-authored-by: Daniel McDonald <d3mcdonald@eng.ucsd.edu> * Adding constraint to ag_login_surveys Adding constraint to creation_time in ag_login_surveys. * Removed magic numbers * Simplified complex behavior in loop Simplified complex conditionals in a loop. Removed a commented-out test. * Resolve conflict with 0108.sql * Changed timestamp used in _get_timestamp() Changed _get_timestamp() to use date and time w/out timezone values from ag.ag_kit_barcodes. To ensure compatibility with other timestamps, the 'America/Los Angeles' timestamp was assigned to the casted timestamp. The value of the casted timestamp is not changed. * Removed survey_template_id from timesstamp query survey_template_id isn't needed for the revised get_timestamp() query, as the timestamps for all results are going to be the same, due to the way the query is written. Moreover, regardless of whether there are say multiple template 10s, or no template 10s, we would want to center our response results around this timestamp. * flake8 * Resolved post-merge test fail * Add temporal awareness to admin_repo.get_survey_metadata * Lint fixes * Addressing comments * Fix translation of best surveys back into answer:template map * Lint fix * Fix * Fix * Remove duplicate questions from metadata * Lint fix * Fix date casting Co-authored-by: Daniel McDonald <d3mcdonald@eng.ucsd.edu> Co-authored-by: Cassidy Symons <csymons@eng.ucsd.edu>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Missing unittests