Y24-190 Milestone 1: API v1 -> v2 Migration #1884

sdjmchattie · 2024-09-02T16:22:27Z

Milestone 1 for #1772

Changes proposed in this pull request

Updated usage of API v2 to use new endpoints added to Sequencescape.
Updated documentation for how to stub v2 endpoints.
See: Y24-190 Milestone 1: API v1 -> v2 Migration sequencescape#4315

Note that these changes were tested by the Integration Suite after deployment to UAT: https://gitlab.internal.sanger.ac.uk/psd/integration-suite/-/pipelines/300247/builds

Instructions for Reviewers

[All PRs] - Confirm PR template filled
[Feature Branches] - Review code

Only just found out about it and it looks like it hasn't been done by anyone in recent times.

It's used by sub-classes so they need to be fixed first

…mium-pipelines Y24-190-2: Use SS API v2 for barcode printers

…cross tests

Y24-190-3: Use v2 transfers and transfer templates

Y24-190-3: Use API v2 StateChange resource

codeclimate · 2024-09-02T16:22:57Z

app/sequencescape/sequencescape/api/v2/tag_layout_template.rb

+
+  # 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)


Identical blocks of code found in 2 locations. Consider refactoring.

This is because we are supporting v1 as well until this work is complete.

codeclimate · 2024-09-02T16:22:57Z

app/sequencescape/sequencescape/api/v2/tag_layout_template.rb

+
+  # Performs the coercion of this instance so that it behaves appropriately given the direction
+  # and walking algorithm information.
+  def coerce


Identical blocks of code found in 2 locations. Consider refactoring.

This is because we are supporting v1 as well until this work is complete.

codecov · 2024-09-02T16:23:51Z

Codecov Report

Attention: Patch coverage is 99.49495% with 6 lines in your changes missing coverage. Please review.

Project coverage is 77.96%. Comparing base (1588ba2) to head (c841bbb).
Report is 91 commits behind head on develop.

Files with missing lines	Patch %	Lines
...escape/sequencescape/api/v2/tag_layout_template.rb	91.66%	2 Missing ⚠️
...ncerns/labware_creators/support_v2_source_plate.rb	85.71%	1 Missing ⚠️
app/models/labels/plate_split.rb	0.00%	1 Missing ⚠️
app/models/labware_creators/pooled_tubes_base.rb	75.00%	1 Missing ⚠️
app/sequencescape/sequencescape/api/v2/transfer.rb	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1884      +/-   ##
===========================================
+ Coverage    77.46%   77.96%   +0.50%     
===========================================
  Files          455      459       +4     
  Lines        17275    17688     +413     
  Branches       229      229              
===========================================
+ Hits         13382    13791     +409     
- Misses        3891     3895       +4     
  Partials         2        2

Flag	Coverage Δ
javascript	`69.72% <100.00%> (+1.10%)`	⬆️
pull_request	`77.96% <99.49%> (+0.50%)`	⬆️
push	`77.96% <99.49%> (+0.50%)`	⬆️
ruby	`91.11% <93.25%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

StephenHulme

Looks good, no red flags, and appreciate the comments 👍

harrietc52

Again minor comments :) can all be ignored. It hasn't been my more thorough code review but a scan over everything looks good and a big step forward. Thanks

harrietc52 · 2024-09-04T11:33:41Z

README.md

+  end
+end
+```
+


harrietc52 · 2024-09-04T11:42:16Z

app/models/labware_creators/final_tube_from_plate.rb

      end
    end

    def tubes_from_transfer
-      @transfer
+      create_transfer!


Just curious, how come you call the method here instead of using @create_transfer which I think should already exist and is not nil?

@create_transfer is the memoized backing store for the method. When you implement getters, you should use them at all times if possible. Not doing so can cause hard to debug issues where the getter is supposed to be managing access to the backing store. The only reason the other mention in this file is not using the method is that you cannot check for a nil value if it is always populated by the getter method.

I don't think we really need to check for a nil value, but I didn't want to get too deep into the weeds and cause a bug myself. The first time you try to get the transfer via the method, it would be called anyway, so why would you need to check if it's been called? I'm not sure.

harrietc52 · 2024-09-04T11:45:44Z

app/models/labware_creators/tagged_plate.rb

+        # 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.
        api


Could you add a comment just to make it super clear that api here is still currently using the SS V1 API

api is always SS V1 throughout Limber. We never access the V2 api via a variable called api.

harrietc52 · 2024-09-04T11:56:27Z

spec/models/labware_creators/quadrant_split_plate_spec.rb

@@ -80,6 +80,10 @@
      outer_requests: quad_a_requests
    )
  end
+  let(:child_plate_a_create_args) do
+    { user_id: user.id, asset_id: child_plate_a.id, metadata: { stock_barcode: "* #{child_plate_a.barcode.machine}" } }


Whats the * for?

No idea. It gets appended in the code so I have to append it in the test as well.

harrietc52 · 2024-09-04T11:59:15Z

spec/models/labware_creators/quadrant_stamp_primer_panel_spec.rb

@@ -304,7 +305,8 @@
          'custom_metadatum_collection-uuid',
          body: json(:v1_custom_metadatum_collection, uuid: 'custom_metadatum_collection-uuid')
        )
-        stub_api_get('user-uuid', body: user)
+        stub_api_get('user-uuid', body: v1_user)


Do you know if there is a reason the V1 User endpoint still needs to be used in this Class?

Not really, no. Removing the line does make the test fail though because the endpoint does get queried:

WebMock::NetConnectNotAllowedError: Real HTTP connections are disabled. Unregistered request: GET http://example.com:3000/user-uuid # ./app/models/labware_metadata.rb:21:in `update!' # ./app/models/labware_creators/quadrant_stamp_base.rb:39:in `block in create_labware!' # ./app/models/labware_creators/multi_stamp.rb:40:in `create_labware!' # ./app/models/labware_creators/quadrant_stamp_base.rb:38:in `create_labware!' # ./app/models/labware_creators/base.rb:56:in `save' # ./app/models/labware_creators/base.rb:52:in `save!' # ./spec/models/labware_creators/quadrant_stamp_primer_panel_spec.rb:328:in `block (4 levels) in <top (required)>'

harrietc52 · 2024-09-04T12:02:06Z

spec/support/api_url_helper.rb

+      # 'Sequencescape::Api::V2::' and returns the given value or else true.
+      receiving_class = "Sequencescape::Api::V2::#{klass}".constantize
+      return_value ||= true
+      allow(receiving_class).to receive(:create!).and_return(return_value)


Does it need to intercept any create (or save above) without the bang too?

I did intercept create without the! and it never gets called in our code so I stopped doing it. I suspect I'll find some case soon that needs it, but I didn't see the point in stubbing something we're not calling. I will be easy to add if it's needed at the time. I have this file open as a permanent fixture during the Limber migration.

harrietc52 · 2024-09-04T12:08:45Z

spec/controllers/robots_controller_spec.rb

@@ -4,61 +4,38 @@
 require './app/controllers/robots_controller'

 RSpec.describe RobotsController, type: :controller, robots: true do
+  has_a_working_api


Is this referring to the V1 api? Is it worth adding a comment saying this can be removed once the update to V2 api is complete (if that is the plan?)

Yeah it does some very dodgy stuff to make V1 APIs be stubbed all over the place. Removal is the plan. This is literally everywhere in the unit tests. It's part of my overall plan to clean up all traces of V1, but there's not much point in adding comments as I'm aware and will just be removing the comments later.

(I've also experimented with removing them when I think the API is no longer needed in some tests and it always highlights how much more v1 is still to be removed because the tests fail)

sdjmchattie · 2024-09-04T15:50:23Z

Again minor comments :) can all be ignored. It hasn't been my more thorough code review but a scan over everything looks good and a big step forward. Thanks

I applaud your determination to get through these pull requests! 122 changes files and a diff of over 4,000 lines is enough to put most off. Much appreciated.

# Conflicts: # app/frontend/javascript/shared/resources.js # app/sequencescape/sequencescape/api/v2/tube.rb

codeclimate · 2024-09-04T16:18:05Z

app/frontend/javascript/shared/resources.js

+    "options": {
+    }
+  },
+  {


Similar blocks of code found in 12 locations. Consider refactoring.

codeclimate · 2024-09-04T16:18:05Z

app/frontend/javascript/shared/resources.js

+    "options": {
+    }
+  },
+  {


Similar blocks of code found in 3 locations. Consider refactoring.

codeclimate · 2024-09-04T16:18:05Z

app/frontend/javascript/shared/resources.js

+    "options": {
+    }
+  },
+  {


Similar blocks of code found in 2 locations. Consider refactoring.

codeclimate · 2024-09-04T16:18:05Z

app/frontend/javascript/shared/resources.js

+    "options": {
+    }
+  },
+  {


Similar blocks of code found in 2 locations. Consider refactoring.

# Conflicts: # spec/models/labware_creators/plate_split_to_tube_racks_spec.rb

codeclimate · 2024-09-17T14:08:01Z

Code Climate has analyzed commit c841bbb and detected 22 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	21

The test coverage on the diff in this pull request is 93.2% (50% is the threshold).

This pull request will bring the total coverage in the repository to 91.1% (0.0% change).

View more on Code Climate.

sdjmchattie added 30 commits July 23, 2024 18:31

Use SS API v2 for getting BarcodePrinters

f18dded

Remove v1 barcode printer factories

095cc2f

Rename type_name to barcode_type

58625fa

Remove unused test variable

4004e22

Update the devour config file

bc41bfc

Only just found out about it and it looks like it hasn't been done by anyone in recent times.

Remove v1 API from LabwareController

6440fb3

Put the api attribute back on Presenter for now

8feebba

It's used by sub-classes so they need to be fixed first

Merge pull request #1805 from sanger/Y24-190-2-use-ss-api-v2-for-chro…

e0a87c9

…mium-pipelines Y24-190-2: Use SS API v2 for barcode printers

Move the LabwareMetadata class over to using the V2 API

e49c4b2

Update some tests with new API v2 helper methods

8655a67

Fix Rubocop concerns

4772f7a

Fix tests for quadrant split plate

38f0809

Remove reference to custom_metadatum_collection from tube

12dea35

Add classes for using v2 API TagLayoutTemplate

86d5934

Start putting V2 tag layout template stubs/factories in place

b73aa7b

Document the approach to stubbing relationships in V2 resources

dff8746

Remove the accidentally committed test debugging objects

4fd7757

Ensure tags generated for a tag_group point to the correct group

1d89cf7

Fix relationships for tag layout templates in FactoryBot

4b82bbb

Fix tests for custom tagged plate spec

3306e6d

Fix tests for tagged_plate_spec

b751825

Merge branch 'develop' into develop-Y24-190

43b514b

Remove unneeded stub

c4940a1

Add a factory for a dual indexed tag layout template

cc2593b

Clean up the naming in shared examples

cbb3f58

Correctly name context blocks

3da27a2

Merge branch 'develop' into develop-Y24-190

6bd2aec

Assign correct class for tag2_group on tag_layout_template

7b0759f

Merge page results when getting tag layout templates

201fb5b

Add TODO about calling methods on resources

7e895d3

sdjmchattie added 8 commits August 22, 2024 15:41

Fix misnamed variable reference

30b85c0

Fix remaining instances of changing persisted transfer template IDs a…

305a657

…cross tests

Merge branch 'develop' into develop-Y24-190

842220c

Merge pull request #1863 from sanger/Y24-190-3-use-v2-transfer-template

0d5e508

Y24-190-3: Use v2 transfers and transfer templates

Switch to using API v2 for StateChange creations

99d5394

Update tests to expect API v2 StateChange creation

863651a

Merge pull request #1882 from sanger/Y24-190-3-use-v2-state-change

bc222b7

Y24-190-3: Use API v2 StateChange resource

Merge branch 'develop' into develop-Y24-190

b2e1d16

sdjmchattie requested review from yoldas, KatyTaylor, harrietc52, BenTopping and StephenHulme September 2, 2024 16:22

sdjmchattie self-assigned this Sep 2, 2024

codeclimate bot reviewed Sep 2, 2024

View reviewed changes

sdjmchattie mentioned this pull request Aug 5, 2024

Y24-190 - Move Sequencescape API v1 calls over to API v2 #1772

Open

2 tasks

StephenHulme approved these changes Sep 4, 2024

View reviewed changes

harrietc52 approved these changes Sep 4, 2024

View reviewed changes

sdjmchattie added 2 commits September 4, 2024 16:54

Merge branch 'develop' into develop-Y24-190

9b277c4

# Conflicts: # app/frontend/javascript/shared/resources.js # app/sequencescape/sequencescape/api/v2/tube.rb

Add new API v2 endpoints to resources.js

130099e

codeclimate bot reviewed Sep 4, 2024

View reviewed changes

sdjmchattie added 2 commits September 6, 2024 16:23

Merge branch 'develop' into develop-Y24-190

630bf1e

Merge branch 'develop' into develop-Y24-190

c841bbb

# Conflicts: # spec/models/labware_creators/plate_split_to_tube_racks_spec.rb

sdjmchattie merged commit 197bcc7 into develop Sep 19, 2024
14 checks passed

sdjmchattie deleted the develop-Y24-190 branch September 19, 2024 09:57

sdjmchattie restored the develop-Y24-190 branch September 19, 2024 09:57

github-actions bot mentioned this pull request Sep 19, 2024

[automated] Merge Develop into Master #1940

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Y24-190 Milestone 1: API v1 -> v2 Migration #1884

Y24-190 Milestone 1: API v1 -> v2 Migration #1884

sdjmchattie commented Sep 2, 2024

codeclimate bot Sep 2, 2024

sdjmchattie Sep 2, 2024

codeclimate bot Sep 2, 2024

sdjmchattie Sep 2, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading

StephenHulme left a comment

harrietc52 left a comment

harrietc52 Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

sdjmchattie Sep 4, 2024

sdjmchattie commented Sep 4, 2024

codeclimate bot Sep 4, 2024

codeclimate bot Sep 4, 2024

codeclimate bot Sep 4, 2024

codeclimate bot Sep 4, 2024

codeclimate bot commented Sep 17, 2024

Y24-190 Milestone 1: API v1 -> v2 Migration #1884

Y24-190 Milestone 1: API v1 -> v2 Migration #1884

Conversation

sdjmchattie commented Sep 2, 2024

Changes proposed in this pull request

Instructions for Reviewers

codeclimate bot Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 2, 2024 • edited Loading

Codecov Report

StephenHulme left a comment

Choose a reason for hiding this comment

harrietc52 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdjmchattie commented Sep 4, 2024

codeclimate bot Sep 4, 2024

Choose a reason for hiding this comment

codeclimate bot Sep 4, 2024

Choose a reason for hiding this comment

codeclimate bot Sep 4, 2024

Choose a reason for hiding this comment

codeclimate bot Sep 4, 2024

Choose a reason for hiding this comment

codeclimate bot commented Sep 17, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading