Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Add Barcodes to Existing Kits #590
Changes from 37 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
There are no files selected for viewing
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.
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 conditionThere 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.
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
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.
How does a barcode differ from a sample?
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 this expected to be the same structure as described above?
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.
I don't think so, as line 355 makes sure
len(barcodes) == len(kit_ids)
rather thannumber_of_samples
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.
I think much of the validation work here is similar or the same as what's done in
create_kits
, should it be decomposed?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.
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 comment
The 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.
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.
I think this should be in a try/except given that
create_kits
is aboveThere 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.
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 comment
The 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
500