Add Barcodes to Existing Kits #590
Conversation
…e-api into creates_barcodes_for_existing_kits
Create Working Branch
| 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.
This test should be performed before the checks against the database under the fail fast principle
There was a problem hiding this comment.
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.
| message=error_str | ||
| ), 422 | ||
|
|
||
| diag = admin_repo.add_barcodes_to_kits(kit_ids, barcodes) |
There was a problem hiding this comment.
I think this should be in a try/except given that create_kits is above
There was a problem hiding this comment.
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.
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 500
|
Lint is failing as GitHub is changing (has changed) what "ubuntu-latest" means for workflow and python 3.7 isn't available in the new version (source: actions/runner-images#10636). I'll revisit updating our workflow files to use Ubuntu 22.04 on Monday. Not a long-term fix as support for that will expire in two years, I'll revisit the longer term implications in a couple of months. |
Merge Master into Working Branch
|
Workflow issue is resolved |
There was a problem hiding this comment.
I'm not sure if this is ready to re-review or not, but I cannot get to it later this week and next week carries a lot of uncertainty. Added some additional comments, but was unable to review the unit tests. The input structure on creation of kits feels surprising still even with documentation, but I may just be looking at this from a different angle -- I'm not sure if @AmandaBirmingham has time for a second opinion
| # 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. |
There was a problem hiding this comment.
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?
if len(barcodes) != number_of_samples:
return jsonify(code=422, message="Unexpected data organization"), 422
for row in barcodes:
if len(row) != number_of_kits:
return jsonify(code=422, message="Unexpected data organization"), 422I 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 of dict 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.
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?
| if len(barcode_list) != 0: | ||
| # 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.
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
| # quantity matches the number of kits | ||
| if len(barcode_list) != number_of_kits: | ||
| errors.append( | ||
| f'Incorrect number of barcodes given for Sample {x}' |
There was a problem hiding this comment.
How does a barcode differ from a sample?
|
|
||
| # Iterate through sample slots and use provided barcodes | ||
| for barcode_list in barcodes: | ||
| if len(barcode_list) > 0: |
There was a problem hiding this comment.
This is always True though from the description of the structure since len(barcodes[x]) == number_of_kits?
| errors = [] | ||
| for barcode_list in barcodes: | ||
| x = 1 | ||
| if len(barcode_list) != 0: |
There was a problem hiding this comment.
From the description, it looks like len(barcode_list) == 0 should raise. From the repo code, I'm less clear as that is a valid condition
| Returns | ||
| ------- | ||
| list of tuples | ||
| The pairings of newly added kit IDs/barcodes |
There was a problem hiding this comment.
These barcodes are not associated with box IDs. It looks like it may be necessary to insert the kit UUID (see
)There was a problem hiding this comment.
Will samples added to an existing kit be shipped via daklapack?
| for prj_id in project_ids: | ||
| barcode_projects.append((barcode, prj_id)) | ||
| if isinstance(new_barcodes, list) and \ | ||
| all(isinstance(item, list) for item in new_barcodes): |
| for barcodes in new_barcodes: | ||
| for barcode in barcodes: | ||
| for prj_id in project_ids: | ||
| barcode_projects.append((barcode, prj_id)) | ||
| else: | ||
| for barcode in new_barcodes: | ||
| for prj_id in project_ids: | ||
| barcode_projects.append((barcode, prj_id)) |
There was a problem hiding this comment.
| for barcodes in new_barcodes: | |
| for barcode in barcodes: | |
| for prj_id in project_ids: | |
| barcode_projects.append((barcode, prj_id)) | |
| else: | |
| for barcode in new_barcodes: | |
| for prj_id in project_ids: | |
| barcode_projects.append((barcode, prj_id)) | |
| flat_barcodes = [bc for barcodes in new_barcodes for bc in barcodes] | |
| new_barcodes = flat_barcodes | |
| for barcode in new_barcodes: | |
| for prj_id in project_ids: | |
| barcode_projects.append((barcode, prj_id)) |
| address = None if address_dict is None else json.dumps(address_dict) | ||
| kit_details = [{KIT_BOX_ID_KEY: box_id, | ||
| KIT_ADDRESS_KEY: address, | ||
| KIT_OUTBOUND_KEY: outbound_fedex_code, | ||
| KIT_INBOUND_KEY: inbound_fedex_code}] | ||
| kit_name_and_barcode_tuples_list = \ | ||
| [(kit_name, x) for x in barcodes_list] | ||
| kit_names = [kit_name] |
There was a problem hiding this comment.
Was this relocation consequential?
| for barcode_list in barcodes: | ||
| if len(barcode_list) > 0: | ||
| # Admin provided barcodes for this sample slot, use them | ||
| kit_barcode_tuples = list(zip(kit_names, barcode_list)) |
There was a problem hiding this comment.
...but create_kits cannot assume it is only ever called from the API endpoint where the validation is presently performed, therefore we do not have assurance that len(kit_names) == len(barcode_list) here. Can that check be added?
This PR addresses two related needs:
The corresponding microsetta-admin PR is biocore/microsetta-admin#110