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

sdjmchattie · 2024-09-02T16:20:19Z

Changes proposed in this pull request

Add new/updated API v2 endpoints for:
- BarcodePrinter
- CustomMetadatumCollection
- StateChange
- TagLayoutTemplate
- Transfer
- TransferTemplate
Minor tweaks to development tool config such as pry.
Updated documentation stubs for API resources.

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

They were failing when they found Concerns and SharedBehaviour sub modules which did not contain JSONAPI::Resource implementations.

Presumably for efficiency, which we shouldn't be concerned about here, but Rubocop insists!

…o-v2-api Y24-190-2: Add barcode printers to v2 API

…creators-in-api-v2

The context is setting up a CustomMetadatumCollection, but we don't need one to be able to #post.

…-creators-in-api-v2 Y24-190-3: Support Limber TagLayoutTemplate in API v2

These contain TODOs for things which are going to require knowledge of the Sequencescape models.

…metadata-in-api-v2 # Conflicts: # spec/requests/api/v2/custom_metadatum_collections_spec.rb

When changing to that driver, the downloads fail to be found

…-state-change Y24-190-3 Support Limber with API v2 StateChange

sdjmchattie · 2024-09-02T16:23:15Z

Cucumber tests will not pass because of ongoing issues with GitHub action runners that are now using the new build image.

codecov · 2024-09-02T16:28:17Z

Codecov Report

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

Project coverage is 87.51%. Comparing base (862edf4) to head (13fd8fc).
Report is 116 commits behind head on develop.

Files with missing lines	Patch %	Lines
lib/tasks/devour.rake	0.00%	5 Missing ⚠️
...pp/resources/api/v2/transfers/transfer_resource.rb	98.48%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4315      +/-   ##
===========================================
+ Coverage    87.48%   87.51%   +0.02%     
===========================================
  Files         1376     1380       +4     
  Lines        29700    29824     +124     
===========================================
+ Hits         25984    26101     +117     
- Misses        3716     3723       +7

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

app/resources/api/v2/transfer_template_resource.rb

harrietc52 · 2024-09-04T11:06:00Z

app/resources/api/v2/user_resource.rb

+
+      # @!method uuid
+      #   A filter to return only users with the given UUID.
+      filter :uuid, apply: lambda { |records, value, _options| records.with_uuid(*value) }


Might this Resource want the creatable_fields to not include uuid too, if it is created by the system on creation?

Bearing in mind that I didn't actually create this endpoint, I only added documentation to it and added the UUID filter.

I have been preferring to use creatable_fields in more recent resources as it gives nicer error messages than using readonly: true on the attribute itself. You get an error with a description that you cannot include the attribute instead of getting an error 500 from the server when you try to set an attribute that it read only. I could go through and update all the existing endpoints, but it will mean more messing around with tests so I've avoided doing it for existing resources.

Sure, no problem. I thought too it looks much nicer, didn’t know the details but that is definitely better handling. Each resource can be updated as and when they are touched 👍

harrietc52

Minor comments, feel free to ignore. Nice work, as always! 👍

spec/requests/api/v2/shared_examples/invalid_post_requests.rb

spec/requests/api/v2/transfers/transfers_spec.rb

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

spec/resources/api/v2/barcode_printer_resource_spec.rb

+  end
+
+  it 'disallows updating of read only fields', :aggregate_failures do
+    expect(resource).not_to have_updatable_field :uuid


To not have updatable field id too maybe?

I stopped adding :id to these simply because I could also check that :banana is not an updatable_field. id doesn't form part of the resource and so testing for its lack of existence didn't feel like it was adding value to these tests. These tests are quite poor anyway. They only test the properties of the attribute entries like readonly. They don't confirm whether the resource is immutable and they don't know how to parse the things like fetchable_fields and creatable_fields methods that are part of the library we're using. The best tests are in the requests spec folder.

harrietc52 · 2024-09-04T11:18:19Z

spec/resources/api/v2/transfers/transfer_resource_spec.rb

+    expect(resource).to have_updatable_field :destination_uuid
+    expect(resource).to have_updatable_field :user_uuid
+    expect(resource).to have_updatable_field :transfers
+    expect(resource).to have_updatable_field :transfer_template_uuid


Quite like the tests of .not_to have updatable_field if you wanted to add any here aswell

Same as above 😄

andrewsparkes

Looks good I think, well done. I went a bit blind by the end. A few minor comments only.

andrewsparkes · 2024-09-04T12:52:59Z

app/controllers/api/v2/transfers/transfers_controller.rb

+        end
+      end
+
+      class BetweenPlateAndTubesController < JSONAPI::ResourceController


I was a bit confused as to what all these controller types are. Different types of transfer controllers presumably.

Yeah, this is me dealing with polymorphism. You might want to only GET the BetweenPlates transfers and in order to have a route in the routes file, you must have a corresponding controller. Although they're all implemented in the base class, this gives you methods that handle creating a resource, patching a resource, etc.

That's useful info, could you add comments to that affect? Just above this BetweenPlateAndTubesController to say 'the following controllers are ...' etc.

andrewsparkes · 2024-09-04T14:00:48Z

spec/factories/transfers.rb

+
+    factory(:pooling_transfer_template) do
+      transfer_class_name { 'Transfer::BetweenPlatesBySubmission' }
+      transfers { nil }


Are these transfers nil because that is correct or because you don't know what they should be? Some comments and or a todo in either case would help.

Yes fair enough. I've added a comment. Basically some transfer types do not have the transfers in the template. These in particular expect the transfers to be passed in the creation request as they are defined by the user in the Limber UI I believe. I only have a vague understanding of how it all works. The production database shows these as nil in the templates.

andrewsparkes · 2024-09-04T14:08:52Z

spec/requests/api/v2/custom_metadatum_collections_spec.rb

@@ -8,105 +8,282 @@

  it_behaves_like 'ApiKeyAuthenticatable'

-  context 'with multiple custom_metadatum_collections' do
+  context 'with multiple collection' do


collections

andrewsparkes · 2024-09-04T14:37:22Z

spec/resources/api/v2/user_resource_spec.rb

@@ -4,17 +4,24 @@
 require './app/resources/api/v2/user_resource'

 RSpec.describe Api::V2::UserResource, type: :resource do
-  subject { described_class.new(resource_model, {}) }
+  subject(:resource) { described_class.new(resource_model, {}) }


how does this work? you define the subject(:resource) but then it's not referred to in the it sections below. Does is_expected magically use the subject?

Yeah it's some sort of rspec magic. is_expected seems to know it needs to use the thing defined as the subject. I didn't know this pattern, but I saw it already existed in our tests. The only change here was to name the subject, which Rubocop insists on when you remove the exception to the rule like I did here in the rubocop config file.

# Conflicts: # features/support/capybara.rb

…llers

sdjmchattie added 30 commits July 22, 2024 18:16

Add a resource for BarcodePrinters on v2 API

fd3a078

Rename type_name to barcode_type

d40d80b

Add print_service attribute to BarcodePrinter

3c61436

Add tests for barcode printers on V2

627b1b1

Clean up implementation of CustomMetadatumCollection

a39e2a3

Separate expectations into different test cases

8c7d622

Capture the expectation before applying the failing method

4f653cd

Fix the devour tasks

6466af5

They were failing when they found Concerns and SharedBehaviour sub modules which did not contain JSONAPI::Resource implementations.

Extract immutable array from loop

aa42dc7

Presumably for efficiency, which we shouldn't be concerned about here, but Rubocop insists!

Merge pull request #4225 from sanger/Y24-190-2-add-barcode-printers-t…

babbbcc

…o-v2-api Y24-190-2: Add barcode printers to v2 API

Update TagLayoutTemplate resource with YARD docs

8f66d79

Merge branch 'develop' into develop-Y24-190

fe8663e

Assign correct class name for tag2_group

118fedd

Improve test coverage of a TagLayoutTemplateResource

db19247

Improve class documentation for TagLayoutTemplate

b11d920

Merge branch 'develop' into develop-Y24-190

347ad0f

Merge branch 'develop-Y24-190' into Y24-190-3-support-limber-labware-…

4afe887

…creators-in-api-v2

Tweak YARD documentation options

9daa2d0

Add immutability as a note on the class

96b13dc

Move #post outside the context

6ec2a17

The context is setting up a CustomMetadatumCollection, but we don't need one to be able to #post.

Improve request tests for TagLayoutTemplates

a029dd4

Exclude API controllers from documentation

685fc92

Tag resources of the V2 API

d453719

Complete documentation for TagLayoutTemplateResource

f91fe71

Fix grammar in immutability note

0d038d1

Merge pull request #4258 from sanger/Y24-190-3-support-limber-labware…

6904687

…-creators-in-api-v2 Y24-190-3: Support Limber TagLayoutTemplate in API v2

Add detailed overviews for each resource class.

6f24ac3

These contain TODOs for things which are going to require knowledge of the Sequencescape models.

Remove double-blank line upsetting Prettier

975c986

Fix YARD documentation of attributes on CustomMetadatumCollection

16aa1b1

Merge branch 'develop-Y24-190' into Y24-190-2-support-limber-labware-…

d429893

…metadata-in-api-v2 # Conflicts: # spec/requests/api/v2/custom_metadatum_collections_spec.rb

sdjmchattie added 5 commits August 30, 2024 10:41

Ensure the chrome driver works with downloads

df9bb4b

When changing to that driver, the downloads fail to be found

Make validator for state_changer conditional on there being a target

31bb922

Change Proc to a lambda function

4e85c97

Merge pull request #4312 from sanger/Y24-190-3-support-limber-with-v2…

d009f9b

…-state-change Y24-190-3 Support Limber with API v2 StateChange

Merge branch 'develop' into develop-Y24-190

f81884c

sdjmchattie self-assigned this Sep 2, 2024

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

sdjmchattie mentioned this pull request Sep 2, 2024

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

Merged

sdjmchattie mentioned this pull request Aug 5, 2024

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

Open

2 tasks

harrietc52 reviewed Sep 4, 2024

View reviewed changes

app/resources/api/v2/transfer_template_resource.rb Outdated Show resolved Hide resolved

harrietc52 reviewed Sep 4, 2024

View reviewed changes

harrietc52 approved these changes Sep 4, 2024

View reviewed changes

Remove Sequencescape URL from documentation

df90ee8

andrewsparkes approved these changes Sep 4, 2024

View reviewed changes

sdjmchattie added 7 commits September 4, 2024 15:58

Use shared examples for Transfer tests

fe1e7b1

Merge branch 'develop' into develop-Y24-190

0ea29cb

# Conflicts: # features/support/capybara.rb

Apply prettier

6c11f94

Comment on why some factories are giving templates nil transfers

4cd0a07

Fix bad grammar in test naming

8b0e5a6

Fix devour Rake task now we have polymorphic transfers

8b949da

Add comments explaining the existence of all the Transfer type contro…

13fd8fc

…llers

sdjmchattie merged commit 7cffb98 into develop Sep 5, 2024
22 of 23 checks passed

sdjmchattie deleted the develop-Y24-190 branch September 5, 2024 09:34

sdjmchattie restored the develop-Y24-190 branch September 5, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sdjmchattie commented Sep 2, 2024 •

edited

Loading

sdjmchattie commented Sep 2, 2024

codecov bot commented Sep 2, 2024 •

edited

Loading

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 5, 2024

harrietc52 left a comment

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

harrietc52 Sep 4, 2024

sdjmchattie Sep 4, 2024

andrewsparkes left a comment

andrewsparkes Sep 4, 2024

sdjmchattie Sep 4, 2024

andrewsparkes Sep 5, 2024

andrewsparkes Sep 4, 2024

sdjmchattie Sep 4, 2024

andrewsparkes Sep 4, 2024

andrewsparkes Sep 4, 2024

sdjmchattie Sep 4, 2024

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

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

Conversation

sdjmchattie commented Sep 2, 2024 • edited Loading

Changes proposed in this pull request

Instructions for Reviewers

sdjmchattie commented Sep 2, 2024

codecov bot commented Sep 2, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this 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

andrewsparkes 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

sdjmchattie commented Sep 2, 2024 •

edited

Loading

codecov bot commented Sep 2, 2024 •

edited

Loading