Y26-125 dual index tags update in mlwh#5745
Conversation
…uses to prevent NoMethod errors, and updated specialised fields to pass in row to enab
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5745 +/- ##
========================================
Coverage 87.30% 87.31%
========================================
Files 1476 1476
Lines 33517 33510 -7
Branches 3549 3553 +4
========================================
- Hits 29263 29258 -5
+ Misses 4233 4231 -2
Partials 21 21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
StephenHulme
left a comment
StephenHulme
left a comment
There was a problem hiding this comment.
Might benefit from someone more familiar with aliquots, but looks good to me.
| # NB. the asset here is a well, and the well_has_single_aliquot? validation ensures there is only one aliquot | ||
| # NB. we are fetching via row so this is the same aliquot instance so saved_changes? works to detect | ||
| # substitutions | ||
| row.aliquots.first.assign_attributes(tag:, tag2:) |
There was a problem hiding this comment.
| row.aliquots.first.assign_attributes(tag:, tag2:) | |
| aliquots.first.assign_attributes(tag:, tag2:) |
I think we don't need the row passed through. If you look at the code for the other types of library manifest, they reference aliquots directly (which is delegated to asset on the base class) - so should work to do the same here.
Equivalent classes for other manifests:
app/sequencescape_excel/sequencescape_excel/specialised_field/i7.rb>app/sample_manifest_excel/sample_manifest_excel/tags/aliquot_updater.rbapp/sequencescape_excel/sequencescape_excel/specialised_field/tag_index.rb
|
|
||
| def tag_group_id | ||
| @tag_group_id ||= ::TagGroup.find_by(id: dual_index_tag_set.tag_group_id, visible: true).id | ||
| # defensive guard to avoid NoMethodError being thrown when dual_index_tag_set is nil, caught by validation |
There was a problem hiding this comment.
Think the comment states the obvious 😃
Great to have fixed the bug at the same time as this story 🙌
KatyTaylor
left a comment
KatyTaylor
left a comment
There was a problem hiding this comment.
Looks great, except I think you can take the row-related changes out, to simplify even more.
3 participants
Closes #5677
Changes proposed in this pull request