-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add Barcodes to Existing Kits #590
base: master
Are you sure you want to change the base?
Changes from all commits
719a8f2
5846301
16f2721
846f978
f9db8b4
5d458d1
ba0e51e
c8fef4b
3207886
4fdc890
1ceb9b9
43dc44f
87b1188
261d466
b8466d3
190b99f
461b98d
0cef118
1f9ac09
686b335
d1f52a5
82493c3
f5cfdd2
783abec
224e5fe
eacb78c
8dcf268
4c256bf
7dbe160
373a24e
07fb798
0d3764a
47986b3
4a52bdc
46346b3
34523d1
c30b63f
4331479
0e4a008
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,7 +60,7 @@ def search_kit_id(token_info, kit_id): | |
admin_repo = AdminRepo(t) | ||
diag = admin_repo.retrieve_diagnostics_by_kit_id(kit_id) | ||
if diag is None: | ||
return jsonify(code=404, message="Kit ID not found"), 404 | ||
return jsonify(code=404, message=f'Kit ID {kit_id} not found'), 404 | ||
return jsonify(diag), 200 | ||
|
||
|
||
|
@@ -267,13 +267,52 @@ def create_kits(body, token_info): | |
number_of_samples = body['number_of_samples'] | ||
kit_prefix = body.get('kit_id_prefix', None) | ||
project_ids = body['project_ids'] | ||
# NB: The barcodes variable is a list of lists, structured with the notion | ||
# of representing barcodes' constituent lists each representing one sample | ||
# slot in the set of kits being created. | ||
# len(barcodes) == number_of_samples && len(barcodes[x]) == number_of_kits | ||
# This allows the creation of a set of kits with a mixture of | ||
# system-generated and admin-provided barcodes. | ||
barcodes = body['barcodes'] | ||
|
||
with Transaction() as t: | ||
admin_repo = AdminRepo(t) | ||
|
||
# Lock the barcode table for the duration of checking the existience | ||
# of barcodes, then (hopefully) creating/inserting new ones | ||
t.lock_table("barcodes.barcode") | ||
|
||
# First, do some basic sanitation | ||
errors = [] | ||
for barcode_list in barcodes: | ||
wasade marked this conversation as resolved.
Show resolved
Hide resolved
|
||
x = 1 | ||
if len(barcode_list) != 0: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the description, it looks like |
||
# If barcodes were provided for a given slot, ensure the | ||
# quantity matches the number of kits | ||
if len(barcode_list) != number_of_kits: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following separation of concerns, the input data structure should be validated prior to its use. But I think deleting this, and using the suggested check in the above comment would do that unless I'm misunderstanding something about the structure |
||
errors.append( | ||
f'Incorrect number of barcodes given for Sample {x}' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does a barcode differ from a sample? |
||
) | ||
|
||
# And verify that the barcodes don't already exist | ||
for barcode in barcode_list: | ||
diag = admin_repo.check_exists_barcode(barcode) | ||
if diag is True: | ||
errors.append(f'Barcode {barcode} already exists') | ||
x += 1 | ||
|
||
if len(errors) > 0: | ||
error_str = "; ".join(errors) | ||
return jsonify(code=422, message=error_str), 422 | ||
|
||
try: | ||
kits = admin_repo.create_kits(number_of_kits, number_of_samples, | ||
kit_prefix, project_ids) | ||
kits = admin_repo.create_kits( | ||
number_of_kits, | ||
number_of_samples, | ||
kit_prefix, | ||
barcodes, | ||
project_ids | ||
) | ||
except KeyError: | ||
return jsonify(code=422, message="Unable to create kits"), 422 | ||
else: | ||
|
@@ -282,6 +321,60 @@ def create_kits(body, token_info): | |
return jsonify(kits), 201 | ||
|
||
|
||
def add_barcodes_to_kits(body, token_info): | ||
validate_admin_access(token_info) | ||
|
||
kit_ids = body['kit_ids'] | ||
barcodes = body['barcodes'] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this expected to be the same structure as described above? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, as line 355 makes sure |
||
generate_barcodes = body['generate_barcodes'] | ||
|
||
errors = [] | ||
|
||
with Transaction() as t: | ||
admin_repo = AdminRepo(t) | ||
|
||
# Lock the barcode table for the duration of checking the existience | ||
# of barcodes, then (hopefully) adding them to kits | ||
t.lock_table("barcodes.barcode") | ||
|
||
# First, make sure all of the Kit IDs are valid | ||
for kit_id in kit_ids: | ||
diag = admin_repo.check_exists_kit(kit_id) | ||
if diag is False: | ||
errors.append(f'Kit ID {kit_id} not found') | ||
|
||
if generate_barcodes is False: | ||
# Next, check that the Barcodes are unique if we're not generating | ||
# novel ones | ||
for barcode in barcodes: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think much of the validation work here is similar or the same as what's done in |
||
diag = admin_repo.check_exists_barcode(barcode) | ||
if diag is True: | ||
errors.append(f'Barcode {barcode} already exists') | ||
|
||
# And verify that the number of barcodes and kit IDs are equal | ||
if len(barcodes) != len(kit_ids): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test should be performed before the checks against the database under the fail fast principle There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this is an admin-facing tool with infrequent usage, I've opted to return a comprehensive set of errors so they can all be remedied at once, rather than simply returning the first error the API encounters. As such, the sequence of checks isn't important. |
||
errors.append("Unequal number of Kit IDs and Barcodes") | ||
else: | ||
if len(barcodes) > 0: | ||
errors.append( | ||
"Barcodes may not be both generated and provided" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The structure description though seems to suggest this mode is supported?
|
||
) | ||
|
||
cassidysymons marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# We found one or more issues with the supplied Kit IDs or Barcodes. | ||
# Return the error message to microsetta-admin. | ||
if len(errors) > 0: | ||
error_str = "; ".join(errors) | ||
return jsonify( | ||
code=422, | ||
message=error_str | ||
), 422 | ||
|
||
diag = admin_repo.add_barcodes_to_kits(kit_ids, barcodes) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be in a try/except given that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not following why this would be in a try/except block. It's a completely separate function/API path from creating kits, and all of the reasonable checks have been performed by the time we attempt to add barcodes to kits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The methods have criteria which would trigger a raise. It would be more useful to provide a error message to the admin than to trigger a |
||
t.commit() | ||
|
||
return jsonify(diag), 201 | ||
|
||
|
||
def get_account_events(account_id, token_info): | ||
validate_admin_access(token_info) | ||
|
||
|
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.
In English, the length of barcodes is the maximum number of barcodes which may be in a kit, and the length of an element of barcodes is the number of kits which have the number of barcodes equal to the index in barcodes? If this is the case, then should the following defensive assertions be added?
I apologize if I'm missing something obvious but I'm still having a hard time understanding this data structure, I think as it implicit rather than explicit. I wonder whether decomposing this endpoint would help -- is there a use case for the endpoint to support BOTH system and admin provided barcodes? If there is, why not a
list
ofdict
where each element is a kit description, e.g. something like:[{'number_of_system_generated_barcodes': X, 'admin_provided_barcodes': [...]}, ]
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.
Is it reasonable to interpret this structure as a 2D matrix, where the index position of a row is the barcode number (1st, 2nd, 3rd, etc) and the columns are the logical kits?