-
Notifications
You must be signed in to change notification settings - Fork 8
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
Y24-099 plate split to racks #2028
Y24-099 plate split to racks #2028
Conversation
…99-plate-split-to-racks
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.
Haven't looked at the big one yet - the labware creator - but have left some comments on the smaller changes.
@@ -280,6 +282,7 @@ nav.robots-list { | |||
.work-completion-button, | |||
.create-plate-button, | |||
.create-tube-button, | |||
.create-tube_rack-button, |
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.
Seems like some of this stuff could just be generic, like .create-labware-button
or params[:limber_labware_id]
in the above file. Let's not add refactoring to the scope of the story though!
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.
Agree, assuming the buttons don't have to have different text or some other difference.
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.
Plus the suggested action Add button now looks like 'Add tube_rack' with an underscore which looks odd (works for tube and plate as they are single word classes).
app/helpers/page_helper.rb
Outdated
@@ -53,6 +53,7 @@ def jumbotron(jumbotron_id = nil, options = {}, &) | |||
# eg. state_badge('pending') | |||
# <span class="state-badge-pending">Pending</span> | |||
def state_badge(state) | |||
return if state.blank? |
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.
What happens if this returns nil
? The state badge is not shown?
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 was temporary to get past the fact that the tube rack didn't have a state. And yes it doesn't show the state badge on the page.
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.
Can be removed once state for TubeRacks is fixed by another story.
def compatible_tube_rack_purposes | ||
construct_buttons(purposes_of_type('tube_rack')) | ||
end | ||
|
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.
Feels like this wants to be refactored to:
def compatible_labware_purposes(labware_type)
construct_buttons(purposes_of_type(labware_type))
end
But again, maybe we can save up refactoring ideas for another story to try to draw a line under this story! Will keep commenting them if I notice and we can decide what to do.
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.
Yeah. I think this is building the 'other actions' grey buttons. That could do with some better ordering/structuring in any case.
:presenter_class: Presenters::SimpleTubePresenter | ||
:state_changer_class: StateChangers::AutomaticTubeStateChanger | ||
child_spare_tube_rack_purpose_name: TR LRC Bank Spare | ||
child_tube_rack_metadata_key: 'tube_rack_barcode' |
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.
We want this key to be the same EVERYWHERE, right? It's part of Abdullah's design doc?
So it doesn't feel quite right here against a specific purpose.
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 do I reference something like a TubeRack class constant from within a yml file?
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 it should be an app-wide configuration setting on the Sequencescape side.
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.
if it will ALWAYS be the same key then there is no need for Limber to know what it is or include it in the purpose config. instead of passing it through for each tube rack in the attributes, I can pull it out of a tuberack constants initializer file like I made in the other story on the SS side.
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.
Yes exactly
…he tuberack, used for fetching active requests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## y24-088-tuberacks-epic #2028 +/- ##
==========================================================
+ Coverage 78.08% 78.11% +0.02%
==========================================================
Files 478 480 +2
Lines 18069 18155 +86
Branches 262 262
==========================================================
+ Hits 14110 14182 +72
- Misses 3957 3971 +14
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -71,6 +71,9 @@ class PlateSplitToTubeRacks < Base | |||
|
|||
PARENT_PLATE_INCLUDES = | |||
'wells.aliquots,wells.aliquots.sample,wells.downstream_tubes,wells.downstream_tubes.custom_metadatum_collection' | |||
SEQ_TUBE_RACK_NAME = 'SEQ Tube Rack' | |||
SPR_TUBE_RACK_NAME = 'SPR Tube Rack' | |||
DEFAULT_TUBE_RACK_SIZE = '96' |
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.
Where is this used?
parent | ||
end | ||
|
||
# Display the children tab in the plate view so we see the child tubes listed. | ||
# We will want to see the list of tubes in the rack | ||
# TODO: as these are racked_tubes and not child tubes, does the tube rack presenter have a relatives tab? |
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 the 'anchor' the tab it lands on, on the destination plate? If so, we're landing on the parent plate (not a tube rack), so the relatives tab is normal and should (hopefully) show us a list of child tube racks.
app/models/state_changers.rb
Outdated
return nil unless FILTER_FAILS_ON.include?(target_state) | ||
|
||
# determine list of tubes requiring the state change | ||
tubes_locations_filtered = labware.tubes.reject { |t| t.state == 'failed' }.map(&:racked_tube.coordinate) |
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.
Discussed whether we should be filtering out 'cancelled' tubes here too? Have this - https://github.com/sanger/limber/pull/2034/files
Also need to test failing and cancelling at the tube rack level.
Think about whether to create a new story for any of these things.
Closes #1696
Changes proposed in this pull request
Adds specific tube rack and tube creation for scRNA