Skin Scoring App #600
Skin Scoring App #600
Conversation
Skin scoring app - Working Branch
| with self._transaction.cursor() as cur: | ||
| cur.execute( | ||
| """ | ||
| SELECT ssac.app_username, ssac.app_password |
| @@ -261,6 +261,9 @@ def get_samples_by_source(self, account_id, source_id, | |||
| sample.kit_id = self._get_supplied_kit_id_by_sample( | |||
| sample.barcode | |||
| ) | |||
| sample.project_id = self._get_project_ids_by_sample( | |||
There was a problem hiding this comment.
I am really uncomfortable with this. There is already a sample_projects property on the Sample object, which is filled during _create_sample_obj; I realize this is not exactly what you want, because it is project names rather than project ids, but I do not think that introducing an entirely parallel but separate list for holding sample projects info, and creating a sample-level property for it that is defined ONLY on samples created in get_samples_by_source and NOT for samples created in, e.g., get_sample or _get_sample_by_id is a good direction. AFAICT this new property and the new function that fill it exist just so that get_skin_scoring_app_credentials_if_exists can check whether the sbi cohort project is associated with a sample. Couldn't it just check in the existing sample_projects list by name instead of by id, as a bunch of code does in other places (see example)?
microsetta-private-api/microsetta_private_api/api/_survey.py
Lines 157 to 158 in 5287a90
There was a problem hiding this comment.
Project names can be edited in Flight, which introduces a level of fuzziness for this purpose that I think is far from ideal. Especially given the fact that people who have access to Flight would largely be unaware of the consequences in editing a project name (I realize there's a separate discussion to be had about whether people should be able to edit project names in Flight given the way they're used in the code base, but that's out of scope).
I realize that there is a precedent for that utilization, but it doesn't feel like something that should be perpetuated since an ID-based link is not prone to the same issues of human intervention (whether accidental or intentional). If you don't agree, I can change it, but it was an intentional choice to not continue down the existing path. If you agree that there's merit to pursuing an ID-based solution, I'm open to streamlining the way I'm implementing it.
There was a problem hiding this comment.
@cassidysymons I do agree that having users able to change the project name in flight is A Bad Thing and an opening for errors. Have there been particular instances in which this happened and messed things up?
I also agree that it is much worse to identify things by names than by ids. However, this code seems to be adding a new public project_id property on Sample that is parallel to but uncoupled from sample_projects, is only defined for some paths to Sample object creation, and isn't integrated with Sample.to_api. I worry this is opening up at least as many possible error sources as the mutable names in Flight.
If we are going to hold project ids as well as project names in Sample, my theoretical preference would be to refactor sample_projects so it held tuples of project and name, or a project object of some kind. I acknowledge, though, that we almost certainly don't have the time to do that refactor since it would touch existing uses of sample_projects and require modifying to_api, etc.
I propose a compromise that is less work. We could add _project_ids as a private property of the Sample object (changing its __init__ and from_db but not to_api because, well, private :) We could then modify SampleRepo._create_sample_obj to populate _project_ids (_create_sample_obj is already calling _retrieve_projects, which, for minimum new coding effort, could be modified with an optional param to return id instead of barcode, and then could be called twice, once to get names and once to get ids--clunky but easy). This would take care of having this info available to samples created by all the canonical sample creation methods. Some teeny .get_project_ids() method or something could be added to Sample to make the contents readable for the survey_template_repo code using the Sample here within microsetta_private_api (I mean, I know there's no such thing as really private in python so we could just reach in and read _project_ids directly, but that would be gauche :D ). Thoughts?
|
|
||
| project_ids = [row[0] for row in rows] | ||
| return project_ids | ||
|
|
There was a problem hiding this comment.
See above comment
|
@AmandaBirmingham This is ready for re-review, as is the corresponding PR on microsetta-interface. Thanks! |
This PR supports the addition of a skin-scoring app as an external survey for members of the Skin Biome Initiative cohort. There are a few key pieces:
The app vendor (who is linked to the cohort sponsor) provided us with a file of username/password pairings that act as credentials to access the app. We ingest these credentials using the migration support mechanism, then maintain a registry of which credentials are allocated to which source.
Given that the credentials are pre-defined and finite, we need to determine whether to display the external survey based on whether the source has a sample associated with SBI and if unallocated credentials are available. On the frontend, we also handle the possibility that credentials are available when the user loads the page, but not available when they actually decide to access the app.
The decision to mark the survey taken when credentials are allocated, rather than when the link is clicked, is a conscious choice. We have no visibility into what happens after they receive credentials and open the survey, nor are we responsible for whether they actually utilize the app.
Corresponding microsetta-interface PR: biocore/microsetta-interface#339