Y26 031 labware location report by retention instructions#5637
Y26 031 labware location report by retention instructions#5637
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5637 +/- ##
===========================================
+ Coverage 87.22% 87.25% +0.02%
===========================================
Files 1461 1461
Lines 33022 33032 +10
Branches 3475 3476 +1
===========================================
+ Hits 28805 28822 +17
+ Misses 4196 4189 -7
Partials 21 21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds retention-instruction support to Location Reports so users can filter returned labware by retention instruction, while persisting the selected instructions on the report record.
Changes:
- Add
retention_instructions(serialized array) toLocationReportand persist it via a new DB column. - Extend the selection form + controller strong params to allow multi-select retention instructions.
- Filter selected labware by retention instruction and add an RSpec scenario for it.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/models/location_report_spec.rb | Adds coverage for filtering by retention instruction. |
| db/schema.rb | Updates schema with the new location_reports.retention_instructions column. |
| db/migrate/20260325134057_add_retention_instructions_to_lcation_reports.rb | Introduces the migration adding the new column. |
| app/views/location_reports/_location_labware_selection_form.html.erb | Adds a multi-select retention instruction filter UI. |
| app/models/location_report/location_report_form.rb | Passes retention_instructions from the form into the model. |
| app/models/location_report.rb | Serializes retention_instructions and filters labware results accordingly. |
| app/controllers/location_reports_controller.rb | Permits retention_instructions as an array param. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def validate_result_size(params) | ||
| count = Labware.search_for_count_of_labware(params) | ||
| if count > configatron.fetch(:location_reports_fetch_count_max, 25000) | ||
| errors.add(:base, I18n.t('location_reports.errors.too_many_labwares_found', count:)) | ||
| return [] | ||
| [] | ||
| end | ||
| # Only plates and tubes are currently supported by this report | ||
| Labware.search_for_labware(params).filter { |labware| labware.is_a?(Plate) || labware.is_a?(Tube) } | ||
| end |
There was a problem hiding this comment.
validate_result_size currently adds an error and returns [], but does not explicitly halt callers (and returns nil on success). Consider making this method return a boolean (eg true/false) or raising/returning consistently so callers can reliably short-circuit when the limit is exceeded.
db/migrate/20260325134057_add_retention_instructions_to_lcation_reports.rb
Outdated
Show resolved
Hide resolved
| :barcodes, | ||
| :barcodes_text, |
There was a problem hiding this comment.
location_report_params permits :barcodes and :barcodes_text twice. This duplication is redundant and can confuse future edits; remove the duplicates so each permitted key appears only once.
| :barcodes, | |
| :barcodes_text, |
| def search_for_labware_by_selection | ||
| params = { faculty_sponsor_ids:, study_id:, start_date:, end_date:, plate_purpose_ids:, barcodes: } | ||
| params = { faculty_sponsor_ids:, study_id:, start_date:, end_date:, plate_purpose_ids:, barcodes:, | ||
| retention_instructions: } | ||
| validate_result_size(params) | ||
|
|
||
| # Only plates and tubes are currently supported by this report | ||
| labwares = Labware.search_for_labware(params).filter { |labware| labware.is_a?(Plate) || labware.is_a?(Tube) } | ||
|
|
||
| filter_labware_by_retention_instructions(labwares) |
There was a problem hiding this comment.
validate_result_size(params) returns an empty array when the result set is too large, but its return value is ignored here. That means the code will still execute Labware.search_for_labware(params) even after adding the too_many_labwares_found error, defeating the safeguard and potentially loading a very large dataset. Short-circuit the method (eg return [] immediately) when the validation fails.
Closes #
Changes proposed in this pull request
add retention instruction column in location report table
add filter by retention instruction on the return record
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version