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.
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
Skin Scoring App #600
Skin Scoring App #600
Changes from 14 commits
30fdd4a
ff86b4a
c54d86a
2791b54
a40b376
fea91e0
19473d2
7698a49
885f4e6
a992cce
61668e8
908db7f
5f41a10
85c6ec8
2c97fd3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 am really uncomfortable with this. There is already a
sample_projects
property on theSample
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 inget_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 thatget_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 existingsample_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 onSample
that is parallel to but uncoupled fromsample_projects
, is only defined for some paths toSample
object creation, and isn't integrated withSample.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 refactorsample_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 ofsample_projects
and require modifyingto_api
, etc.I propose a compromise that is less work. We could add
_project_ids
as a private property of theSample
object (changing its__init__
andfrom_db
but notto_api
because, well, private :) We could then modifySampleRepo._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 toSample
to make the contents readable for thesurvey_template_repo
code using theSample
here withinmicrosetta_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?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.
See above comment