create barcodes for exisiting kits #99
Conversation
|
Added part 2: The user can now provide their own barcode(s) if needed when they're creating a kit. This can be done either by providing the barcode(s) in the field(s) OR they can upload a list of barcodes via a csv file. |
There was a problem hiding this comment.
I haven't thoroughly reviewed this, but noted a couple of things that jumped out at me. Given the fact that there's still some work to do on the Private API portion, and potential for change here, I'm going to defer on further review until those changes have been addressed.
| if 'kit_ids' in request.form: | ||
| kit_ids = request.form['kit_ids'] | ||
|
|
||
| if 'user_barcode' in request.form: | ||
| user_barcode = request.form['user_barcode'] | ||
| barcodes.append(user_barcode) | ||
| else: | ||
| user_barcode = None | ||
|
|
||
| generate_barcode_single = \ | ||
| request.form.get('generate_barcode_single') | ||
| generate_barcodes_multiple = \ | ||
| request.form.get('generate_barcodes_multiple') | ||
|
|
||
| if 'kit_ids' in request.files: | ||
| kit_ids_file = request.files['kit_ids'] | ||
| kit_ids = _read_csv_file(kit_ids_file) | ||
|
|
||
| if 'barcodes_file' in request.files: | ||
| barcodes_file = request.files['barcodes_file'] | ||
| barcodes = _read_csv_file(barcodes_file) | ||
|
|
||
| if generate_barcode_single or user_barcode: |
There was a problem hiding this comment.
I feels like there's a lot of redundancy here. For example, we know that if the user selects Add Barcode to Single Kit, kit_ids should be present as a string and either user_barcode or generate_barcode_single should be present. Is it necessary to check for each of those individually?
| if not user_barcode: | ||
| payload = { | ||
| 'action': 'create', | ||
| 'kit_ids': kit_ids, | ||
| 'generate_barcode_single': generate_barcode_single | ||
| } | ||
| barcodes = _handle_add_barcodes_api_request(payload) | ||
| if isinstance(barcodes, tuple): | ||
| return barcodes | ||
|
|
||
| payload = { | ||
| "action": "insert", | ||
| "barcodes": barcodes, | ||
| "kit_ids": kit_ids | ||
| } | ||
| result = _handle_add_barcodes_api_request(payload) | ||
| if isinstance(result, tuple): | ||
| return result | ||
|
|
||
| return render_template('add_barcode_to_kit.html', | ||
| barcodes=barcodes, | ||
| kit_id=kit_ids, | ||
| **build_login_variables()) | ||
|
|
||
| if generate_barcodes_multiple: | ||
| payload = { | ||
| 'action': 'create', | ||
| 'kit_ids': kit_ids, | ||
| 'generate_barcodes_multiple': generate_barcodes_multiple | ||
| } | ||
| barcodes = _handle_add_barcodes_api_request(payload) | ||
| if isinstance(barcodes, tuple): | ||
| return barcodes | ||
|
|
||
| if len(kit_ids) != len(barcodes): | ||
| error_message = f'The number of kit IDs ({len(kit_ids)}) '\ | ||
| f'does not match the number of barcodes '\ | ||
| f'({len(barcodes)}).' | ||
| return render_template('add_barcode_to_kit.html', | ||
| error_message=error_message, | ||
| **build_login_variables()), 400 | ||
|
|
||
| payload = { | ||
| "action": "insert", | ||
| "barcodes": barcodes, | ||
| "kit_ids": kit_ids | ||
| } | ||
| result = _handle_add_barcodes_api_request(payload) | ||
| if isinstance(result, tuple): | ||
| return result | ||
|
|
||
| return _save_and_send_csv(kit_ids, barcodes) |
There was a problem hiding this comment.
This code would be considerably more readable if it was an if/elif block, rather than cascading through conditional return statements that require cross-referencing which variable is being used.
| total_provided_samples += len(new_barcodes) | ||
|
|
||
| # Calculate remaining samples to be generated | ||
| total_needed_samples = num_kits * num_samples | ||
| remaining_samples_to_generate = total_needed_samples - \ | ||
| total_provided_samples | ||
|
|
||
| if remaining_samples_to_generate < 0: | ||
| error_message = (f'Too many samples provided ' | ||
| f'({total_provided_samples}) ' | ||
| f'for the specified number of kits and samples ' | ||
| f'({total_needed_samples}).') | ||
| return render_template('create_kits.html', | ||
| error_message=error_message, | ||
| projects=projects, | ||
| **build_login_variables()) |
There was a problem hiding this comment.
Given the fact that the code should be enforcing the number of provided samples on a per-slot basis, does this block still serve a purpose?
Taking from task description, this PR covers part 1:
Add a barcode to an existing kit (new feature). In this scenario, there already exists a kit in the database with one or more barcodes. The user will log into microsetta-admin and add barcodes to one or more kits, and it needs to provide the following options:
Add barcodes to a single kit. The user would enter a Kit ID into a text field, then have the option to either enter a barcode to add to the kit OR dynamically generate a new barcode to add to the kit (using the existing barcode generation function). If it's a generated barcode, the output should inform the user of the newly created barcode. Otherwise, the output should be a success/failure message.
Add barcodes to several kits. The user would upload a CSV file with Kit IDs in the first column. They would then have the option to either add a dynamically generated barcode to each kit OR put a barcode in the second column of the CSV file. In either case, the tool should return a CSV file with the Kit IDs and the newly added barcodes.
API PR can be found here.