Skip to content
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

create barcodes for existing kits #567

Closed
wants to merge 33 commits into from

Conversation

ayobi
Copy link
Contributor

@ayobi ayobi commented Mar 8, 2024

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.

@ayobi ayobi marked this pull request as ready for review March 8, 2024 21:34
@ayobi
Copy link
Contributor Author

ayobi commented Mar 26, 2024

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.

Copy link
Collaborator

@cassidysymons cassidysymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the UI changes in microsetta-admin combined with collapsing the two API paths into one will introduce significant changes, so let's knock those out, then I'll do another round of code review.

microsetta_private_api/api/microsetta_private_api.yaml Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
@cassidysymons
Copy link
Collaborator

@ayobi Can you please take a look at the conflict on this PR? I think you'll need to merge biocore:master into your branch to pick up new changes, and then possibly re-adjust the conflicting function. Thanks.

microsetta_private_api/api/microsetta_private_api.yaml Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cassidysymons cassidysymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know if you have any questions about isolated issues and/or this task in broad strokes. Each round of review has been a combination of steps forward and backwards and I'm concerned that it's not moving towards a polished and accurate addition to the codebase.

microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
@ayobi ayobi marked this pull request as draft August 7, 2024 17:21
@ayobi ayobi marked this pull request as ready for review August 15, 2024 04:44
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cassidysymons cassidysymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please let me know if you have any questions about what I've flagged here.

microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Show resolved Hide resolved
microsetta_private_api/admin/admin_impl.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
microsetta_private_api/repo/admin_repo.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@cassidysymons cassidysymons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Along with the noted comment, there are three issues identified through user testing:

  1. When I tried to Create a Kit using barcodes that already existed, I received an error of "Error message not found in the results."
  2. When I click Add Barcode to Kit in the menu, click the Add Barcode to Single Kit button, enter a valid Kit ID, and choose Generate barcode, the output displays a barcode in red, but that barcode does not exist in the database.
  3. When I click Add Barcode to Kit in the menu, click the Add Barcode to Multiple Kits button, upload a list of valid Kit IDs, and choose Generate barcodes, the output displays a single barcode in red, which does not exist in the database.

@@ -83,6 +83,7 @@ def per_sample(project, barcodes, strip_sampleid):
ffq_complete, ffq_taken, _ = vs_repo.get_ffq_status_by_sample(
sample.id
)
print("ffq complete", ffq_complete, "ffq taken", ffq_taken)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
print("ffq complete", ffq_complete, "ffq taken", ffq_taken)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants