Skip to content
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

Y24-190-3: Use SS v2 for TagLayoutTemplates #1835

Merged
merged 19 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
86d5934
Add classes for using v2 API TagLayoutTemplate
sdjmchattie Jul 31, 2024
b73aa7b
Start putting V2 tag layout template stubs/factories in place
sdjmchattie Jul 31, 2024
dff8746
Document the approach to stubbing relationships in V2 resources
sdjmchattie Aug 1, 2024
4fd7757
Remove the accidentally committed test debugging objects
sdjmchattie Aug 1, 2024
1d89cf7
Ensure tags generated for a tag_group point to the correct group
sdjmchattie Aug 1, 2024
4b82bbb
Fix relationships for tag layout templates in FactoryBot
sdjmchattie Aug 1, 2024
3306e6d
Fix tests for custom tagged plate spec
sdjmchattie Aug 1, 2024
b751825
Fix tests for tagged_plate_spec
sdjmchattie Aug 1, 2024
c4940a1
Remove unneeded stub
sdjmchattie Aug 2, 2024
cc2593b
Add a factory for a dual indexed tag layout template
sdjmchattie Aug 2, 2024
cbb3f58
Clean up the naming in shared examples
sdjmchattie Aug 2, 2024
3da27a2
Correctly name context blocks
sdjmchattie Aug 5, 2024
7b0759f
Assign correct class for tag2_group on tag_layout_template
sdjmchattie Aug 5, 2024
201fb5b
Merge page results when getting tag layout templates
sdjmchattie Aug 5, 2024
7e895d3
Add TODO about calling methods on resources
sdjmchattie Aug 6, 2024
d92ff39
Stub v2 TagLayoutTemplates with paging
sdjmchattie Aug 6, 2024
ef03d4b
Fix tests for creating_a_tag_plate_spec using V2
sdjmchattie Aug 6, 2024
98fa7a1
Fix final tests for tag_layout_templates move to V2
sdjmchattie Aug 6, 2024
ff89668
Merge branch 'develop-Y24-190' into Y24-190-3-use-ss-v2-for-labware-c…
sdjmchattie Aug 8, 2024
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
58 changes: 57 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -200,12 +200,68 @@ There are a few tools available to assist with writing specs:

- Helpers: `with_has_many_associations` and `with_belongs_to_associations` can be used in factories to set up the relevant json. They won't actually mock up the relevant requests, but ensure that things like actions are defined so that the api knows where to find them.

#### Request stubbing
#### Request stubbing for the Sequencescape v1 API

Request stubs are provided by webmock. Two helper methods will assist with the majority of mocking requests to the api, `stub_api_get` and `stub_api_post`. See `spec/support/api_url_helper.rb` for details.

**Note**: Due to the way the api functions, the factories don't yet support nested associations.

#### Request stubbing for the Sequencescape v2 API

The V2 API uses `JsonApiClient::Resource` sub-classes to represent the records in memory.
Generally these are quite dynamic so you don't need to explicitly specify every property the API will respond with.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for writing this documentation.

Generally these are quite dynamic so you don't need to explicitly specify every property the API will respond with.

Not quite sure what this sentence means. How will the record have the property if you haven't specified it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Magic. Basically the resources mostly look like this:

# frozen_string_literal: true

class Sequencescape::Api::V2::Project < Sequencescape::Api::V2::Base
end

but even so, that's just a template for the client gem to know that it should load a resource from that endpoint and the attributes from the API will be available to you when you use this class. They are dynamically assigned.

The exception is for relationships. If the API supports a resource having associated other resources, you do need to specify has_one, has_many etc in order for it to make those accessible via the client gem.

You can also add custom logic to these, like I did for TagLayoutTemplate, so you can change the named algorithms into implementations of that algorithm on the Limber side.

The base class also provides us with methods that are familiar to Rails for finding one or more records that match criteria.
So to stub the API, the easiest thing to do is to get FactoryBot to make up resources using the specific resource sub-class for the V2 API, and then mock the calls to those lookup methods.
Many of these have already been done for you in `spec/support/api_url_helper.rb` such as `stub_v2_study` and `stub_v2_tag_layout_templates` which sets up the `find` method for studies by name and the `all` method for tag layout templates, respectively.
However there's also `stub_api_v2_post`, `stub_api_v2_patch` and `stub_api_v2_save` which ensures that any calls to the `create`, `update` and the `save` method for resources of a particular type are expected and give a return value.
If none of the existing method suit your needs, you should add new ones.

##### FactoryBot is not mocking my related resources correctly

Nested relationships, such as Wells inside a Plate, the resource should indicate this with keywords like `has_one`, `has_many`, `belongs_to`, etc.
See the [json_api_client repository](https://github.com/JsonApiClient/json_api_client) for this topic and more.
However, FactoryBot does not get on well with some of these relationship methods and will not mock them properly using standard FactoryBot definitions.

If you find that FactoryBot is not giving you the expected resource for a related record, you can inject the related resource directly into the `JsonApiClient::Resource`'s cache of relationships.
To do that, define the related resource as a `transient` variable and use an `after(:build)` block to assign the resource to the `_cached_relationship` method.
For example, where the Resource might be defined as the following class:

```ruby
class Sequencescape::Api::V2::RootRecord < JsonApiClient::Resource
has_one :related_thing
end
```

You might expect to be able to use FactoryBot in the following way:

```ruby
FactoryBot.define do
factory :root_record, class: Sequencescape::Api::V2::RootRecord do
skip_create

related_thing { create :related_thing }
end
end
```

But the related thing will not be the one you defined to be generated by another factory.
It appears the `has_one` method in the resource over-rides the mocked value and you get an empty record back instead.
So, instead, you should create the `related_thing` record as a transient and assign it to the `root_record` in an `after(:build)` block as shown here.

```ruby
FactoryBot.define do
factory :root_record, class: Sequencescape::Api::V2::RootRecord do
skip_create

transient { related_thing { create :v2_tag_group_with_tags } }

after(:build) do |record, factory|
record._cached_relationship(:related_thing) { factory.related_thing } if evaluator.related_thing
end
end
end
```

### Lefthook

[Lefthook](https://github.com/Arkweid/lefthook) is a git-hook manager that will
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def transfer_hash
end

def tag_plates
@tag_plates ||= LabwareCreators::Tagging::TagCollection.new(api, labware, purpose_uuid)
@tag_plates ||= LabwareCreators::Tagging::TagCollection.new(labware, purpose_uuid)
end

#
Expand Down
4 changes: 4 additions & 0 deletions app/models/labware_creators/tagged_plate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ def convert_tag_plate_to_new_purpose

def create_labware!
create_plate! do |plate_uuid|
# TODO: {Y24-190} Work out a way to call the `create!` method on TagLayoutTemplate model in Sequencescape
# using the V2 API. I think either we need to misuse the PATCH method with some kind of
# attributes telling Sequencescape to run the `create!` method, or we need to create a new
# endpoint associated with a TagLayoutTemplate that will run the `create!` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought you were planning to redesign it so that you could call create! on TagLayout instead, and that pulled info from the TagLayoutTemplate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we don't have TagLayout in V2 yet. I plan to do that in a soon pull request. This comment was written before we came across that solution. I'm not sure it's worth updating since it's coming out in the next week.

api
.tag_layout_template
.find(tag_plate.template_uuid)
Expand Down
7 changes: 3 additions & 4 deletions app/models/labware_creators/tagging/tag_collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ class TagCollection # rubocop:todo Style/Documentation
#
# Create a tag collection
#
# @param [Sequencescape::Client::Api] api an api object used to retrieve tag templates
# @param [Limber::Plate] plate The plate from which the tag layout will be generated
# @param [String] purpose_uuid The uuid of the purpose which is about to be created
#
def initialize(api, plate, purpose_uuid)
@api = api
def initialize(plate, purpose_uuid)
@plate = plate
@purpose_uuid = purpose_uuid
end
Expand Down Expand Up @@ -105,7 +103,8 @@ def tags_by_column(layout)
end

def tag_layout_templates
@api.tag_layout_template.all.map(&:coerce)
query = Sequencescape::Api::V2::TagLayoutTemplate.paginate(per_page: 100)
Sequencescape::Api::V2.merge_page_results(query).map(&:coerce)
end
end
end
46 changes: 46 additions & 0 deletions app/sequencescape/sequencescape/api/v2/tag_layout_template.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# frozen_string_literal: true

# tag layout template resource
class Sequencescape::Api::V2::TagLayoutTemplate < Sequencescape::Api::V2::Base
has_one :tag_group
has_one :tag2_group, class_name: 'TagGroup'

def dual_index?
tag2_group.present?
end

# Performs the coercion of this instance so that it behaves appropriately given the direction
# and walking algorithm information.
def coerce
extend("limber/tag_layout_template/in_#{direction.gsub(/\s+/, '_')}s".camelize.constantize)
extend("limber/tag_layout_template/walk_#{walking_by.gsub(/\s+/, '_')}".camelize.constantize)
rescue NameError => e
Rails.logger.warn("Unrecognised layout options: #{e.message}")
extend Unsupported

Check warning on line 19 in app/sequencescape/sequencescape/api/v2/tag_layout_template.rb

View check run for this annotation

Codecov / codecov/patch

app/sequencescape/sequencescape/api/v2/tag_layout_template.rb#L18-L19

Added lines #L18 - L19 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

So this method and the one below are taken from the existing app > models > limber > tag_layout_template.rb file... is that file needed any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaving removal of V1 until the end. It's still unclear how tightly tied they are into the system and I do still need it until we remove the TODO items.

ensure
self
end

# This returns an array of well location to pool pairs. The 'walker' is responsible for actually doing the walking
# of the wells that are acceptable, and it calls back with the location of the well being processed.
def group_wells(plate)
well_to_pool = plate.wells.each_with_object({}) { |well, store| store[well.location] = well.pool_id }

# We assume that if a well is unpooled then it is in the same pool as the previous pool.
prior_pool = nil
callback =
lambda do |row_column|
prior_pool = pool = well_to_pool[row_column] || prior_pool # or next
well_empty = well_to_pool[row_column].nil?
well = pool.nil? ? nil : row_column
[well, pool, well_empty] # Triplet: [ A1, pool_id, well_empty ]
end
yield(callback)
end
private :group_wells

def tag_ids
tag_group.tags.map { |t| t['index'].to_i }.sort
end
private :tag_ids
end
2 changes: 1 addition & 1 deletion spec/factories/asset_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
purpose_uuid { 'example-purpose-uuid' }
purpose { create :v2_purpose, name: purpose_name, uuid: purpose_uuid }

# Mock the relationships. Should probably handle this all a bit differently
# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) { |asset, evaluator| asset._cached_relationship(:purpose) { evaluator.purpose } }
end
end
3 changes: 1 addition & 2 deletions spec/factories/labware_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@
factory(:labware_tube) { type { 'tubes' } }
factory(:labware_tube_rack) { type { 'tube_racks' } }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |labware, evaluator|
# see plate_factories -> v2_plate factory -> after(:build) for an explanation of _cached_relationship
# basically, it allows you to call .purpose on the labware and get a Sequencescape::Api::V2::Purpose
labware._cached_relationship(:purpose) { evaluator.purpose } if evaluator.purpose
labware._cached_relationship(:ancestors) { evaluator.ancestors } if evaluator.ancestors
end
Expand Down
12 changes: 1 addition & 11 deletions spec/factories/plate_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,7 @@
updated_at { '2017-06-29T09:31:59.000+01:00' }
custom_metadatum_collection { nil }

# Set up the relationships.
# json_client_api handles assigning of relationship information in a frustrating manner
# which isn't amenable to setting up objects for testing. Instead it tends to strip
# the attributes off the associated records, leaving just a type and an id. This is not
# useful if you want to use this data later.
# Even more frustratingly is that if you attempt to bypass this and set the attribute directly
# the getter attempts to fetch the object via a cache instead.
# Here we populate the cache directly with the objects we want. This is *MUCH* faster
# than achieving the same through mocks.
# We could probably avoid needing to do anything sneaky at all if we instead generated
# json-api data and generated the objects from that.
# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |plate, evaluator|
Sequencescape::Api::V2::Plate.associations.each do |association|
plate._cached_relationship(association.attr_name) { evaluator.send(association.attr_name) }
Expand Down
1 change: 1 addition & 0 deletions spec/factories/receptacle_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

transient { qc_results { [] } }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |receptacle, evaluator|
receptacle._cached_relationship(:qc_results) { evaluator.qc_results || [] }
receptacle._cached_relationship(:requests_as_source) { evaluator.requests_as_source || [] }
Expand Down
1 change: 1 addition & 0 deletions spec/factories/request_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
factory :library_request do
request_type { create :library_request_type }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) { |request, evaluator| request._cached_relationship(:request_type) { evaluator.request_type } }

# Library request with primer panel information
Expand Down
1 change: 1 addition & 0 deletions spec/factories/sample_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
sample_manifest { create(:v2_sample_manifest) }
uuid { SecureRandom.uuid }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) { |sample, evaluator| sample._cached_relationship(:sample_metadata) { evaluator.sample_metadata } }
end

Expand Down
14 changes: 12 additions & 2 deletions spec/factories/tag_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,23 @@ def generate_oligo
factory :v2_tag, class: Sequencescape::Api::V2::Tag do
skip_create

sequence(:map_id) { |i| i }
sequence(:oligo) { |_index| generate_oligo }
tag_group { create :v2_tag_group }
tag_group { create :v2_tag_group, v2_tags: [instance] }
end

factory :v2_tag_group, class: Sequencescape::Api::V2::Tag do
factory :v2_tag_group, class: Sequencescape::Api::V2::TagGroup do
skip_create

transient { v2_tags { [] } }
sequence(:name) { |index| "TagGroup#{index}" }
tags { v2_tags.map { |t| { index: t.map_id, oligo: t.oligo } } }
Copy link
Contributor

Choose a reason for hiding this comment

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

So v2_tags get passed into this factory as instances of Sequencescape::Api::V2::Tag, but this data structure seems to change them into a simple hash? Is that how it appears in real API results too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I wouldn't have done it this way as it's not how V1 works, but the V2 endpoint was pre-existing with this and if I change it, who knows what else might break.

One of the biggest (and possibly least obvious) issues with APIs is that they're even harder to change than other aspects of systems. You're setting up a contract with people outside your team's scope for how they can interface with your application. Making decisions about API implementations should never be taken lightly. Deploying a bad decision has to be maintained until the next version of the API.


factory :v2_tag_group_with_tags do
transient do
size { 96 }
v2_tags { (1..size).map { |i| create(:v2_tag, map_id: i, tag_group: instance) } }
end
end
end
end
23 changes: 23 additions & 0 deletions spec/factories/tag_layout_template_factories.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,29 @@
# frozen_string_literal: true

FactoryBot.define do
factory :v2_tag_layout_template, class: Sequencescape::Api::V2::TagLayoutTemplate do
skip_create

uuid
sequence(:name) { |index| "TagLayoutTemplate#{index}" }
direction { 'column' }
walking_by { 'wells of plate' }
transient do
tag_group { create :v2_tag_group_with_tags }
tag2_group { nil }
end

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |tag_layout_template, evaluator|
tag_layout_template._cached_relationship(:tag_group) { evaluator.tag_group } if evaluator.tag_group
tag_layout_template._cached_relationship(:tag2_group) { evaluator.tag2_group } if evaluator.tag2_group
end

factory :v2_dual_index_tag_layout_template do
transient { tag2_group { create :v2_tag_group_with_tags } }
end
end

# API V1 tag layout template. The inheriting factories set up the patterns
# commonly seen by Limber
factory :tag_layout_template, class: Limber::TagLayoutTemplate, traits: [:api_object] do
Expand Down
2 changes: 1 addition & 1 deletion spec/factories/tube_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@
purpose { create :v2_purpose, name: purpose_name, uuid: purpose_uuid }
end

# Mock the relationships. Should probably handle this all a bit differently
# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |asset, evaluator|
asset._cached_relationship(:purpose) { evaluator.purpose }
ancestors_scope = JsonApiClient::Query::Builder.new(Sequencescape::Api::V2::Asset)
Expand Down
1 change: 1 addition & 0 deletions spec/factories/tube_rack_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
created_at { '2017-06-29T09:31:59.000+01:00' }
updated_at { '2017-06-29T09:31:59.000+01:00' }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |tube_rack, evaluator|
Sequencescape::Api::V2::TubeRack.associations.each do |association|
tube_rack._cached_relationship(association.attr_name) { evaluator.send(association.attr_name) }
Expand Down
3 changes: 2 additions & 1 deletion spec/factories/well_factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
sub_pool { nil }
coverage { nil }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |well, evaluator|
well._cached_relationship(:qc_results) { evaluator.qc_results || [] }
well._cached_relationship(:aliquots) { evaluator.aliquots || [] }
Expand Down Expand Up @@ -273,8 +274,8 @@
sample { create :v2_sample, sample_attributes }
request { outer_request }

# See the README.md for an explanation under "FactoryBot is not mocking my related resources correctly"
after(:build) do |aliquot, evaluator|
# Set up relationships downstream
Sequencescape::Api::V2::Aliquot.associations.each do |association|
aliquot._cached_relationship(association.attr_name) { evaluator.send(association.attr_name) }
end
Expand Down
Loading
Loading