Y24-099 scRNA - tube rack banking sequencescape api #4416
Conversation
… endpoint, and added tube rack purpose to v2 api
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## y24-088-tuberacks-epic #4416 +/- ##
========================================================
Coverage 86.55% 86.55%
========================================================
Files 1394 1397 +3
Lines 29867 30003 +136
========================================================
+ Hits 25851 25969 +118
- Misses 4016 4034 +18 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…, was returning nil for request type
…types of labwares
There was a problem hiding this comment.
Did you and Stuart discuss your opinions on whether this is a good pattern for the API to follow?
If so could put a sentence or two under line 5 to explain the pattern.
| self.table_name = 'specific_tube_rack_creation_purposes' | ||
| belongs_to :specific_tube_rack_creation | ||
| belongs_to :tube_rack_purpose, class_name: 'Purpose' | ||
| end |
There was a problem hiding this comment.
Where's the link between rack and purpose?
Should it be instead that SpecificTubeRackCreation::ChildTubeRack has a relationship belongs_to :purpose, or just a string with the purpose name or something?
There was a problem hiding this comment.
Link between rack and purpose is in the TubeRack model / resource?
This endpoint can make multiple tube racks of different purposes remember.
I copied the pattern from SpecificTubeCreation.
Can discuss.
There was a problem hiding this comment.
Looking at this again, I think you are right, there is a problem here.
It should be just the one table, not two.
There was a problem hiding this comment.
I have tried to change it for one table with belongs_to on purpose added but it breaks all the unit tests. I haven't been able to fix it.
| else | ||
| redirect_existing_barcode(existing_barcode_record, new_tube_rack, tube_rack_barcode) | ||
| end | ||
| end |
There was a problem hiding this comment.
This (handle_tube_rack_barcode) feels like a core method that should probably be elsewhere - on the tube rack model, or purpose perhaps 🤔
There was a problem hiding this comment.
yeah I agree, too much code in this labware creator. can discuss.
There was a problem hiding this comment.
@KatyTaylor Create refactor story to create central tube rack creation code to be shared between this and sample manifest upload code.
| raise StandardError, error_message | ||
| end | ||
| Barcode.create!(labware: new_tube_rack, barcode: tube_rack_barcode, format: barcode_format) | ||
| end |
There was a problem hiding this comment.
I thought that this kind of thing was probably replicated in the sample manifest upload code, and then found this comment -
So looks like there might be 3 places that could do with a refactor!
| return if pm.save | ||
|
|
||
| raise StandardError, "New metadata for tube rack (key: #{metadata_key}, value: #{tube_rack_barcode}) did not save" | ||
| end |
There was a problem hiding this comment.
This also feels like a core method for tube rack creation, perhaps should be on the tube rack model.
| return if existing_tube_barcode_record.nil? | ||
|
|
||
| error_message = "The tube barcode '#{tube_barcode}' is already in use, cannot continue." | ||
| raise StandardError, error_message |
There was a problem hiding this comment.
Is this replicating behaviour that would happen already if you tried to insert a tube with a pre-existing barcode?
There was a problem hiding this comment.
Might be worth having a discussion about error handling for API endpoints in general, as I'm not very clear on what the best way to do all this is. This endpoint has to make multiple tube racks, each with multiple tubes. If anything goes wrong at any point I want the whole lot rolled back and a suitable human friendly error message passed back through the api response to be displayed to the user.
Does raising an explicit StandardError like this trigger the full rollback? And does that error message make it back to Limber and get displayed? Would I get the same result attempting an insert and that failing with a uniqueness error (given barcode is a different table than labwares / receptacles)? Does it still rollback everything and return a clear message to Limber?
There was a problem hiding this comment.
Shall we put it on our list to ask Stuart about?
| has_many :state_changes, readonly: true | ||
| has_one :custom_metadatum_collection, foreign_key_on: :related | ||
| has_many :ancestors, readonly: true, polymorphic: true | ||
| # NB. no child or descendent associations as tube racks can't have children (tubes have children). |
There was a problem hiding this comment.
I don't think it's true that tube racks can't have children?
There was a problem hiding this comment.
we discussed this and decided it was the tubes that have children:
- parent plate has child tube rack
- tube rack has racked tubes (not children)
- tubes have child plate
what would the child of a tube rack be?
There was a problem hiding this comment.
I think in the specific scRNA Core case that's true, but not necessarily in the general case.
In the scRNA Core case, the LRC Bank Seq/Spare tube rack doesn't have a direct child because the tubes are taken out, given to a different team for storage in a different labware, and then come back in different groupings.
If it was just a simple plate --> tube rack --> plate within a pipeline, then I could see all the parent-child relationships being from the tube rack - and I think that's what our data model / design is expecting.
# Conflicts: # db/schema.rb
…rather than pass from Limber
…sequencescape-api
andrewsparkes
deleted the
y24-099-tube-rack-banking-sequencescape-api
branch
Closes #sanger/limber#1696
Changes proposed in this pull request
Adds specific tube rack creation v2 api endpoint