Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions app/models/labware.rb
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,18 @@ def obtain_retention_instructions
metadata.symbolize_keys[:retention_instruction]
end

def lookup_labwhere_location
lookup_labwhere(machine_barcode) || lookup_labwhere(human_barcode)
def lookup_labwhere_location # rubocop:todo Metrics/AbcSize, Metrics/CyclomaticComplexity
machine_lookup = lookup_labwhere(machine_barcode)
human_lookup = lookup_labwhere(human_barcode)

valid_lookups = [machine_lookup, human_lookup].compact.select { |lookup| lookup[:location].present? }
return valid_lookups.max_by { |lookup| lookup[:updated_at] || Time.zone.at(0) }[:location] if valid_lookups.any?

return 'Not found - There is a problem with Labwhere' if [machine_lookup, human_lookup].any? do |lookup|
lookup&.dig(:error)
end

nil
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to reduce the complexity of this function (possibly by pulling out some private methods)?

It's difficult to understand exactly what the new code does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored.


def lookup_labwhere(barcode)
Expand All @@ -378,9 +388,23 @@ def lookup_labwhere(barcode)
rescue StandardError => e
# rescue LabWhereClient::LabwhereException => e
Rails.logger.error { e }
return 'Not found - There is a problem with Labwhere'
return { error: true }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we maybe return a message as well? I don't think true provides enough feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added error messages in return

end
info_from_labwhere.location.location_info if info_from_labwhere.present? && info_from_labwhere.location.present?

return nil unless info_from_labwhere.present? && info_from_labwhere.location.present?

{
location: info_from_labwhere.location.location_info,
updated_at: parse_labwhere_timestamp(info_from_labwhere.updated_at)
}
end

def parse_labwhere_timestamp(timestamp)
return if timestamp.blank?

Time.zone.parse(timestamp)
rescue ArgumentError
nil
end
end
# rubocop:enable Metrics/ClassLength
8 changes: 5 additions & 3 deletions lib/lab_where_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def self.included(base)
class Labware < Endpoint
endpoint_name 'labwares'

attr_reader :barcode, :location
attr_reader :barcode, :location, :updated_at

def self.find_by_barcode(barcode)
return nil if barcode.blank?
Expand All @@ -102,7 +102,8 @@ def self.find_by_barcode(barcode)

def initialize(params)
@barcode = params['barcode']
@location = Location.new(params['location'])
@updated_at = params['updated_at']
@location = Location.new(params['location']) if params['location'].present?
end
end

Expand Down Expand Up @@ -155,7 +156,7 @@ def valid?
class Location < Endpoint
endpoint_name 'locations'

attr_reader :name, :parentage, :barcode
attr_reader :name, :parentage, :barcode, :updated_at

def self.find_by_barcode(barcode)
return nil if barcode.blank?
Expand All @@ -168,6 +169,7 @@ def initialize(params)
@name = params['name']
@parentage = params['parentage']
@barcode = params['barcode']
@updated_at = params['updated_at']
end

def location_info
Expand Down
22 changes: 22 additions & 0 deletions spec/lib/lab_where_client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,26 @@
end
end
end

describe LabWhereClient::Labware do
let(:params) do
{
'barcode' => '123456',
'updated_at' => 'Wednesday May 6 2026 10:46',
'location' => {
'name' => 'f1',
'parentage' => 'Sanger / Ogilvie / AA216',
'barcode' => 'lw-f1-26214',
'updated_at' => 'Tuesday June 6 2023 16:26'
}
}
end

it 'captures labware and location updated_at values from the API payload' do
labware = described_class.new(params)

expect(labware.updated_at).to eq('Wednesday May 6 2026 10:46')
expect(labware.location.updated_at).to eq('Tuesday June 6 2023 16:26')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This date format is nice for humans, but non-standard and difficult for computers to parse. May I suggest that the updated_at fields are always represented as ISO 8601 standard strings?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

end
end
end
60 changes: 60 additions & 0 deletions spec/models/labware_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,66 @@
plate = create(:plate)
expect(plate.storage_location).to eq('Not found - There is a problem with Labwhere')
end

it 'chooses the newer location when both machine and human barcode lookups return values' do
plate = create(:plate)

allow(LabWhereClient::Labware).to receive(:find_by_barcode).with(plate.machine_barcode).and_return(
LabWhereClient::Labware.new(
{
'barcode' => plate.machine_barcode,
'updated_at' => 'Wednesday May 6 2026 10:46',
'location' => {
'name' => 'f1',
'parentage' => 'Sanger / Ogilvie / AA216',
'barcode' => 'lw-f1-26214',
'updated_at' => 'Wednesday May 6 2026 10:46'
}
}
)
)
allow(LabWhereClient::Labware).to receive(:find_by_barcode).with(plate.human_barcode).and_return(
LabWhereClient::Labware.new(
{
'barcode' => plate.human_barcode,
'updated_at' => 'Wednesday May 7 2026 10:46',
'location' => {
'name' => 'f2',
'parentage' => 'Sanger / Ogilvie / AA216',
'barcode' => 'lw-f2-26214',
'updated_at' => 'Wednesday May 7 2026 10:46'
}
}
)
)

expect(plate.labwhere_location).to eq('Sanger / Ogilvie / AA216 - f2')
end

it 'falls back to human barcode location when machine barcode lookup raises an error' do
plate = create(:plate)

allow(LabWhereClient::Labware).to receive(:find_by_barcode).with(plate.machine_barcode).and_raise(
StandardError,
'Timed out reading data from server'
)
allow(LabWhereClient::Labware).to receive(:find_by_barcode).with(plate.human_barcode).and_return(
LabWhereClient::Labware.new(
{
'barcode' => plate.human_barcode,
'updated_at' => 'Wednesday May 7 2026 10:46',
'location' => {
'name' => 'f2',
'parentage' => 'Sanger / Ogilvie / AA216',
'barcode' => 'lw-f2-26214',
'updated_at' => 'Wednesday May 7 2026 10:46'
}
}
)
)

expect(plate.labwhere_location).to eq('Sanger / Ogilvie / AA216 - f2')
end
end

context 'when retrieving retention instructions' do
Expand Down
4 changes: 4 additions & 0 deletions spec/models/location_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@
end

describe 'report generation' do
before do
allow(LabWhereClient::Labware).to receive(:find_by_barcode).and_return(nil)
end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this mocking out the actual code we are testing? Or is it not important to this test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-write to include human barcode


shared_examples 'a successful report' do
it 'generates the expected report rows' do
expect(location_report.save).to be_truthy
Expand Down
Loading