From 33ced4431f81ea4dfbc49068fe762ddf3fef8cd8 Mon Sep 17 00:00:00 2001 From: Stuart McHattie Date: Wed, 18 Dec 2024 09:48:09 +0000 Subject: [PATCH 1/4] Allow QcAbles to filter by UUID on API v2 --- app/resources/api/v2/qcable_resource.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/resources/api/v2/qcable_resource.rb b/app/resources/api/v2/qcable_resource.rb index 3ffc30d7b7..d21791fd91 100644 --- a/app/resources/api/v2/qcable_resource.rb +++ b/app/resources/api/v2/qcable_resource.rb @@ -31,6 +31,7 @@ class QcableResource < BaseResource # Filters filter :barcode, apply: ->(records, value, _options) { records.with_barcode(value) } + filter :uuid, apply: ->(records, value, _options) { records.with_uuid(value) } # Custom methods # These shouldn't be used for business logic, and are more about From b64b85ffa02edfb7d4eb37ca5ff054b348e16a54 Mon Sep 17 00:00:00 2001 From: Stuart McHattie Date: Wed, 18 Dec 2024 16:48:01 +0000 Subject: [PATCH 2/4] Create tests for QcableResource and add lots of documentation --- app/resources/api/v2/qcable_resource.rb | 130 +++++++-- spec/factories/z_tag_qc_factories.rb | 3 +- spec/requests/api/v2/qcables_spec.rb | 251 +++++++++++++++--- .../api/v2/shared_examples/requests.rb | 35 +++ spec/resources/api/v2/qcable_resource_spec.rb | 8 +- 5 files changed, 364 insertions(+), 63 deletions(-) diff --git a/app/resources/api/v2/qcable_resource.rb b/app/resources/api/v2/qcable_resource.rb index d21791fd91..dcc74add2a 100644 --- a/app/resources/api/v2/qcable_resource.rb +++ b/app/resources/api/v2/qcable_resource.rb @@ -2,49 +2,127 @@ module Api module V2 - # @todo This documentation does not yet include a detailed description of what this resource represents. - # @todo This documentation does not yet include detailed descriptions for relationships, attributes and filters. - # @todo This documentation does not yet include any example usage of the API via cURL or similar. + # Provides a JSON:API representation of {Qcable} which represents an element of a lot which needs to be approved + # by QC before it can be used. # # @note Access this resource via the `/api/v2/qcables/` endpoint. # - # Provides a JSON:API representation of {Qcable}. + # @example POST request to create a {Qcable}. + # POST /api/v2/qcables/ + # { + # "data": { + # "type": "qc_files", + # "attributes": { + # "filename": "qc_file.csv", + # "contents": "A1,A2,A3\n1,2,3\n4,5,6\n" + # }, + # "relationships": { + # "asset": { + # "data": { + # "type": "labware", + # "id": 456 + # } + # } + # } + # } + # } + # + # @example PATCH request to change the {Asset} of a {Qcable}. + # PATCH /api/v2/qcables/ + # { + # "data": { + # "type": "qc_files", + # "id": 123 + # "relationships": { + # "asset": { + # "data": { + # "type": "labware", + # "id": 456 + # } + # } + # } + # } + # } + # + # @example GET request for all {Qcable} resources + # GET /api/v2/qcables/ + # + # @example GET request for a {Qcable} with ID 123 + # GET /api/v2/qcables/123/ # # For more information about JSON:API see the [JSON:API Specifications](https://jsonapi.org/format/) # or look at the [JSONAPI::Resources](http://jsonapi-resources.com/) package for Sequencescape's implementation # of the JSON:API standard. class QcableResource < BaseResource - # Constants... - - # model_name / model_hint if required - default_includes :uuid_object, :barcodes - # Associations: - has_one :lot - has_one :asset, polymorphic: true - + ### # Attributes - attribute :uuid, readonly: true - attribute :state, write_once: true - attribute :labware_barcode, write_once: true - - # Filters - filter :barcode, apply: ->(records, value, _options) { records.with_barcode(value) } - filter :uuid, apply: ->(records, value, _options) { records.with_uuid(value) } + ### - # Custom methods - # These shouldn't be used for business logic, and are more about - # I/O and isolating implementation details. + # @!attribute [r] labware_barcode + # @return [Hash] the barcodes of the labware associated with this {Qcable}. + # This includes the EAN13 barcode, the machine barcode and the human barcode. + # Note however that some of these barcodes may be `nil`. + attribute :labware_barcode, readonly: true def labware_barcode { - 'ean13_barcode' => _model.ean13_barcode, - 'machine_barcode' => _model.machine_barcode, - 'human_barcode' => _model.human_barcode + 'ean13_barcode' => @model.ean13_barcode, + 'machine_barcode' => @model.machine_barcode, + 'human_barcode' => @model.human_barcode } end - # Class method overrides + # @!attribute [r] state + # @return [String] a string representation of the state this {Qcable} is in. + # The state is changed by a state machine via events that occur as the {Qcable} is processed. + # The possible states are: + # - `created` + # - `pending` + # - `failed` + # - `passed` + # - `available` + # - `destroyed` + # - `qc_in_progress` + # - `exhausted`. + attribute :state, readonly: true + + # @!attribute [r] uuid + # @return [String] the UUID of this {Qcable}. + attribute :uuid, readonly: true + + ### + # Relationships + ### + + # @!attribute [rw] asset + # @return [LabwareResource] the {Labware} resource associated with this {Qcable}. + # @deprecated Use the {#labware} relationship instead. + has_one :asset + + # @!attribute [rw] labware + # @return [LabwareResource] the {Labware} resource associated with this {Qcable}. + has_one :labware, foreign_key: :asset_id + + # @!attribute [rw] lot + # @return [LotResource] the {Lot} resource associated with this {Qcable}. + has_one :lot + + ### + # Filters + ### + + # @!method filter_barcode + # Apply a filter across all {Qcable} resource , matching by barcode. + # @example Get all {Qcable} resources with a specific barcode. + # /api/v2/qcables?filter[barcode]=1234567890123 + filter :barcode, apply: ->(records, value, _options) { records.with_barcode(value) } + + # @!method filter_uuid + # Apply a filter across all {Qcable} resources, matching by UUID. + # @example Get all {Qcable} resources with a specific UUID. + # /api/v2/qcables?filter[uuid]=12345678-1234-1234-1234-123456789012 + filter :uuid, apply: ->(records, value, _options) { records.with_uuid(value) } end end end diff --git a/spec/factories/z_tag_qc_factories.rb b/spec/factories/z_tag_qc_factories.rb index b2b9e98c84..ea3395d6de 100644 --- a/spec/factories/z_tag_qc_factories.rb +++ b/spec/factories/z_tag_qc_factories.rb @@ -66,10 +66,11 @@ # callbacks. lot { create(:lot) } qcable_creator { create(:qcable_creator) } + transient { sanger_barcode { nil } } factory :qcable_with_asset do state { 'created' } - asset { create(:full_plate) } + asset { create(:full_plate, sanger_barcode:) } end end diff --git a/spec/requests/api/v2/qcables_spec.rb b/spec/requests/api/v2/qcables_spec.rb index 03f2aff4c1..bcbc6213ff 100644 --- a/spec/requests/api/v2/qcables_spec.rb +++ b/spec/requests/api/v2/qcables_spec.rb @@ -2,56 +2,241 @@ require 'rails_helper' require './spec/requests/api/v2/shared_examples/api_key_authenticatable' +require './spec/requests/api/v2/shared_examples/requests' -describe 'Qcables API', with: :api_v2 do - let(:base_endpoint) { '/api/v2/qcables' } +describe 'QcFiles API', tags: :lighthouse, with: :api_v2 do + let(:model_class) { Qcable } + let(:base_endpoint) { "/api/v2/#{resource_type}" } + let(:resource_type) { model_class.name.demodulize.pluralize.underscore } it_behaves_like 'ApiKeyAuthenticatable' - context 'with multiple Qcables' do - before { create_list(:qcable, 5) } + context 'with a list of resources' do + let(:resource_count) { 5 } + let!(:resources) { Array.new(resource_count) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } } - it 'sends a list of qcables' do - api_get base_endpoint + describe '#GET all resources' do + before { api_get base_endpoint } - # test for the 200 status-code - expect(response).to have_http_status(:success) + it 'responds with a success http code' do + expect(response).to have_http_status(:success) + end - # check to make sure the right amount of messages are returned - expect(json['data'].length).to eq(5) + it 'returns all the resources' do + expect(json['data'].count).to eq(resource_count) + end end - # Check filters, ESPECIALLY if they aren't simple attribute filters + describe '#filter' do + let(:target_resource) { resources.sample } + let(:target_id) { target_resource.id } + + describe 'by ean13 barcode' do + before { api_get "#{base_endpoint}?filter[barcode]=#{target_resource.ean13_barcode}" } + + it_behaves_like 'has filtered to a resource with target_id correctly' + end + + describe 'by human barcode' do + before { api_get "#{base_endpoint}?filter[barcode]=#{target_resource.human_barcode}" } + + it_behaves_like 'has filtered to a resource with target_id correctly' + end + + describe 'by machine barcode' do + before { api_get "#{base_endpoint}?filter[barcode]=#{target_resource.machine_barcode}" } + + it_behaves_like 'has filtered to a resource with target_id correctly' + end + + describe 'by uuid' do + before { api_get "#{base_endpoint}?filter[uuid]=#{target_resource.uuid}" } + + it_behaves_like 'has filtered to a resource with target_id correctly' + end + end + end + + context 'with a single resource' do + describe '#GET resource by ID' do + let(:resource) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } + + context 'without included relationships' do + before { api_get "#{base_endpoint}/#{resource.id}" } + + it 'responds with a success http code' do + expect(response).to have_http_status(:success) + end + + it 'returns the resource with the correct id' do + expect(json.dig('data', 'id')).to eq(resource.id.to_s) + end + + it 'returns the resource with the correct type' do + expect(json.dig('data', 'type')).to eq(resource_type) + end + + it 'responds with the correct labware_barcode attribute value' do + barcode_hash = { + 'ean13_barcode' => resource.ean13_barcode, + 'machine_barcode' => resource.machine_barcode, + 'human_barcode' => resource.human_barcode + } + expect(json.dig('data', 'attributes', 'labware_barcode')).to eq(barcode_hash) + end + + it 'responds with the correct state attribute value' do + expect(json.dig('data', 'attributes', 'state')).to eq(resource.state) + end + + it 'responds with the correct uuid attribute value' do + expect(json.dig('data', 'attributes', 'uuid')).to eq(resource.uuid) + end + + it 'returns a reference to the asset relationship' do + expect(json.dig('data', 'relationships', 'asset')).to be_present + end + + it 'returns a reference to the lot relationship' do + expect(json.dig('data', 'relationships', 'lot')).to be_present + end + + it 'does not include attributes for related resources' do + expect(json['included']).not_to be_present + end + end + + context 'with included relationships' do + it_behaves_like 'a GET request including a has_one relationship', 'asset' + it_behaves_like 'a GET request including a has_one relationship', 'lot' + end + end end - context 'with a Qcable' do - let(:resource_model) { create(:qcable) } + describe '#PATCH a resource' do + let(:resource) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } + + context 'with a valid payload' do + let(:new_labware) { create(:full_plate) } - let(:payload) do - { - 'data' => { - 'id' => resource_model.id, - 'type' => 'qcables', - 'attributes' => { - # Set new attributes + context 'with non-deprecated relationships' do + let(:new_lot) { create(:lot) } + let(:payload) do + { + data: { + id: resource.id, + type: resource_type, + relationships: { + labware: { + data: { + type: 'labware', + id: new_labware.id + } + }, + lot: { + data: { + type: 'lots', + id: new_lot.id + } + } + } + } } - } - } + end + + before { api_patch "#{base_endpoint}/#{resource.id}", payload } + + it 'updates the labware/asset on the resource' do + expect(resource.reload.asset).to eq(new_labware) + end + + it 'updates the lot on the resource' do + expect(resource.reload.lot).to eq(new_lot) + end + end + + context 'with deprecated relationships' do + let(:payload) do + { + data: { + id: resource.id, + type: resource_type, + relationships: { + asset: { + data: { + type: 'labware', + id: new_labware.id + } + } + } + } + } + end + + before { api_patch "#{base_endpoint}/#{resource.id}", payload } + + it 'updates the labware/asset on the resource' do + expect(resource.reload.asset).to eq(new_labware) + end + end end - it 'sends an individual Qcable' do - api_get "#{base_endpoint}/#{resource_model.id}" - expect(response).to have_http_status(:success) - expect(json.dig('data', 'type')).to eq('qcables') + context 'with a read-only attribute in the payload' do + context 'with labware_barcode' do + let(:payload) do + { data: { id: resource.id, type: resource_type, attributes: { labware_barcode: { human_barcode: '1-2' } } } } + end + + it_behaves_like 'a PATCH request with a disallowed value', 'labware_barcode' + end + + context 'with state' do + let(:payload) { { data: { id: resource.id, type: resource_type, attributes: { state: 'pending' } } } } + + it_behaves_like 'a PATCH request with a disallowed value', 'state' + end + + context 'with uuid' do + let(:payload) { { data: { id: resource.id, type: resource_type, attributes: { uuid: 'new-uuid' } } } } + + it_behaves_like 'a PATCH request with a disallowed value', 'uuid' + end end + end + + describe '#POST a create request' do + # This test is a bit weird, because for whatever reason, the resource is currently incomplete for Qcables. Qcables + # validate the presence of a lot, asset, and qcable_creator, but the API doesn't allow you to set a qcable_creator. + # So, we can't actually create a valid Qcable through the API. + # + # I don't know why this is the case - it seems like it was an oversight on this endpoint, but the endpoint is in use + # already, so I don't want to modify it more than I have to. It's quite likely that the endpoint should be + # immutable, but it wasn't configured that way and I don't know what existing users may already be trying to do with + # this endpoint, so I'm going to leave it alone. For now, let's just test that the endpoint doesn't allow you to + # create a Qcable and it can be adjusted in future if we need to be able to create them at that time. + + let(:labware) { create(:plate) } + let(:lot) { create(:lot) } + + let(:labware_relationship) { { data: { id: labware.id, type: 'labware' } } } + let(:lot_relationship) { { data: { id: lot.id, type: 'lots' } } } + + let(:base_attributes) { {} } + let(:base_relationships) { { labware: labware_relationship, lot: lot_relationship } } + + context 'with a complete payload' do + let(:payload) do + { data: { type: resource_type, attributes: base_attributes, relationships: base_relationships } } + end + + it 'fails with an unprocessable entity status' do + api_post base_endpoint, payload + expect(response).to have_http_status(:unprocessable_entity) + end - # Remove if immutable - it 'allows update of a Qcable' do - api_patch "#{base_endpoint}/#{resource_model.id}", payload - expect(response).to have_http_status(:success) - expect(json.dig('data', 'type')).to eq('qcables') - # Double check at least one of the attributes - # eg. expect(json.dig('data', 'attributes', 'state')).to eq('started') + it 'does not create a new resource' do + expect { api_post base_endpoint, payload }.not_to change(model_class, :count) + end end end end diff --git a/spec/requests/api/v2/shared_examples/requests.rb b/spec/requests/api/v2/shared_examples/requests.rb index 829b65e79c..13b896be5d 100644 --- a/spec/requests/api/v2/shared_examples/requests.rb +++ b/spec/requests/api/v2/shared_examples/requests.rb @@ -113,3 +113,38 @@ expect(model_class.last.send(related_name)).to eq(value) end end + +shared_examples 'a PATCH request with a disallowed value' do |disallowed_value| + def do_patch + api_patch "#{base_endpoint}/#{resource.id}", payload + end + + it 'does not modify the resource attributes' do + # Note that attributes also includes IDs for relationships. + expect { do_patch }.not_to(change { resource.reload.attributes }) + end + + it 'responds with bad_request' do + do_patch + expect(response).to have_http_status(:bad_request) + end + + it 'specifies which value was not allowed' do + do_patch + expect(json.dig('errors', 0, 'detail')).to eq("#{disallowed_value} is not allowed.") + end +end + +shared_examples 'has filtered to a resource with target_id correctly' do + it 'responds with a success http code' do + expect(response).to have_http_status(:success) + end + + it 'returns one resource' do + expect(json['data'].count).to eq(1) + end + + it 'returns the correct resource' do + expect(json['data'].first['id']).to eq(target_id.to_s) + end +end diff --git a/spec/resources/api/v2/qcable_resource_spec.rb b/spec/resources/api/v2/qcable_resource_spec.rb index 3bd5e5d8a3..e4d9355966 100644 --- a/spec/resources/api/v2/qcable_resource_spec.rb +++ b/spec/resources/api/v2/qcable_resource_spec.rb @@ -12,14 +12,16 @@ it { is_expected.to have_model_name 'Qcable' } # Attributes - it { is_expected.to have_write_once_attribute :labware_barcode } - it { is_expected.to have_write_once_attribute :state } + it { is_expected.to have_readonly_attribute :labware_barcode } + it { is_expected.to have_readonly_attribute :state } it { is_expected.to have_readonly_attribute :uuid } # Relationships - it { is_expected.to have_a_writable_has_one(:lot).with_class_name('Lot') } it { is_expected.to have_a_writable_has_one(:asset).with_class_name('Labware') } + it { is_expected.to have_a_writable_has_one(:labware).with_class_name('Labware') } + it { is_expected.to have_a_writable_has_one(:lot).with_class_name('Lot') } # Filters it { is_expected.to filter(:barcode) } + it { is_expected.to filter(:uuid) } end From cf5814bbe5df887352a75452b479f2b40f1244dc Mon Sep 17 00:00:00 2001 From: Stuart McHattie Date: Wed, 18 Dec 2024 17:39:05 +0000 Subject: [PATCH 3/4] Fix labware relationships --- app/resources/api/v2/qcable_resource.rb | 2 +- spec/requests/api/v2/qcables_spec.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/resources/api/v2/qcable_resource.rb b/app/resources/api/v2/qcable_resource.rb index dcc74add2a..e3622aa8cf 100644 --- a/app/resources/api/v2/qcable_resource.rb +++ b/app/resources/api/v2/qcable_resource.rb @@ -102,7 +102,7 @@ def labware_barcode # @!attribute [rw] labware # @return [LabwareResource] the {Labware} resource associated with this {Qcable}. - has_one :labware, foreign_key: :asset_id + has_one :labware, relation_name: 'asset', foreign_key: :asset_id # @!attribute [rw] lot # @return [LotResource] the {Lot} resource associated with this {Qcable}. diff --git a/spec/requests/api/v2/qcables_spec.rb b/spec/requests/api/v2/qcables_spec.rb index bcbc6213ff..6ef365a681 100644 --- a/spec/requests/api/v2/qcables_spec.rb +++ b/spec/requests/api/v2/qcables_spec.rb @@ -97,6 +97,10 @@ expect(json.dig('data', 'relationships', 'asset')).to be_present end + it 'returns a reference to the labware relationship' do + expect(json.dig('data', 'relationships', 'labware')).to be_present + end + it 'returns a reference to the lot relationship' do expect(json.dig('data', 'relationships', 'lot')).to be_present end @@ -108,6 +112,7 @@ context 'with included relationships' do it_behaves_like 'a GET request including a has_one relationship', 'asset' + it_behaves_like 'a GET request including a has_one relationship', 'labware' it_behaves_like 'a GET request including a has_one relationship', 'lot' end end From 474eabbe496acc6e4a416c71c8b1ae7f3835de48 Mon Sep 17 00:00:00 2001 From: Stuart McHattie Date: Wed, 18 Dec 2024 17:39:31 +0000 Subject: [PATCH 4/4] Make all factory created Qcables have a Sanger Ean13 barcode --- spec/factories/z_tag_qc_factories.rb | 2 +- spec/requests/api/v2/qcables_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/factories/z_tag_qc_factories.rb b/spec/factories/z_tag_qc_factories.rb index ea3395d6de..50381ea43b 100644 --- a/spec/factories/z_tag_qc_factories.rb +++ b/spec/factories/z_tag_qc_factories.rb @@ -66,7 +66,7 @@ # callbacks. lot { create(:lot) } qcable_creator { create(:qcable_creator) } - transient { sanger_barcode { nil } } + transient { sanger_barcode { create(:sanger_ean13) } } factory :qcable_with_asset do state { 'created' } diff --git a/spec/requests/api/v2/qcables_spec.rb b/spec/requests/api/v2/qcables_spec.rb index 6ef365a681..21a7be0b8e 100644 --- a/spec/requests/api/v2/qcables_spec.rb +++ b/spec/requests/api/v2/qcables_spec.rb @@ -13,7 +13,7 @@ context 'with a list of resources' do let(:resource_count) { 5 } - let!(:resources) { Array.new(resource_count) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } } + let!(:resources) { Array.new(resource_count) { create(:qcable_with_asset) } } describe '#GET all resources' do before { api_get base_endpoint } @@ -59,7 +59,7 @@ context 'with a single resource' do describe '#GET resource by ID' do - let(:resource) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } + let(:resource) { create(:qcable_with_asset) } context 'without included relationships' do before { api_get "#{base_endpoint}/#{resource.id}" } @@ -119,7 +119,7 @@ end describe '#PATCH a resource' do - let(:resource) { create(:qcable_with_asset, sanger_barcode: create(:sanger_ean13)) } + let(:resource) { create(:qcable_with_asset) } context 'with a valid payload' do let(:new_labware) { create(:full_plate) }