-
Notifications
You must be signed in to change notification settings - Fork 34
Y26-680 Automated buffer addition for cherrypicking #5565
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
Open
andrewsparkes
wants to merge
43
commits into
develop
Choose a base branch
from
Y26-680-automated-buffer-addition
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+4,299
−642
Open
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
750c1f2
Add changes for generating screenshots
yoldas 918d412
Update worksheet screenshot based on feedback
yoldas 60301fc
Add poly_metadata association to Batch for storing buffer_volume_for_…
yoldas e9a372d
Add buffer volume for empty wells option to params for pass through
yoldas 4b7b7e7
Remove pry
yoldas 6a0d6db
Add HasPolyMetadata concent to set the option on batch
yoldas c5244aa
Call set_poly_metadata on the batch in cherrypick handler
yoldas f2d190d
Update worksheet with buffer_volume_for_empty_wells option from batch…
yoldas 0247d43
Add buffer steps for empty wells in tecan behaviour
yoldas 03927d5
Add buffer steps for empty wells in TecanV3 driver file generator
yoldas 13e85a3
Check if the destination well is empty, in case of partial plate
yoldas e755668
Add guard for generator test without batch
yoldas 521a3dd
Add the word volume to the note and legend in worksheet
yoldas 0d0787c
Add Automatic buffer addition for empty wells required? checkbox
yoldas 7349453
Add script to toggle buffer_volume_for_empty_wells textbox based on a…
yoldas df31627
Add code documentation about new data_object generation
yoldas 416ab29
Add comment to explain why src_well is checked
yoldas 85a6b2a
Add batch behaviour to handle poly_metadata for cherrypick options
yoldas 6177332
Add task helper to set form params in batch polymetadata
yoldas e106709
Add PolyMetadataBehaviour to batch
yoldas 0dad66a
Add option handling in pick helpers
yoldas 0655cfe
Add pass through for automatic_buffer_addition
yoldas e70a38e
Tidy up the worksheet ERB
yoldas 4ca4650
Use ERB comments instead of HTML comments
yoldas 4f00987
Revert redundant change
yoldas b7183cd
Remove extra closing ERB tag
yoldas 213445a
Handle checkbox value as 1 or on; passing through hidden form fields
yoldas c66d6ef
Fix broken TecanV3 tests
yoldas 733ebc9
Fix broken Tecan generator tests
yoldas a37e275
Merge branch 'develop' into y26-012-automating-buffer-addition
andrewsparkes 5656a8b
Merge branch 'y26-012-automating-buffer-addition' into Y26-680-automa…
andrewsparkes e897666
Merge branch 'develop' into Y26-680-automated-buffer-addition
andrewsparkes 2226ad5
added jest and tests for cherrypick_strategies
andrewsparkes fb35abe
added tests for buffer volume for empty wells option module
andrewsparkes 70d3e84
added tests for the check in buffers method for src_well
andrewsparkes 9c6fa34
added tests for tecan default changes
andrewsparkes c656849
added tests for buffer addition, fixed plate barcodes in all test fil…
andrewsparkes f1c20a6
fix to pass eslint
andrewsparkes 9650b30
linted
andrewsparkes adbaa8b
added tests for has poly metadata
andrewsparkes 7a9c636
Merge branch 'develop' into Y26-680-automated-buffer-addition
andrewsparkes fe0d527
Merge branch 'develop' into Y26-680-automated-buffer-addition
andrewsparkes 294edd1
changes after code review
andrewsparkes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| // cherrypick_strategies.test.js | ||
| // Tests for cherrypick_strategies.js buffer input toggle logic | ||
|
|
||
| /* global describe, it, expect, beforeEach, jest */ | ||
| /* @jest-environment jsdom */ | ||
|
|
||
| describe("Buffer input toggle", () => { | ||
| let bufferInput, autoBufferCheckbox; | ||
|
|
||
| beforeEach(() => { | ||
| document.body.innerHTML = ` | ||
| <input id="buffer_volume_for_empty_wells" type="number" /> | ||
| <input id="automatic_buffer_addition" type="checkbox" /> | ||
| `; | ||
| // Re-require the script to attach event listeners | ||
| jest.resetModules(); | ||
| require("./cherrypick_strategies.js"); | ||
| bufferInput = document.getElementById("buffer_volume_for_empty_wells"); | ||
| autoBufferCheckbox = document.getElementById("automatic_buffer_addition"); | ||
| }); | ||
|
|
||
| it("disables buffer input when checkbox is unchecked", () => { | ||
| autoBufferCheckbox.checked = false; | ||
| document.dispatchEvent(new Event("DOMContentLoaded")); | ||
| expect(bufferInput.disabled).toBe(true); | ||
| }); | ||
|
|
||
| it("enables buffer input when checkbox is checked", () => { | ||
| autoBufferCheckbox.checked = true; | ||
| document.dispatchEvent(new Event("DOMContentLoaded")); | ||
| expect(bufferInput.disabled).toBe(false); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # frozen_string_literal: true | ||
| module Batch::PolyMetadataBehaviour | ||
| # Returns whether the Cherrypick automatic_buffer_addition option is enabled | ||
| # @return [Boolean] whether the automatic_buffer_addition option is enabled | ||
| def automatic_buffer_addition? | ||
| # tests 1 and on to cover both visible and hidden option in the forms of successive pages | ||
| %w[1 on].include?(get_poly_metadata(:automatic_buffer_addition)) | ||
yoldas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| end | ||
|
|
||
| # Returns the Cherrypick buffer_volume_for_empty_wells option value if | ||
| # automatic_buffer_addition is enabled, nil otherwise. | ||
| # @return [Float, nil] the buffer_volume_for_empty_wells value | ||
| def buffer_volume_for_empty_wells | ||
| get_poly_metadata(:buffer_volume_for_empty_wells).to_f if automatic_buffer_addition? | ||
| end | ||
| end | ||
25 changes: 25 additions & 0 deletions
25
app/models/cherrypick/task/buffer_volume_for_empty_wells_option.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Cherrypick::Task::BufferVolumeForEmptyWellsOption | ||
| def create_buffer_volume_for_empty_wells_option(params) | ||
| return unless @batch | ||
|
|
||
| key = :automatic_buffer_addition | ||
| # The checkbox value is either "1", or nil if not checked. | ||
| @batch.set_poly_metadata(key, params[key]) | ||
|
|
||
| return unless %w[1 on].include?(params[key]) | ||
|
|
||
| # If automatic buffer addition for empty wells is required, check and | ||
| # set the required buffer volume in the batch polymetadata. | ||
| key = :buffer_volume_for_empty_wells | ||
|
|
||
| # method valid_float_param? is defined in Cherrypick::Task::PickHelpers | ||
| unless valid_float_param?(params[key]) | ||
| raise Cherrypick::VolumeError, | ||
| "Invalid buffer volume for empty wells: #{params[key]}" | ||
| end | ||
|
|
||
| @batch.set_poly_metadata(key, params[key]) | ||
| end | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||
| # frozen_string_literal: true | ||||||
|
|
||||||
| module HasPolyMetadata | ||||||
| extend ActiveSupport::Concern | ||||||
|
|
||||||
| included do | ||||||
| has_many :poly_metadata, as: :metadatable, dependent: :destroy, inverse_of: :metadatable | ||||||
| end | ||||||
|
|
||||||
| # Sets a PolyMetaDatum for the given key and value. | ||||||
| # If value is present, it will create or update the PolyMetaDatum with the | ||||||
| # given key and value, otherwise it will destroy the PolyMetaDatum with the | ||||||
| # given key if that exists. | ||||||
yoldas marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| # NB: this is because PolyMetaDatum validations prevent key duplication and blank values, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Thanks for the explanation. I still had a minor confusion, hence the suggestion. |
||||||
| # although there are no such DB constraints. | ||||||
| # @param key [String] The key of the PolyMetaDatum to set. | ||||||
| # @param value [String] The value of the PolyMetaDatum to set. If nil or empty, the PolyMetaDatum will be destroyed. | ||||||
| # @return [void] | ||||||
| def set_poly_metadata(key, value) | ||||||
| record = poly_metadata.find_by(key:) | ||||||
| if value.present? | ||||||
| if record | ||||||
| record.update!(value:) | ||||||
| else | ||||||
| poly_metadata.create!(key:, value:) | ||||||
| end | ||||||
| else | ||||||
| record&.destroy! | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| # Returns the value of the PolyMetaDatum with the given key. | ||||||
| # @param key [String] The key of the PolyMetaDatum to retrieve. | ||||||
| # @return [String, nil] The value of the PolyMetaDatum, or nil if it does not exist. | ||||||
| def get_poly_metadata(key) | ||||||
| poly_metadata.find_by(key:)&.value | ||||||
| end | ||||||
| end | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,6 +96,8 @@ def self.plate_length(plate_size) | |
| PLATE_DIMENSIONS[plate_size].last | ||
| end | ||
|
|
||
| # well number counting by columns, length is the number of rows in the plate | ||
| # e.g. B5 sends this 34 and 8 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, I remember adding a similar comment elsewhere 😅 |
||
| def self.vertical_position_to_description(well_position, length) | ||
| desc_letter = (((well_position - 1) % length) + 65).chr | ||
| desc_number = ((well_position - 1) / length) + 1 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -365,4 +365,8 @@ def name | |
| def library_name | ||
| nil | ||
| end | ||
|
|
||
| def empty? | ||
| aliquots.blank? | ||
| end | ||
| end | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
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.
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.
note: the test may be OK because after DOMContentLoaded event, it registers a change event and then calls
toggleBufferInputonce to do an initial toggle. However, the toggle behaviour could be tested on the registered change event on the checkbox elementautomatic_buffer_additionby a dispatch as inautoBufferCheckbox.dispatchEvent(new Event("change"));note: this PR adds JS testing (package.json) although there is no
yarn testin CI workflows yet.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.
tried changing:
to:
but test fails. expect is false. I'm missing something.
Uh oh!
There was an error while loading. Please reload this 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.
I meant the following, which can be added as well.