-
Notifications
You must be signed in to change notification settings - Fork 37
Genotyping data derived from genotyping plate (display, download, delete) #5413
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
base: master
Are you sure you want to change the base?
Conversation
|
$where_clause = "nd_experiment_protocol.nd_protocol_id = $genotyping_protocol_id"; | ||
} | ||
|
||
my $q = "SELECT nd_experiment_genotype.nd_experiment_id, nd_experiment_genotype.genotype_id, nd_experiment_protocol.nd_protocol_id |
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.
What happens if you call the delete_genotype_data function without any of ids...may be check at least one of them is passed to the delete object before making the query to avoid runtime error...
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.
Fixed
lib/CXGN/Genotype/Delete.pm
Outdated
|
||
if ($genotyping_plate_id) { | ||
my @plate_list = (); | ||
@plate_list = ($genotyping_plate_id); |
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.
combine line 72 and 73?
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.
Fixed
lib/CXGN/Genotype/Delete.pm
Outdated
} elsif ($genotyping_project_id) { | ||
$where_clause = "nd_experiment_project.project_id = $genotyping_project_id"; | ||
} elsif ($genotyping_protocol_id) { | ||
$where_clause = "nd_experiment_protocol.nd_protocol_id = $genotyping_protocol_id"; |
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.
We should try to use only placeholders in sql statements. Could you replace the embedded variables with placeholders (?) ?
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.
@lukasmueller I have been using placeholders in all cases, except this type of statements "join ("," , @$sample_list)", because it doesn't work with the placeholder.
@isaak Thanks Isaak. I fixed both requests. |
Checklist
/t/data/fixture/patches/
./docs
has been updated./js
to/js/source/legacy
.